PT-4184: Add the verse-aligned Grid view to the Text Collection - #2781
Conversation
jolierabideau
left a comment
jolierabideau
left a comment
There was a problem hiding this comment.
🤖 AI-assisted review summary
Ran a seven-perspective AI review panel (security, scope, contracts, architecture, tests, UX, clarity) against 3a993c5, with PT-4184's acceptance criteria and named requirements (F2/F3/F4, R3/R6/R7/R8, and the F6 banding note) given to each reviewer. Findings below are filtered and reconciled by hand, not pasted raw — where two reviewers disagreed I read the code and adjudicated. Flagging it as AI-assisted so you can weight it accordingly.
Short version: the architecture delivers approach D and the engineering discipline is genuinely strong. Two things block merge and neither is about code quality. Your PR description had already disclosed most of what the panel found, which made this much easier to review.
Blocking
1. The branch doesn't build from the committed manifests.
You flag the pin in "Known limitations", but I think it's worse than "bump both manifests once it publishes":
- npm's latest published
platform-editoris 0.8.15. 0.8.16 isn't on the registry at all. - The PR bumps nothing — no diff to the root manifest, the extension manifest, or
package-lock.json, which still resolvesplatform-editor-0.8.15.tgz.
So npm ci + npm run typecheck fails on the BLOCK_VERSE_VIEW_MODE import (TS2305), and three of upstream-editor-contract.test.ts's six cases go red. Extension tests do run in CI (npm test covers --workspaces --if-present).
One thing I could not determine and am not asserting: whether 0.8.16 actually contains the block-verse work. The only 0.8.16 I can see is a local yalc link built from a sibling scripture-editors checkout that's well behind its remote, and it has neither the constant nor verse-block/data-verse-start/data-verse-end. That's equally explained by a stale local build. Which published version should this pin to?
2. The AC 7 frame-budget check can't fail — scripture-text-grid.spec.ts:791-822.
openAlignedGridWithResources awaits getByTestId('scripture-text-grid-aligned') toBeVisible before returning (scripture-text-grid.page.ts:288). The five [role="region"] columns are AlignedGrid's own children from the same React pass, so the synchronous pre-check at :812 already sees ≥5 and resolves immediately — elapsedMs reads ~0 regardless of real cost. The chapter original works because switchToChapterView() returns before the render.
The pattern was reused structurally; the measurement window was lost. Two safeguards also didn't come across: the 15s give-up setTimeout (:478-483), and the pre-check resolves without observer.disconnect().
Worth a look before merge
- Per-column zoom may defeat alignment. Zoom lands as inline CSS
zoomon[data-cell-content](resource-cell-view.component.tsx:325), whichaligned-grid.styles.ts:82-90makes agrid-template-rows: subgridbox.zoomscales used layout values, so a zoomed column measures against scaled row tracks while its neighbours don't. The ticket's "native subgrid handles this" reasoning holds for differing font sizes — I'm not confident it holds forzoom. You list this as unverified; this seems like the mechanism that makes it a real risk. - Verse-0 on the placement side. AC 3 names it separately.
buildVerseRowRulesstarts at 1, and the guard ataligned-grid.styles.ts:111only catches blocks missingdata-verse-start— so a block withdata-verse-start="0"(or >200) passes the:not()filter, gets nogrid-row, and is auto-placed into the shared grid's first free row: the exact silent misalignment that rule's own comment describes. In practice:127suppresses it wholesale instead, so alignment holds because the content is gone — chapter view keeps that front matter. Inverting the guard to park anything the rules didn't place would close both cases. - Localization immutability.
%webView_scriptureTextGrid_viewOptions_comingSoon%is edited in place inen(:182) andes(:370). The Localization Guide marks modifying existing key/value pairs as CRITICAL — new key +fallbackKeyin themetadatasection (already at:386). Simplest fix is probably reverting the reword, especially since the string is currently unreachable (isChapterEnabledis passed unconditionally atscripture-text-grid.web-view.tsx:549). - Chapter-view drag. You called this out, so just confirming the coverage consequence:
cell-reorder.spec.tsstill passes only because it runs in the default verse view, where the listitem is still draggable. Chapter-mode column drag now has no coverage. Thedata-testid="scripture-text-grid-cell-draggable"also now names an element that's a drop target but not a drag source — worth renaming. - Keyboard operability of the scroll port.
aligned-grid.component.tsx:54has notabIndex, and the read-only editors aren't focusable, so nothing inside the single port takes focus — Arrow/PageDown can't scroll it, and content below the fold is pointer-only. In chapter mode this was per-column; here it's the whole view. - Placeholder cells render off-screen.
grid-row: 1/-1plush-full/justify-centercentres a spinner or "Download failed" at the midpoint of the full chapter height — so one late resource among five looks like a blank column. - The reference-scroll observer watches the root with
subtree: true, callsrequestScroll()per mutation batch unthrottled, and never stands down — a forced sync layout (querySelectorAllover ~176×6 blocks + three rect reads) per batch, across six mutating Lexical editors. Squarely inside what AC 7 is meant to measure. Coalescing into one rAF and disconnecting once applied would help. viewModeis re-spelled as an inline literal union in three places beyond the exportedResourceCollectionViewMode, and persistedviewModeisn't coerced on read — once a user picks Grid,'aligned'is in their saved layout permanently, so a revert or downgrade renders the verse list while handingviewMode="aligned"toResourceCell.
On the coupling question you asked about
You asked whether the contract test is the right guard. I think yes in design, incomplete in coverage: it resolves via createRequire and reads the real installed dist/index.js, and the bundle guard means a missing dist fails loudly rather than vacuously — that's the right shape. But CONTRACT omits the two upstream API symbols the code actually imports (BLOCK_VERSE_VIEW_MODE, getViewOptions), which are the highest-risk part, and resource-cell.component.test.tsx:57-61 mocks the whole module so nothing observes whether they exist. It also omits .editor-placeholder, which :132 selects — an upstream rename there would silently make a read-only grid invite edits. Adding those three would close the gap the docblock claims to close. (Caveat: toContain matches anywhere in the bundle, so a rename that leaves the old string in a comment still passes. Fine for a tripwire.)
Things that are notably well done
- The hidden-view rule is fully honoured — both sanctioned helpers, deferral documented at the sync site with a pointer to the rule file, repeats collapsed, consumed instantly, and backed by a real test that mounts hidden, changes the reference, flips visibility, and asserts the catch-up fires. That's the rule most often skipped.
- The ADR is properly done — correct
LC_ALL=Cslug position, all required fields, alternatives honestly recorded. One inaccuracy: Consequences say an out-of-range verse "would fall outside the explicit grid", but your own stylesheet comment at:106-110says the more accurate and worse thing. - The e2e helper move is byte-for-byte faithful — checked symbol by symbol; additions only, all four consuming specs repointed, no assertion weakened.
- Comment discipline is clean across ~50 new comments — no change narration, no review-finding IDs, no stage tags. One in-PR ticket banner (
scripture-text-grid.spec.ts:703) is the only exception. - Security: zero findings. The generated CSS interpolates only integer-coerced verse numbers, so no selector injection.
useStylesheetis established here (four existing consumers),contentOverflowmatches the ticket's specified shape exactly, andResourceColumnis a net reduction in duplicated drag wiring.
AC status
Cleanly satisfied: 2, 5, 8/R8, 9, F2, F3/R7, F4, R6, scroll-ownership, third-branch. Partial: 3 (verse-0), 4 (locale reversal implicit — no story or assertion; the existing RTL e2e asserts flexDirection, which the grid branch doesn't use), 7 (above). Satisfied but unverifiable without the app: 1, 6. F6 is vacuous — no banding implemented, so nothing can violate the row-index rule yet. R3 still needs Alex's sign-off, and the rule is broader than \s headings — it hides every non-verse-block child, so \d titles and intro material go too.
Also: the ticket's open question about verse-number placement is no longer free — the accessibility story leans on the numbers being inline, so a once-per-row gutter would take it with it.
Generated with Claude Code (Opus 5) and edited by hand before posting. Full report available on request; happy to be wrong on any of these, particularly the zoom/subgrid interaction, which I reasoned about but did not run.
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
Thanks — this was a genuinely useful review, and the two blocking items were both real. Everything below is pushed as 51a0127. I also ran a separate multi-agent review pass on top of yours, which found several more things in the same areas; where that changed an answer you gave me, I've said so.
Blocking
- "The branch doesn't build from the committed manifests… Which published version should this pin to?"
None — the pin isn't where the answer lives. Platform.Bible deliberately builds the editor from scripture-editors' platform-yalc branch rather than from npm releases; that branch's own README says so ("to avoid the overhead of making releases for every little change we need to make to the editor"). dev-packages.json names revision: platform-yalc, and npm ci → postinstall → link-dev-packages checks it out, builds it, and yalc-links it over node_modules. That branch is at ddd55c4, which contains PT-4304 "Add read-only block-verse view mode" (#538) and is versioned 0.8.16 — so the installed editor does have BLOCK_VERSE_VIEW_MODE, verse-block, and both range attributes. CI is green on all three platforms, npm test included. Bumping the manifests to ~0.8.16 would break npm install outright, since that version isn't on the registry: the ~0.8.15 pins are the floor npm resolves before the link replaces it.
Your local 0.8.16 was a stale build — so was mine, which is a good catch in itself; npm run link-dev-packages rebuilt it and the symbols appeared. What I have changed is the failure mode: upstream-editor-contract.test.ts now checks BLOCK_VERSE_VIEW_MODE and getViewOptions in the installed bundle, so a stale or unlinked editor fails with a test that names the missing symbol instead of a TS2305 deep in a build. The ADR now records where the requirement actually comes from, and I'll fix the "bump both manifests once it publishes" line in the PR description, which was wrong.
- "The AC 7 frame-budget check can't fail… the measurement window was lost."
Correct, and worse than you diagnosed: switchToChapterView also ends with an awaited press('Escape'), so the chapter "original" had the same defect. Both tests now arm the measurement before the view switch and read it after (armColumnRenderMeasure / readColumnRenderMs) — the only ordering that actually brackets the render. The 15s give-up is back, every exit path disconnects and clears it, and a give-up now throws rather than returning 15000 as though it were a measurement.
Worth a look
"Per-column zoom may defeat alignment." You identified the mechanism correctly, and the ticket's "native subgrid handles this" reasoning doesn't cover it — differing font sizes and a scaled box aren't the same thing. Rather than argue it, I moved the zoom off that box: ResourceCellView takes a zoomTarget, and in the grid it publishes the factor as --aligned-zoom instead of setting zoom, with the stylesheet zooming the verse blocks. Tracks are never inside a zoomed box, and a row still takes the height of its tallest — now larger — cell. New MixedZoom story so Chromatic sees it, plus a test asserting [data-cell-content] carries no zoom. Still worth your eyes in the app.
"Verse-0 on the placement side… Inverting the guard to park anything the rules didn't place would close both cases." Done that way. Verse blocks are display: none by default and the generated start rule adds display: block alongside grid-row-start, so being placed is what makes a block visible; the old :not([data-verse-start]) rule is gone as a special case. hasAlignableVerse now applies the same test (and searches the whole paragraph subtree), so a chapter of reversed bridges or verses past 200 reports as empty instead of rendering a silent blank column.
"Localization immutability." Straight violation, thank you. Both values restored; the hint reads a new %…_viewOptions_chapterComingSoon%. I did not add the fallbackKey redirect: comingSoon isn't being replaced — its value and meaning are unchanged and still correct as a generic hint — so a fallback pointing at a chapter-specific string would be inert (the old key still resolves) and misleading. It's now unreferenced, which the immutability rule means we keep. Happy to add the metadata entry if you'd rather follow the recipe literally.
"Chapter-view drag… the testid names an element that's a drop target but not a drag source." Renamed: the column is scripture-text-grid-column-drop-target (now present only when it is one) and the header band is scripture-text-grid-column-drag-source; both on the page object. On coverage — chapter-mode column drag is covered at the unit level (the "chapter view reorder" block drags header-r-b onto a column, plus ring, self-hover, clear-on-drop, keyboard). Your underlying point still landed, though: those tests mock ResourceCell and hand-roll the header, so deleting draggable from the real component would leave them green. Added ResourceCellView header drag source tests against the real component.
"Keyboard operability of the scroll port." Added tabIndex={0} plus a focus-visible ring, since it's now the one focusable thing in the view with no control of its own to show focus on. My first justification comment was wrong — there are tab stops inside (grip, zoom kebab); the real reason is that both sit in the sticky header and neither scrolls the port. Comment and test rationale corrected.
"Placeholder cells render off-screen." Fixed — pinned to the top of its column rather than centred over a chapter's height. New ColumnWithNothingToShow story and a stylesheet test.
"The reference-scroll observer… never stands down." Both halves. Batches coalesce into at most one check per animation frame, and once the reader takes the port over the callback returns on a boolean and reads no layout until the reference changes. findVerseBlockForVerse also tries a direct querySelector first, so the common case never collects every block. I kept the observer connected rather than disconnecting, so re-arming is a ref reset rather than a re-subscribe. Related, from my own pass: the "did the reader move it?" heuristic had a real bug — the browser clamps scrollTop when content shrinks (unchecking a resource, zooming a column out), and that was read as the reader, standing the scroll down permanently and killing the late-column correction in exactly the case it exists for. Now compared against the position we wrote, clamped into the currently valid range, with a test.
"viewMode is re-spelled… and persisted viewMode isn't coerced on read." ResourceCollectionViewMode is the single spelling now — ScriptureTextGrid, ResourceCell, ResourceColumn (as Exclude<…, 'verse'>) — derived from a frozen RESOURCE_COLLECTION_VIEW_MODES that also backs a new isResourceCollectionViewMode. The toggle uses the guard instead of its own literal check, and the web view coerces the persisted value through it, falling back to 'verse'. Guard tests added.
The coupling question
"Yes in design, incomplete in coverage… CONTRACT omits the two upstream API symbols… It also omits .editor-placeholder." All three added. The contract is split into a DOM group (checked in the bundle and in our stylesheet, now including editor-placeholder) and an API group (bundle only, since those never appear in CSS). The docblock states your toContain caveat so nobody reads it as stronger than a tripwire. I did not build the structural jsdom test my own pass suggested — mounting the real editor in jsdom to walk the display: contents chain is a bigger investment than this PR should carry, and the e2e geometry test already assertsinto a DOM group (checked in the bundle and in our stylesheet, now including editor-placeholder) and an API group (bundle only, since those never appear in CSS). The docblock states your toContain caveat so nobody reads it as stronger than a tripwire. I did not build the structural jsdom test my own pass suggested — mounting the real editor in jsdom to walk the display: contents chain is a bigger investment than this PR should carry, and the e2e geometry test already asserts that invariant where there's layout. Say the word and I'll file it.
Separately, my own pass found the :has(.verse-block) gate's documented consequence was wrong in a way that matters: a column whose editor produced no verse blocks doesn't degrade to "unaligned" — its paragraphs are grid items of the shared subgrid with an auto row, so they auto-place and stretch rows for every other column too. One misconfigured resource collapsed the whole grid. The gate is gone; an empty column is the recoverable failure, and the contract test names its cause.
Docs and claims
"One inaccuracy: Consequences say an out-of-range verse 'would fall outside the explicit grid'." Corrected — and the placement fix changed the answer anyway: such a verse is now dropped rather than auto-placed, so the paragraph says that, notes the verse stays readable in Verse and Chapter view, and records the zoom placement too.
"R3… the rule is broader than \s headings." True, and neither the comment nor the ADR said so. Both now describe the rule by what it does. R3 sign-off from Alex still outstanding — mine to chase.
"Partial: 4 (locale reversal implicit — no story or assertion)." Added a grid-mode RTL e2e that measures where the first column lands under dir=ltr vs dir=rtl; the chapter test's flexDirection assertion has nothing to say about grid tracks. Still local-only, like the rest of that suite.
"One in-PR ticket banner is the only exception." Dropped the ticket; the description of what the block tests stays.
Two things I'd flag back
- Verse-view keyboard reorder never worked. The grip renders only in ResourceCellView's header-band layout, and verse view uses the inline-name layout — so showDragHandle/onReorderKeyDown are accepted and discarded there. Pre-existing, but this PR asserted the opposite in three places, and the component test's mock renders its own grip regardless of nameDisplay, which is why it looked covered. I corrected the claims (component TSDoc, shortcuts catalog, the mock) rather than adding a visible control to the default view without UX review. Wants a ticket.
- useStylesheet injects from a passive effect, so the grid paints one frame before the layout rules land. Fixing it means touching a shared hook with four existing consumers, so I've left it and am noting it rather than burying it.
Also fixed in passing: two e2e specs (cell-reorder, scripture-text-grid-zoom) were selecting a gridcell role removed in PT-4157, so their assertions ran over an empty list; the per-cell empty message is plain text again, because EmptyState's role="status" gave every cell its own live region and N announcements per reference change; and a genuinely empty chapter is now told it's empty rather than sent to a view with nothing to show either.
Verification: format:check, typecheck (4/4), lint, extension suite 1492/1492, dotnet test 1716 — all clean. Root suite 3538/3540; the two failures are in web-view.component.test.tsx and web-view.service-shard.test.ts, which this branch doesn't touch and which pass in isolation. Still not run: e2e (needs a running app plus E2E_TEST_RESOURCE_IDS) and Chromatic.
@katherinejensen00 made 1 comment.
Reviewable status: 0 of 35 files reviewed, all discussions resolved.
Suggestion: have
|
51a0127 to
5f789e0
Compare
|
@captaincrazybro Good call, and better to fix it here than fork the hook's design. Done in 5f789e0: Behavior is unchanged: the hidden-then-activated catch-up test in On the renames: go ahead on your branch. I'd rather not rename files in this PR after it has been through review. |
Question, not a defect: the grid lands a verse flush at the top, the editor leaves 80px of context above itThanks for taking the One more thing came out of reviewing PT-4543, and this one I genuinely don't know the answer to, so I'd rather ask than assume.
port.scrollTop += block.getBoundingClientRect().top - getFirstVisibleY(port);The Scripture editor's So after the same action — a BCV move in the toolbar — the verse lands at the very top of a Text Collection surface and 80px down in the editor. In Simple mode those are side by side in one window, and PT-4543 extends the grid's behavior to a third surface (the chapter-context pane and chapter columns), so whichever is right, it's about to be more visible. I am not proposing a change here, for two reasons:
What I'd like is for it to be a decision rather than an artifact of the two surfaces having been written separately — mostly so the next person doesn't "fix" one to match the other in either direction. If you're happy with flush-to-top, a line in the ADR saying so would settle it permanently, and I'll point the Text Collection's chapter surfaces at the same answer. |
|
@captaincrazybro A. Keep flush-to-top
B. Share the editor's 80px constant
|
5f789e0 to
43404cb
Compare
|
🤖 Claude: Heads-up for this PR's next rebase. Rolf decided on 2026-09-23, in review of #2844, that the Text Collection's own per-column zoom goes: its level multiplied with the pane's content zoom, so a column could render anywhere from 25 % to 900 % with no control showing the combined size. #2849, which merges after #2825 and #2844, removes it in 141eb15 ( What #2849 removes or changes in files this PR also touches:
What that means for this PR's own additions: the aligned view's per-column zoom has nothing to carry any more. That covers |
PT-4543. Records the approved design before implementation: parameterize #2781's reference-scroll hook by verse-target finder rather than extracting the older settle loop from resource-text-panel.component.tsx, and call it from ResourceCell for viewMode === 'chapter'. Documents why the settle loop was rejected (no reader stand-down, no re-check as content arrives, no shrink-clamp discrimination, a smooth rather than instant hidden-tab catch-up, and success recorded on scrollToVerse's return value even when findScrollContainer found no overflowing container), and why the shared port math needs no per-layout branch: getFirstVisibleY looks up [data-cell-header] inside the port, which is correctly empty in chapter mode because the header band is the port's sibling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nine fixes from a review pass over this branch, plus the tests and records that pin them. Correctness: - CSS `zoom` sat on `[data-cell-content]`, which this branch makes the scroll port, so `scrollPortToBlock`'s viewport-space delta was applied to a zoom-scaled `scrollTop`. Measured in Chromium: at 2x the port overshot by ~7800px and oscillated without converging; at 0.5x it landed a pane short. The factor moves to `[data-cell-pad]`, leaving the port unzoomed. Rendering measures identical at every factor. - `isBlockInPortView` had been forked on target height, which silently gave the aligned Grid view full-containment semantics: a partly visible verse block a reader clicked was scrolled to the top of the port. The overlap rule is restored and the containment rule moves to `isMarkerFullyInPortView`, injected as `isTargetVisible`. `aligned-grid.component.tsx` is untouched, so the Grid view keeps exactly the behavior PT-4184/#2781 reviewed. - A chapter cell now skips the scroll-group echo of a reference it published itself, reusing `isEchoOfPublishedScrRef`. The docstring's claim that a block anchor needs no latch holds; a one-line marker anchor does need one, because a click deep inside a long verse publishes a verse whose marker is above the fold. - Chapter cells scroll with `VERSE_NUMBER_SCROLL_OFFSET`, the lead-in the editor, the model text panel and the reference panels already share, instead of parking the verse flush against the pane's top border. - The single-resource cell is width-filling again in its non-ready states; it had become a shrink-to-fit flex item, so a short placeholder rendered at the message's width rather than the pane's. CI and contracts: - `isViewVisible` was a required prop with 71 call sites never updated, so `npm run typecheck` failed with 5 errors from the stories alone. - `AlignedGrid` took the prop instead of opening a second `IntersectionObserver` for an answer its parent already held. - `data-marker` and `data-number` are registered in the upstream editor DOM contract; the marker selector is composed from `VERSE_MARKER_SELECTOR` rather than respelled; `VerseTargetFinder`'s port narrows to `HTMLElement`. - The `main.ts` comment no longer claims pinning would overwrite a power-mode choice, which the two pinners it cites cannot do. Tests: - `ResourceCell`'s chapter wiring had no coverage at all. Five cases pin the port identity, the view-mode gate, an end-to-end scroll, and the echo latch. - A parity table pins both finders against each other so Grid and Chapter cannot drift on which verse a reference resolves to. - `[data-cell-header]` staying outside `[data-cell-content]` is now asserted on the rendered component; the ADR had claimed this was pinned when only the arithmetic was. - Every new test was checked by mutation: each fails when the behavior it pins is removed. PT-4543. Session-URL: https://claude.ai/code/54702569-a224-47cb-a68c-76051ebac749 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I say this is fine. Go ahead and keep flush-to-top. |
|
Found these while reviewing PT-4543 (#2856), which is stacked on this branch and reuses the scroll 1. A verse-0 reference scrolls to the wrong row when columns start at different verses
The "reference above every block" fallback returns const blocks = [...port.querySelectorAll<HTMLElement>('.verse-block[data-verse-start]')].filter(isPlacedBlock);
const [firstBlock] = blocks;
if (!firstBlock) return undefined;
if (!Number.isFinite(verseNum)) return firstBlock; // ← same element
...
return best ?? firstBlock; // ← hereRepro: a commentary covering only MAT 5:10-12 in the left column, a full text starting at verse 1 The single-column fixture in Reproduced against the shipped function in jsdom. Lowest- 2. The scroll-clamp guard reads inverted, and can stand the sync down permanently
const expected = maxScrollTop > 0 ? Math.min(applied, maxScrollTop) : applied;The comment says the unclamped branch exists so a port with nothing to scroll does not read this Flagging this as plausible rather than confirmed: in a real browser Verification that came out cleanSince the PR body says this has not been run in the app, I rendered the generated
Two stale items in the PR body
Non-blocking observation: AI-assisted review (Claude Code). Findings above were reproduced by running the code rather than |
43404cb to
88f31c6
Compare
|
@captaincrazybro Thanks, both findings were real. Fixed in 88f31c6, which sits on a fresh rebase onto 1. Verse 0 with columns that start at different verses. Fixed. When the reference is above every block, or is non-finite, 2. The clamp guard. You were right that it can happen outside jsdom. Uncheck resources until what's left fits the port, and the browser clamps Flush-to-top. Thanks for settling it. I added it to PR body. Both stale items are fixed: the pin checklist item is struck, and the
@rolfheij-sil Thanks for the heads-up. #2849 hasn't merged yet (it's stacked on #2844 → #2825), and the per-column zoom it removes is still on Verification on the rebased head: |
|
Re-reviewed at
One new finding:
|
…zoom scope and area label - ADR adr-text-collection-resources-are-zoom-areas: one content zoom area per resource (resource-<id>, text-collection fallback), the zoom scope and area label attributes, the restored menus on the platform commands, and the consequences (memory per project x resource, keys on one resource, no migration of the per-column levels, the #2781 path). - Amendments to adr-resource-panes-name-their-zoom-areas (the grid zooms per resource again, as areas) and adr-zoom-areas-mark-project-text (a zoom scope decides for an unmarked target inside it). - Component-Builder-Patterns 1.7.8 and Extension-Development-Guide 1.1.7: when to label an area and when to put a zoom scope on a container. Part of PT-4585 (Text Collection per-resource zoom areas). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KTCXDyYLo9NMf8wGoDdWSc
PT-4543. Records the approved design before implementation: parameterize #2781's reference-scroll hook by verse-target finder rather than extracting the older settle loop from resource-text-panel.component.tsx, and call it from ResourceCell for viewMode === 'chapter'. Documents why the settle loop was rejected (no reader stand-down, no re-check as content arrives, no shrink-clamp discrimination, a smooth rather than instant hidden-tab catch-up, and success recorded on scrollToVerse's return value even when findScrollContainer found no overflowing container), and why the shared port math needs no per-layout branch: getFirstVisibleY looks up [data-cell-header] inside the port, which is correctly empty in chapter mode because the header band is the port's sibling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nine fixes from a review pass over this branch, plus the tests and records that pin them. Correctness: - CSS `zoom` sat on `[data-cell-content]`, which this branch makes the scroll port, so `scrollPortToBlock`'s viewport-space delta was applied to a zoom-scaled `scrollTop`. Measured in Chromium: at 2x the port overshot by ~7800px and oscillated without converging; at 0.5x it landed a pane short. The factor moves to `[data-cell-pad]`, leaving the port unzoomed. Rendering measures identical at every factor. - `isBlockInPortView` had been forked on target height, which silently gave the aligned Grid view full-containment semantics: a partly visible verse block a reader clicked was scrolled to the top of the port. The overlap rule is restored and the containment rule moves to `isMarkerFullyInPortView`, injected as `isTargetVisible`. `aligned-grid.component.tsx` is untouched, so the Grid view keeps exactly the behavior PT-4184/#2781 reviewed. - A chapter cell now skips the scroll-group echo of a reference it published itself, reusing `isEchoOfPublishedScrRef`. The docstring's claim that a block anchor needs no latch holds; a one-line marker anchor does need one, because a click deep inside a long verse publishes a verse whose marker is above the fold. - Chapter cells scroll with `VERSE_NUMBER_SCROLL_OFFSET`, the lead-in the editor, the model text panel and the reference panels already share, instead of parking the verse flush against the pane's top border. - The single-resource cell is width-filling again in its non-ready states; it had become a shrink-to-fit flex item, so a short placeholder rendered at the message's width rather than the pane's. CI and contracts: - `isViewVisible` was a required prop with 71 call sites never updated, so `npm run typecheck` failed with 5 errors from the stories alone. - `AlignedGrid` took the prop instead of opening a second `IntersectionObserver` for an answer its parent already held. - `data-marker` and `data-number` are registered in the upstream editor DOM contract; the marker selector is composed from `VERSE_MARKER_SELECTOR` rather than respelled; `VerseTargetFinder`'s port narrows to `HTMLElement`. - The `main.ts` comment no longer claims pinning would overwrite a power-mode choice, which the two pinners it cites cannot do. Tests: - `ResourceCell`'s chapter wiring had no coverage at all. Five cases pin the port identity, the view-mode gate, an end-to-end scroll, and the echo latch. - A parity table pins both finders against each other so Grid and Chapter cannot drift on which verse a reference resolves to. - `[data-cell-header]` staying outside `[data-cell-content]` is now asserted on the rendered component; the ADR had claimed this was pinned when only the arithmetic was. - Every new test was checked by mutation: each fails when the behavior it pins is removed. PT-4543. Session-URL: https://claude.ai/code/54702569-a224-47cb-a68c-76051ebac749 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…zoom scope and area label - ADR adr-text-collection-resources-are-zoom-areas: one content zoom area per resource (resource-<id>, text-collection fallback), the zoom scope and area label attributes, the restored menus on the platform commands, and the consequences (memory per project x resource, keys on one resource, no migration of the per-column levels, the #2781 path). - Amendments to adr-resource-panes-name-their-zoom-areas (the grid zooms per resource again, as areas) and adr-zoom-areas-mark-project-text (a zoom scope decides for an unmarked target inside it). - Component-Builder-Patterns 1.7.8 and Extension-Development-Guide 1.1.7: when to label an area and when to put a zoom scope on a container. Part of PT-4585 (Text Collection per-resource zoom areas). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KTCXDyYLo9NMf8wGoDdWSc
88f31c6 to
3544fac
Compare
|
@captaincrazybro Thanks. The Label assertions. I took your third option. The two you left unfiled. I agree with leaving both as they are:
Rebase notes. There were two conflicts, both in Verification on the rebased head:
AI-assisted (Claude Code). |
PT-4543. Records the approved design before implementation: parameterize #2781's reference-scroll hook by verse-target finder rather than extracting the older settle loop from resource-text-panel.component.tsx, and call it from ResourceCell for viewMode === 'chapter'. Documents why the settle loop was rejected (no reader stand-down, no re-check as content arrives, no shrink-clamp discrimination, a smooth rather than instant hidden-tab catch-up, and success recorded on scrollToVerse's return value even when findScrollContainer found no overflowing container), and why the shared port math needs no per-layout branch: getFirstVisibleY looks up [data-cell-header] inside the port, which is correctly empty in chapter mode because the header band is the port's sibling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nine fixes from a review pass over this branch, plus the tests and records that pin them. Correctness: - CSS `zoom` sat on `[data-cell-content]`, which this branch makes the scroll port, so `scrollPortToBlock`'s viewport-space delta was applied to a zoom-scaled `scrollTop`. Measured in Chromium: at 2x the port overshot by ~7800px and oscillated without converging; at 0.5x it landed a pane short. The factor moves to `[data-cell-pad]`, leaving the port unzoomed. Rendering measures identical at every factor. - `isBlockInPortView` had been forked on target height, which silently gave the aligned Grid view full-containment semantics: a partly visible verse block a reader clicked was scrolled to the top of the port. The overlap rule is restored and the containment rule moves to `isMarkerFullyInPortView`, injected as `isTargetVisible`. `aligned-grid.component.tsx` is untouched, so the Grid view keeps exactly the behavior PT-4184/#2781 reviewed. - A chapter cell now skips the scroll-group echo of a reference it published itself, reusing `isEchoOfPublishedScrRef`. The docstring's claim that a block anchor needs no latch holds; a one-line marker anchor does need one, because a click deep inside a long verse publishes a verse whose marker is above the fold. - Chapter cells scroll with `VERSE_NUMBER_SCROLL_OFFSET`, the lead-in the editor, the model text panel and the reference panels already share, instead of parking the verse flush against the pane's top border. - The single-resource cell is width-filling again in its non-ready states; it had become a shrink-to-fit flex item, so a short placeholder rendered at the message's width rather than the pane's. CI and contracts: - `isViewVisible` was a required prop with 71 call sites never updated, so `npm run typecheck` failed with 5 errors from the stories alone. - `AlignedGrid` took the prop instead of opening a second `IntersectionObserver` for an answer its parent already held. - `data-marker` and `data-number` are registered in the upstream editor DOM contract; the marker selector is composed from `VERSE_MARKER_SELECTOR` rather than respelled; `VerseTargetFinder`'s port narrows to `HTMLElement`. - The `main.ts` comment no longer claims pinning would overwrite a power-mode choice, which the two pinners it cites cannot do. Tests: - `ResourceCell`'s chapter wiring had no coverage at all. Five cases pin the port identity, the view-mode gate, an end-to-end scroll, and the echo latch. - A parity table pins both finders against each other so Grid and Chapter cannot drift on which verse a reference resolves to. - `[data-cell-header]` staying outside `[data-cell-content]` is now asserted on the rendered component; the ADR had claimed this was pinned when only the arithmetic was. - Every new test was checked by mutation: each fails when the behavior it pins is removed. PT-4543. Session-URL: https://claude.ai/code/54702569-a224-47cb-a68c-76051ebac749 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@katherinejensen00 — the grid's reference scroll misses once the Text Collection pane is zoomed. #2844 (now on Cause. #2844 wraps the grid body in port.scrollTop += block.getBoundingClientRect().top - getFirstVisibleY(port) - leadInPx;Rects are in screen pixels and include every ancestor's zoom. Measured in Chromium 145 with the same DOM shape and the code's formula. The number is the gap between the verse and the top of the port after each attempt (0 = correct):
At 1.5× the scroll overshoots and slowly settles. At 2× it bounces forever. Above 2× it grows until it hits the end of the content. At 100% nothing shows, so normal testing misses it. Suggested fix, in the one shared function. Measuring the scale directly covers zoom from any ancestor without reading the custom property: const scale = port.getBoundingClientRect().height / port.offsetHeight || 1;
port.scrollTop += (block.getBoundingClientRect().top - getFirstVisibleY(port)) / scale - leadInPx;( #2856 (stacked here) calls the same function for the chapter cells, so one fix here covers both. Caveat: this was measured on a synthetic page, not the running app. To confirm in the app, zoom the Text Collection pane to 200% in Grid view and change the verse. |
|
@captaincrazybro Thanks, this one was real. Fixed in cf62a6f. The fix is in the one shared function, so #2856's chapter cells get it too. I took your suggestion and measure the scale off the port itself: function getViewportPxPerLayoutPx(port: HTMLElement): number {
return port.offsetHeight > 0 ? port.getBoundingClientRect().height / port.offsetHeight : 1;
}
Tests. There's a new Verification: the AI-assisted (Claude Code). |
…zoom scope and area label - ADR adr-text-collection-resources-are-zoom-areas: one content zoom area per resource (resource-<id>, text-collection fallback), the zoom scope and area label attributes, the restored menus on the platform commands, and the consequences (memory per project x resource, keys on one resource, no migration of the per-column levels, the #2781 path). - Amendments to adr-resource-panes-name-their-zoom-areas (the grid zooms per resource again, as areas) and adr-zoom-areas-mark-project-text (a zoom scope decides for an unmarked target inside it). - Component-Builder-Patterns 1.7.8 and Extension-Development-Guide 1.1.7: when to label an area and when to put a zoom scope on a container. Part of PT-4585 (Text Collection per-resource zoom areas). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KTCXDyYLo9NMf8wGoDdWSc
…zoom scope and area label - ADR adr-text-collection-resources-are-zoom-areas: one content zoom area per resource (resource-<id>, text-collection fallback), the zoom scope and area label attributes, the restored menus on the platform commands, and the consequences (memory per project x resource, keys on one resource, no migration of the per-column levels, the #2781 path). - Amendments to adr-resource-panes-name-their-zoom-areas (the grid zooms per resource again, as areas) and adr-zoom-areas-mark-project-text (a zoom scope decides for an unmarked target inside it). - Component-Builder-Patterns 1.7.8 and Extension-Development-Guide 1.1.7: when to label an area and when to put a zoom scope on a container. Part of PT-4585 (Text Collection per-resource zoom areas). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KTCXDyYLo9NMf8wGoDdWSc
Content zoom now scales only project text. Following the UX review on the content-zoom epic (PT-4575), the feature is narrowed to do one job: make small project text easier to read. Views that show no project data (Home, New Tab, Get resources, Manage books, Registration, Marketplace, the Send/Receive dialog, URL views) are no longer zoomed and no longer offer zoom. In the views that do zoom — Scripture editor, Comments, Text Collection, Enhanced Resources, Find, the four inventories, Checks, the Markers Checklist and the Lexical Tools dictionary — only the text grows; buttons, inputs, filters, headers, card frames and every pop-up keep interface scale. - Framework: each first-party zoomable web-view type is declared with a kind and default zoom area; a pane is zoomable only when declared or actively reporting an area (the old whole-iframe fallback is gone). Project text is marked directly (ContentZoomRoot, ContentZoomTextProvider) instead of by region. A new data-platform-content-zoom-scope attribute lets an unscaled container (a row, column or card) route clicks/wheel to its area without itself scaling, and a label prop names the area in the on-screen zoom-level badge. - Text Collection: each resource is its own zoom area again — Ctrl+wheel, pinch, the right-click menu and the chapter view's "⋮" zoom one resource at a time; the shipped menu strings are restored (no longer deprecated). A resource's level persists per project and follows "Tab content default zoom" until zoomed individually. - Resource panes: only project text is marked inside each — the Scripture editor's text tree (character-marker bar and empty-state views excluded), Comments' snippets/bodies/diffs, Text Collection's per-resource cells inside the scroll box, Enhanced Resources' entry text, Find's results/previews, the inventories' item columns, Checks' checked text, and the Markers Checklist's marker tokens. - Settings: "Tab content default zoom" and "Interface scaling" both use a shared percentage stepper (50-300%) that wraps to two rows instead of clipping in a narrow pane, and accepts a typed percentage (Enter/blur commits, Escape abandons); the stored values (factor, level) are unchanged. - Pop-ups fixed at interface scale: context menus, popovers, tooltips, the marker/footnote palettes and the inline footnote/comment editors no longer scale with content zoom; the area-following pop-up code and the whole-iframe fallback are removed. - Docs/ADRs: new entries record that content zoom applies only to zoomable panes, that pop-ups stay at interface scale, that zoom areas mark project text, and that Text Collection resources are their own zoom areas; superseded/withdrawn entries are marked, not deleted. The Extension Development Guide and Component Builder Patterns document the opt-in for third-party views. Deferred: Word List and Compare Versions are not yet declared zoomable — their text markers ship in follow-up PRs in paratext-bible-extensions and paratext-bible-internal-extensions. Eight pre-existing, zoom-unrelated Text Collection grid e2e failures are tracked as PT-4779. PR #2781 (verse-aligned Grid view) needs a rebase onto this change. Known, not caused by this PR and not fixed here: Ctrl+wheel over the Scripture editor's character-marker bar or the blank space below a short chapter, and Enhanced Resources' entries area while its text is unmounted, target the last-used zoom area rather than the one under the pointer (accepted as documented behavior). In Text Collection, focus returning to the View Options button after Escape can leave its tooltip covering a neighboring control until focus moves elsewhere; poetry lines collapse at high zoom in a narrow column (a pre-existing viewport-unit issue in the editor stylesheet, not specific to Text Collection); and a zoom menu's enabled/disabled state can trail the visible level by up to ~250ms. Hidden tabs: no catch-up logic was needed. Zoomability and zoom levels are pushed as CSS variables and a MutationObserver-driven event, both of which need no layout, so an inactive tab is already correct by the time it's shown; only the on-screen level badge needs geometry, and it only appears for a direct user action on a visible pane. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KTCXDyYLo9NMf8wGoDdWSc Claude-Session: https://claude.ai/code/session_01MddYES5CZ1wg3CjnyakeXw Claude-Session: https://claude.ai/code/session_017NQ6fkAtScn8yARdcQr6ez
captaincrazybro
left a comment
captaincrazybro
left a comment
There was a problem hiding this comment.
Approving. Everything raised in review is fixed or deliberately deferred, and I re-checked the fixes
rather than just the replies.
- Verse-0 / non-finite fallback and the clamp guard — both correct, both tested, and the
rewritten comments now describe what the code does. cell-reorder.spec.ts—getCellResourceIdsreadsdata-resource-idand all seven
assertions compare ids; no name-basedtoBe('Resource …')remain. Reading the id rather than a
substring of a localized template is the right call.- Zoom conversion — I ran the shipped implementation against the same harness that found the
bug, with a bordered port so theclientTopscaling is exercised. Gap is 0 at 1.5×, 2× and 3×
with zoom above the port, below it, and both stacked; the "both stacked" case previously read
−7459 and never settled.
Two improvements over what was suggested, worth noting: the offsetHeight > 0 guard instead of
|| 1 is genuinely better, since a stubbed rect over a zero offsetHeight yields Infinity rather
than NaN and would have collapsed the scroll to 0; and scaling clientTop inside
getFirstVisibleY caught a unit mix that wasn't in the report.
Deferrals I agree with: the noVersesToShow wording belongs with UX under the string-immutability
rule, and columnDropTarget / columnDragSource being ahead of a chapter-drag spec is fine.
What this approval does not cover: neither of us has run the Grid view in the app. Your own
"200% Grid-view repro" is still outstanding, and the updated cell-reorder.spec.ts hasn't been run
(local-only, needs a running app and E2E_TEST_RESOURCE_IDS). Both are worth doing before merge —
the zoom class of bug is invisible at 100%, which is how it survived this long.
AI-assisted review (Claude Code); the measurements above are from headless Chromium, not inferred.
A third VIEW mode where verse N of every resource shares a row, so several translations can be read in parallel down a passage. Verse and Chapter are unchanged. Layout. Alignment is native: the grid root owns a single scroll port and the row axis, and a `display: contents` / subgrid chain carries that axis down to the editor's block-verse layout. Nothing measures anything, so a row is as tall as its tallest cell even when per-resource zoom puts columns at different sizes. The chain flattens three elements this repo does not own (`.editor-container`, `.editor-inner`, `.editor-input`), because one editor per column is what keeps selecting and copying a passage down a column working; adr-aligned-grid-flattens-the-editor-dom records why. A contract test reads the installed editor bundle and fails by name if a class or export the layout depends on disappears, since a rename would otherwise break alignment silently. Placement is opt-in. Each verse block is placed by an explicit `grid-row` from its own `data-verse-start`/`-end` (so a bridge spans its rows and a missing verse cannot shift the rows below it), and a block the rules cannot place is hidden rather than auto-placed into the shared grid. Row lines are local to the content subgrid below the sticky header. Everything between verse blocks — section headings, chapter descriptions — is hidden in this view; rows are a visual relationship, not ARIA table semantics. Both are recorded in the ADR. Zoom. Each resource stays its own content-zoom area (PT-4585's model): its text is marked with `ContentZoomRoot`, and the column carries the zoom scope. In this view the marker sits inside the subgrid chain, so the aligned stylesheet makes it `display: contents` and pins its own zoom to 1, and the verse blocks take the area's level from the platform's `--platform-content-zoom-<area>` variable instead. Zooming the marker or the content wrapper would scale the shared row tracks with the text. Checked in Chromium: rows stay aligned with one column at 150 %, and the level applies once, not squared. Reference sync scrolls explicitly (Lexical skips the scroll-into-view for a read-only editor) and flush under the sticky header. It leaves a verse already on screen alone, re-checks as late columns arrive, stands down once the reader scrolls until the reference changes, tells a browser clamp after content shrinks from the reader, and converts rect distances into the port's layout pixels under an ancestor zoom. It defers while the tab is hidden and catches up on activation, per .claude/rules/cross-view-sync-hidden-views.md. Cells: a column whose chapter has no verses to align says so instead of rendering blank, placeholders span the column, the header band (not the whole column) is the reorder drag source so text selection keeps working, and the reorder wiring is shared between the chapter row and the Grid view through `ResourceColumn`. The keyboard shortcut catalog notes reorder applies here too. Sub-verse markers (\v 3a, \v 3b) collapse to one row; filed upstream as PT-4559. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KHpKaxZxC4sBAAXQFVKEmp Claude-Session: https://claude.ai/code/session_01STzLD6CiarMZUP37MVxYc5
cf62a6f to
9151195
Compare
PT-4184's verse-aligned Grid view (#2781) added resource-column.component.tsx, importing GridResource from resource-cell.component. This branch moved that type to grid-resources.utils, so the merge failed typecheck:workspaces with TS2614 even though neither side did alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4 participants
Adds a third VIEW mode to the Text Collection — Grid — where verse N of every resource sits on
one row, so translations can be read in parallel down a passage. Verse and Chapter are unchanged.
The one idea
Rows are CSS grid tracks on the grid root, shared by every column. A
display: contents/subgridchain carries that row axis down through each column to the verse-block elements theeditor produces upstream (PT-4304), and each
block is placed on an explicit
grid-rowderived from its owndata-verse-start/-end.Two consequences worth holding onto while reading:
per-resource zoom needs no special handling.
elements owned by
@eten-tech-foundation/platform-editor. That coupling is the main thing toscrutinize; it exists because the view keeps one editor per column so a reader can select and
copy a passage down it.
Where to look, in review order
aligned-grid.styles.tsaligned-grid.component.tsxuse-aligned-reference-scroll.hook.tsresource-column.component.tsxscripture-text-grid.component.tsxviewModeseam — a third branch, not a variant of the flex ones.resource-cell{,-view}.component.tsxupstream-editor-contract.test.tsaligned-grid.component.stories.tsxadr-aligned-grid-flattens-the-editor-domWhat to scrutinize
breaks alignment silently — the grid still renders, just wrong — so the guard is a contract test
that reads the installed editor bundle. Is that the right guard?
draggablehijacks click-dragtext selection in Chromium. This changes the chapter view too — intentional, worth confirming.
(clicking a verse reports it as the new reference), it re-checks as slower columns arrive, and it
stands down once the reader scrolls. The "did the reader move it?" test compares
scrollToptowhat the hook last wrote — a heuristic, with
overflow-anchor: noneto stop the browser tripping it.horizontal scroll, and a chapter with no verses all deserve eyes.
Known limitations
\v 3aand\v 3bboth resolve to row 3 and paint on top of eachother. No core-side fix exists — filed as
PT-4559 for an upstream change.
deliberate, both recorded in the ADR with the reasoning and the path to revisiting.
Testing
npm run typecheck(4/4),npm run lint, andnpm testall pass — 88 files / 1461 tests in thisextension. Note the root lint run ignores
extensions/, so these files were also linted with theextensions/config directly; that is what caught two type-assertion errors.Not run: e2e (needs a running app plus
E2E_TEST_RESOURCE_IDS) and Chromatic.Full review summary — findings, decisions, and suggested agenda
Findings
Critical — Must address before merge
None.
Important — Should address before merge
verse to the top, including changes the grid itself originates — clicking or selecting text in
a column reports a new reference, so a click moved the passage under the cursor. (fixed during
review: a verse already on screen is left alone, the rule
useBcvSyncScrollalready implementsfor the comment list)
the target back off screen with no correction. (fixed during review: re-checked as columns
arrive; stops once the reader moves the port themselves, until the reference changes)
scrollTopon the grid root, which the "did the readerscroll?" check read as the reader — firing exactly in the late-column case it exists for.
(fixed during review:
overflow-anchor: noneon the port)selection — the capability one-editor-per-column exists to protect. (fixed during review: the
header band is the drag source; the column stays the drop target. Applies to chapter view too,
by decision)
npm run lintdid not cover these files. The root ESLint run ignoresextensions/bydefault, so two
no-type-assertionerrors in new test code went unreported. (fixed duringreview: real
DOMRectinstances instead ofas DOMRectliterals; verified with theextensions-scoped config)
.editor-inputcarriescounter-reset: caller crossref, anddisplay: contentsdestroys that scope. (fixed duringreview: re-homed onto the per-column content wrapper)
and the call violated
resolveDisplayVerseNum's documented "chapter surfaces must not callthis". (fixed during review: the raw verse is used, plus versification in the key)
data-verse-startfor a reversedbridge (
3-1); such a block matched no row rule and was auto-placed into the first free row ofthe shared grid. (fixed during review: those blocks are dropped, which keeps the rest of the
column aligned; the verse is still readable in Verse and Chapter view)
the reader cannot see. (fixed during review: gated on the verse view)
review: keyboard-move, header-drag, and drop-target tests in the aligned block)
implementations of one behavior. (fixed during review: it consumes
buildReorder)EmptyStateexists, and so lacked therole="status"a zero state should have. (fixed during review)was appended after the disabled Chapter item — the component default, and what the stories
show. (fixed during review: the hint names the mode it describes)
Only the e2e geometry test validates against the real editor DOM, and it is CI-skipped, so(fixed during review: a newan upstream rename would leave the other guards green.
upstream-editor-contract.test.tsreads the installed editor bundle and fails when a class orattribute the layout depends on is gone — and asserts our stylesheet still names it)
The editor pin does not express what the code needs.(moot since the rebase ontomain: both manifests depend on the staged
file:dev-packages/staging/platform-editorbuild (0.8.16), so there is no registry pin to bump — see
adr-dev-packages-staged-file-deps)Minor — Consider
repeat(0, …)is invalid CSS and the web view briefly passes an empty resource list (fixedduring review: the track count floors at one and is counted from the columns rather than taken
as a prop that could disagree with them)
comment tying the two spellings together)
overflow-wrap, so a long unbreakable run could paint over the next column (fixed duringreview)
scrollPortToBlockdropped theclientTopsubtraction thatgetTopWithinScrollContainermakes, which matters because this view allows an external border (fixed during review)
its assertion (fixed during review: a bounded wait, and the observer disconnects on every
path)
emptyMessageconflated "should this be empty?" with "did the string resolve?" (fixed duringreview: falls back to the key)
review: reworded, and it now says what to do next)
stg-alignedcoined an abbreviation used nowhere else in the repo, in the DOM and three testfiles (fixed during review: spelled out)
hasAlignableVerse's table/sidebar claim was untested (fixed during review)greek,cpb) rather than what eachdemonstrates (fixed during review)
locationspointed at a file with no keyboard handler (fixed duringreview)
Per-column zoom in a view whose purpose is alignment(measured at 1.5× in Chromium duringdesign: rows stayed aligned, because the row is sized by its tallest cell. Kept — an
original-language column often needs a larger font. Worth confirming in the app.)
ResourceColumnhas no storystories, components that render the PAPI-connected
ResourceCelldo not)Grid view always scrolls sideways below a 300px pane(inherent to a column-per-resourcelayout, and the same floor the chapter view already uses; the root is still the single scroll
port)
"Grid" names a layout while Verse and Chapter name a unit of scripture(author chose Gridover Aligned and Parallel; internal value stays
aligned).styles.tsis a new file-name suffix in this directoryand
.const.ts/.utils.tswould describe it less well)\v 3aand\v 3bboth resolve todata-verse-start="3", soboth blocks land in one grid area and paint over each other. No CSS-only fix exists — stacking
would need a per-row wrapper the editor does not emit. Filed as PT-4559 (upstream fix);
Grid view renders overlapping text for those verses until then.
reachable in Verse and Chapter view). Deliberate — recorded in the ADR with the path to
spanning them across a full-width row later.
would need a row-major DOM, which one-editor-per-column rules out. Verse numbers at the start
of each block are what let a screen-reader user correlate columns. Flagged for AT validation.
Template Propagation
Shared Regions Modified
None. No changed file contains a
#region shared withmarker.Extension Config Changes
None. No
package.json,tsconfig*.json,.eslintrc*,webpack.config.*,.erb/configs/, or.github/workflows/file was touched.Positive Observations
.claude/rules/cross-view-sync-hidden-views.md) is satisfied end to end —useViewVisibility+useRunWhenVisible, a comment at the sync site, and the catch-up test therule asks for (mount hidden, change the reference, flip visibility).
ResourceColumnis a genuine deduplication: the chapter row's inline region/drag markup and thenew aligned column are now one component, with the layout difference isolated to two props.
editor bundle, a story that reproduces the markup for Chromatic, and an e2e test that measures real
block geometry with a positive control so it cannot pass vacuously.
declared row count ≥ emitted rules ≥ Psalm 119's 176 verses.
enandes, registered in the frozen key lists, andnow covered by the parity test.
slug byte order, and referenced from the code by slug rather than restated.
Interview Notes
Stated purpose: add the verse-aligned Grid view to the Text Collection so verse N of every
resource lines up on one row.
Decisions the author made during review:
fix, and a JS pass would fight the editor's DOM.
:has()gate, then the gate was removed — an empty columnis the recoverable failure (see the ADR).
label with
alignedas the internal value, and one PR rather than staged ones.question, and both are now recorded as settled decisions in the ADR.
Verification note for the reviewer: the grid component test mocks
useStylesheet— thestylesheet is inert under jsdom (no layout) — and its content is asserted directly in
aligned-grid.styles.test.ts.Verified outside the app (2026-09-23, @captaincrazybro in review): the generated stylesheet was
rendered in headless Chromium and measured — row alignment through the
display: contents/subgridchain, bridged and verse-200 placement, hidden unplaceable blocks, the sticky header (including under
horizontal scroll), mixed-zoom alignment, placeholder columns, and
scrollTopsurvivingdisplay: none.Not verified in the running app: none of this has been exercised in Platform.Bible itself. The e2e tests that
measure real geometry are local-only and need
E2E_TEST_RESOURCE_IDS; the Storybook stories have notbeen rendered. The Chromatic snapshots are unconfirmed.
In-Review Quality Check
npm run typecheck— all four projects clean.npm run lint— clean. Also ran ESLint with theextensions/config, which the root runignores; that is what surfaced the two type-assertion errors, now fixed.
npm test— 245 core files and every workspace pass; the extension's own suite is 88 files /1461 tests. Exit 0.
Suggested Review Focus
display: contents. Is the contract test the right guard, and is anything else lost byflattening beyond the padding and counter scope already accounted for?
behavior there, not just in Grid.
as columns arrive, stand down once the reader scrolls) — the "did the reader move it?" check is
a heuristic comparing
scrollTopto what the hook last wrote.under horizontal scroll, and a chapter with no verses are all worth a look.
~0.8.16before or at merge.the limitation is acceptable meanwhile.
AI-assisted — session
This change is