[Feature Branch] Implement Highlights on the React Native Expo SDK 🖍️ - #100
[Feature Branch] Implement Highlights on the React Native Expo SDK 🖍️#100cameronapak wants to merge 46 commits into
Conversation
28cb320 to
5890153
Compare
5890153 to
e36cf62
Compare
f5257fc to
32910ee
Compare
Desired End State — locked for #100 / v1Kody Bot on behalf of Cam. Summary from a reconciliation pass with Ticket Griller and Critique Researcher. Written with the simple-english skill. Ship targetPR #100 is the ship target for Highlights v1 on the RN Expo SDK. PR #104 is example-only. It will not merge. In scope for #100
Out of scope for v1 (later follow-ups)
Architecture notesCuts between orchestrator and permission-flow layouts can differ. Those differences are not missing product pieces for Desired End State. Swift checkThe YouVersion Swift SDK on Bottom line#100 matches Desired End State for v1 when #129 is in the stack. Keep YPE-4499 separate. |
…PE-4169) (1/3) (#97) * chore: add .claude/ to .gitignore to exclude Claude-related files from version control * feat(core): wrap platform-core HighlightsClient with RN token auth Adopt @youversion/platform-core@2.3.0 so native can get/create/delete highlights with an explicit access token and typed Result failures (auth vs transient), without exporting the surface from the package index yet. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(example): request highlights as AuthPermission, not a scope The auth server drops unknown OIDC scopes; wire permissions:['highlights'] and keep createHighlightsApi off the package barrel (relative example import). Also restore main .gitignore (drop unrelated .claude ignore) and harden createHighlight failure-path tests. Co-authored-by: Cursor <cursoragent@cursor.com> * revert(example): remove local highlights Profile harness from PR Dev-only simulator buttons and permissions wiring were for local testing, not part of YPE-4169. Co-authored-by: Cursor <cursoragent@cursor.com> * chore: add changeset for internal highlights client wrapper Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(core): use descriptive Result generic names Rename single-letter type params to Value/Error for clearer intent. Co-authored-by: Cursor <cursoragent@cursor.com> * test(core): cover 5xx paths for create and delete highlights Co-authored-by: Cursor <cursoragent@cursor.com> * chore: update .gitignore to include .claude/ directory for exclusion --------- Co-authored-by: Cursor <cursoragent@cursor.com>
… (2/3) (#98) * chore: add .claude/ to .gitignore to exclude Claude-related files from version control * feat(core): wrap platform-core HighlightsClient with RN token auth Adopt @youversion/platform-core@2.3.0 so native can get/create/delete highlights with an explicit access token and typed Result failures (auth vs transient), without exporting the surface from the package index yet. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(example): request highlights as AuthPermission, not a scope The auth server drops unknown OIDC scopes; wire permissions:['highlights'] and keep createHighlightsApi off the package barrel (relative example import). Also restore main .gitignore (drop unrelated .claude ignore) and harden createHighlight failure-path tests. Co-authored-by: Cursor <cursoragent@cursor.com> * revert(example): remove local highlights Profile harness from PR Dev-only simulator buttons and permissions wiring were for local testing, not part of YPE-4169. Co-authored-by: Cursor <cursoragent@cursor.com> * chore: add changeset for internal highlights client wrapper Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(core): use descriptive Result generic names Rename single-letter type params to Value/Error for clearer intent. Co-authored-by: Cursor <cursoragent@cursor.com> * test(core): cover 5xx paths for create and delete highlights Co-authored-by: Cursor <cursoragent@cursor.com> * chore: update .gitignore to include .claude/ directory for exclusion * feat(core): add MMKV Server Colors highlights cache Sync get/set/clear for Highlight Scope snapshots so Subtask 3 can hydrate without fetching in this layer; purge all yvp.highlights.* keys on sign-out. Co-authored-by: Cursor <cursoragent@cursor.com> * feat(core): add MMKV Server Colors highlights cache Sync get/set/clear for Highlight Scope snapshots so Subtask 3 can hydrate without fetching in this layer; purge all yvp.highlights.* keys on sign-out. Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(highlights): update highlights caching and schema handling * chore: add changeset for internal highlights cache Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…d request_id (YPE-4192) (#99) * chore: add .claude/ to .gitignore to exclude Claude-related files from version control * feat(core): wrap platform-core HighlightsClient with RN token auth Adopt @youversion/platform-core@2.3.0 so native can get/create/delete highlights with an explicit access token and typed Result failures (auth vs transient), without exporting the surface from the package index yet. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(example): request highlights as AuthPermission, not a scope The auth server drops unknown OIDC scopes; wire permissions:['highlights'] and keep createHighlightsApi off the package barrel (relative example import). Also restore main .gitignore (drop unrelated .claude ignore) and harden createHighlight failure-path tests. Co-authored-by: Cursor <cursoragent@cursor.com> * revert(example): remove local highlights Profile harness from PR Dev-only simulator buttons and permissions wiring were for local testing, not part of YPE-4169. Co-authored-by: Cursor <cursoragent@cursor.com> * chore: add changeset for internal highlights client wrapper Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(core): use descriptive Result generic names Rename single-letter type params to Value/Error for clearer intent. Co-authored-by: Cursor <cursoragent@cursor.com> * test(core): cover 5xx paths for create and delete highlights Co-authored-by: Cursor <cursoragent@cursor.com> * chore: update .gitignore to include .claude/ directory for exclusion * feat(core): add MMKV Server Colors highlights cache Sync get/set/clear for Highlight Scope snapshots so Subtask 3 can hydrate without fetching in this layer; purge all yvp.highlights.* keys on sign-out. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(core): shim crypto.randomUUID so RN highlight creates send a valid request_id platform-core's HighlightsClient mints the API-required request_id via the global crypto.randomUUID, absent on RN Hermes (Expo SDK 56). It silently falls back to a yvp-<hex> id the highlights API rejects with 422 (uuid_parsing), breaking every create from React Native (reads/deletes carry no request_id and are unaffected). Install an idempotent, self-installing shim backing crypto.randomUUID with expo-crypto (already a core dep; the same native UUID source used in installation-id.ts) before any platform-core client runs, so creates send a real RFC-4122 v4 UUID. Mirrors ui/lib/dom-local-storage.ts. Only randomUUID is shimmed; a native implementation is never overridden. Bridge until platform-core exposes an injectable request_id generator upstream. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(core): pin the crypto shim wiring in the highlight create path The existing request_id assertion passes on any runtime that already has crypto.randomUUID, which Node >= 19 and therefore CI does, so it could not tell whether createHighlightsApi installs the shim at all. Add a case that drops the crypto global to reproduce RN Hermes and asserts the id came from expo-crypto. Verified to have teeth: commenting out the ensureCryptoRandomUUID call fails it with the production symptom, "yvp-19fa3d69052-637b519a", while the pre-existing regex assertion still passes. Also record why the null half of the cryptoScope guard stays: it is there for TypeScript narrowing, not runtime paranoia, and removing it needs a non-null assertion that ESLint rejects in source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(core): shim crypto.randomUUID so RN highlight creates send a valid request_id platform-core's HighlightsClient mints the API-required request_id via the global crypto.randomUUID, absent on RN Hermes (Expo SDK 56). It silently falls back to a yvp-<hex> id the highlights API rejects with 422 (uuid_parsing), breaking every create from React Native (reads/deletes carry no request_id and are unaffected). Install an idempotent, self-installing shim backing crypto.randomUUID with expo-crypto (already a core dep; the same native UUID source used in installation-id.ts) before any platform-core client runs, so creates send a real RFC-4122 v4 UUID. Mirrors ui/lib/dom-local-storage.ts. Only randomUUID is shimmed; a native implementation is never overridden. Bridge until platform-core exposes an injectable request_id generator upstream. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(core): pin the crypto shim wiring in the highlight create path The existing request_id assertion passes on any runtime that already has crypto.randomUUID, which Node >= 19 and therefore CI does, so it could not tell whether createHighlightsApi installs the shim at all. Add a case that drops the crypto global to reproduce RN Hermes and asserts the id came from expo-crypto. Verified to have teeth: commenting out the ensureCryptoRandomUUID call fails it with the production symptom, "yvp-19fa3d69052-637b519a", while the pre-existing regex assertion still passes. Also record why the null half of the cryptoScope guard stays: it is there for TypeScript narrowing, not runtime paranoia, and removing it needs a non-null assertion that ESLint rejects in source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(core): record the accepted trade-off in the crypto shim header The header claimed a partial getRandomValues/subtle shim would be a worse footgun than a missing one, which inverts the YPE-4192 risk analysis and does not hold up: expo-crypto's getRandomValues is a real native CSPRNG, and defining globalThis.crypto at all already creates the partial surface. State the actual trade instead -- a library gating on the crypto object rather than the method loses its fallback -- why it is accepted, and when to revisit. Addresses review feedback on the ticket/code divergence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(core): share one crypto-global fixture between the shim suites SHIM_UUID, the crypto teardown, and the globalThis cast were duplicated across api.test.ts and ensure-crypto-uuid.test.ts. Move them to src/test-utils, beside the ui package's existing test-utils and outside __tests__ so jest does not collect the helper as a suite. Centralizing the teardown is the real win: globalThis.crypto is process-wide, so a restore that misses leaks a stubbed crypto into later suites. The shared helper also restores the original property descriptor rather than the value, which ensure-crypto-uuid.test.ts was not doing -- Node defines crypto as an accessor, and putting back a plain data property changes the global's shape. The jest.mock('expo-crypto') line stays per-file; jest hoists it, so sharing it would need a require() inside the factory for no real gain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…nt cache (YPE-3708) (3/3) (#101) * chore: add .claude/ to .gitignore to exclude Claude-related files from version control * feat(core): wrap platform-core HighlightsClient with RN token auth Adopt @youversion/platform-core@2.3.0 so native can get/create/delete highlights with an explicit access token and typed Result failures (auth vs transient), without exporting the surface from the package index yet. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(example): request highlights as AuthPermission, not a scope The auth server drops unknown OIDC scopes; wire permissions:['highlights'] and keep createHighlightsApi off the package barrel (relative example import). Also restore main .gitignore (drop unrelated .claude ignore) and harden createHighlight failure-path tests. Co-authored-by: Cursor <cursoragent@cursor.com> * revert(example): remove local highlights Profile harness from PR Dev-only simulator buttons and permissions wiring were for local testing, not part of YPE-4169. Co-authored-by: Cursor <cursoragent@cursor.com> * chore: add changeset for internal highlights client wrapper Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(core): use descriptive Result generic names Rename single-letter type params to Value/Error for clearer intent. Co-authored-by: Cursor <cursoragent@cursor.com> * test(core): cover 5xx paths for create and delete highlights Co-authored-by: Cursor <cursoragent@cursor.com> * chore: update .gitignore to include .claude/ directory for exclusion * feat(core): add MMKV Server Colors highlights cache Sync get/set/clear for Highlight Scope snapshots so Subtask 3 can hydrate without fetching in this layer; purge all yvp.highlights.* keys on sign-out. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(core): shim crypto.randomUUID so RN highlight creates send a valid request_id platform-core's HighlightsClient mints the API-required request_id via the global crypto.randomUUID, absent on RN Hermes (Expo SDK 56). It silently falls back to a yvp-<hex> id the highlights API rejects with 422 (uuid_parsing), breaking every create from React Native (reads/deletes carry no request_id and are unaffected). Install an idempotent, self-installing shim backing crypto.randomUUID with expo-crypto (already a core dep; the same native UUID source used in installation-id.ts) before any platform-core client runs, so creates send a real RFC-4122 v4 UUID. Mirrors ui/lib/dom-local-storage.ts. Only randomUUID is shimmed; a native implementation is never overridden. Bridge until platform-core exposes an injectable request_id generator upstream. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(core): pin the crypto shim wiring in the highlight create path The existing request_id assertion passes on any runtime that already has crypto.randomUUID, which Node >= 19 and therefore CI does, so it could not tell whether createHighlightsApi installs the shim at all. Add a case that drops the crypto global to reproduce RN Hermes and asserts the id came from expo-crypto. Verified to have teeth: commenting out the ensureCryptoRandomUUID call fails it with the production symptom, "yvp-19fa3d69052-637b519a", while the pre-existing regex assertion still passes. Also record why the null half of the cryptoScope guard stays: it is there for TypeScript narrowing, not runtime paranoia, and removing it needs a non-null assertion that ESLint rejects in source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(core): shim crypto.randomUUID so RN highlight creates send a valid request_id platform-core's HighlightsClient mints the API-required request_id via the global crypto.randomUUID, absent on RN Hermes (Expo SDK 56). It silently falls back to a yvp-<hex> id the highlights API rejects with 422 (uuid_parsing), breaking every create from React Native (reads/deletes carry no request_id and are unaffected). Install an idempotent, self-installing shim backing crypto.randomUUID with expo-crypto (already a core dep; the same native UUID source used in installation-id.ts) before any platform-core client runs, so creates send a real RFC-4122 v4 UUID. Mirrors ui/lib/dom-local-storage.ts. Only randomUUID is shimmed; a native implementation is never overridden. Bridge until platform-core exposes an injectable request_id generator upstream. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(core): pin the crypto shim wiring in the highlight create path The existing request_id assertion passes on any runtime that already has crypto.randomUUID, which Node >= 19 and therefore CI does, so it could not tell whether createHighlightsApi installs the shim at all. Add a case that drops the crypto global to reproduce RN Hermes and asserts the id came from expo-crypto. Verified to have teeth: commenting out the ensureCryptoRandomUUID call fails it with the production symptom, "yvp-19fa3d69052-637b519a", while the pre-existing regex assertion still passes. Also record why the null half of the cryptoScope guard stays: it is there for TypeScript narrowing, not runtime paranoia, and removing it needs a non-null assertion that ESLint rejects in source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(core): record the accepted trade-off in the crypto shim header The header claimed a partial getRandomValues/subtle shim would be a worse footgun than a missing one, which inverts the YPE-4192 risk analysis and does not hold up: expo-crypto's getRandomValues is a real native CSPRNG, and defining globalThis.crypto at all already creates the partial surface. State the actual trade instead -- a library gating on the crypto object rather than the method loses its fallback -- why it is accepted, and when to revisit. Addresses review feedback on the ticket/code divergence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(core): share one crypto-global fixture between the shim suites SHIM_UUID, the crypto teardown, and the globalThis cast were duplicated across api.test.ts and ensure-crypto-uuid.test.ts. Move them to src/test-utils, beside the ui package's existing test-utils and outside __tests__ so jest does not collect the helper as a suite. Centralizing the teardown is the real win: globalThis.crypto is process-wide, so a restore that misses leaks a stubbed crypto into later suites. The shared helper also restores the original property descriptor rather than the value, which ensure-crypto-uuid.test.ts was not doing -- Node defines crypto as an accessor, and putting back a plain data property changes the global's shape. The jest.mock('expo-crypto') line stays per-file; jest hoists it, so sharing it would need a require() inside the factory for no real gain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(core): useHighlights — optimistic highlight writes over an instant cache (YPE-3708) (3/3) Composes the client wrapper (1/3) and the MMKV cache (2/3) into the public hook. Paints from the cache synchronously on first render, applies and removes optimistically, reconciles against the server, and reverts writes that fail. Overlay math lives in a pure, React-free `optimistic.ts` ported from the web highlights machine, so both SDKs agree on what the user sees mid-write: - per-op ownership tokens, so a slow failure cannot wipe paint a newer write put down; - remove overlays that survive a stale replica echoing back the colour we just deleted ("vapor"), with a colour-aware retirement rule that is a deliberate, one-line-revertible divergence from web — see ADR 0013; - web's wire pattern: ranged POSTs per contiguous run, one DELETE per verse. Writes hold through the token-loading window rather than reporting not-signed-in for a user who is genuinely signed in, `error` stays fetch-only so a write failure cannot evict a live fetch error, and non-auth 4xx re-classify as `invalid` rather than `transient` so a permanent failure stops presenting as flaky network. Beyond the ticket, called out as intentional: `refresh()` and `isRefreshing` (named for "a GET is in flight" — `highlights` is always safe to render). The API wrapper, the MMKV cache, and the Result seam stay internal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(core): unify the highlight write path and close two test gaps Review follow-ups on the useHighlights commit. The apply and remove branches carried the same tally loop and remove built its passage id by hand, which contradicted the comment above it claiming both paths route through the same helper. Both now derive `{passageId, verses}` units and share one tally, so the ternary that picks ranged POSTs vs per-verse DELETEs really is the single call site to change if range DELETE is ever confirmed server-side. Rename `normalizeVerses` to `normalizeVerseSelection`: the plan rejected porting web's `normalizeVerses`, which is a different function private to `verse-share.ts`. Ours exists because writes need the canonical verse list while `collapseVerseRuns` yields runs — the header now says so. Adds the two tests the plan asked for and the first pass missed: a layer-1 reset case (asserting a cleared writeIntent makes a stale-scope settle a no-op) and a refresh landing mid-write, which must reconcile rather than clobber the optimistic overlay. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(core): update useHighlights hook version to patch * refactor(core): remove internal comments from index.ts and streamline exports * fix(core): guard user identity in useHighlights hook * refactor(core): improve state management in useHighlights hook --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* chore(deps): update Web SDK packages to 2.4.0
Bumps `@youversion/platform-core` (core, 2.3.0) and
`@youversion/platform-react-ui` (UI, 2.2.0) to 2.4.0, which also pulls
`@youversion/platform-core` and `@youversion/platform-react-hooks` 2.4.0
into the UI subtree — one copy of each now resolves across the workspace.
2.4.0 carries the contracts the native highlights track is built against:
BibleReader's controlled highlights mode (YPE-3705), the `ApiClient` fix
that reads an empty-body 2xx (a successful highlight DELETE) as success,
and the data-exchange primitives the just-in-time `highlights` permission
grant needs.
`platform-react-ui@2.4.0` requires `better-result`, which ships ESM only
(`type: module`, `.mjs`, no CJS build), so two UI suites importing a value
from the Web SDK barrel failed to parse. Allowing it through
`transformIgnorePatterns` is not enough on its own: jest-expo's transform
key is `\.[jt]sx?$`, which never matches `.mjs`, so the file reached the
CJS runtime untransformed. The UI jest config moves from package.json to
jest.config.js so the added `.mjs` entry can reuse the preset's own
babel-jest options rather than duplicate its absolute paths.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(ui): keep the DOM reader in controlled highlights mode
Web SDK 2.4.0 changed what an absent `highlights` prop means. It used to
select a localStorage-backed demo — a no-op on native, where our
localStorage is a per-WebView in-memory shim — and now selects the live,
server-backed path. Combined with the access token `dom-apply` already
hands the WebView, a color tap wrote a real highlight to the user's
account, and a missing `highlights` permission could redirect the reader
WebView to the hosted consent page.
Passing `highlights={[]}` latches controlled mode at first mount, which
makes the highlight slice a pure projection: no highlights API calls, no
local store, no auth surface from the highlight path. That restores the
invariant that native owns highlights (locked decision 1) until U1
(YPE-3710) wires the prop to real `useHighlights` data — presence is
constant across both, so the mode never toggles.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…PE-4169) (1/3) (#97) * chore: add .claude/ to .gitignore to exclude Claude-related files from version control * feat(core): wrap platform-core HighlightsClient with RN token auth Adopt @youversion/platform-core@2.3.0 so native can get/create/delete highlights with an explicit access token and typed Result failures (auth vs transient), without exporting the surface from the package index yet. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(example): request highlights as AuthPermission, not a scope The auth server drops unknown OIDC scopes; wire permissions:['highlights'] and keep createHighlightsApi off the package barrel (relative example import). Also restore main .gitignore (drop unrelated .claude ignore) and harden createHighlight failure-path tests. Co-authored-by: Cursor <cursoragent@cursor.com> * revert(example): remove local highlights Profile harness from PR Dev-only simulator buttons and permissions wiring were for local testing, not part of YPE-4169. Co-authored-by: Cursor <cursoragent@cursor.com> * chore: add changeset for internal highlights client wrapper Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(core): use descriptive Result generic names Rename single-letter type params to Value/Error for clearer intent. Co-authored-by: Cursor <cursoragent@cursor.com> * test(core): cover 5xx paths for create and delete highlights Co-authored-by: Cursor <cursoragent@cursor.com> * chore: update .gitignore to include .claude/ directory for exclusion --------- Co-authored-by: Cursor <cursoragent@cursor.com>
* chore(deps): update Web SDK packages to 2.4.0
Bumps `@youversion/platform-core` (core, 2.3.0) and
`@youversion/platform-react-ui` (UI, 2.2.0) to 2.4.0, which also pulls
`@youversion/platform-core` and `@youversion/platform-react-hooks` 2.4.0
into the UI subtree — one copy of each now resolves across the workspace.
2.4.0 carries the contracts the native highlights track is built against:
BibleReader's controlled highlights mode (YPE-3705), the `ApiClient` fix
that reads an empty-body 2xx (a successful highlight DELETE) as success,
and the data-exchange primitives the just-in-time `highlights` permission
grant needs.
`platform-react-ui@2.4.0` requires `better-result`, which ships ESM only
(`type: module`, `.mjs`, no CJS build), so two UI suites importing a value
from the Web SDK barrel failed to parse. Allowing it through
`transformIgnorePatterns` is not enough on its own: jest-expo's transform
key is `\.[jt]sx?$`, which never matches `.mjs`, so the file reached the
CJS runtime untransformed. The UI jest config moves from package.json to
jest.config.js so the added `.mjs` entry can reuse the preset's own
babel-jest options rather than duplicate its absolute paths.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(ui): keep the DOM reader in controlled highlights mode
Web SDK 2.4.0 changed what an absent `highlights` prop means. It used to
select a localStorage-backed demo — a no-op on native, where our
localStorage is a per-WebView in-memory shim — and now selects the live,
server-backed path. Combined with the access token `dom-apply` already
hands the WebView, a color tap wrote a real highlight to the user's
account, and a missing `highlights` permission could redirect the reader
WebView to the hosted consent page.
Passing `highlights={[]}` latches controlled mode at first mount, which
makes the highlight slice a pure projection: no highlights API calls, no
local store, no auth surface from the highlight path. That restores the
invariant that native owns highlights (locked decision 1) until U1
(YPE-3710) wires the prop to real `useHighlights` data — presence is
constant across both, so the mode never toggles.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* chore(deps): update Web SDK packages to 2.5.0 Bumps @youversion/platform-react-ui (packages/ui) and @youversion/platform-core (packages/core) from 2.4.0 to 2.5.0, pulling platform-core and platform-react-hooks 2.5.0 with them so a single copy of each resolves across the workspace. 2.5.0 swaps the reader's serif face from Source Serif 4 to Untitled Serif, which required two native-side changes: - reader-fonts mirrors the new UNTITLED_SERIF_FONT stack and carries it over the bridge as an `untitled-serif` token. The Web SDK's picker now emits that stack, and without a token for it encodeFontFamilyForDom passes the raw quoted string across the bridge — the exact input that corrupts @expo/dom-webview's prop injection on iOS and renders the reader blank (ADR 0009). SOURCE_SERIF_FONT stays, deprecated, so values persisted by earlier versions still encode to a known token. - The reader settings store defaults to Untitled Serif and migrates a persisted Source Serif value on read. The Web SDK runs that migration itself only when fontFamily is uncontrolled; we always pass it controlled, so the reader would have kept the deprecated stack and matched neither button in the picker. Also corrects the AGENTS.md cooldown section: pnpm 11.11 verifies the committed lockfile against minimumReleaseAge on every install, including --frozen-lockfile, so CI is not exempt as previously documented. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore: exempt @youversion/* from the minimumReleaseAge cooldown The 3-day cooldown mitigates hijacked third-party releases by giving the ecosystem time to spot one. For the Web SDK packages we publish ourselves it provides little of that signal and blocks us from consuming our own work on release day — which is what held up the 2.5.0 bump. pnpm's minimumReleaseAgeExclude accepts scope globs, so '@youversion/*' covers platform-core, platform-react-hooks, and platform-react-ui. Verified against `pnpm install --frozen-lockfile`, the command CI runs, which now passes the lockfile policy check. The tradeoff is deliberate: a compromised YouVersion npm token would reach our builds with no waiting period, so these packages rely on publish-side controls rather than this cooldown. Every third-party dependency keeps the full 3 days. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… (YPE-3709) (1/3) (#112) * feat(core): granted_permissions read-back + per-user permission cache (YPE-3709) (1/3) Parse granted_permissions off the OAuth app redirect (before the /auth/callback hop, which drops it), cache it per user in MMKV, and expose grantedPermissions / hasPermission / invalidatePermissions on the auth context, seeded synchronously so the first render answers correctly. AuthPermission widens to an open union so server-side additions never read as denials. Lean reimplementation replacing PR #105: no sign-in epoch guard (the pre-existing race is filed in .claude/bugs/auth-session-commit-serialization.md with its structural fix), no isSameUser branching (the user-scoped cache read makes user switches self-healing), and ADR 0014 trimmed to the decision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(auth): simplify comments in auth context and permission handling * chore(core): address review feedback on granted-permissions read-back Follow-ups from code review of #112. No behaviour change. - Cite the gateway OpenAPI spec for the three-state grant contract. The `null` / `[]` / populated model is the server's documented contract, not an inference, but no live denial has been measured (the YPE-3706 spike never ran). Note the dependency explicitly on the `else` branch that restores a cached grant, since that branch is only correct while a denial arrives as an empty value rather than an absent key. - Add a parity test for GRANTED_PERMISSIONS_KEY_PATTERN. The pattern restates one that lives inline in platform-core's parseGrantedPermissions; platform-core exports neither the pattern nor a presence-detecting helper, so the duplication cannot be removed from this side. The test drives both through their public API over 15 key spellings and fails if they drift. - Convert the new granted-permissions tests to userEvent per AGENTS.md. Pre-existing fireEvent calls elsewhere in the file are left alone. - Rewrite the changeset in plain language, and correct it: the AuthPermission open union does not stop unknown permissions reading as denials. That is done by grantedPermissions being typed readonly string[] | null and readGrantedPermissions returning unfiltered strings. The union only lets AuthConfig.permissions and hasPermission() accept unknown strings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
…/3) (#113) * feat(core): just-in-time data-exchange permission grant (YPE-3709) (2/3) Restacked onto the lean subtask-1 redo (PR #112). Content is PR #106's, with three adaptations: grant-cache imports follow the module split (granted-permissions-cache.ts), mergeGrantedPermissions lands in the cache module with its tests, and the id-less-user test now pins the redo's semantic — the grant is honored in memory but never persisted under a null userId. The data-exchange ADR is renumbered 0014 → 0015 (0014 is the cached-grant-is-a-hint ADR from subtask 1). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(auth): replace epoch with sessionId in identity management * docs(auth): enhance initiator guard and clarify identity management * refactor(auth): review fixes for the data-exchange grant Five fixes from review of PR #113. No change to the grant flow's shape. - Split "another request is already running" out of `transient` into its own `in-progress` reason. `transient` is the reason callers retry on immediately, and an immediate retry lands back in the same branch while the consent page is still open. `in-progress` says the actionable thing. - Refresh the access token before minting. Refresh was otherwise driven only by bootstrap and the AppState `active` handler, so a long foreground session could carry an expired token into the mint. That 401s, and every mint 401 reads as `not-permitted` — telling the user their app key is misconfigured when the token was merely stale. The token is read from a new ref, falling back to the render closure so a session cleared mid-flow still reaches the initiator guard and reports `user-changed` rather than changing contract. - Correct AGENTS.md and README.md on concurrency. Both claimed overlapping callers get the same outcome; a differing permission set never did, and a test already pinned the real behaviour. - Fix stale docs: the `getCurrentUserId` JSDoc (renamed to `getCurrentIdentity`) and CONTRIBUTING.md's `Linking.createURL` example, which no longer matches the example app's explicit `{ scheme }` form. - Extract `toMessage(caught)` to `src/error-message.ts`, replacing three copies of the `instanceof Error` ternary. typecheck, lint, and prettier pass; core is 306 tests across 20 suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(auth): data exchange returns to the app's redirectUri An approved permission grant was silently discarded on Android. Verified on device (Pixel 6 Pro API 34, real app key): the user consents, the server confirms the grant, and `requestPermissions` resolves `cancel` with nothing written to the cache. `requestDataExchange` passed a hardcoded `youversionauth://callback` to `openAuthSessionAsync`, on the premise that the return scheme is SDK-owned and unrelated to the app's OAuth `redirectUri`. The hosted consent page in fact returns to whatever callback URL is registered for the app key, so the auth session waited on a URL that never arrived, reported `dismiss`, and the grant was lost. Every outcome — granted, denied, and error alike — collapsed to `cancel`. An app key has exactly one callback URL (confirmed with the API team), and sign-in already owns it, so a separate SDK-owned return URL cannot exist alongside it: registering one instead breaks sign-in with `invalid_request: redirect_uri does not match registered callback URL`. `requestDataExchange` now takes `redirectUri` and the provider passes `config.redirectUri`, so both browser round-trips share one URL — which is what Swift (`Users+SignIn.swift`, `DataExchangeSession.swift`) and Kotlin (`DEFAULT_AUTH_CALLBACK`) already do. `DATA_EXCHANGE_RETURN_URL` is deleted. The example app and docs use `youversionauth://callback` as that single value, matching the native SDKs, with `"scheme": "youversionauth"` in `app.json`. The `scheme` array, the "register youversionauth in addition to your own scheme" instruction, and the `Linking.createURL` ordering hazard all go away with the second scheme. ADR 0015 is rewritten rather than deleted — it records the measurements, the one-callback-URL constraint, and why the original parity claim was wrong. Verified end to end after the change: sign-in and the grant both return through `youversionauth://callback`, and `hasPermission('highlights')` flips to true. typecheck, lint, and prettier pass; core is 306 tests across 20 suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: align CONTEXT.md and wording with the redirectUri model `d3df9d1` changed where data exchange returns but left the ubiquitous language behind. CONTEXT.md still described the return URL as "the hardcoded, SDK-owned `youversionauth://callback`", and its _Avoid_ line told readers not to call it a redirect URI "because the app's OAuth `redirectUri` is a different, app-owned thing" — which is now exactly backwards. They are the same URL. Also drops "SDK-owned" from the README and the example app's comment. The value is a convention shared with Swift and Kotlin, but calling it SDK-owned is what led to treating it as separate from `redirectUri` in the first place. Audited the rest of docs/adr: no other ADR references the return URL, the redirect, or data exchange, so nothing else went stale. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
* feat(core): guarded highlight permission flow (YPE-3709) (3/3) Restacked onto the rewritten subtask 1 (#105) and subtask 2 (#106) branches. Content is PR #107's, applied without semantic changes; only the AGENTS.md exports line needed a hand-merge with the redo's additions (KnownAuthPermission, granted-permissions-cache split). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(core): map the in-progress failure reason in toWriteReason Rebase integration, not new behavior. #113 split `in-progress` out of `transient` in `DataExchangeFailureReason` after this branch was cut, so `toWriteReason`'s exhaustive switch stopped covering the union and the build failed with TS2366. `in-progress` joins `transient`: a write refused for holding the consent flow open is retryable, just not this instant, and `HighlightWriteReason` has no finer bucket. The flow already reports an overlapping tap as `transient` for the same reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(core): join an in-flight token refresh instead of skipping it `refreshToken` tracked its in-flight request with a boolean, so a second caller returned immediately rather than waiting — resolving on the very token the refresh existed to replace. The trigger is ordinary, not exotic: the app foregrounds, the `AppState` listener starts a refresh, and the user acts a moment later. Anything auth-sensitive in that window read the expired token and got a 401. For the highlight permission flow that 401 classifies as `auth`, `auth` reads as a stale grant, and the user is asked to grant a permission they already granted. Hold the request as a promise and hand it to the second caller, matching how `inFlightRequestRef` already shares an in-flight data exchange in this same file. `ensureFreshToken` now means what its name says, so its "does not guarantee a fresh token" caveat goes away rather than needing somewhere to live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(core): scope the pending highlight to the chapter it was tapped in `PendingHighlight.scope` was written twice and read nowhere, so the field documented an invariant the code did not hold. The render-time RESET and the generation token only protect a flow that already exists. Two windows open before one does, and both replay through `highlightsRef`, which follows the reader: 1. Reader on JHN.3, permission cached, user taps verses 16-18. 2. The write goes out bound to JHN.3. Correct so far. 3. Reader moves to JHN.4. RESET runs, but there is no flow and no waiting caller, so the generation token is not bumped. 4. The write comes back `reason: 'auth'`. A corrective flow opens, capturing JHN.4 as the scope and keeping verses 16-18. 5. Consent is granted, and the highlight lands on JHN.4:16-18 — text the user never selected. The pre-flight `ensureFreshToken()` round-trip is the same class, and its window is widest exactly when a refresh is actually due. Claim the scope before each await and compare it before replaying. The write-refused case still drops the stale grant, because that part was right; it just stops re-prompting for a passage the reader has left, and resolves the caller with the write's own outcome. The abandoned-tap case resolves `noop`, like every other abandonment. Both windows are regression-tested, and the `useHighlights` mock now derives its scope from the options it was rendered with — a fixed scope let the leak through unnoticed. Also shares one copy of `NOT_SIGNED_IN_MESSAGE` between the write path and the flow that wraps it, rather than two that can drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: record the permission flow decisions in ADR 0016 Three decisions of ADR weight landed in AGENTS.md: the pre-flight branch point over reason-first, the exactly-once re-prompt bound, and the in-memory pending highlight. Adjacent decisions of the same weight got ADRs 0013, 0014, and 0015, and AGENTS.md itself says `docs/adr/` is where architectural decisions live. Move them there, with the alternative each one rejects and the residual each one accepts, and leave AGENTS.md pointing at it. Records the scope guard as load-bearing, and drops three references to `.claude/bugs/auth-provider-expired-access-token.md` — `.claude/` is gitignored and no such file exists, so one of them shipped in the published `.d.ts` telling consumers to read something they cannot see. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * perf: paint a highlight on tap, not after the token refresh `useHighlightPermissionFlow.apply` awaited `ensureFreshToken()` before it reached the code that paints. Whenever a refresh was actually due — the access token at or inside its 60-second leeway, or a refresh already running from the `AppState` foreground listener — the user tapped a colour and watched nothing happen for a full token round-trip. The refresh moves into `useHighlights.runWrite`, next to the existing `waitForAuthSettled()`. The token is still current when the request goes out, which is what keeps a 401 from being misread as a stale permission grant, but the optimistic claim now paints on tap. `remove` and direct `useHighlights` consumers pick up the same freshness guarantee, which previously only `apply` had. `apply` is synchronous up to its branch, so the guard comparing `pending.scope` across the pre-flight await is gone — the window it covered cannot open. The dev harness stopped gating its swatches on the full round-trip, which was hiding the optimistic paint entirely, and now reports writes in flight and the tap-to-settle time so the remaining latency can be attributed on device. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
After rebasing onto main (YPE-4875), getOrSetInstallationId is synchronous — drop async/waitFor and mockResolvedValue from the highlight queue drain test. Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
…lights Port three consumer-facing UI deltas onto the highlights branch: - BibleReaderHandle.refreshHighlights via React 19 ref + useImperativeHandle - onHighlightError for queued and transient write outcomes only - useSignOutGuard shared by BibleReader toolbar and YouVersionAuthButton Sign-out guard preserves highlights-tip behavior: always Alert (normal or pending copy), confirm calls signOut() only — no discard-first path. Tests cover the hook, auth button parity, ref refresh, and error filtering. Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
|
@greptile-apps, please review for production readiness of our Highlights feature on our React Native Expo SDK |
Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
Scroll offset stays in a ref; React only stores the fade-gate booleans so a drag does not rebuild the verse action sheet every frame. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
…n-highlights-8389 feat(ui): refreshHighlights, onHighlightError, sign-out guard on highlights
|
Too many files changed for review (137 files, 100 file limit). Bypass the limit by tagging |
Move the remaining-distance predicate to layer 1 so the arithmetic can be tested without booting BibleReader, and store tray metrics as one ref. Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
…l-state fix(ui): skip per-frame re-renders on the swatch tray
- Remove `ensureFreshToken` from the auth context. `getAccessToken` runs the identical leeway-gated single-flight refresh and reports whether it worked, so the side-effect-only variant was strictly less useful. `HighlightQueueDrainHost` derives the drain's internal `DrainAuth.ensureFreshToken` from it; `drain.ts` is unchanged. - Stop exporting `SignOutGuardAuth`. It only named `useSignOutGuard`'s parameter, which structural inference already covers. - Stop exporting `isValidHighlightHex` and `KnownAuthPermission`. - Update AGENTS.md, ADR 0016, the READMEs, and the changeset. None of these shipped — all are new on this branch inside unreleased minors, so no changeset entry is needed for the removals.
refactor(api): narrow the published surface
|
ADDRESSED AND FIXED Four release-blocking findings from a review of this branch. |
* fix(core): keep parked highlights when a forced refresh fails
The drain treated a leftover token as a mint, then un-painted on the
second 401. getAccessToken({ force: true }) reports whether the mint
landed. Also skip the sign-in prompt while auth is loading, align
platform-core to 2.6.2, and pin expo-application in the example app.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(core): keep auth-provider imports above RefreshOutcome
A type alias sat in the middle of the import block and tripped
import/first under --max-warnings=0.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(ui): prompt sign-in after bootstrap settles signed out
A tap during the loading window skipped the sign-in sheet. When
bootstrap found no session the write reverted and the tap was lost.
Hold the intent until auth settles, then prompt or apply.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(core): settle unsigned bootstrap before a cached-grant write
A returning grant is seeded before the token lands. Writing on that hint still reverts if bootstrap finds no session. Wait only while loading and unsigned, and remint after a force joins an in-flight refresh so the drain does not un-paint on a leftover.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(core): remint after a join without calling refreshToken
The force path called refreshToken inside its own useCallback. ESLint treats that as access before declaration.
Co-authored-by: Cursor <cursoragent@cursor.com>
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
…ordering, export boundary (#137) * fix(ui): keep the access token out of the reader WebView The DOM reader is a pure view: highlights are controlled from native, auth is native-owned, and nothing inside the WebView reads the token. Passing it across the bridge put it in WebView localStorage, which is disk-backed on iOS and outlives both sign-out and relaunch. Drop the `accessToken` prop from the DOM and native readers. Replace the per-render `applyAuthToken` with a mount-scoped `clearAuthResidue()`, which removes the `accessToken`, `refreshToken`, and `expiryDate` keys an older SDK version left behind on upgrading installs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(core): clear tokens before caches on sign-out clearAuthState ended with the secure-store write, so a Keychain rejection left a signed-out looking app whose session came back on the next launch. The displayed state lied about the stored state. Move the token clear to the front, unguarded. A rejection now aborts the sign-out before any cache is touched, and the app goes on showing the session it still holds. The MMKV purges keep their best-effort try/catch shape; only the ordering changes. The revoked-refresh path routes through the same routine and keeps its try/catch, so the never-throws contract on getAccessToken and requestPermissions is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(core): pin the crash repair and the value-comparison retirement Two invariants were guarded only by prose. ADR 0018's value-comparison retirement in enqueueWrites now has a layer-1 test, and the mount-time repair of a crash between the queue write and the cache write has a case on the real-queue harness. Both were verified non-vacuous by mutation: `local === server` -> `false` fails 6 of the 10 new queue tests, and dropping applyQueuedWrites from initialStateFor fails both crash-repair cases. Also corrects four prose claims the sign-out reorder made backwards, in queue.ts, cache.ts, ADR 0018, and ADR 0014. Each still argues correctly for best-effort purges; only the stated ordering was wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(auth): scope the loud token-clear failure to the sign-out gesture Phase 2 made `clearAuthState` clear tokens first and unguarded, so a rejecting `saveTokens` aborts the whole routine. That is right for the sign-out gesture: caches must not be purged behind a session whose tokens are still on the device. It is wrong for the other two callers. Bootstrap and the revoked-refresh path are involuntary teardowns — the session is already over, and ADR 0014 names the bootstrap clear as the bound on a cached `userInfo` the store refused to remove. Aborting before `setIdentity(null)` removes that bound. `clearAuthState` now takes `abortOnTokenFailure`, default false. Only `signOut` passes true. Tests: a mount case and a revoked-session case pinning that the caches and identity still clear when the token write rejects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(ui): pin the package export surface, and mark core's plumbing @internal Design Questions 2 and 3. `packages/ui/src/__tests__/exports.test.ts` mirrors core's: the nine components plus `useSignOutGuard` are present, the SDK-owned sheets are absent, and a third case pins the namespace to exactly that list so a new export cannot land without a docs update. `useYVAuthOptional` and `YouVersionContextValue` get `@internal` JSDoc. Both stay importable; the annotation says they may change without semver ceremony. AGENTS.md's Exports section now lists the UI `*Props` types and `YouVersionTheme`, names both export tests, and records the two `@internal` core exports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(core): align token-storage comments with token-first sign-out Phase 2 clears tokens before the session. The old comments still described the reverse order. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* docs: shorten the highlights changelog and ADRs Partners get one release note. The six feature ADRs keep the decision and drop the runbook. Co-authored-by: Cursor <cursoragent@cursor.com> * docs(adr): native owns highlight paint in ADR 0017 The Web SDK renders controlled highlights. Saying it owns paint conflicts with ADR 0013. Co-authored-by: Cursor <cursoragent@cursor.com> * docs: restore load-bearing ADR rationale from review queued is a new minor union member. Fade gates, Copy/Share, and the 0018 drop/backoff clauses were still cited from source. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
Work in progress feature branch
Greptile Summary
The PR introduces native Highlights support across core auth, optimistic persistence and queue draining, permission handling, and the native Bible reader.
Confidence Score: 4/5 ·
The PR is not yet safe to merge because authenticated users whose token has no associated user ID still cannot apply or remove highlights.
The current write entry point rejects a null user ID as signed out before checking the valid token, and the test suite explicitly preserves this behavior for an authenticated session with an access token.
Files Needing Attention: packages/core/src/highlights/use-highlights.ts
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant Reader as Native BibleReader participant Flow as Highlight Permission Flow participant Hook as useHighlights participant Queue as Persistent Write Queue participant API as Highlights API User->>Reader: Select verse and color Reader->>Flow: apply(color, verses) Flow->>Flow: Check sign-in and permission Flow->>Hook: apply(color, verses) Hook->>Queue: Persist intended write Hook->>Reader: Paint optimistically Hook->>API: Send write alt Request succeeds API-->>Hook: Success Hook->>Queue: Remove queued entry else Network or server unavailable API-->>Hook: Transient failure Hook-->>Reader: Keep optimistic paint Queue->>API: Retry when drain wakes else Request refused API-->>Hook: Authorization or validation failure Hook->>Reader: Revert paint endReviews (2): Last reviewed commit: "test(core): align drain-host test with s..." | Re-trigger Greptile
Context used: