[P3] markers-checklist: follow-up - TJ review#2254
Closed
rolfheij-sil wants to merge 5 commits into
Closed
Conversation
…mental marker Addresses TJ Couch's review on the merged PR #2219 (markers-checklist). Changes (PT10-dev-perspective feedback): - T1 stage 1: Fix wrong "versification is fixed at project open" docstring on IVersificationService. Versification IS mutable at runtime — PT9's ProjectSettings setter is public, PT10's ParatextProjectDataProvider fires SendDataUpdateEvent on the setting change. The service is safe under mutation (reads fresh each call) but the docstring lied. Also flagged the service as a candidate for projectInterface conversion (stage 2 in a separate PR). - T9: Better TSDoc on lookupFinalVerseNumbersInBook — explain WHY index 0 is filler (1-based ergonomic access, no off-by-one), include a worked example. - T4: Replace ChecklistScriptureVerseRef with platform SerializedVerseRef (the docstring already admitted they were the same shape). - T5: Replace ChecklistScriptureRange with platform ScriptureRange. Platform type's end is optional; the C# DTO already accepts a null End so this is wire-compatible and the TS sites all still populate end explicitly. - T6: Expand TSDoc on ChecklistComparativeTextRef.name — document the real fallback scenarios (legacy PT9 mementos, malformed GUIDs, PTX-23529 identical short names) instead of the bare "fallback resolution method". - T7/T8: Expand TSDocs across IChecklistService, ChecklistRequest, ChecklistResultResponse, MarkerSettingsValidationResult, ResolvedComparativeTexts, and the openMarkersChecklistSettings command. Drop all "data-contracts.md §X" references — TSDocs should stand alone for downstream consumers per TJ's proposed workflow rule. - T11: Tighten ScopeSelector TSDocs on rangeStart/rangeEnd/currentScrRef to explain they are NOT duplicates — currentScrRef is a fallback in the chain (rangeStart → currentScrRef → GEN 1:1), and also drives the dropdown trigger's secondary display text. - T12: Mark LinkedScrRefButton @experimental — slated for removal once PR #1949 (LinkedScrRefDisplay) lands. Not addressed in this PR (deferred / pushback): - T1 stage 2 (projectInterface conversion): larger refactor, follow-up PR. - T2 (NetworkObject → DataProvider for IChecklistService): pushed back — service is genuinely stateless RPC, no cached state to invalidate. - T3 (validateMarkerSettings as project-setting validator): coupled to marker-settings-as-project-setting refactor, backlog. - T10 (getEndVerse callback as data prop): pushed back — the consumer only needs current-book data; a data prop would require pre-fetching all ~66 books and re-fetching on book change. Verified: - npm run lint — 0 errors (same pre-existing warnings as ai/main). - npm run typecheck — clean across all workspaces. - npm test — all 523 platform-bible-react tests pass; 5 storybook FAIL files are pre-existing on ai/main (stale storybook cache export issue).
rolfheij-sil
requested review from
irahopkinson,
jolierabideau,
lyonsil and
tjcouch-sil
as code owners
20 tasks
TJ Couch (PR #2219 review, T6): > Why do we need a name here? Shouldn't project IDs be plenty? PT10 is greenfield: every project carries a canonical GUID, so PT9's name-fallback (which existed for mementos that pre-dated GUID assignment) covers scenarios we don't actually have. The duplicate-short-name case (PTX-23529) is unaffected — GUID is what disambiguates there, and that's what we keep. Changes: - TS `ChecklistComparativeTextRef` is now `{ id: string }` only. TSDoc rewritten to drop the fallback rationale. - TS web view `handleComparativeTextsChange` no longer looks up project short-names just to populate `name`. - C# `ComparativeTextRef` record is now `(string Id)` — single positional field. File header notes that the Name field was dropped per TJ's review and points at this PR. - C# `ChecklistService.ResolveComparativeTexts`: - Drop step 2(b) name-fallback block (the `ScrTextCollection.Find(requested.Name)` call). - Step 2(d) sets `Name: found?.Name ?? string.Empty` (no requested.Name to fall back on). - Provenance comment + XML docs updated to remove "GUID-first / name-fallback" framing — now "GUID-only". - C# tests: - Outer acceptance (Mixed paths): drop the BRAVO entry; the test now confirms ALPHA-resolves / ACTIVE-excluded / CHARLIE-unavailable. - Delete the dedicated TS-047 name-fallback test (`InvalidGuidValidName_FallsBackToFindByName`). - Delete the name-fallback variant of self-exclusion (`ComparativeRefIsActiveProjectByName_Excluded`). - Rename `InvalidGuidInvalidName_MarkedUnavailable` → `UnresolvableGuid_MarkedUnavailable`; add a sibling test for the malformed-GUID case (HexId.FromStrSafe → null → unavailable). - Tighten TS-048 (duplicate short name): name now omitted from the request, confirming GUID is the disambiguator. - `ChecklistDataModelTests.ComparativeTextRef_RoundTripsThroughJson` and `ComparativeTextRef_WithExpressionProducesNewInstance` updated to the new single-field shape. Verified: - npm run typecheck — clean across all workspaces. - npm run lint — 0 errors (pre-existing warnings only). - npm test — all TS tests pass; same 5 pre-existing storybook FAIL files (storybook cache export issue on ai/main, unrelated). - dotnet test in c-sharp-tests — 704 passing, 6 skipped (same totals as before; the deleted tests are replaced by tighter coverage of the remaining GUID-only contract). Wire compatibility note: this changes the JSON shape of the `requestedTexts` argument to `IChecklistService.resolveComparativeTexts`. The only producer is the markers-checklist web view in this repo, which this commit updates in lockstep. No external consumer is affected. Note: `.context/features/markers-checklist/data-contracts.md` §2.4 still mentions the Name field. That doc lives in ai-prompts; per Rolf's direction we are not opening an ai-prompts PR in this round, so the doc will drift slightly until it's swept later.
rolfheij-sil
changed the title
20 tasks
CI's stricter prettier pass flagged the scope-selector.component.tsx TSDocs I rewrote in commit 84e53a2 — prettier preferred different line breaks across `—` em-dashes and `*not*` → `_not_` markdown emphasis. Pure whitespace + markdown punctuation; no semantic change. Also rebuilt platform-bible-react/dist/ to pick up the matching JSDoc text in index.d.ts. Local pre-commit hook didn't catch this because it runs `prettier --write` on staged files only, but the format-check on CI runs across the whole workspace and applies prettier with the prose-wrap rules defined in lib/platform-bible-react/.prettierrc.
rolfheij-sil
changed the title
rolfheij-sil
force-pushed
the
ai/feature/markers-checklist-followup-rolf-05-11-2026
branch
from
e6d686f to
53580a9
Compare
rolfheij-sil
changed the title
6 tasks
…ct fix - ChecklistService: honor platform ScriptureRange contract — End == null now narrows to a single-verse range at Start (was: scan to last verse of project). No production caller triggers the changed branch; new regression test BuildChecklistData_VerseRangeEndOmitted_ScansSingleVerseAtStart covers it. - platform-scripture.d.ts: tighten verseRange.end TSDoc to reference the canonical ScriptureRange semantics; document '' fallback on ResolvedComparativeTexts.texts[].name / fullName when available is false. - LinkedScrRefButton: mirror the @experimental marker on the function itself so IDE / TypeDoc surfaces the warning at every call site (was: only on LinkedScrRefButtonProps). - popover.tsx (PopoverPortalContainerProvider): rephrase JSDoc so the inline useState<HTMLElement | null>(null) reference lands mid-line and isn't reflowed across paragraphs by Prettier. - Rename test ResolveComparativeTexts_MixedResolutionPaths_… → ResolveComparativeTexts_MixedAvailability_… (only one resolution path remains after name-fallback was removed). - Regenerate lib/platform-bible-react/dist for the JSDoc / @experimental changes. Includes benign CSS reordering churn (build-tool non-determinism). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
rolfheij-sil
changed the title
rolfheij-sil
changed the title
rolfheij-sil
deleted the
ai/feature/markers-checklist-followup-rolf-05-11-2026
branch
This was referenced May 13, 2026
Contributor
Author
|
Closed inadvertently when the head branch was renamed to the canonical Replaced by #2262 — same commits, same diff, canonical branch name so automated status reports pick it up. |
This was referenced May 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Code Review Summary
Branch: ai/feature/markers-checklist-followup-rolf-05-11-2026
Base: origin/ai/main
Date: 2026-05-12
Review model: Claude Opus 4.7
Files changed: 12 (pre-review) → 16 (after in-review changes; +2 staged source edits + 1 new test + dist regen)
Overview
Follow-up PR to the merged markers-checklist work (PR #2219) addressing TJ Couch's code-review comments. The branch swaps two duplicated extension-local types (
ChecklistScriptureVerseRef,ChecklistScriptureRange) for the platform's canonicalSerializedVerseRef/ScriptureRange, drops the redundantnamefield fromChecklistComparativeTextRef(PT10 is greenfield — GUIDs are canonical), expands TSDocs across the checklist contract surface, and adds an@experimentalmarker toLinkedScrRefButtonwith an explicit migration target (LinkedScrRefDisplayfrom PR #1949). All test suites updated coherently with the contract changes; no orphaned name-fallback assertions remain.During the review, three items were fixed in-place: a Prettier-mangled JSDoc block on
PopoverPortalContainerProvider(pre-existing from PR #2219, surfaced by the dist regen here), a docs-vs-code mismatch onChecklistRequest.verseRange.endsemantics (now honors the platformScriptureRangecontract), and minor TSDoc /@experimental/ test-name hygiene. The C#ResolveVerseRangechange is the only behavior change introduced by this review — covered by a new regression test and no production caller exercises the changed path today.API Changes
extensions/src/platform-scripture/src/types/platform-scripture.d.ts:ChecklistScriptureVerseRefexport (consumers should useSerializedVerseRef).ChecklistScriptureRangeexport (consumers should useScriptureRange).ChecklistComparativeTextRef— removedname: stringproperty (now{ id: string }only).ChecklistRequest.verseRange— type changed fromChecklistScriptureRange | undefinedtoScriptureRange | undefined. Field-by-field compatible at the wire level; with the in-review C# fix,end === undefinednow narrows to a single-verse range atstartper the platform contract.IVersificationService,ChecklistMarkerSettings,ChecklistRequest,ChecklistResultResponse,MarkerSettingsValidationResult,ResolvedComparativeTexts,IChecklistService, and the'platformScripture.openMarkersChecklistSettings'command. Behavioral note onIVersificationServicecorrects an earlier wrong claim ("fixed at project open") to "reads fresh each call" — matches the existing C# implementation; no code change.lib/platform-bible-react/:LinkedScrRefButtonPropsand theLinkedScrRefButtonfunction — TSDoc@experimentalmarkers added (function-level marker added during review for IDE visibility at every call site).ScopeSelectorProps.rangeStart/rangeEnd/currentScrRef— TSDoc-only expansions.PopoverPortalContainerProvider— TSDoc rephrased so<HTMLElement | null>(null)lands mid-line (fixed during review).lib/platform-bible-react/dist/index.{d.ts,cjs,cjs.map,js,js.map}— regenerated to reflect source changes. Includes a benign CSS reordering churn for.banded-rowrules (build-tool non-determinism, no semantic change).ResolveVerseRange(c-sharp/Checklists/ChecklistService.cs) — behavior change:range.End == nullnow returns(start, start)(single-verse) instead of(start, lastVerseOfProject), matching the platformScriptureRangecontract. No production caller currently triggers the changed branch.Findings
Critical — Must address before merge
None.
Important — Should address before merge
Removed public-type exports ((Author dismissed: rip-the-bandaid is intentional. PR [P3][backend+ui] markers-checklist: Backend + UI implementation (C# data provider, React web view, E2E tests) #2219 just merged with no time for external consumers to depend on these types; wire compatibility is preserved (the C# DTO ignores the extraChecklistScriptureVerseRef,ChecklistScriptureRange,ChecklistComparativeTextRef.name) are technically breaking changes to an extension.d.ts. Suggested: keep@deprecatedaliases for one release cycle.namefield). The PR body documents this.)IVersificationService.lookupFinalVerseNumbersInBook@exampleblock was corrupted by Prettier reflow (Constcapitalized; statements collapsed onto run-on lines).IVersificationServiceentirely, replacing it withIVersificationProjectDataProvideron theParatextProjectDataProvider. The mangled example lives in code that's being removed.)PopoverPortalContainerProvider— inlineuseState<HTMLElement | null>(null)was split across paragraphs because|started a new line. Pre-existing from PR [P3][backend+ui] markers-checklist: Backend + UI implementation (C# data provider, React web view, E2E tests) #2219 but surfaced by the dist regen here. (Fixed during review: rephrased the prose so the inline type stays mid-line; dist regenerated.)ChecklistRequest.verseRangeTSDoc vs C# mismatch — TSDoc claimed "When the range omitsend, only the verse atstartis included" but the C# treatedEnd == nullas "scan to last verse of last book". (Fixed during review — chose Option B over Option A: updated C#ResolveVerseRangeto honor the platformScriptureRangecontract (End == null→ single-verse atStart); updated TSDoc to reference the platform semantics; added regression testBuildChecklistData_VerseRangeEndOmitted_ScansSingleVerseAtStart. Rationale: TJ's whole T4/T5 request was "use the canonical type" — adopting the type without honoring its contract leaves a trap for future readers. No production caller currently triggers the changed branch, so behavior risk is zero.)Minor — Consider
IVersificationServiceNOTE atplatform-scripture.d.ts:816-818references "the markers-checklist post-merge review" — internal review process leaking into public TSDoc.ResolvedComparativeTexts.texts[].name/fullNameTSDoc — should document the""fallback whenavailable: false(C# returnsstring.Emptyfor unresolved entries). (Fixed during review: added "or''whenavailableisfalse" to both fields' TSDocs.)UX-affordance trade-off: with(Author dismissed: intentional per PR body. PT10 is greenfield and every project has a canonical GUID, so PT9's name-fallback covers scenarios we don't have; no renderer code currently consumes the unresolved name.)requested.Nameno longer available, the renderer can no longer display a user-typed name when a comparative text fails to resolve.ResolveComparativeTexts_MixedResolutionPaths_…no longer accurate — with name-fallback removed there is only one resolution path (GUID). (Fixed during review: renamed toResolveComparativeTexts_MixedAvailability_PreservesOrderAndCorrectlyFlagsAvailabilityin both the method name and the xmldoc<see cref>.)Comment numbering inconsistency in(Author dismissed: cosmetic. The body'sResolveSingleComparativeRef— header uses(a)/(b)/(c)while the body usesStep 2(a)/(b)/(c).Step 2prefix is intentional — it continues the callerResolveComparativeTexts'sStep 1, Step 2numbering. The header's bare(a)/(b)/(c)is a preamble, not a continuation. Both are correct in context.)@experimentalJSDoc only onLinkedScrRefButtonProps, not on the exported function — IDE / TypeDoc surfaces the marker only at the type-import site, not at every call site. (Fixed during review: added@experimentalJSDoc block to theLinkedScrRefButtonfunction itself, linking toLinkedScrRefButtonPropsfor the full migration detail; dist regenerated.)Template Propagation
Shared Regions Modified
None.
Extension Config Changes
None — no extension config/build files (
package.json,tsconfig.json,webpack.config.ts,.eslintrc*) underextensions/were modified in this PR.Positive Observations
IChecklistService,ChecklistRequest, andChecklistResultResponseis substantive and improves the public surface — the new prose explains intent (e.g. "stateless RPC — no internal cache") rather than just restating field names.IVersificationServicecorrecting "fixed at project open" → "reads fresh each call" aligns the docs with the actual C# implementation (verified againstc-sharp/Projects/VersificationService.cs:42-71).@experimentalmarker onLinkedScrRefButtonis the right tool for "shipped but expected to be replaced" — better than silently removing later. The marker links to the superseding PR (PT-3662, PT-3744, partially PT-3661 #1949) and names the migration target (LinkedScrRefDisplay).ComparativeTextRefchange is internally consistent: the record, network-object wiring, service, and unit tests all updated together; no danglingNamereferences remain. The file header retains aHistory:block explaining why the field was dropped — exactly the kind of "future archaeologist" context that prevents this from being re-added later.handleComparativeTextsChangecallback dropped itsallProjectsdependency cleanly whennamelookup disappeared.ScenarioId,Invariant) were trimmed where the old scenario (TS-047) no longer applies — no stale traceability metadata left behind.ChecklistScriptureRange/ChecklistScriptureVerseReffor the platform's canonicalScriptureRange(andSerializedVerseRefalready in use) is a clean DRY win — removes duplicated types that had drifted from the platform contract.Interview Notes
IVersificationService@examplefinding as moot (the entire service is being deleted in that sibling PR); explicitly referenced PR [P3][backend+ui] markers-checklist: Backend + UI implementation (C# data provider, React web view, E2E tests) #2219's recent merge to dismiss the removed-exports finding as a deliberate "rip-the-bandaid" call; mapped each individual review finding back to TJ's stated preferences (canonical types T4/T5, drop redundant fields T6, thorough TSDocs T7/T8, mark experimental T12) before deciding to fix or dismiss.ChecklistRequest.verseRange.end, author asked which truly honored the platform contract and chose Option B (fix the C# to match the platformScriptureRangecontract) over Option A (tighten the docs to describe the C#'s non-standard behavior). Rationale: TJ's whole T4/T5 was "adopt the canonical type"; half-adopting (using the type but not its contract) creates a trap for future readers. Author was aware this added a behavior change to a PR scoped as docs/types but accepted that trade-off after confirming the changed code path has no production caller.@experimentalon the function itself).In-Review Quality Check
dotnet test --filter "FullyQualifiedName~Checklists"): 194 passed, 2 unrelated skipped, 0 failed. New regression testBuildChecklistData_VerseRangeEndOmitted_ScansSingleVerseAtStartpasses.npx prettier --checkonpopover.tsxsource: clean (no reflow on the rephrased prose).dotnet csharpier .inc-sharp/: ran clean (172 files formatted; no diff on the edited file beyond the intentional edit).npm run format: clean for all 11 in-review files; one unrelated mdx file diff (pre-existing JSX indentation churn inlib/platform-bible-react/src/stories/guidelines/design-principles.mdx) was reverted by the quality-check subagent to keep this PR focused.npm testfull TypeScript suite: 1648 passed, 1 skipped, 0 failed. Note: The 5 storybook FAILs mentioned in the PR body did not recur in this run — likely because the dist rebuild cleared the cache. Not a regression.npm run typecheckandnpm run lint: each returned 1 pre-existing error —src/main/services/app.service-host.ts(9,23)cannot find'../../../release/app/buildInfo.json'. This file is generated by the build pipeline and is absent in fresh worktrees; not touched by any in-review change. Runningnpm run build(or the buildInfo-generation step) before CI clears it. Not blocking the review.Suggested Review Focus
Prioritized areas for the author-reviewer meeting:
ResolveVerseRangecontract change (I-4 fix) — this is the only behavioral change introduced by the review. Reviewer should confirm the platformScriptureRange.end?semantics ("single verse at start" when end is omitted) is the right interpretation for the checklist, and that no in-flight caller (e.g., [P3] markers-checklist: follow-up - Sebastian UX-2 review (9 work packages) #2258 work, or future projectInterface migrations from [P3] markers-checklist: follow-up T1-stage-2 - Versification NetworkObject → projectInterface on ParatextProjectDataProvider #2256) silently relies on the old "scan to end of project" behavior. Regression testBuildChecklistData_VerseRangeEndOmitted_ScansSingleVerseAtStartcovers the new contract.ChecklistScriptureVerseRef/ChecklistScriptureRange/ChecklistComparativeTextRef.namebetween the PR [P3][backend+ui] markers-checklist: Backend + UI implementation (C# data provider, React web view, E2E tests) #2219 merge and this PR. If yes, consider re-introducing@deprecatedaliases.platform-scripture.d.ts,checklist.web-view.tsx), confirm the merge ordering plan in the PR body still holds. Either of [P3] markers-checklist: follow-up - TJ review #2254 / [P3] markers-checklist: follow-up T1-stage-2 - Versification NetworkObject → projectInterface on ParatextProjectDataProvider #2256 can merge first; the other will need a rebase..banded-rowrules) — confirm the reviewer is aware this is build-tool non-determinism, not a meaningful style change. If the project wants deterministic dist builds, that's a separate workflow issue worth filing.buildInfo.jsontypecheck/lint error — flag for the wider team if they hit it in fresh worktrees; not blocking this PR but is a paper-cut for new contributors.Summary
Follow-up PR for the merged markers-checklist work (PR #2219). Addresses TJ Couch's code-review comments (12 items) on the original branch.
The original commits in this PR address TJ's concerns directly; later UX-review work (Sebastian, 2026-05-05) was moved off this branch to its own PR for clarity.
TJ review — 12 comments
Addressed (9 items):
IVersificationService. Investigation confirmed versification IS mutable at runtime: PT9ProjectSettings.Versificationsetter is public + invalidates cache; PT10ParatextProjectDataProvider.SetProjectSettingfiresSendDataUpdateEvent. The service is safe under mutation (fresh read per call) but the docstring lied.SerializedVerseRef?"SerializedVerseRef.ScriptureRange?"ScriptureRange. Wire-compatible (C# DTO already acceptsVerseRef? End).ChecklistComparativeTextRefis now{ id: string }only. C#ComparativeTextRefrecord is(string Id).ChecklistService.ResolveComparativeTextsdoes GUID-only resolution.IChecklistService, allChecklist*types,IVersificationServicemethods, andopenMarkersChecklistSettings. Removed everydata-contracts.md §X.Xreference.lookupFinalVerseNumbersInBook.currentScrRefandrangeStart"rangeStart/rangeEnd/currentScrRefto explain the fallback chain (rangeStart→currentScrRef→ GEN 1:1).@experimental"LinkedScrRefButton@experimentalwith an explicit "to-be-removed when PR #1949 lands" warning.Deferred / pushed back (3 items):
IVersificationService→projectInterfaceon a PDP): Implemented as a separate PR ([P3] markers-checklist: follow-up T1-stage-2 - Versification NetworkObject → projectInterface on ParatextProjectDataProvider #2256) — methods now live directly onParatextProjectDataProviderunder a new'platformScripture.Versification'projectInterface.IChecklistServiceNetworkObject → DataProvider): pushed back with evidence — the service is genuinely stateless RPC, no cached state to invalidate.validateMarkerSettingsas project-setting validator): coupled to a marker-settings-as-project-setting refactor; backlog.getEndVersecallback as data prop): pushed back with cost/benefit — consumer is single-book-at-a-time.Wire compatibility note
The T6 change alters the JSON shape of the
requestedTextsargument toIChecklistService.resolveComparativeTexts(drops thenamefield). The only producer is the markers-checklist web view in this repo, which this PR updates in lockstep. No external consumer is affected.Doc-drift note
.context/features/markers-checklist/data-contracts.md§2.4 still mentions the Name field. That doc lives in ai-prompts; per Rolf's direction we are not opening an ai-prompts PR in this round for that sweep, so it will drift slightly until swept later.Test plan
npm run lint— 0 errors (1 pre-existing warning, unrelated)npm run typecheck— cleannpm test— TS suites pass except the 5 pre-existing storybook failures unrelated to this PRdotnet testinc-sharp-tests/— 706 passed, 0 failed, 6 skippedlib/platform-bible-react/dist/rebuilt and staged🤖 Generated with Claude Code
This change is