Skip to content

feat: support custom installationId; fallback when crypto.randomUUID is missing#220

Merged
cameronapak merged 4 commits intomainfrom
crypto-drifto
May 1, 2026
Merged

feat: support custom installationId; fallback when crypto.randomUUID is missing#220
cameronapak merged 4 commits intomainfrom
crypto-drifto

Conversation

@cameronapak
Copy link
Copy Markdown
Collaborator

@cameronapak cameronapak commented May 1, 2026

Summary

Fixes crashes in runtimes without crypto.randomUUID (e.g. Expo DOM components) by falling back to a non-secure installation id.

Changes

  • core: YouVersionPlatformConfiguration now uses crypto.randomUUID() when available, otherwise falls back to yvp-<utc-iso-timestamp>-<random>.
  • hooks: no public API change. The previously proposed installationId prop on YouVersionProvider was intentionally dropped.
  • changeset: core-only patch for the runtime fallback.

Why no installationId prop

Consumers can already override the install identifier deliberately via:

  • YouVersionPlatformConfiguration.installationId = '...'
  • new ApiClient({ installationId: '...' })

Making it a prominent YouVersionProvider prop is an easy footgun (passing userId, a constant, or a fresh random per render breaks per-install metrics/observability).

Verification

  • pnpm --filter @youversion/platform-core test
  • pnpm --filter @youversion/platform-react-hooks test

Greptile Summary

This PR fixes a crash in runtimes that lack crypto.randomUUID (e.g. Expo DOM components) by falling back to a yvp-<iso-timestamp>-<random> installation ID, and tidies YouVersionProvider by extracting the duplicated contextValue object into a single declaration.

Confidence Score: 5/5

Safe to merge; only P2 findings remain after previous review threads addressed the substantive concerns

No P0 or P1 findings. The one new finding (missing test coverage for the fallback path) is P2. All higher-severity issues were flagged in prior review rounds.

packages/core/src/YouVersionPlatformConfiguration.ts — fallback branch needs a dedicated test

Important Files Changed

Filename Overview
packages/core/src/YouVersionPlatformConfiguration.ts Adds crypto.randomUUID availability check with a timestamp+random fallback; fallback path is never exercised by the existing test suite
packages/hooks/src/context/YouVersionProvider.tsx Refactors duplicate contextValue creation into a single object; no functional changes to auth or non-auth paths
packages/hooks/src/context/YouVersionProvider.test.tsx New test file verifying installationId is provided via context; beforeEach reset behaviour is subtly incorrect (setter generates an ID rather than clearing to null)
.changeset/installation-id-prop.md Changeset only bumps platform-core; platform-hooks also received changes but is absent, risking version parity violation

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[getOrSetInstallationId] --> B{window defined?}
    B -- No --> C[return empty string]
    B -- Yes --> D{localStorage has x-yvp-installation-id?}
    D -- Yes --> E[return existing ID]
    D -- No --> F{crypto defined AND crypto.randomUUID is a function?}
    F -- Yes --> G[crypto.randomUUID]
    F -- No --> H[yvp-ISO timestamp-Math.random base36]
    G --> I[localStorage.setItem]
    H --> I
    I --> J[return newId]
Loading

Fix All in Claude Code Fix All in Cursor

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
packages/core/src/YouVersionPlatformConfiguration.ts:25-28
**Fallback path has no test coverage**

The new fallback branch (`yvp-${new Date().toISOString()}-...`) is never exercised by the test suite. `YouVersionPlatformConfiguration.test.ts` stubs `crypto` with a working `randomUUID` mock at the top of the file, so `typeof crypto.randomUUID === 'function'` is always `true` in tests and the ternary's false branch is dead code from the test runner's perspective. Consider adding a test that removes/stubs out `crypto.randomUUID` and asserts the fallback ID is generated and stored correctly.

Reviews (5): Last reviewed commit: "fix(core): use UTC timestamp in installa..." | Re-trigger Greptile

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 1, 2026

🦋 Changeset detected

Latest commit: e5efed9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@youversion/platform-core Patch
@youversion/platform-react-hooks Patch
@youversion/platform-react-ui Patch
vite-react Patch

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

Comment thread packages/hooks/src/context/YouVersionProvider.tsx Outdated
Comment thread packages/hooks/src/context/YouVersionProvider.test.tsx
Comment thread packages/core/src/YouVersionPlatformConfiguration.ts Outdated
…missing

- core: YouVersionPlatformConfiguration falls back to a timestamp+random id
  when crypto.randomUUID is unavailable (e.g. Expo DOM components).
- hooks: YouVersionProvider accepts an optional installationId prop. Synced
  to YouVersionPlatformConfiguration synchronously during render and used
  directly when constructing the context value, avoiding any race with
  useEffect.
- Tests + changeset (minor across all 3 packages).

Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019de441-0a5a-774c-826b-8adf0406c7d7
Keep installation id override as an explicit power-user escape hatch via core config/API client; changeset now reflects only the core crypto.randomUUID fallback.

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread .changeset/installation-id-prop.md
cameronapak and others added 2 commits May 1, 2026 11:54
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread packages/hooks/src/context/YouVersionProvider.tsx
@cameronapak
Copy link
Copy Markdown
Collaborator Author

@greptile, please re-review this because the PR and description got updated.

Copy link
Copy Markdown
Collaborator

@bmanquen bmanquen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fix the changeset per Greptile and it is good to merge.

Copy link
Copy Markdown
Collaborator

@bmanquen bmanquen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need to fix the changeset so that all packages bump together

@cameronapak
Copy link
Copy Markdown
Collaborator Author

Just need to fix the changeset so that all packages bump together

it turns out they do bump all together! So we can specify which packages actally have the change but all bump together. I saw it happen in another PR: #218

@cameronapak cameronapak merged commit da88c10 into main May 1, 2026
5 checks passed
@cameronapak cameronapak deleted the crypto-drifto branch May 1, 2026 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants