Allow users to interlinearize "verse 0" of books#123
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds end-to-end verse-0 (chapter superscription) support: ChangesVerse-0 Superscription Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
alex-rawlings-yyc
force-pushed
the
verse-zero
branch
from
c67b43c to
d13523b
Compare
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
Devin (https://app.devin.ai/review/sillsdev/interlinearizer-extension/pull/123) caught a bug, but the code otherwise looks good to me. I'm still doing manual testing.
@imnasnainaec reviewed 14 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
|
I believe I addressed that bug, but please let me know if I'm wrong about that. |
|
@alex-rawlings-yyc I think the extension's handling of verse 0 and the global ref selectors handling could be better connected. In one direction, clicking in or out of a verse 0 segment should update the global ref accordingly. In the other direction, if you're on verse 1 and click < in the global ref handler to go back a verse, it shows verse 0 but the extension doesn't match. (I recognize it might be a more significant refactor of how we avoid unnecessary jumps to actual get those synced up.) |
|
@imnasnainaec I told Claude to investigate and here's what it came up with. I don't think the refactor will be too much work. Let me know what you think: Verse 0 ↔ global scroll-group reference syncDesign note on two related desync bugs between the interlinearizer's verse-0 (chapter BackgroundA chapter's pre-verse-1 content (chiefly Psalm superscriptions) is modeled as a synthetic verse-0 The global reference is the scroll-group scripture reference the extension reads/writes via the Relevant host (paranext-core) factsTwo facts from
This corrects an earlier assumption in our code comments that "the host scroll-group reference cannot The two bugsDirection 1 — clicking in/out of a verse-0 segment does not update the global refSelecting a verse-0 segment (or focusing a token inside one) should move the global reference to that
The verse-0 token stays focusable, but the selection never leaves the extension, so other panels in This is the tractable half: the host accepts a verse-0 write and will display it. The write was Direction 2 — host
|
| Delivery | Should be | Today |
|---|---|---|
Host re-broadcasting the chapter as verseNum:0 after a real verse nav (spurious echo) |
ignored | ignored ✅ |
User clicking < from v1 to land on v0 (intentional) |
honored | ignored ❌ |
They cannot be told apart by value. The only distinguishing signal is provenance/timing: the
spurious echo is an unsolicited follow-up to a navigation the extension itself just caused; the
intentional < is a fresh external action with no pending internal navigation.
Why Direction 2 is the hard one
The current verse-0 stickiness is a blunt value-match that predates / sidesteps the extension's
existing provenance machinery. That machinery already solves exactly this "is this host delivery my
own echo or a real external move?" problem for ordinary verses:
pendingInternalNavRef+consumeInternalNav
(InterlinearNavContext.tsx:257-289) record a
TTL-stamped marker per internal navigation and consume it when the matching host echo returns.navigate(ref, 'internal')records the marker; an unmarked (or expired) delivery is treated as a
genuine external move.
Verse 0 simply never got routed onto this path — it uses the value shortcut instead.
Proposed direction
Unify verse 0 onto the existing internal-nav arbitration rather than inventing new state:
-
Replace the blanket verse-0 stickiness with the provenance check used elsewhere. A
verseNum:0-same-chapter delivery is suppressed only when it correlates with a recently emitted
internal nav to that chapter (the post-nav echo). Absent a fresh marker, treat it as a real
external move to verse 0 — which flows to the loader, where
InterlinearizerLoader.tsx:245-249 already
resolves verse 0 to the superscription segment when one exists (and to verse 1 otherwise). -
Re-enable the verse-0 write in
focusToken/handleSegmentSelect(drop theverse === 0
guard), and havenavigate(ref, 'internal')record the marker for the verse-0 key so the resulting
host echo is consumed normally — closing Direction 1 without reintroducing the chapter bounce.
This is a refactor of which path verse 0 takes, not new machinery: verse 0 joins the same
marker-based echo arbitration every other verse already uses.
Open risk to decide before implementing
Because the <-from-v1 echo and the post-verse-nav chapter echo are byte-identical, even a
marker-based scheme has a narrow race: if a user clicks < to verse 0 immediately after an internal
nav to that same chapter (marker still fresh), the intentional verse-0 move would be eaten as an echo.
The window is small, but it is inherent to the host emitting indistinguishable references, so it
should be decided deliberately rather than discovered later. Options:
- Accept the race (rare; the user can re-click
<). - Shorten the verse-0 marker TTL specifically.
- Track echo provenance more precisely (e.g. only suppress the first same-chapter verse-0 within a
tighter window of an internal nav).
Files
- src/components/Interlinearizer.tsx —
focusToken,
handleSegmentSelect(Direction 1 write guards) - src/components/InterlinearNavContext.tsx —
liveScrRef
verse-0 stickiness,pendingInternalNavRef/consumeInternalNav(Direction 2 arbitration) - src/components/InterlinearizerLoader.tsx —
activeScrRef
verse-0 → superscription resolution (already correct; the destination both directions feed)
|
@alex-rawlings-yyc I'd say the Direction 1 recommendations look good for this pr. The subtlety and subjectivity of Direction 2 inclines me to just copy those details over to a new issue to handle independently. |
|
@alex-rawlings-yyc ...but if you have a clear preference for implementing Direction 2 now, you're welcome to go for it now. |
|
@imnasnainaec sounds good. I think making Dir. 2 a separate issue is a good call. I'll write up the issue and get Dir 1 finished. |
2 participants
This change is
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests