Hoist focus state into a dedicated store with origin at the call site - #267
Draft
imnasnainaec wants to merge 1 commit into
Draft
Hoist focus state into a dedicated store with origin at the call site#267imnasnainaec wants to merge 1 commit into
imnasnainaec wants to merge 1 commit into
Conversation
The focused word token was `useState` in `Interlinearizer`, threaded as props into both views, with six writers — three of them coordinated by hook declaration order — and each consumer reconstructing where a change came from by comparing prop values against refs kept for the purpose. Move it to a `FocusProvider` mounted inside `Interlinearizer` below the book indexes: - Every write records a `FocusOrigin` at the call site, and one table documents what each origin asks of the strip and of the list. The two readings of "internal" that were explained twice, in prose, in two files are now one table with two columns. - The seed and the three reseed rules collapse into a single effect that branches in priority order, so an outside focus request outranks both reseeds by rule rather than by source-file layout. Both "keep this declared above" comments are gone, as is the closure-age argument the claim depended on. - Focus rides `useSyncExternalStore`, so an event-time reader takes the current value through a getter without subscribing, and a focus move re-renders only the views that read it — not `Interlinearizer`, and not the loader above it. Deleted: the `onFocusedTokenRefChange` prop and its ref mirror, the in-flight internal marker, and the render-phase block that cleared it. `lastDisplayUpdateWasInternalRef` stays, since something still has to carry the fade/glide decision across the fade timer, but it now carries a declared origin instead of inferring one. Behavior is unchanged, including the reseed guard that leaves the strip still for an external navigation within the focused segment. Two strip tests were rewritten rather than ported: both asserted on a state only the prop round-trip could produce (an internal nav in flight that the parent never echoed). They now cover the same invariants — resetting the step count on a focus the strip did not choose, and the window falling back to the live focus when the displayed ref names a token this book lacks — through states the store can actually reach. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements #263.
focusedTokenRefwasuseStateinInterlinearizer, threaded as props into both views. Six places wrote it, three of them coordinated by hook declaration order, and each consumer reconstructed where a change came from by comparing prop values against refs kept for the purpose.What changed
src/components/FocusStore.tsx(new) owns the focused token for one mounted book:FocusOrigin(seed/strip/list/reseed/request), and the type's doc comment is the one place that says what each origin asks of the strip and of the list. The two readings of "internal" that were explained twice, in prose, inContinuousView.tsxanduseSegmentWindow.tsare now one table with two columns; both files point at it.Keep this declared above the focus-request claim belowcomments are gone, along with the closure-age argument the claim's precedence rested on.useSyncExternalStore, so the readers that only need the value at event time take it throughuseFocusGetter()without subscribing, and a focus move re-renders only the views that read it — notInterlinearizer, and notInterlinearizerLoaderInnerabove it.focusTokenkeeps its navigation pairing. It still pushes the internal navigation when the token lives in another verse, which is why the provider is mounted insideInterlinearizer(it needs the book indexes and the nav surface together) rather than up in the loader.Deleted: the
onFocusedTokenRefChangeprop and its ref mirror,internalFocusedTokenRefRef, and the render-phase block that cleared it.onSelectprop-threading fromInterlinearizergoes too —SegmentListViewtakes it from the store.Kept, deliberately:
lastDisplayUpdateWasInternalRef— something still has to carry the fade-or-glide decision across the fade timer that separates the focus-change effect from the scroll effect; it now carries a declared origin instead of inferring one. The pending-index resync also stays, gated onorigin !== 'strip'instead of on an in-flight marker.Deviation from the issue
The issue proposed a
seqcounter alongside the value, to distinguish two writes naming the same ref. The store instead drops a write naming the token already focused, which matches thesetStatebail-out the old code got for free and makes a sequence number dead weight — so there isn't one.Behavior
Unchanged, including the reseed guard that leaves the strip completely still for an external navigation within the focused segment. That guard and its comment stay exactly where they were in the rule order.
Test plan
npm test— 1920 passing, 69 suites, 100% coverage held (statements / branches / functions / lines).npm run lint— clean (eslint, stylelint, tsc, dependency scope).npm run build— clean.FocusStore.test.tsxcovers the store primitives, the three hooks (including each throwing outside a provider), seeding, both actions' navigation decisions, and every resolution rule — including a focus request beating the verse reseed that lands in the same commit, and an unresolvable request warning and falling through to it.Interlinearizer.test.tsxthat a focus move inside the active verse re-rendersInterlinearizerzero times.Interlinearizertests pass with no changes to any test body — the mockedContinuousViewreads the store instead of props.Two strip tests were rewritten rather than ported, because both asserted on a state only the prop round-trip could produce — an internal nav in flight that the parent never echoed. They now cover the same invariants through states the store can actually reach: resetting the step count on a focus the strip did not choose, and the window falling back to the live focus while the displayed ref names a token this book lacks.
Not in scope
displayContinuousScrollstays on its upward callback.Interlinearizer's own render reads it to decide whether to mount the strip, so with the provider below it, reading from the store would makeInterlinearizera focus-rate subscriber and cancel the re-render win. It also isn't focus state.🤖 Generated with Claude Code
This change is