Cut the continuous view's cost per navigation step - #235
Cut the continuous view's cost per navigation step#235alex-rawlings-yyc wants to merge 6 commits into
Conversation
alex-rawlings-yyc
added
the
up next
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughThe change centralizes token-chip accessibility labels in phrase-strip context and adds adaptive phrase-window sizing based on DOM measurements. It also stabilizes group refs, focus selection, arc measurements, and related tests. ChangesToken labels and adaptive layout
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR sizes the rendered strip to the viewport, but content-width changes can leave that window stale, causing too few or too many groups to render and potentially affecting navigation. Merge should wait for content-size invalidation or explicit owner acceptance of this bounded correctness risk. Sequence Diagram(s)sequenceDiagram
participant ContinuousView
participant PhraseStripParts
participant usePhraseWindowHalf
participant DOM
participant ResizeObserver
ContinuousView->>PhraseStripParts: render phrase groups with data-phrase-group marker
usePhraseWindowHalf->>DOM: measure viewport and phrase-group widths
usePhraseWindowHalf->>ResizeObserver: observe viewport resize
ResizeObserver->>usePhraseWindowHalf: report viewport change
usePhraseWindowHalf->>ContinuousView: return stepped and clamped half-window
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Size the strip's render window to the viewport instead of a fixed 100 groups each side, keep the per-group and per-slot memoization from being defeated by per-render callback identities, read each phrase box's geometry once per arc pass, and resolve token-chip labels once per strip rather than per chip. An arrow press drops from ~790ms to ~100ms on an 8th-gen i5.
The window is measured from the content the window itself mounts, so two sizes can each measure into the other, and React escalates the nested layout-effect updates into a "Maximum update depth exceeded" crash. A re-measure at an unchanged viewport width may now only widen the window — monotone and capped, so the sequence always settles — while a resize still narrows it to fit. The hook's test file becomes .tsx so the regression can mount a strip whose group count follows the window it measures.
a6d384c to
4783e48
Compare
Sizing the render window to the panel made its size follow the viewport, and a wider window mounts groups ahead of the focus as well as behind it. The scroll offset is untouched, so the focused group slides sideways by their combined width — on a panel drag, far enough to carry the phrase the reader is working on off the strip. No existing path corrects it: every one keys off a focus or option change, and the focus has not moved. The browser does not either, because scroll anchoring adjusts the block axis only while this strip scrolls on the inline axis. Re-center on the window size itself rather than on the resize, since the window also widens at an unchanged viewport width when taller content fires the measurement. The test double now records what each observer was pointed at, so the regression test can drive the window's observer without the centering hold's answering for it.
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
Review authored by Claude Opus 5 (1M context) (inspired by Devin), not reviewed by me.
Comments are on the render-window sizing, the token-chip label bundle, and the group ref-setter map, read at 4ca4b15. Prefix convention: :question: needs a decision or a confirmation from you, :pick: is a nitpick or optional.
| // focusPhraseIndex is intentionally excluded: it has its own scroll effect above. centerGroup | ||
| // is stable. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [phraseWindowHalf]); |
There was a problem hiding this comment.
❓ This corrects the synchronous shift, but it does not hold through the reflow that follows. holdCentered's own doc argues that mounting window groups is precisely what reflows asynchronously — 'the window mounts dozens of groups whose glosses, morpheme rows, and arcs finish laying out asynchronously over many frames, and each such reflow of the content left of the focus shifts the focused box sideways' — and both the instant-jump path and the committedActiveSegmentId path return holdCentered(focusPhraseIndex) for that reason. A drag-resize changes the window while the strip is otherwise idle, so no other hold is alive to absorb the late shift.
Should this effect return holdCentered(focusPhraseIndex) like its sibling, or is the post-resize reflow known not to move the inline axis (arc levels and stripTopPadding being vertical, gloss placeholder widths already resolved strip-wide, gutter paddings staying 0 in a single-row strip)? If it is the latter, worth saying so here, since the comment two effects up argues the opposite for the jump path.
| const renderWindowStart = Math.max(0, focusPhraseIndex - PHRASE_WINDOW_HALF); | ||
| const renderWindowEnd = Math.min(phraseGroups.length - 1, focusPhraseIndex + PHRASE_WINDOW_HALF); | ||
| const renderWindowStart = Math.max(0, focusPhraseIndex - phraseWindowHalf); | ||
| const renderWindowEnd = Math.min(phraseGroups.length - 1, focusPhraseIndex + phraseWindowHalf); |
There was a problem hiding this comment.
❓ groupTokens gives a discontiguous phrase one group per contiguous run, and arcs exist only between two boxes of the same phrase — so when a phrase's fragments sit more than phraseWindowHalf groups apart, the far one is unmounted, measurePhraseBoxes sees a single box for that phrase, and computeAllArcPaths draws no arc at all. That includes the leg that would have crossed the visible viewport, so the on-screen fragment loses its phrase cue entirely rather than just losing an off-screen anchor.
The gloss-input dedup has the same dependency: showGlossInput in PhraseStrip keys seenPhraseIds off the rendered items, so with the first fragment unmounted the input hops to whichever fragment is mounted first. With the old fixed 100 neither could happen in a real verse; at a measured 16-32 — and at 8 before the first measurement lands — both can.
Acceptable, or worth extending the window bounds to cover the full group span of any phrase link that intersects the window (bounded by phrase span, so cheap)? Deriving showGlossInput from the full phraseGroups list would at least make input ownership window-independent on its own.
| * keeps one identity for as long as the strip lives, so handing the callback down cannot | ||
| * invalidate a memoized child on a render that changed nothing else about it. | ||
| */ | ||
| const getGroupRefSetter = useCallback((groupIndex: number) => { |
There was a problem hiding this comment.
⛏️ What makes one setter identity per absolute index safe is React's commit ordering: refs detach in the mutation phase and attach in the layout phase for the whole commit, so a group moving into an index another group just vacated cannot write null over the newer element. That is the non-obvious WHY worth a line in this doc — and it warns off anyone later moving these writes into a layout effect, where the ordering guarantee no longer holds.
| const phraseRefs = useRef<(HTMLSpanElement | null)[]>([]); | ||
|
|
||
| /** Ref-setter callbacks for {@link phraseRefs}, keyed by the group index each one writes. */ | ||
| const groupRefSetters = useRef(new Map<number, (el: HTMLSpanElement | null) => void>()); |
There was a problem hiding this comment.
⛏️ The map grows one closure per group index and is never pruned, and ContinuousView has no key at its call site in Interlinearizer, so this map and the phraseRefs array both outlive book switches, sized to the largest book seen. Bounded and cheap, so entirely optional — but a clear() when the book changes costs nothing.
| const wanted = Math.ceil((groupsPerViewport * VIEWPORTS_PER_SIDE) / WINDOW_HALF_STEP); | ||
| const stepped = wanted * WINDOW_HALF_STEP; | ||
| const measured = Math.max(MIN_PHRASE_WINDOW_HALF, Math.min(stepped, MAX_PHRASE_WINDOW_HALF)); | ||
| const mayNarrow = viewportWidth !== measuredViewportWidthRef.current; |
There was a problem hiding this comment.
mayNarrow is true only when the viewport's own width moved, so nothing narrows the window when the content gets wider at an unchanged viewport width. Turning showMorphology on widens every group; the window sized for narrow chips then keeps mounting far more groups than the viewport needs — the case where the per-step cost this PR is cutting matters most.
The observer does fire on that toggle (default contentBox observation reports the height change, and the viewport is content-sized in height under the tw:items-center row), so remeasure runs and then declines to narrow. The widen-only rule is what guarantees termination, so relaxing it is not the fix; an explicit re-derivation on a content-affecting option change is — e.g. a generation param the view bumps for showMorphology/simplifyPhrases that clears measuredViewportWidthRef, so the next measurement reads as a new constraint rather than an echo.
| const mayNarrow = viewportWidth !== measuredViewportWidthRef.current; | ||
| measuredViewportWidthRef.current = viewportWidth; | ||
| setWindowHalf((prev) => { | ||
| const next = mayNarrow ? measured : Math.max(prev, measured); |
There was a problem hiding this comment.
Because the observer fires on height changes too — as 4ca4b15's message puts it, 'the window also widens at an unchanged viewport width when taller content fires the measurement' — this Math.max makes the window a high-water mark of every transient geometry measured at a fixed viewport width. Each stripTopPadding application during arc settling fires a remeasure, and a measurement taken while fewer groups are mounted reads a smaller mean group width and so asks for a bigger window. Only a viewport width change ever resets the mark, and the ceiling is MAX_PHRASE_WINDOW_HALF (120) — above the fixed 100 this PR replaces, so a ratcheted strip can end up mounting more than before.
Two ways out, either of which keeps termination: ignore height-only resize entries so only width changes re-derive, or measure per-group width from a bounded sample near the focus so the measurement stops depending on how many groups are mounted — which also dissolves the two-sizes-each-measure-into-the-other pathology that forced the widen-only rule in the first place.
| // unbounded per-group width and clamp the window straight to its maximum — mounting the whole | ||
| // book. Leave the window as it is until there is something real to divide. | ||
| if (contentWidth <= 0) return; | ||
| const viewportWidth = viewport.clientWidth; |
There was a problem hiding this comment.
❓ clientWidth here can move without the panel being resized. The strip viewport is styled { overflowX: 'hidden', overflowY: 'visible' } in ContinuousView, and per the CSS overflow pairing rule a visible value computes to auto when the other axis is neither visible nor clip — so that element is a vertical scroll container, not the overflow-visible box the declaration reads as. A vertical scrollbar appearing would shrink clientWidth, which line 85 treats as a genuine resize rather than an echo, leaving scrollbar-appears -> narrow -> scrollbar-disappears -> widen free to move both ways, outside the widen-only rule this hook's doc relies on for termination.
I could not confirm the height feedback that would close that loop (items-center leaves the viewport content-sized, and SVG ink overflow does not create scrollable overflow), so this is a question rather than a finding: was overflowY: 'visible' meant to compute to auto? overflowX: 'clip' is not a substitute — the element has to stay a scroll container for scrollIntoView to center a phrase in it — but overflowY: 'hidden', or an explicit auto with a note, would say what actually happens.
| acceptSuggestion: strings[TOKEN_CHIP_LABEL_KEYS.acceptSuggestion], | ||
| promoteSuggestion: strings[TOKEN_CHIP_LABEL_KEYS.promoteSuggestion], | ||
| }), | ||
| [strings], |
There was a problem hiding this comment.
Memoizing on the whole record makes this bundle's identity depend on the localization hook returning a stable object. Everywhere else in the codebase the hook's result is destructured into individual strings, which are stable by value whatever the record's identity; this is the first place the identity itself gates memoization.
The real hook holds resolved data in useState, so it is stable in the normal case — but its error path returns a freshly built defaultState on every render (isPlatformError(localizedStrings) ? defaultState : localizedStrings), and that covers the 'updated 100 times in the last 1000 milliseconds' PlatformError branch in createUseDataHook. In that state tokenChipLabels, the whole strip context value, and every labels prop churn each render, silently undoing the memoization this PR adds. Depending on the seven strings individually removes the coupling.
Related: __mocks__/papi-frontend-react.ts builds a fresh record on every call, so under Jest the context value never holds its identity either — a render-count assertion written against chips or slots would be measuring the mock rather than production behavior. Caching the mock's record by key set would make such a test meaningful.
Measuring the whole row made the reading depend on the window it fed, so the window could only ever widen and ratcheted up on every transient reflow. Sampling a fixed run of groups around the focus lets it narrow again, including when morpheme rows widen the content at an unchanged panel width. Also mounts every fragment of a phrase the window touches, so a discontiguous phrase keeps its arc, decides gloss-input ownership from the phrase's own token order rather than the rendered items, and holds the focus centered through the reflow a resize sets off.
A hold armed by a window resize or an active-segment flip kept re-centering the group it captured for up to two seconds, so a navigation inside that window was overridden and the strip stayed parked on the previous phrase.
A phrase's gloss input sat on the fragment matching its stored first token, so a baseline edit that stranded that ref left the phrase with no gloss field anywhere. Own it by the phrase's earliest token the book still has.
2 participants
Size the strip's render window to the viewport instead of a fixed 100 groups each side, keep the per-group and per-slot memoization from being defeated by per-render callback identities, read each phrase box's geometry once per arc pass, and resolve token-chip labels once per strip rather than per chip. An arrow press drops from ~790ms to ~100ms on an 8th-gen i5.
This change is
Summary by CodeRabbit
New Features
Improvements