PT-4543: Scroll Text Collection chapter views to the reference - #2856
captaincrazybro wants to merge 18 commits into
Conversation
898e80f to
2b52631
Compare
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. 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 verse blocks PT-4304 added upstream. Nothing measures anything, so a row is as tall as its tallest cell even when per-resource zoom puts columns at different sizes. Each verse block is placed by an explicit `grid-row` derived from its own `data-verse-start`/`-end`. Subgrid alone lays blocks out in document order, so a resource missing a verse would shift every row below it; explicit rows also give a bridged verse (`14-15`) its two rows from one node. Row lines are local to the content subgrid, which starts below the sticky header — counting the header there shifts every verse by one and pushes a chapter's last verse off the explicit grid, where columns stop sharing rows. The chain has to flatten three elements this repo does not own (`.editor-container`, `.editor-inner`, `.editor-input`), because AC6 wants one editor per column rather than one per cell. Verified by measuring block geometry across columns in a real browser before writing any of this, and pinned by an e2e test that measures the running app; a unit test pins the selectors, since a rename upstream would break alignment silently rather than loudly. Reference sync scrolls explicitly: the layout is read-only, and Lexical skips the DOM-selection write — which is where scroll-into-view lives — for a read-only editor. The scroll defers while the tab is hidden and catches up on activation, per .claude/rules/cross-view-sync-hidden-views.md. Section headings are suppressed in v1 (they are translation-specific and disagree across columns; the model still carries them, so showing them later is a view-layer change). Needs Alex's sign-off. Depends on the block-verse view mode published as platform-editor 0.8.16; the pins here still say ~0.8.15 and must move once it is on npm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KHpKaxZxC4sBAAXQFVKEmp
…he reader Review of the previous commit, plus the structural and documentation work it needed. Defect: any reference change scrolled the target verse to the top of the port, including the changes this grid itself originates. Clicking or selecting text in a column reports a new reference, so a click moved the passage out from under the reader. A verse already on screen is now left alone — the rule `useBcvSyncScroll` already implements for the comment list. Two more, found while fixing that: the scroll latched on the first verse block to render, so a column arriving later pushed the target back off screen with no correction; and nothing distinguished the reader's own scrolling from ours. Scrolling is now re-checked as columns arrive, and stops once the reader moves the grid themselves until the reference changes. Also: `repeat(0, ...)` is invalid CSS and the web view briefly passes an empty resource list, so the track count floors at one, and it is counted from the columns rather than taken as a prop that could disagree with them. The column floor was 15rem against the chapter row's 16rem; they now match. Structure: the aligned view, its column, and its scroll behavior move out of `scripture-text-grid.component.tsx` (615 lines, three layouts) into their own files. The column is shared with the chapter row, which had the same markup inline. The e2e Scripture Text Grid helpers move into a page object beside the existing ones, and its three aligned specs stop repeating their setup. Storybook: the aligned grid gets stories that reproduce the real DOM chain with stand-in verse blocks, so Chromatic can see an alignment break — no unit test can, since jsdom lays nothing out. `contentOverflow` gets one too. Tests: the port had no stubbed height, so "already visible" could never be true and the new rule would have been asserted vacuously. Geometry is now realistic, and the visible/hidden boundaries are unit-tested directly. Docs: comments trimmed throughout, and ticket-internal requirement numbers replaced with what they actually say. Keyboard shortcut catalog updated — reorder now applies in this view too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KHpKaxZxC4sBAAXQFVKEmp
A resource whose chapter has no verses rendered as a blank column: this view shows verse blocks and hides everything between them, so a commentary or a chapter of front matter had nothing left to draw and no explanation for it. The cell now says so, the same way verse view already reports a missing verse. The same layout gap applied to every other placeholder. Downloading, "not installed", "book not in this text" and download-failed all render a centered message where the editor would go — which, inside the content subgrid, landed in verse 1's row and was squeezed to its height. Placeholders now span the column, with a floor for when every column is in that state and there are no row heights to borrow. `isVerseEmpty` becomes `emptyMessage`, so the presentational cell renders the message it is given rather than knowing which localization key each view wants. Decisions recorded in the architecture log now that they are settled rather than awaiting sign-off: section headings stay hidden (they sit between verse blocks, so they have no row, and they disagree across translations — putting one on the following verse's row is not expressible in CSS), and rows stay a visual relationship rather than an announced one (ARIA table semantics need a row-major DOM, which one-editor-per-column rules out; the verse number at the start of each block is what lets a screen-reader user line the columns up). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KHpKaxZxC4sBAAXQFVKEmp
Fixes from the /review-paratext passes and a deep multi-agent review. Correctness. Chrome's scroll anchoring rewrites scrollTop on the grid root, which the "did the reader scroll?" check read as the reader — in exactly the late-column case that check exists for; the port now opts out. `display: contents` on the editor input destroyed the `counter-reset: caller crossref` scope, so footnote callers continued across columns instead of restarting per column. The scroll key ran the verse through `resolveDisplayVerseNum`, making verse 0 and verse 1 the same key so stepping between them never re-armed — and violating that helper's own "chapter surfaces must not call this". Upstream drops the range attributes for a marker it cannot parse (an imported reversed bridge), and such a block matched no row rule, so auto-placement dropped it into a shared row and misaligned the column from there down; those blocks are now left out. Handing scrolling to the grid removed the column's last clipping boundary, so verse blocks wrap anywhere. The scroll math subtracts the port's `clientTop`, which matters because this view allows an external border. Reader-facing. The whole column was an HTML5 drag source, which in Chromium hijacks click-drag text selection — the capability one-editor-per-column exists to protect; the header band is the drag source now and the column stays the drop target, in the chapter row as well. Escape was swallowed in Grid view by the capture-phase chapter-context handler for a split that is not on screen. The "Coming soon" hint sat after the whole toggle group, so appending Grid made it read as a label for Grid; it names the mode it describes. The zero state uses `EmptyState` rather than hand-rolled markup, which is also where its `role= "status"` comes from, and says what to do next in words the UI uses — the view is called Grid, and "align" appears nowhere a reader can see. Missing block verses hid every paragraph, so the documented "degrades to unaligned, not blank" was false; the rule is gated on the editor having produced blocks. `:has()` is supported in Electron 39; jsdom's rule matcher is not, so the component test does not inject the sheet — it is inert there anyway, and `aligned-grid.styles.test.ts` asserts its content. Reuse and tests. The verse view still carried its own copy of the drag wiring this branch centralized. Aligned-view reorder was asserted in three places and tested in none. The three claimed guards against an upstream rename were all blind — one asserted a string this repo generates, one hand-writes the class names, one skips in CI — so a contract test now reads the installed editor bundle and fails when a class or attribute the layout depends on is gone. Also: the en/ es parity test covers the View Options keys, `hasAlignableVerse` covers the table case, and the frame-budget e2e fails on its assertion rather than hanging. Two `as DOMRect` casts violated `no-type-assertion`. The root lint run ignores `extensions/`, which is why an earlier "lint clean" report was wrong. Sub-verse markers (\v 3a, \v 3b) collapse to one row and overlap; there is no core-side fix, so it is filed as PT-4559 for an upstream change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KHpKaxZxC4sBAAXQFVKEmp
…und them Review pass on the verse-aligned Grid view. Placement is now opt-in. Verse blocks are hidden by default and revealed by the generated row rule that places them, so a block the rules cannot place — no parsable range, verse 0, or a verse past the last row — is dropped instead of auto-placed into the first free row of the shared grid, which silently misaligned the column from there down. `hasAlignableVerse` asks the same question the layout does (a placeable range, searched through the whole paragraph subtree) rather than "does any paragraph open a verse", so it no longer reports a chapter as alignable that renders blank, nor gates out a verse marker nested one level deeper. Per-resource zoom moved off the subgrid box. `zoom` scales the used value of the lengths inside the element it sits on, and the column's content wrapper carries the row tracks it inherits from the shared grid; a zoomed column would have measured its rows against a different scale than its neighbours. The factor now rides down to the verse blocks as a custom property, so a row still takes the height of its tallest — now larger — cell. Dropped the `:has(.verse-block)` gate on the between-verses rule. A column whose editor produced no verse blocks does not 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. An empty column is the recoverable failure, and the contract test names its cause. Reference scroll: a browser clamp after content shrinks is no longer read as the reader moving the port, which used to stand the scroll down permanently and kill the late-column correction in the one case it exists for. Mutation batches coalesce into at most one layout read per frame and stop entirely once the reader does take the port over. The scroll utilities ignore blocks the layout never placed, whose all-zero rect otherwise pinned the reader to the top of the chapter, and both take their origin from one helper so they cannot disagree about the port's border. The frame-budget e2e tests measure again. The window is bracketed around the view switch — arming before it and reading after — because the switch ends with an awaited keypress, so a measurement started afterwards reported ~0ms however slow the render was. A give-up now throws instead of returning 15000 as if it were a measurement. Also: the grid root takes a tab stop with a focus ring, since the only controls inside it sit in the sticky header and none of them scroll it; the per-cell empty message is plain text again, because `EmptyState`'s `role="status"` gave every cell its own live region; an empty chapter is told it is empty rather than sent to a view with nothing to show either; the reorder testids name what the elements are, with the header drag source covered against the real component; and the `comingSoon` string is restored, with the chapter-specific hint as a new key. Corrected claims that were not true: the keyboard reorder path reaches only the column views (the grip renders in the header-band layout alone), and two e2e specs were selecting a `gridcell` role removed in PT-4157, so their assertions ran over an empty list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STzLD6CiarMZUP37MVxYc5
… scroll `useAlignedReferenceScroll` called `useViewVisibility()` itself, so every consumer built its own IntersectionObserver over the same iframe body. A per-cell consumer (PT-4543's chapter surfaces) would build one per resource. The hook now takes `isViewVisible`, matching `useBcvSyncScroll` and `useFocusSearchOnInvoke`, and `AlignedGrid` asks once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er a full shrink - findVerseBlockForVerse: the verse-0 / non-finite fallback took the first block in document order, which is the first column's first verse. When that column starts later than another (a commentary on 10-12 beside a full text), MAT 5:0 landed on row 10. It now takes the lowest data-verse-start. - useAlignedReferenceScroll: content shrinking until nothing overflows clamps scrollTop to 0, but the guard left that case unclamped and read it as the reader, standing the sync down. Only a port with no height (no layout) is left unclamped now. - ADR: record the flush-to-top scroll placement agreed in review, and replace the yalc / registry-pin paragraph, stale since the editor moved to staged file: dependencies (adr-dev-packages-staged-file-deps). - Contract test comments: same yalc staleness. Rebased onto main; resolved the Architecture-Decisions.md conflict by slug and moved the grid e2e spec onto main's enhanced-resources fixture. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The draggable listitem's aria-label is the localized "{resourceName},
{reference}" template, so the exact-equality checks against "Resource A" /
"Resource B" could never pass in a real app run. Read data-resource-id instead
and compare against the configured ids.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
88f31c6 to
3544fac
Compare
katherinejensen00
left a comment
There was a problem hiding this comment.
PR #2856 review: PT-4543, scroll Text Collection chapter views to the reference
- PR: #2856 (author: Levi Wenger)
- Reviewed range:
5f789e0b293..898e80f9bba. That is the PR's own commits; it is stacked on #2781. - Anchors: every file:line below is a line added in this range at PR head
898e80f9bba.- Commenting works whether GitHub diffs against
mainor Reviewable diffs against the #2781 revision. use-reference-scroll.hook.ts,reference-scroll.utils.tsand their tests show as whole new files (git pairs them with the old names at under 50% similarity), so any line in them can take a comment.
- Commenting works whether GitHub diffs against
- How the review ran:
- Three passes: quality/soundness, architecture, and comments/docs.
- An antagonistic reviewer tried to refute each finding.
/code-review maxran 10 finder angles, each candidate got a verifier, cap raised to 30.- Findings were merged and deduplicated. Anything refuted or reduced to taste was dropped or marked as a nit.
- Repros: in the session scratchpad; nothing was changed in the repo.
- Item 1: a jsdom repro that drives the real hook, plus an Electron page repro.
- Item 2: a jsdom repro.
- Item 8: a jsdom repro.
Summary table
| # | Severity | File:line | One-liner |
|---|---|---|---|
| 0 | Medium | PR-level | Manual verification not done; the two bugs below are exactly what it would catch |
| 1 | High | extensions/src/platform-scripture-editor/src/scripture-text-grid/use-reference-scroll.hook.ts:113 |
Jumping to a verse in another chapter/book lands at the top of the chapter |
| 2 | Medium | extensions/src/platform-scripture-editor/src/scripture-text-grid/scripture-text-grid.component.tsx:507 |
Switching the chapter-context resource never scrolls the new resource |
| 3 | Medium | extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell.component.test.tsx:729 |
Cell tests don't pin the chapter wiring (visibility rule, hidden path, content reload) |
| 4 | Medium | .context/designs/2026-09-16-text-collection-chapter-verse-scroll-design.md:41 |
Design doc's decisions are reversed by the code it ships with |
| 5 | Low | extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:147 |
Verse 0 lands 80px above verse 1, not at the top of the chapter |
| 6 | Low | extensions/src/platform-scripture-editor/src/scripture-text-grid/use-reference-scroll.hook.ts:152 |
Echo branch stands down but an already-queued frame still scrolls |
| 7 | Low | extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:101 |
A verse marker inside a hidden (display:none) node makes the view drift to the top |
| 8 | Low | extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:242 |
80px lead-in parks the marker below the fold in a short pane, permanently |
| 9 | Low | extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:217 |
"Fully visible" counts a marker hidden behind the horizontal scrollbar |
| 10 | Low | extensions/src/platform-scripture-editor/src/scripture-text-grid/scripture-text-grid.component.tsx:355 |
Reordering chapter columns resets the moved columns to the top |
| 11 | Low | .context/standards/Architecture-Decisions.md:2963 |
ADR/PR body claims about the aligned grid are inaccurate |
| 12 | Low | .context/standards/Architecture-Decisions.md:2968 |
ADR narrates PR history; deferred work has no ticket |
| 13 | Low | extensions/src/platform-scripture-editor/src/scripture-text-grid.web-view.tsx:178 |
"One IntersectionObserver" rationale doesn't hold, since the web view already has one |
| 14 | Low | extensions/src/platform-scripture-editor/src/scripture-text-grid/upstream-editor-contract.test.ts:41 |
Selector contract test cannot fail |
| 15 | Low | extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell-view.component.test.tsx:877 |
Sticky-header comment has the reasoning backwards |
| 16 | Low | extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell.component.tsx:211 |
"Lands the same way whichever view" is not true |
| 17 | Low | extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell-view.component.tsx:476 |
Chapter port leaves overflow-anchor on (the grid turns it off for this hook) |
| 18 | Nit | extensions/src/platform-scripture-editor/src/scripture-text-grid/use-reference-scroll.hook.ts:35 |
Hook TSDoc: "one thing" vs "second thing"; leadInPx undocumented; loose pairing |
| 19 | Nit | extensions/src/platform-scripture-editor/src/scripture-text-grid/use-reference-scroll.hook.test.tsx:181 |
Test comment says the hook needs no echo latch; it has one |
| 20 | Nit | extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell.component.test.tsx:95 |
Dead useViewVisibility stubs |
| 21 | Nit | extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:161 |
Stale TSDoc: "(the grid root)", hidden-pane claim, "top of the window" |
| 22 | Nit | extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell-view.component.tsx:481 |
Zoom docs still say zoom sits on the content wrapper |
| 23 | Nit | extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:140 |
Duplicated nearest-start scan and verse selector |
| 24 | Nit | extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell-view.component.test.tsx:367 |
Assertions that pass when the element is missing; a duplicate test |
| 25 | Nit | extensions/src/platform-scripture-editor/src/scripture-text-grid/scripture-text-grid.component.tsx:278 |
Same rationale repeated in 2–5 places each (PR-level) |
| 26 | Nit | extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:103 |
Backward-facing / now-false wording |
Comments to post (addressed to Levi)
0. Medium: PR-level (general comment, no line)
Before this merges, please do the manual pass the PR lists as "not yet done". Items 1 and 2 below are real bugs that only show up with real layout, and all of the scroll tests use scripted geometry.
Please cover:
- a chapter jump to a verse well down the chapter (for example JHN 1:20 → JHN 3:16) in a chapter column, the chapter-context split, and the single-resource view;
- clicking a verse row of a different resource while the chapter-context split is open;
- a chapter-column reorder;
- all of the above at 100% and at a non-default zoom.
The Testing checkboxes (typecheck, lint, test) are also unticked; please tick or run them.
Separately, the PR body still has the <!-- VERIFY THIS LINK BEFORE OPENING THE PR --> note next to the session link.
1. High: extensions/src/platform-scripture-editor/src/scripture-text-grid/use-reference-scroll.hook.ts:113
Summary: moving the reference to a verse in another chapter or book leaves a visible chapter view at the top of the new chapter instead of at the verse, which is the PT-4543 symptom, on the navigation readers use most.
This was confirmed independently by 9 of the 13 review passes. It reproduces with the real hook in jsdom and in an Electron page with real layout.
The sequence:
useDataraisesisLoadingin an effect (create-use-data-hook.util.ts, thesetIsLoading(true)effect keyed onselector). So the render wherescrRefchanges still has the old chapter withisLoading === false, andderiveCellStatesays'ready'.- The re-arm effect (hook ~L136–162) calls
requestScroll()right away.findVerseMarkerForVersematches on verse number only, so it scrolls the old chapter to its own verse N. That is a one-frame visible jump in the wrong chapter.appliedScrollTopRefbecomes X > 0. - The next render is
'downloading'. The placeholder replaces the editor inside the same[data-cell-content]port, the port stops overflowing, and Chromium clampsscrollTopto 0. - The next mutation frame:
maxScrollTop <= 0, so L113 setsexpected = applied(X). Then|0 − X| > 1setshasStoodDownRef = true. - The new chapter lands; the observer returns early at L175. There is no scroll.
Second hole: if the placeholder still overflows a little, L121–122 return without touching applied. The stale X is then compared against the clamped scrollTop, and the hook stands down anyway.
Scope:
- It hits all three chapter surfaces.
- A hidden tab escapes, because it defers the scroll and
appliedis undefined on catch-up. - A next-chapter → verse 1 move mostly escapes, because verse 1 plus the lead-in clamps to 0.
- The same stand-down logic was already in #2781's aligned grid, but this PR makes the chapter surfaces depend on it for the headline feature.
Suggested fix:
-
Don't act on, or record a position against, content that isn't the reference's chapter. For example, gate with
isEnabled: viewMode === 'chapter' && state === 'ready'(plus a loaded-chapter-matches-scrRefcheck). The existingisEnableddependency then re-arms after the load. -
Or re-arm on content identity, the way
resource-text-panel.component.tsxdoes withisUsjLoading/hasNewScrollTarget. -
Also, a port with nothing to scroll cannot have been scrolled by the reader:
const expected = maxScrollTop > 0 ? Math.min(applied, maxScrollTop) : port.scrollTop;
Clear
appliedScrollTopRefwhen no target is found (L121–122). That version passed both repros and all 12 existing hook tests. Note that the Electron repro showed the clamp alone still stands down once the new content lands. Recordingappliedagainst the pre-reload content is the root cause, so the readiness/identity gate is the real fix. -
Add a cell-level test: ready (old chapter) → loading/placeholder → ready (new chapter), asserting the port lands on the new verse.
2. Medium: extensions/src/platform-scripture-editor/src/scripture-text-grid/scripture-text-grid.component.tsx:507
Summary: with the chapter-context split open, clicking a verse row of a different resource shows that resource's chapter at the top, not at the verse.
Why:
- The chapter-context
<ResourceCell resourceRef={chapterContext}>has nokey, sosetChapterContext(B)reuses the same instance and the same hook refs. - The reference hasn't changed, so the re-arm effect doesn't run.
- If the reader had clicked in resource A, the echo branch already stood the hook down. Otherwise B's load shows the placeholder and item 1's stand-down fires.
- Either way, every row click after the first reproduces the original bug in the split.
- Verified with a jsdom repro: 0 where 700 is expected. The single-resource cell (~L305) is unkeyed too.
Suggested fix: key={chapterContext.resourceId} (and the same for the single resource), plus a test that switches chapterContext. The content-identity fix in item 1 would also cover this.
3. Medium: extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell.component.test.tsx:729
Summary: the new cell tests would stay green if the chapter-cell wiring were broken in three ways that matter.
What the tests miss:
- The visibility rule is unpinned. Deleting
isTargetVisible: isMarkerFullyInPortView(resource-cell.component.tsx:209) keeps every test green. The only geometric cell test puts the marker 500px below a 100px port, where the any-part rule and the full-containment rule agree. That silently brings back "verse number showing, verse cut off at the bottom". Add a marker clipped at the bottom edge (for example top 90, height 20, port 100) and assert the port scrolls. - The hidden path isn't tested at cell or grid level. Every
ResourceCelltest renders visible, and the grid tests mockResourceCell. Passingtrue, or JSX shorthandisViewVisibleat a chapter call site, keeps everything green. The hook tests follow the hidden → navigate → show recipe, but nothing checks the wiring. See.claude/rules/cross-view-sync-hidden-views.md: "back it with a test". - The finder is mocked. The tests mock the sibling
./reference-scroll.utilsmodule (L44) and assert on the finder's call arguments, so the real selector never runs against the cell's DOM. Testing-Guide says to mock only at external boundaries.
Plus the content-reload case from item 1.
4. Medium: .context/designs/2026-09-16-text-collection-chapter-verse-scroll-design.md:41
Summary: the design doc landing in this PR states decisions the code in this PR reverses. Agents read .context/designs/ as prior art, so it will mislead the next feature.
What contradicts the code:
- Decision 4, "No echo latch … makes
isEchoOfPublishedScrRefunnecessary" (L41), and the table's "obviated" (L60). The hook now haspublishedScrRefRef. - The hook signature (~L184–198) lacks
isViewVisible,isTargetVisible,publishedScrRefRefandleadInPx. - "
isBlockInPortView… unchanged" / the visible-verse check (L111, L342). Chapter cells useisMarkerFullyInPortView. - "No layout change" (L219). Zoom moved to
[data-cell-pad]and the single-resource flex chain was rebuilt. - A mocked
useViewVisibility(L333). It is a parameter now. - L398, "
npm run typecheckis skipped by team preference", contradicts CLAUDE.md and CI. Please delete it: agents follow it.
Suggested fix: either update the decisions, table and signature to what shipped, or add a short "Changed during implementation" list under the Frozen-record fence. It needs 5–6 bullets: echo latch added, marker visibility rule, lead-in, visibility as a parameter, zoom placement, single-resource layout. The Sequencing/Risk section ("#2781 is not yet approved", "retarget to main") expires at merge and can go.
5. Low: extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:147
Summary: a verse-0 reference lands 80px above verse 1's number, not at the top of the chapter as the comment says, so chapter headings or the book intro scroll out of view.
Details:
findVerseMarkerForVersereturnsfirstMarker, and the cell scrolls it with the 80px lead-in.- Anything above verse 1 taller than about 72px scrolls off:
\c,\s,\din Psalms, and in chapter 1 the book title and intro. - Verse 0 is a real position: the editor publishes it for a caret in the intro or a heading.
- The editor's
scrollToVerse(editor-dom.util.ts) andresource-text-panelboth go totop: 0for any verse below 1. - The TSDoc at L92–94 and the hook comment at ~L77–80 make the same "at the top of the chapter" claim.
Fix: set port.scrollTop = 0 when verseNum < 1, or when no marker starts at or before it.
6. Low: extensions/src/platform-scripture-editor/src/scripture-text-grid/use-reference-scroll.hook.ts:152
Summary: the echo branch stands the hook down, but an animation frame queued before that still runs requestScroll(), which is the jump the latch exists to prevent.
Details:
run()never checkshasStoodDownRef, and the echo branch has also clearedapplied, so the frame scrolls to the clicked verse.- Trigger: clicking inside a long verse while a just-navigated chapter is still rendering.
- The comment at L90 ("Only ever set from inside
requestScroll") is no longer true either.
Fix: one line. Check hasStoodDownRef.current inside the rAF callback (~L176) or at the top of run.
7. Low: extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:101
Summary: the TSDoc says no hidden verse marker is known, but the editor can emit one, and the finder would then drift the view to the top.
Details:
UnknownNode(dev-packages/scripture-editors/libs/shared/src/nodes/features/UnknownNode.ts) setsdisplay:nonebut still renders its children.\esbsidebars and\periphbecome UnknownNode, and the editor's own 2SA fixture has\v 27inside an unclosed\esb.- A zero-size marker gives
top 0, so each check doesscrollTop += -firstVisibleY - 80and the view walks toward the top while the chapter loads.
Fix: check getClientRects().length on the chosen target only, which avoids the per-candidate cost the TSDoc worries about. Or exclude unknown-node descendants in the selector. Update the TSDoc either way.
8. Low: extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:242
Summary: in a short pane the 80px lead-in puts the verse number below the fold, and the hook never corrects it.
Details:
scrollPortToBlockplaces the marker atfirstVisibleY + leadInPx. If the visible height is under about 100px, that is off-screen.- Every later check computes the same delta, so nothing moves. The tall-target fallback in
isMarkerFullyInPortViewdoesn't cover "the lead-in doesn't fit". - This is plausible in the chapter-context split (
minSize={25}) in a short Simple-mode column, and more likely at higher zoom. - Verified with a jsdom repro: a 60px port ends with the marker at y 80–100.
Fix: Math.max(0, Math.min(leadInPx, visibleHeight - markerHeight)).
9. Low: extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:217
Summary: portBottom is the border-box bottom, so a marker hidden behind the horizontal scrollbar counts as "fully visible" and is not scrolled to.
Details: poetry indents are in viewport units (.usfm_q1 { margin-left: 15vw }). They overflow a narrow (min-w-3xs) column on wide windows (above about 1470px), and the column then shows a horizontal scrollbar.
Fix: port.getBoundingClientRect().top + port.clientTop + port.clientHeight. The same applies at L193.
10. Low: extensions/src/platform-scripture-editor/src/scripture-text-grid/scripture-text-grid.component.tsx:355
Summary: reordering chapter columns leaves the moved columns at the top of the chapter, and the hook then stands down instead of restoring them.
Details:
- React moves keyed siblings with
insertBefore. A scroll container detached and re-inserted loses its scroll position in Chromium. - The observer only watches the port's own subtree, so nothing re-checks. The next mutation reads 0 against
appliedand stands down. - Moving the last of four columns to the front re-inserts the other three.
- Please confirm in the app (read each column's
scrollTopbefore and after a keyboard reorder). If it reproduces, re-arm on reorder or keep columns in place and reorder with CSSorder.
11. Low: .context/standards/Architecture-Decisions.md:2963
Summary: the ADR (and the PR body) describe the aligned grid's wiring inaccurately. The behavior claim is right; the specifics aren't.
Details:
- "The aligned grid takes both defaults —
findVerseBlockForVerseandisBlockInPortView":findTargetis a required positional parameter, andaligned-grid.component.tsx:57passesfindVerseBlockForVerseexplicitly. Only the visibility test is defaulted. - L2972, "
aligned-grid.component.tsxis unchanged", and the PR body's "byte-identical to5f789e0b293": that file changes in this PR (imports, a new requiredisViewVisibleprop, theuseViewVisibility()call removed, the hook call swapped). - The ADR also doesn't mention the echo-latch decision, which is the one the design doc gets backwards.
Suggested wording: "The aligned grid passes findVerseBlockForVerse and keeps the default isBlockInPortView; its scroll behavior is unchanged." Reviewers seeing "byte-identical" may skip a file that did change.
12. Low: .context/standards/Architecture-Decisions.md:2968
Summary: parts of the ADR narrate this PR's history. Recast them as alternatives or drop them, so the entry reads as a decision rather than a changelog.
Details:
- "An earlier form of this change forked
isBlockInPortViewon target height…" → make it alternative (d): ForkisBlockInPortViewon target height, rejected because it silently gives the Grid view containment semantics. - L2957, "renamed here to
use-reference-scroll.hook.ts" → "(formerly the aligned grid's controller)". It is also a 160+ character unwrapped line. - L2998, "stacked on PT-4184/#2781" is branch process →
Source: PT-4543. - Moving the reference panel's settle loop onto this hook is deferred with no ticket. Per
forward-facing-comments.md, please name one (PT-XXXX).
13. Low: extensions/src/platform-scripture-editor/src/scripture-text-grid.web-view.tsx:178
Summary: isViewVisible is now a required prop threaded through four components "so there is one IntersectionObserver". But the web view already creates a second one via useTabIconSelection, so the rationale for the prop drilling doesn't hold as written.
Details:
useTabIconSelection(~L415) also callsuseViewVisibility, so there are two observers in every mode.- Either pass this
isViewVisibleinto the tab-icon path (for examplepickTabIconUrl(isDarkTheme, isViewVisible, TAB_ICON_URLS)), - or call
useViewVisibility()once insideScriptureTextGrid. That drops the required prop from four components and about 46 mechanical test/story edits. - The "one observer" rationale is also written out five times (web view,
ScriptureTextGridProps,AlignedGridProps,ResourceCellProps, hook). Keep it once at the root.
14. Low: extensions/src/platform-scripture-editor/src/scripture-text-grid/upstream-editor-contract.test.ts:41
Summary: the new selector contract checks can't fail, so they don't protect the span[data-marker="v"][data-number] shape the chapter scroll depends on.
Details:
toContain('data-marker')andtoContain('data-number')match almost any editor bundle: chapter nodes also setdata-number, and nearly every node setsdata-marker.- If verse spans dropped
data-number, this stays green while chapter scrolling silently does nothing.
Fix: assert on the verse node's DOM creation. For example, render an ImmutableVerseNode (or grep the bundle for the verse node's createDOM output) and check the attributes on the resulting element.
15. Low: extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell-view.component.test.tsx:877
Summary: this comment has the reasoning backwards: a sticky header inside the port is exactly what getFirstVisibleY accounts for. Only a non-sticky in-port header would offset the scroll.
Details:
- "Making this header sticky inside the scroll box … would silently land every chapter-mode scroll one header-height too low" is wrong.
getFirstVisibleYadds the height of a[data-cell-header]found inside the port precisely because it assumes that header is sticky and covers the top. That is the aligned grid's case, and it lands correctly. - Suggested: "Moving the header inside the port as a non-sticky element would offset every chapter-mode scroll by its height, because
getFirstVisibleYtreats any in-port header as covering the top." - Related: in
reference-scroll.utils.test.ts, the test at ~L332 passes with the header inside the port, contrary to its comment, and ~L342 repeats ~L268. Only ~L353 actually distinguishes the placements.
16. Low: extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell.component.tsx:211
Summary: "so a reference lands the same way whichever view the reader is looking at" isn't true. Only the 80px offset is shared.
How the editor's scrollToVerse differs:
- It needs an exact verse-number match, so its panels never land a bridged verse; these cells do.
- Verse 0 differs (item 5).
- It re-frames a visible verse, while these cells leave it alone.
- It animates, while these cells scroll instantly.
Suggested: "The lead-in the Scripture editor and the other panels use."
17. Low: extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell-view.component.tsx:476
Summary: the new chapter port leaves overflow-anchor at its default. The aligned grid turns it off because this same hook reads any unexplained scrollTop change as the reader scrolling.
Details:
aligned-grid.styles.tssetsoverflow-anchor: noneon the grid root, with a comment that scroll anchoring "silently rewrites"scrollTop.- That rule is scoped to
.aligned-grid, so[data-cell-content]has anchoring on. - A layout shift above the viewport after the hook's write (for example a pane resize) can let anchoring move
scrollTop, and the next mutation then stands the hook down. - I didn't measure this in the app. Can you check whether the chapter port should match the grid, or say why it doesn't need to?
18. Nit: extensions/src/platform-scripture-editor/src/scripture-text-grid/use-reference-scroll.hook.ts:35
Summary: the hook's TSDoc contradicts itself and misses one option.
Details:
- L35: "Which element represents a verse is the one thing they disagree on". L50:
isTargetVisibleis "the second thing". - The
optionsblob (L45) documentsisEnabled,isTargetVisibleandpublishedScrRefRef, but notleadInPx. findTargetis required while its matchingisTargetVisibleandleadInPxdefault to the block framing. A future marker-based caller that passes only the finder silently gets the wrong rule.- Suggested: per-field TSDoc on the options type. Optionally, export
VERSE_BLOCK_ANCHOR/VERSE_MARKER_ANCHORconstants (finder + visibility test + lead-in) so the three can't be mismatched. - Also
publishedScrRefRefis an effect dependency with no "must be stable" note.
19. Nit: extensions/src/platform-scripture-editor/src/scripture-text-grid/use-reference-scroll.hook.test.tsx:181
Summary: "It is also why this hook needs no echo latch" contradicts the latch the hook now has, which the same file tests from ~L278. It could lead someone to delete the latch. Please drop the sentence.
20. Nit: extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell.component.test.tsx:95
Summary: these useViewVisibility stubs are dead. Nothing these tests render calls the hook any more, because it moved to the web view root.
Details:
- The comment here already says no test reads it.
- The same applies to the
useViewVisibility:entry inscripture-text-grid.component.test.tsx(the mock at L94).mockVisibilityitself is still live as the prop at L830. - In
scripture-text-grid.zoom-integration.test.tsx, the comment "The grid asks whether its tab is visible" is no longer true. - Remove the stubs, or state why they're defensive.
21. Nit: extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:161
Summary: a few TSDoc lines still describe only the grid, or claim things that aren't so.
Details:
@param port The scroll port (the grid root)at L161 and L229 (and L46). These now also take a chapter cell's content box and a verse marker. Say "The scroll port" and "the verse target (block or marker)".isMarkerFullyInPortView's TSDoc (~L204–208) says the fallback "also covers a port reporting no visible area at all — a hidden pane". Zero area actually enters the fallback, which returns false and scrolls.editor-dom.util.ts:6: "from the top of the window" is inaccurate. It is measured within the scroll container, andscrollToAnnotationalso uses it as a bottom margin.
22. Nit: extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell-view.component.tsx:481
Summary: the zoom docs still say 'box' zoom scales "the content wrapper". Now that the factor is on [data-cell-pad], stale docs invite moving it back onto the port, which is what caused the ~7800px overshoot.
Details:
- Update the
zoomTargetTSDoc (~L66, L123–127) and thealigned-grid.styles.tsheader note. - The stub column in
aligned-grid.component.stories.tsx(~L105) still puts--aligned-zoomon[data-cell-content]while claiming to publish it "exactly as ResourceCellView" does.
23. Nit: extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:140
Summary: the marker finder copies the block finder's nearest-at-or-before scan. The two are kept in step by a comment and a parity table rather than shared code.
Details:
- One
pickNearestStartAtOrBefore(candidates, startOf, verseNum)helper would remove the "change one and change the other" comment. - They have already drifted slightly: the marker finder skips the block finder's filter before taking
firstMarker. That is harmless today. VERSE_MARKER_SELECTOR(L84) also restates the selector ineditor-dom.util.ts'sscrollToVerse. That file already exports shared selectors (EDITOR_PARA_SELECTOR), so it could live there.- Not blocking; the duplication is documented.
24. Nit: extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell-view.component.test.tsx:367
Summary: expect(port instanceof HTMLElement && port.style.zoom).toBeFalsy() also passes when the element isn't found at all. The same pattern is at L389 and resource-cell.component.test.tsx:618.
Details:
- Assert the element exists first, then
expect(port.style.zoom).toBe(''). - Also, in
use-reference-scroll.hook.test.tsx, the test at ~L154–175 duplicates ~L119–133. Its extra assertions (noscrollTo/scrollIntoView) can't fail in jsdom, which has neither. The prototype save/restore at ~L108–116 exists only for it.
25. Nit: extensions/src/platform-scripture-editor/src/scripture-text-grid/scripture-text-grid.component.tsx:278 (PR-level)
Summary: several rationales are written out in 2–5 places each, which makes the important comments harder to find. Keep each once, at the site it governs, and point to it elsewhere.
Details:
- Echo latch: 5 copies (hook ~L26–30, ~L52–54, ~L138–143;
resource-cell.component.tsx~L193–194, ~L276–277). Keep the one at the effect. - "One IntersectionObserver": 5 copies (see item 13).
- The flex chain: two comments about 15 lines apart here (L278–285 and L295–301). Merge them.
Also, "Same chain as the chapter-context split" (~L284) is accurate for height only. The split (~L502) lacks the [&>*]:flex-1 width fix this PR adds for the single-resource view. That predates this PR; worth adding to the follow-ups next to ResourceColumn.
26. Nit: extensions/src/platform-scripture-editor/src/scripture-text-grid/reference-scroll.utils.ts:103
Summary: a couple of comments narrate the review rather than inform a later maintainer, or are no longer true after this PR.
Details:
- L103: "it was weighed, not overlooked" is aimed at reviewers. The rest of the paragraph (L97–103) can shrink to two lines: why there's no zero-rect screen, and when to add one. See also item 7, which shows a case now does exist.
src/renderer/hooks/papi-hooks/use-scroll-group-scr-ref.hook.test.ts:476: "nothing at those call sites states the dependency" is no longer true. This PR adds exactly that comment inmain.ts(~L1076–1081).
Raised by a finder but not included (after adversarial review)
- Moving
useViewVisibilityinto eachResourceCell: rejected. The PR lists memoizinguseViewVisibilityas a follow-up; item 13 covers the accurate part. - Chapter cells resolve bridged verses but the editor doesn't: not a defect. It's an improvement, and fine as a follow-up to share
findVerseMarkerForVersewithscrollToVerse. - Per-resource versification isn't converted before the verse lookup: pre-existing in verse, aligned and reference-panel modes too (PSA 51, English vs Original, lands 2 verses early). Worth a follow-up ticket, not this PR. The finder TSDoc's "versification gap" means a missing verse number, not conversion.
- Hook owning the echo latch and returning
markPublished()/ a shared consume-echo helper: tidier, but no defect. [&>*]:flex-1child selector instead of aclassNameprop: taste; it's explained in a comment.- Design doc length (418 lines): not raised on length alone (other
.contextdocs run longer). Item 4 covers the stale content. - Refuted: the single-slot echo latch failing with two clicks in flight (the scroll group echoes synchronously); the root re-rendering on every tab switch (already true via
useTabIconSelection). - Checked and sound:
- ADR slug placement (
LC_ALL=C sort -c) and entry format. - Hidden-view deferral and catch-up in the hook.
- rAF coalescing terminates.
useRunWhenVisiblehas no stale closures.- The
--aligned-zoominheritance throughdisplay: contents. - The aligned grid's scroll behavior matches
5f789e0b293. - Prettier is clean on the changed TS files.
- No backward-facing ticket IDs in code (the
PT-4559reference points forward).
- ADR slug placement (
@katherinejensen00 partially reviewed 43 files and made 2 comments.
Reviewable status: 26 of 43 files reviewed, 1 unresolved discussion (waiting on captaincrazybro).
extensions/src/platform-scripture-editor/src/scripture-text-grid/use-reference-scroll.hook.ts line 113 at r1 (raw file):
// unclamped: that is an environment that lays nothing out (jsdom), where scrollTop keeps // whatever was written and clamping would read this hook's own write as the reader's. const hasLayout = port.clientHeight > 0;
1. High: extensions/src/platform-scripture-editor/src/scripture-text-grid/use-reference-scroll.hook.ts:113
Summary: moving the reference to a verse in another chapter or book leaves a visible chapter view at the top of the new chapter instead of at the verse, which is the PT-4543 symptom, on the navigation readers use most.
This was confirmed independently by 9 of the 13 review passes. It reproduces with the real hook in jsdom and in an Electron page with real layout.
The sequence:
useDataraisesisLoadingin an effect (create-use-data-hook.util.ts, thesetIsLoading(true)effect keyed onselector). So the render wherescrRefchanges still has the old chapter withisLoading === false, andderiveCellStatesays'ready'.- The re-arm effect (hook ~L136–162) calls
requestScroll()right away.findVerseMarkerForVersematches on verse number only, so it scrolls the old chapter to its own verse N. That is a one-frame visible jump in the wrong chapter.appliedScrollTopRefbecomes X > 0. - The next render is
'downloading'. The placeholder replaces the editor inside the same[data-cell-content]port, the port stops overflowing, and Chromium clampsscrollTopto 0. - The next mutation frame:
maxScrollTop <= 0, so L113 setsexpected = applied(X). Then|0 − X| > 1setshasStoodDownRef = true. - The new chapter lands; the observer returns early at L175. There is no scroll.
Second hole: if the placeholder still overflows a little, L121–122 return without touching applied. The stale X is then compared against the clamped scrollTop, and the hook stands down anyway.
Scope:
- It hits all three chapter surfaces.
- A hidden tab escapes, because it defers the scroll and
appliedis undefined on catch-up. - A next-chapter → verse 1 move mostly escapes, because verse 1 plus the lead-in clamps to 0.
- The same stand-down logic was already in #2781's aligned grid, but this PR makes the chapter surfaces depend on it for the headline feature.
Suggested fix:
-
Don't act on, or record a position against, content that isn't the reference's chapter. For example, gate with
isEnabled: viewMode === 'chapter' && state === 'ready'(plus a loaded-chapter-matches-scrRefcheck). The existingisEnableddependency then re-arms after the load. -
Or re-arm on content identity, the way
resource-text-panel.component.tsxdoes withisUsjLoading/hasNewScrollTarget. -
Also, a port with nothing to scroll cannot have been scrolled by the reader:
const expected = maxScrollTop > 0 ? Math.min(applied, maxScrollTop) : port.scrollTop;
Clear
appliedScrollTopRefwhen no target is found (L121–122). That version passed both repros and all 12 existing hook tests. Note that the Electron repro showed the clamp alone still stands down once the new content lands. Recordingappliedagainst the pre-reload content is the root cause, so the readiness/identity gate is the real fix. -
Add a cell-level test: ready (old chapter) → loading/placeholder → ready (new chapter), asserting the port lands on the new verse.
katherinejensen00
left a comment
There was a problem hiding this comment.
@katherinejensen00 reviewed 17 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on captaincrazybro).
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>
The aligned grid's reference-scroll controller is layout-agnostic apart from one dependency: how to find the element representing a verse. Make that a parameter so a chapter cell can reuse the rules rather than grow a second loop. Renames aligned-scroll.utils.ts to reference-scroll.utils.ts and use-aligned-reference-scroll.hook.ts to use-reference-scroll.hook.ts, since both now serve chapter surfaces too. The control loop is unchanged: appliedScrollTopRef and its shrink-clamp comparison, hasStoodDownRef, the targetReference key, the reset-on-reference-change effect, and the per-frame-coalesced MutationObserver all keep their behavior and rationale. Adds findVerseMarkerForVerse for the editor's inline layout, whose anchor is a marker span rather than a placed block. It follows findVerseBlockForVerse's rule — exact start, else nearest preceding — which is what resolves a reference inside a bridge: \v 14-15 emits no [data-number="15"], so an exact match alone never finds verse 15. Adds isEnabled so a caller whose layout is scrolled by an ancestor, or that has nothing to scroll to, can keep the hook call unconditional while reading no geometry and attaching no observer. Two tests pin the premise that lets one set of port math serve both layouts: getFirstVisibleY looks the resource-name header up INSIDE the port, so a chapter cell — whose header is the port's sibling — contributes no header offset, while the aligned grid's sticky headers do. Both fail if that lookup ever climbs out of the port. PT-4543. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Clicking a verse row opens the chapter-context pane, which rendered the chapter at the top and never moved to the verse — nor when the shared reference changed. It is the Text Collection surface that shows a whole chapter and ignored where the reader was. ResourceCell now runs the reference scroll for viewMode === 'chapter', against its own content box and the inline layout's marker spans. That one gate serves all three chapter surfaces: the chapter-context split, a chapter column, and the single-resource full-width view. The other modes stay off deliberately — verse mode is already reduced to the reference's verse, and aligned mode is scrolled by the grid root instead. ResourceCellView forwards a contentRef to its [data-cell-content] box so the cell can hand its port to the hook. The header band stays the port's sibling: that is what makes getFirstVisibleY contribute no header offset here, and a test in reference-scroll.utils.test.ts pins it. Adds the hook's first direct tests, covering the behavior the older settle loop in resource-text-panel.component.tsx does not have: the hidden-tab catch-up fires once on activation and snaps rather than animating, repeats while hidden collapse to the latest reference, a verse already on screen is left alone, the sync stands down once the reader scrolls and re-arms on the next reference, a content shrink is not mistaken for the reader, and a disabled cell reads no geometry at all. The fixture models a target's rect as moving with scrollTop, since scrollPortToBlock adjusts by a delta it reads back. resource-cell.component.test.tsx stubs useViewVisibility, whose real implementation needs an IntersectionObserver jsdom does not provide. PT-4543. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…in.ts The comment on createResourceTextPanelProvider said Bible texts and commentaries "are not scroll-synced with the scripture editor in simple mode". The provider never sets scrollGroupScrRef, and an absent value resolves to scroll group 0 in use-scroll-group-scr-ref.hook.ts — so they do follow the shared reference, and have for as long as that code has looked like this. It now says what the code does, and why not pinning is still the right call: a pin would overwrite a power-mode user's choice of a different group. Adds adr-one-reference-scroll-hook-parameterized-by-verse-anchor, in byte-order slug position, recording why the aligned grid's controller was parameterized rather than copied or replaced by the older settle loop, and noting that the header-outside-the-port adjacency the shared port math depends on is now load-bearing. PT-4543. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comment corrected in platform-scripture-editor's main.ts denied that Bible
texts and commentaries follow the scroll group in Simple mode. What makes them
follow it is useScrollGroupScrRef's `?? 0` default, and none of that hook's 16
existing cases covered it — so the behavior the comment denied was resting on
one unpinned line.
Four cases, contrasting with the detached-view block above them: an absent
scrollGroupScrRef reports group 0 rather than no group, an update published to
group 0 is followed, an update published to a DIFFERENT group is ignored, and a
new reference is published to group 0 rather than written back to the view
definition. The wrong-group case is the falsifiable half: without it a hook that
followed every group would pass.
The group id is asserted as a literal 0 because it is the hook's resolution
being pinned; setScrRefSync defaults an absent group to 0 itself, so the write
would land in group 0 either way.
Corrects the design doc on two counts. scroll-group-sync.spec.ts IS Power-pinned
— `test.use({ interfaceMode: 'power' })`, a first-class isolated.fixture option
the fixture asserts the app came up in — so the ticket was right and the doc's
claim that it inherits dev-appdata settings was wrong. And the proposed
Simple-mode E2E is dropped rather than written: the isolated project's
globalSetup rejects the running app that the grid's page object needs, opening
the chapter-context pane requires a real resource that CI does not have, and
Simple mode renders no dock tabs for such a spec to wait on. CI runs only
test:e2e:smoke, so no placement would have bought CI protection anyway.
PT-4543.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The single-resource full-width branch wrapped the cell in a block container that scrolled itself. The reference scroll drives the CELL's content box, and that box only becomes a scroll port when an ancestor constrains the cell's height — ResourceCellView's root is `flex flex-col` with no height of its own and takes no className prop, so it gets a height only as a stretched flex item. With the wrapper scrolling instead, the cell's box could never overflow: scrollTop clamped to 0, and because the box's rect spanned the whole chapter every marker read as already visible, so no scroll was attempted at all. So of the three chapter surfaces this ticket covers, this one ran the code and did nothing. The other two were already fine for layout reasons rather than by design: the chapter row hands vertical overflow down with overflow-y-hidden, and the chapter-context split has a full flex/min-h-0/flex-1 chain. Adopts that same chain here — a non-scrolling flex column wrapper plus the `flex min-h-0 flex-1` box the split pane uses verbatim — and records why it is load-bearing, so the overflow does not migrate back up. Found by two independent review passes reading the layout classes across all three surfaces; jsdom lays nothing out, so no unit test can catch this. PT-4543. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Require a target shorter than the visible area to fit inside it, instead of counting any part showing. That rule was written for a whole verse block, where scrolling back to the top would fight a reader part-way through a long verse. A chapter cell's target is a one-line verse marker whose text follows after it, so a marker clipped at the bottom edge counted as showing and the leave-a-visible-verse-alone rule then declined to scroll — the reference move appeared to do nothing. Targets taller than the visible area keep the old rule, since they can never fit. Gate the re-arm effect on isEnabled and list it as a dependency. A disabled cell was still arming the deferred run while hidden, spending two state updates per cell on activation to reach a body that returns immediately — and every cell hosts an editor. Listing the flag also makes re-enabling re-arm, rather than resuming from a position measured before the hook was switched on. Pin the two defensive branches in findVerseMarkerForVerse: a malformed reference lands at the top of the chapter, and a marker whose verse number does not parse is skipped rather than allowed to win the scan. Correct three comments that overclaimed. The isEnabled TSDoc and the disabled-mode test title both said the flag stops everything, when the visibility subscription is read before it; both now say what is actually true and point at the fix. And a comment in the scroll-group test narrated what this branch changed in main.ts rather than what a later reader needs. Adds a pointer tying the two finders' nearest-start tie-break together, since the rule is written out twice and the halves could drift. PT-4543. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pt-4184-grid now has useReferenceScroll take isViewVisible rather than calling useViewVisibility itself, so a view subscribes once however many consumers of the hook it renders. The Text Collection is the caller that motivated it: a cell is rendered per resource, so subscribing inside the hook built one IntersectionObserver per resource, all watching the same document for the same answer, in every view mode including the two where the hook does nothing with it. The web view root now owns the call alongside scrRef — the other fact about this view that every cell needs — and it travels the same path: ScriptureTextGrid, through ResourceColumn where there is one, to ResourceCell. The hook's own test drops its platform-bible-react mock entirely. Visibility was only ever mocked because the hook reached for it internally and jsdom has no IntersectionObserver; as a parameter it is just a value the test passes, so the tests now exercise the unmodified module. Also restores Element.prototype.scrollIntoView after the test that spies on it. It is shared with every other test file in the worker, and leaving a spy behind is the kind of thing that makes unrelated suites fail intermittently. PT-4543. 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>
2b52631 to
d5d57a0
Compare
Correctness: - A chapter cell enables the reference scroll only once it is `ready` and its USJ names the reference's chapter (`isUsjForChapter`). A data hook raises `isLoading` in an effect, so the render after a navigation still held the old chapter: the scroll aimed at it, the loading placeholder's clamp read as the reader scrolling, and the hook stood down before the new chapter arrived. Re-enabling re-arms, which also covers switching the context resource. - A frame queued before the echo of the reader's own click no longer scrolls. - Verse 0 lands on the chapter's first laid-out block, i.e. the chapter top, as the editor, model text and reference panels do, instead of on verse 1. - The chosen marker is screened for a layout box, so a verse inside content the editor hides (an `\esb` sidebar, a `\periph`) leaves the port alone rather than drifting it toward the top. - A marker behind a horizontal scrollbar no longer counts as fully visible; the visible bottom is scaled from layout to on-screen pixels so it holds under pane zoom (checked in Chromium at 1x/1.5x/2x). - Reordering chapter columns re-arms each column's scroll (`rearmKey`, fed `orderIndex`), since the browser resets a moved scroll container to the top. - The chapter port turns scroll anchoring off, as the aligned grid root does, so only the hook, the reader, or a clamp moves `scrollTop`. - The tab icon reads the web view's one visibility answer via `pickTabIconUrl`, so the web view holds a single IntersectionObserver. Tests and records: - Cell tests for the chapter change, the resource switch, a bottom-clipped marker, the hidden-tab catch-up and a column move; a grid test that every chapter surface receives the real visibility; each mutation-checked. - `chapter-verse-marker.contract.test.tsx` runs the real finder against the real read-only editor, replacing bundle `toContain` checks that could not fail. Dead `useViewVisibility` stubs removed. - ADR and design doc corrected to what shipped; stale TSDoc, zoom docs and test comments fixed. PT-4543. Session-URL: https://claude.ai/code/879aca7d-ee3e-4ef1-848e-85910f61aaea Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@katherinejensen00 — worked through all 27 items; changes are in 1feb37e. Item numbers are yours.
|
cf62a6f to
9151195
Compare
katherinejensen00
left a comment
There was a problem hiding this comment.
Looks good. Thanks, Levi, for your hard work on text collection syncing! I would recommend just one more change but other than that this PR is good to go so going ahead and approving it.
- Medium, fix before merge: at any Text Collection zoom other than 100%, chapter views scroll the verse off screen. At 2× the view keeps jumping and never settles; measured in Electron. This is the bug your base-branch commit cf62a6f fixed for the aligned grid, and this PR is built on the commit before it. Rebasing and carrying that fix through the file rename, including where the 80px lead-in is subtracted, lands correctly at 1×, 1.5× and 2×.
@katherinejensen00 partially reviewed 46 files, made 1 comment, and resolved 1 discussion.
Reviewable status: 11 of 46 files reviewed, all discussions resolved.
Summary
The Text Collection's chapter surfaces render a whole chapter but never scrolled to the scroll
group's verse, so moving the reference appeared to do nothing on three surfaces: a chapter column,
the chapter-context split, and the single-resource full-width view (PT-4543, dup PT-4170). Verse
mode needs no scroll —
sliceUsjToVersehas already reduced the cell to the verse.Rather than write a second scroll implementation, this generalizes the controller PT-4184/#2781
wrote for the aligned grid. The layouts disagree about what represents a verse — a placed block
carrying
data-verse-startvs. a barespan[data-marker="v"][data-number]— so that lookup isinjected rather than branched on.
use-aligned-reference-scroll.hook.ts→use-reference-scroll.hook.tsandaligned-scroll.utils.ts→reference-scroll.utils.ts(bothgit mv).Why review this
Part of the Simple is coherent for Saroj epic. Moving the reference is the primary navigation
gesture in Simple mode, and on these three surfaces it silently did nothing — the Text Collection
showed a chapter that never followed the toolbar.
Stacked on #2781 — this PR targets
pt-4184-grid, so the diff shows only this PR's commits.Changes
useReferenceScrolltakes the layouts' differences as parameters:findTargetandisTargetVisible. The aligned grid takes both defaults and is unchanged.viewMode === 'chapter'.\v 14-15emits no[data-number="15"], so the finderfalls back to the nearest marker at or before the reference.
panels.
VERSE_NUMBER_SCROLL_OFFSET, the lead-in the other three views share.itself, so the cell's box never overflowed and every marker read as already visible. The feature
ran and did nothing there.
adr-one-reference-scroll-hook-parameterized-by-verse-anchor; design doc in.context/designs/.Hidden views (
.claude/rules/cross-view-sync-hidden-views.md) — please scrutiniseA hidden rc-dock pane has no layout, so scrolling while hidden would silently do nothing. The hook
defers, collapses repeats into one pending catch-up, and consumes it instantly on activation. In
Simple mode this is the common path — column 3 shows one tab at a time. Tests in
use-reference-scroll.hook.test.tsx.For reviewer judgment
zoomsat on[data-cell-content], which this PR makesthe scroll port — rect deltas and
scrollTopare then in different coordinate spaces. Measured inChromium: at 2× the scroll overshot ~7800px and never converged. It now lands on
[data-cell-pad]; rendering is measurably identical at every factor. The inline (verse) branchkeeps the old placement — it has no port — so the two branches now differ.
isEnabled. An earlier formforked
isBlockInPortViewon target height, which silently gave the Grid view full-containmentsemantics (a partly visible block you clicked would jump to the top). Reverted:
aligned-grid.component.tsxonly switches to the renamed hook, passing the block finder andkeeping the default visibility rule, so the Grid view scrolls exactly as before. Both rules are now
pinned on the same geometry.
<ChapterScrollSync>would removeisEnabledand itsthree guards, but is aesthetic-only.
findVerseMarkerForVersehas no zero-rect screen unlike theblock finder — no case is known, and the guard costs a rect read per candidate on a per-frame
scan. Both recorded in TSDoc/ADR.
Testing
npm run typecheck— cleanplatform-scripture-editor— cleannpm test— 1884 passing inplatform-scripture-editorAdded coverage for the cell→hook wiring, a finder-parity table so the layouts cannot drift on which
verse a reference resolves to, and a guard that
[data-cell-header]stays outside[data-cell-content](the adjacency the port math depends on).jsdom lays nothing out, so no unit test catches a broken flex chain — the class of bug behind the
single-resource fix. Worth a manual pass over all three chapter surfaces, at 100% and at a
non-default zoom.
Risk Level
Medium — three surfaces plus a shared hook and utils module #2781 also uses. Mitigated by that PR's
call sites being unchanged and its behavior pinned by its own tests.
Follow-ups (not here)
ContentZoomRootputs CSSzoomabove every scrollport, and
scrollPortToBlockadds rect (zoomed) deltas toscrollTop(unzoomed). The scrollovershoots by (Z − 1)×, and at 2× it never settles. This affects the Grid view and the chapter
cells alike, so the fix belongs in the shared function on PT-4184: Add the verse-aligned Grid view to the Text Collection #2781; see
PT-4184: Add the verse-aligned Grid view to the Text Collection #2781 (comment).
useViewVisibilitybuilds an observer per call though it always watchesdocument.body;memoizing would drop the
isViewVisibleprop drill and help 5 other consumers.ResourceColumnhas the same shrink-to-fit width issue fixed here for single-resource(pre-existing, PT-4184: Add the verse-aligned Grid view to the Text Collection #2781).
AI Involvement
AI-assisted — session
Claude Code drafted the implementation and tests under review at each step; the human developer is
the author and reviewed every change. A separate AI review pass raised 13 findings: 9 fixed, 3
resolved by recording a deliberate decision, 1 deferred as pre-existing. Behavioral claims were
verified by running the code — the zoom and layout defects above were measured in Chromium, not
inferred — and each new test was mutation-checked to confirm it fails when the behavior it pins is
removed.
🤖 Generated with Claude Code
This change is