Highlights auth/write flow: xstate statechart + sign-in dialog (2/4)#285
Merged
Conversation
🦋 Changeset detectedLatest commit: 353d526 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
Collaborator
Author
|
Superseded by #288, which consolidates the full YPE-1034 stack into a single PR rebased onto main. |
cameronapak
force-pushed
the
claude/ype-1034-pr1-highlights-api
branch
from
July 20, 2026 20:15
5b7d334 to
7f66a99
Compare
cameronapak
force-pushed
the
claude/ype-1034-pr2-highlight-auth-flow
branch
from
July 20, 2026 20:15
f275bbf to
879e04e
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}
cameronapak
force-pushed
the
claude/ype-1034-pr2-highlight-auth-flow
branch
2 times, most recently
from
July 21, 2026 16:17
75bdf08 to
d26b1f4
Compare
cameronapak
force-pushed
the
claude/ype-1034-pr2-highlight-auth-flow
branch
from
July 21, 2026 16:43
d26b1f4 to
10f47c6
Compare
Turn a color tap without a session or the `highlights` permission into an applied highlight across the two auth paths, behind the internal HIGHLIGHTS_LIVE flag. Core: DataExchangeClient (POST /data-exchange/token, Zod-validated), the hosted-grant URL builder + callback parser/handler, and an optimistic permission cache on YouVersionPlatformConfiguration seeded from `granted_permissions` on the sign-in and data-exchange callbacks (server 401/403 invalidates it). SignInWithYouVersionResult gains `permissions`. Hooks: useHighlightAuthActions exposes one-fell-swoop sign-in (requesting `highlights`), the just-in-time data-exchange redirect, permission reads/invalidation, and the data-exchange return handler. UI: useBibleReaderHighlights runs the state machine — pending highlights persist to sessionStorage (~10-min expiry) across the redirect round-trip and apply on a granted return; a permission confirm dialog (copy matched to the native SDK, en/fr/es) gates the grant. Write failures route by status (401/403 re-prompts and keeps pending; 5xx/network reverts and discards). Apply/remove writes are serialized through a FIFO queue with per-verse ownership, closing the two concurrency windows PR1 documented. Copy/share-only behavior with no auth provider is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on hydrates The resume effect consumed the data-exchange return status into a local, but bailed on `!isAuthenticated` first. The shipped YouVersionAuthProvider hydrates userInfo asynchronously, so the first effect run after a redirect return is always unauthenticated: the status was consumed and lost, and when the session flipped the cancel/failure branch was skipped — the pending highlight survived a decline and the just-declined dialog re-opened. Discard the pending highlight at consume time for any non-granted return, before the auth gate: it runs exactly once and needs no session (a decline kills the intent regardless of who signs in). Tests: the cancel-return test previously set signedIn=true synchronously — a timing the real provider never produces — which hid the bug. It now mounts signed out and flips the session after mount (failed against the old code, passes now), plus a failure-status variant with the same timing and the granted-return test aligned for realism. Also document the three review-deferred follow-ups (expired-token 401 misrouting, resume-write failure handling, remove-failure re-prompt) at their code sites and in the changeset. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- ui: replace hardcoded 'This app' sign-in fallback with i18n key `signInAppNameFallback` (en/es/fr), computed via `t()` in BibleReader. - ui: settle/scope-change now clean up the highlights machine's per-verse `writeIntent` claims — `settleWrite` releases a verse's claim when the settling op still owns it, and `resetForScopeChange` clears `writeIntent` so a stale-scope write can't pollute the new scope's reconcile/overlay under a colliding verse number. Adds direct machine unit tests. - core: `SignInWithYouVersionResult.permissions` is now `readonly`; grants are computed before construction and passed through the constructor in Users.handleAuthCallback instead of post-construction mutation. - core: correct the stale `setGrantedPermissions` docstring (it is a wholesale replace; the data-exchange callback uses saveGrantedPermissions). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The DataExchangeTokenResponseSchema was defined inline in data-exchange.ts; core convention keeps Zod schemas in schemas/ as the single source of truth. No schema-shape or public entry-point export changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Pin xstate to exact 5.32.5 (supply-chain decision); dedupes an orphaned
esbuild@0.25.11 in the lockfile.
- Add non-mutating peekPendingHighlight and use it in the machine guards so
xstate guards stay pure; the clear-on-expiry side effect stays in the
consume/action paths.
- Export scopesEqual from the machine and drop the duplicate scopesMatch copy
in the adapter.
- Extract a shared claimVerses helper for the three write-claim assign blocks.
- Narrow consumeDataExchangeReturn status to the real union and read the write
actor's DoneActorEvent instead of an ad-hoc { output } shape.
- Initialize the services ref with a real value (latest-ref pattern), removing
the null-as-unknown double cast.
- Extract shared Radix dialog chrome into components/ui/dialog.tsx and refactor
both dialogs onto it (byte-equivalent markup).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cameronapak
force-pushed
the
claude/ype-1034-pr2-highlight-auth-flow
branch
from
July 21, 2026 17:07
10f47c6 to
65a2ab2
Compare
cameronapak
commented
Jul 21, 2026
…ose permission Make the pending-highlight sessionStorage stash multi-entry so two queued optimistic writes that each fail 401/403 both survive the re-grant. A single-slot stash let the second permission-denied settle overwrite the first, silently losing the earlier tap's highlight after the data-exchange redirect. - stashPendingHighlight replaces the whole list (fresh tap supersedes); appendPendingHighlight merges with verse-level last-wins. - peekPendingHighlights (pure) / readPendingHighlights (clears stale) return live entries; per-entry timestamp drives per-entry expiry. - settleWrite appends on permission-denied apply; applyPendingHighlight resumes every entry, first-to-last, each to its own scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tash A user apply reaching the write queue via startApplyWrite never stashes an intent of its own, so the network/5xx branch in settleWrite had nothing to drop. The only entries in the stash mid-writing are ones a sibling batch appended via the 401 path moments earlier — clearing them there wiped a permission-lost intent that must survive the re-grant, so nothing resumed. Remove the clearPendingHighlight() call from the non-permission failure path; the decline/failure consume, resume-consume, redirect-failure catches, and dialog cancel/decline actions remain the intended owners of the clear. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
thank you sir greppy |
jaredhightower-youversion
left a comment
jaredhightower-youversion
left a comment
Collaborator
There was a problem hiding this comment.
Ponytail pass (over-engineering only) — all non-blocking. Correctness was already well covered by the existing threads.
Dustin-Kelley
previously approved these changes
Jul 21, 2026
Addresses the 18 accepted items from the over-engineering review pass: drop the unused SignInWithYouVersionResult.permissions field and setGrantedPermissions (permission cache is the single source of truth), share formatPassageId from usfm-ranges, inline DialogOverlay, simplify pending-highlight merge and parseGrantedPermissions, unsort updateToken payload, module-level consts in useHighlightAuthActions, vi.waitFor and real-writer seeding in tests, drop redundant restores/clears/duplicate tests, and trim internal tracking from docs and changesets. Declined as stack-owned: getAuthToken dedupe and serverColorsEqual removal (both superseded by PR 294's hardening commits). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jaredhightower-youversion
previously approved these changes
Jul 21, 2026
jaredhightower-youversion
left a comment
jaredhightower-youversion
left a comment
Collaborator
There was a problem hiding this comment.
Approved but there is one small comment from greptile.
…to minor useHighlights.createHighlight/deleteHighlight lost their per-call auto-refetch in this PR (batch writes coalesce to one GET). The hook is public API and 2.3.0 ships the old behavior, so document the new contract as JSDoc on the return type and reclassify the hooks changeset as minor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jaredhightower-youversion
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
The full highlights interaction flow — auth, data-exchange permission, writes, resume — is an xstate v5 statechart, with a SignInDialog for signed-out color taps. This presents the machine in its final reviewed form; the staging fixes from old #287 are embedded, so that PR stays closed.
Stack: 1/4 #283 → 2/4 (this PR) → 3/4 #286 → 4/4 #294 · restored from #288
Changes
SignInWithYouVersionViewas a dialog, not a bottom sheet) before OAuth;appNameandsignInPromptMessageare new optional statics onYouVersionPlatformConfiguration.bible-reader-highlights-machine.ts(xstate v5) owns the whole flow — fetch, permission, write batches, pending-write resume, sign-out — anduseBibleReaderHighlightsbecomes a thin adapter; mermaid statechart atdocs/highlight-flow-statechart.md.{userId, permissions}in localStorage.highlightsInteractivegating. (xstateis exact-pinned to 5.32.5 in this PR.)Start here: change 2 — the statechart is the review unit; the mermaid doc mirrors its states.
Test plan
pnpm build && pnpm typecheck && pnpm lint && pnpm testat this branch: all green (889 tests — core 342 / hooks 284 / ui 263).pnpm check:i18npasses (warnings only: es/fr keys pending upstream sync).Greptile Summary
This PR wires the full highlights auth flow — signed-out color tap → sign-in dialog → OAuth redirect → pending resume; signed-in without permission → just-in-time data-exchange flow — into an xstate v5 statechart (
bible-reader-highlights-machine.ts), replacing the hand-rolled effect orchestration.useBibleReaderHighlightsbecomes a thin adapter;SignInDialogis ported from the Swift SDK; and fixes absorbed from staging (#287, #288) are embedded here.bible-reader-highlights-machine.ts): parallelflow+writerregions handle the auth dialogs, serialized write queue, per-verse ownership tokens, and the vapor fix (remove overlay entries never retire, preventing stale-replica ghosts).pending-highlight.ts): upgraded from single-slot to a list with verse-level last-wins merge so multiple concurrent 401-failing writes each survive the re-grant.YouVersionPlatformConfiguration): user-scoped{userId, permissions}in localStorage; seeded from both sign-in and data-exchange callbacks; invalidated on 401/403; cleared on sign-out.useHighlightsrefetch removal:createHighlight/deleteHighlightno longer auto-refetch; callers coalesce and callrefetch()once after the batch settles.Confidence Score: 5/5
Safe to merge. The statechart, permission cache, and pending-highlight list are all correctly implemented; previous review feedback is fully incorporated.
All major edge cases are handled and tested: concurrent 401-failing writes both survive the re-grant (verse-level last-wins merge), stale-replica ghost highlights are suppressed (remove overlay entries never retire), scope changes synchronously clear ownership tokens so stale writes cannot pollute new-scope state, and the permission cache is correctly user-scoped. The three previously flagged bugs have verified fixes with new direct-machine tests. Deferred items are explicitly documented with follow-up tracking.
No files require special attention. bible-reader-highlights-machine.ts is the review unit per the PR description and holds up well under scrutiny.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant U as User participant BR as BibleReader participant M as HighlightsMachine participant SS as SessionStorage participant LS as LocalStorage participant API as YouVersion API Note over M: boot to enabled (flow + writer in parallel) U->>BR: tap color (signed out) BR->>M: TAP_COLOR M->>SS: stashPendingHighlight M-->>BR: "lastTapOutcome = flow" Note over M: flow to signInDialog U->>BR: confirm sign-in BR->>M: CONFIRM_SIGN_IN M->>API: startSignInForHighlights (full-page redirect) API-->>BR: redirect back with code + granted_permissions BR->>LS: saveUserInfo then saveGrantedPermissions Note over M: new page load resuming M->>SS: consumeDataExchangeReturn (null) Note over M: pending + authed + hasPermission to applyPendingHighlight M->>SS: readPendingHighlights + clearPendingHighlight M->>API: createHighlight API-->>M: 200 OK M->>API: refetch%%{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 U as User participant BR as BibleReader participant M as HighlightsMachine participant SS as SessionStorage participant LS as LocalStorage participant API as YouVersion API Note over M: boot to enabled (flow + writer in parallel) U->>BR: tap color (signed out) BR->>M: TAP_COLOR M->>SS: stashPendingHighlight M-->>BR: "lastTapOutcome = flow" Note over M: flow to signInDialog U->>BR: confirm sign-in BR->>M: CONFIRM_SIGN_IN M->>API: startSignInForHighlights (full-page redirect) API-->>BR: redirect back with code + granted_permissions BR->>LS: saveUserInfo then saveGrantedPermissions Note over M: new page load resuming M->>SS: consumeDataExchangeReturn (null) Note over M: pending + authed + hasPermission to applyPendingHighlight M->>SS: readPendingHighlights + clearPendingHighlight M->>API: createHighlight API-->>M: 200 OK M->>API: refetchReviews (8): Last reviewed commit: "docs(hooks): contract-note the no-auto-r..." | Re-trigger Greptile