Wire BibleReader highlights to the highlights API (YPE-1034) (1/4)#283
Merged
Conversation
🦋 Changeset detectedLatest commit: a36263a The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This was referenced Jul 10, 2026
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dark-launch flag Replace the localStorage highlight store (YPE-642 stand-in) with real API wiring per YPE-1034 ADR-001: highlights are server-only account data, deleted locally with no migration. - New internal useBibleReaderHighlights seam: fetches the current chapter's highlights via useHighlights (chapter USFM passage_id, keyed on version), renders through an in-memory optimistic overlay, reverts + console.error on write failure (toasts and 401/403 handling are PR 2) - Contiguous verse runs collapse to range USFMs on the wire (usfm-ranges.ts, mirroring the verse-share run-grouping idiom); colors sent as lowercase hex - Internal HIGHLIGHTS_LIVE dark-launch flag (off, not exported from the package entry) gates fetches, writes, and rendering; setHighlightsLive() is the test/Storybook-only override - Auth-gated: reads YouVersionAuthContext directly (now exported from hooks) so a missing auth provider degrades to signed-out instead of the useYVAuth throw; sign-out un-renders highlights immediately - better-result added to the ui package for error typing at the new seam's write boundary; core's throwing clients are untouched Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… responses Adversarial review of the highlights wiring surfaced one blocker and two races, all rooted in useApiData: - The fetch effect ignored enabled transitions: a hook that mounted disabled (auth still resolving) never fetched once enabled flipped true, so a signed-in reader rendered zero highlights until a write or navigation. enabled now rides alongside the caller-supplied deps. - Disabling kept the last response, so stale account data could render across sign-out or a host-controlled auth user switch. Disabling now clears data and error. - refetch-initiated requests escaped cancellation: a stale refetch for a previous chapter resolving late could clobber the new chapter's data. All requests now go through a monotonic sequence; only the latest-issued request may commit state. In the BibleReader seam hook: - Successfully settled writes now hand their verses to a confirmed set whose overlay entries are dropped when the post-write refetch lands, so server truth wins again instead of the optimistic entry masking later remote changes until navigation. - Documented the two remaining apply/remove concurrency windows (in-flight POST vs DELETE ordering, snapshot revert vs concurrent write) at the write boundary; a real operation queue is deliberately PR 2. New tests: useApiData enabled transitions and stale-response handling, and a seam-hook integration test through the real useHighlights/useApiData path (module-level mocks had hidden the enabled-flip bug). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cameronapak
force-pushed
the
claude/ype-1034-pr1-highlights-api
branch
from
July 20, 2026 20:15
5b7d334 to
7f66a99
Compare
cameronapak
changed the title
cameronapak
marked this pull request as ready for review
July 20, 2026 20:18
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…lay assertions Cross-scope contamination: confirmedVersesRef enrolled a settled write's verses regardless of whether the user had since navigated to another chapter/version, letting the drain effect erase the new scope's optimistic overlay entry (verse numbers collide across scopes). Capture the overlay scope at write start and only enroll on settle if it still matches the current scope ref; also clear the confirmed set on scope change alongside the existing render-time overlay reset. Also replace two non-null assertions (`overlay[verse]!`, `snapshot[verse]!`) with `as string | null` casts: null means "optimistically removed" in HighlightOverlay, so `!` stripped a legitimate value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The success path already skipped enrolling verses in confirmedVersesRef when the write's captured scope no longer matched. The failure path had the same gap: after navigating chapters, a stale write's revertOverlay ran against a snapshot that predates the reset, so its absent entries would `delete` the new chapter's optimistic verses (verse numbers collide across scopes). Gate both revertOverlay calls on `overlayScopeRef.current === scopeAtWrite`, matching the success path. console.error logging stays unconditional. Update the invariant comment to cover both settle paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jaredhightower-youversion
left a comment
jaredhightower-youversion
left a comment
Collaborator
There was a problem hiding this comment.
Did a simplification-focused pass over this PR — not looking for bugs (the earlier greptile threads covered those and they're all resolved; I haven't re-raised any of them), just places where the diff could be smaller and easier to maintain. Found five. None of them are blockers, but together they'd cut roughly 70 lines and one dependency, and each comment below explains what's going on and includes the replacement code so it should be quick to action.
Collaborator
There was a problem hiding this comment.
Added conventional comments format, all non-blocking.
Dustin-Kelley
left a comment
Dustin-Kelley
left a comment
Collaborator
There was a problem hiding this comment.
I'm pretty happy with this! I think a lot of lines will be removed when/if we add a proper query library in the future. Just address Jareds comment's and I can approve.
- use Promise.withResolvers in useApiData tests - collapse feature-flags.ts to minimal shape - trim CONTEXT.md glossary to implemented terms Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dustin-Kelley
approved these changes
Jul 21, 2026
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
3 participants
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.
Summary
BibleReader renders a signed-in user's server highlights, dark-launched behind the
HIGHLIGHTS_LIVEflag (off by default). Restored from consolidated #288 and rebased onto current main.Stack: 1/4 (this PR) → 2/4 #285 → 3/4 #286 → 4/4 #294 · review the stack bottom-up
Changes
HIGHLIGHTS_LIVEis enabled — no visible change while the flag is off.useApiDatahardening in hooks).Test plan
pnpm build && pnpm typecheck && pnpm lint && pnpm testat this branch: all green (core 315 / hooks 281 / ui 211).Greptile Summary
This PR wires BibleReader's highlight system to the server-side highlights API behind an internal
HIGHLIGHTS_LIVEdark-launch flag (currentlyfalse), replacing the temporary localStorage store with a server-only model. It also hardensuseApiDatawith a monotonic sequence number to suppress stale responses from both effect-initiated and refetch-initiated requests, and exportsYouVersionAuthContextfor consumers that must tolerate a missing auth provider.useApiDatahardening: replaces per-effectcanceledclosures with a sharedrequestSeqRef; addsenabledto effect deps so afalse\u2192trueflip fires the fetch; clearsdata/erroron disable to prevent stale account data from lingering across sessions.useBibleReaderHighlights: new seam hook with optimistic overlay, scope-gated confirmed-verse drain, auth-tolerant context read, and dark-launch gate. All previous-review bugs are fixed and regression-tested.usfm-ranges.ts: pure utility for collapsing verse lists into USFM range strings (JHN.3.16-18), fully unit-tested.Confidence Score: 5/5
Safe to merge — the entire highlight path is dark-launched behind HIGHLIGHTS_LIVE=false, so no changed behavior reaches production until the flag flips.
The two observations are both narrow and flag-gated: the stale captured-closure refetch interaction only matters while HIGHLIGHTS_LIVE is on, and the module-level flag state is only mutable via an @internal test helper. The core logic is correct and thoroughly tested.
useApiData.ts and use-bible-reader-highlights.ts warrant a second look before HIGHLIGHTS_LIVE is flipped: the stale-closure refetch race should be resolved in PR 2 as planned.
Important Files Changed
canceledclosure with a monotonicrequestSeqRefsequence number; addsenabledto effect deps; clears data/error on disable. Subtle interaction with stale captured closures fromcreateHighlightflagged as a known deferred concurrency issue.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant BR as BibleReader participant UBH as useBibleReaderHighlights participant UH as useHighlights participant UAD as useApiData participant API as Highlights API Note over BR,API: Flag ON + signed-in BR->>UBH: "mount {versionId, book, chapter}" UBH->>UH: useHighlights passage_id JHN.3 enabled true UH->>UAD: fetchFn getHighlights UAD->>API: GET highlights JHN.3 API-->>UAD: verse highlights data UAD-->>UBH: highlightedVerses UBH-->>BR: highlightedVerses map Note over BR,API: User taps color BR->>UBH: apply color verses UBH->>UBH: patchOverlay optimistic UBH->>UH: createHighlight range USFM UH->>API: POST highlights API-->>UH: 201 Created UH->>UAD: refetch UAD->>API: GET highlights JHN.3 API-->>UAD: updated data UBH->>UBH: drain confirmed overlay entries Note over BR,API: Sign-out UBH->>UH: enabled false UAD->>UAD: setData null UBH-->>BR: highlightedVerses empty%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant BR as BibleReader participant UBH as useBibleReaderHighlights participant UH as useHighlights participant UAD as useApiData participant API as Highlights API Note over BR,API: Flag ON + signed-in BR->>UBH: "mount {versionId, book, chapter}" UBH->>UH: useHighlights passage_id JHN.3 enabled true UH->>UAD: fetchFn getHighlights UAD->>API: GET highlights JHN.3 API-->>UAD: verse highlights data UAD-->>UBH: highlightedVerses UBH-->>BR: highlightedVerses map Note over BR,API: User taps color BR->>UBH: apply color verses UBH->>UBH: patchOverlay optimistic UBH->>UH: createHighlight range USFM UH->>API: POST highlights API-->>UH: 201 Created UH->>UAD: refetch UAD->>API: GET highlights JHN.3 API-->>UAD: updated data UBH->>UBH: drain confirmed overlay entries Note over BR,API: Sign-out UBH->>UH: enabled false UAD->>UAD: setData null UBH-->>BR: highlightedVerses emptyReviews (4): Last reviewed commit: "refactor: apply non-blocking review sugg..." | Re-trigger Greptile