Conversation
🦋 Changeset detectedLatest commit: c524bb1 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 |
Greptile SummaryThis PR introduces i18next support in Confidence Score: 5/5Safe to merge — all P0/P1 concerns from prior rounds are resolved; only a minor devDependencies convention suggestion remains. All previously-flagged blocking issues (unified versioning, SSR safety, silent init failure, optional peer deps, tsup externalization) are addressed. The one remaining finding (peer deps not mirrored in devDependencies) is P2 and does not affect correctness in the current pnpm workspace setup. packages/ui/package.json — minor devDependencies convention note only.
|
| Filename | Overview |
|---|---|
| .changeset/add-i18next-support.md | Correctly bumps all three packages (core, react-ui, react-hooks) together, resolving the unified-versioning concern from the previous review round. |
| packages/ui/package.json | i18next and react-i18next correctly moved to peerDependencies (non-optional); they are absent from devDependencies, relying on pnpm's auto-install-peers behaviour to make them available locally. |
| packages/ui/src/i18n/index.ts | Uses createInstance() for isolation, removes LanguageDetector (SSR-safe), adds .catch() for error visibility — all previous concerns addressed. |
| packages/ui/src/components/verse-of-the-day.tsx | Passes the SDK's isolated i18n instance directly to useTranslation, bypassing the host app's provider — intentional design choice consistent with the isolated instance approach. |
| packages/ui/tsup.config.ts | i18next and react-i18next added to external array, ensuring they are not bundled and the peer-dep contract is honoured. |
| packages/ui/src/i18n/locales/en.json | Single English translation key added; straightforward. |
| .gitignore | Adds .omc/ to gitignore — trivial housekeeping. |
| pnpm-lock.yaml | Lock file updated to reflect resolution of i18next@26.0.4 and react-i18next@17.0.2 as peer dependencies; no unexpected dependency changes. |
Sequence Diagram
sequenceDiagram
participant App as Consumer App
participant VOTD as VerseOfTheDay
participant i18n as src/i18n/index.ts
participant i18next as i18next (peer)
participant react_i18next as react-i18next (peer)
Note over i18n: Module load (singleton, isolated instance)
i18n->>i18next: createInstance()
i18n->>react_i18next: .use(initReactI18next)
i18n->>i18n: .init({ resources: {en}, fallbackLng: 'en' }).catch(...)
App->>VOTD: render VerseOfTheDay
VOTD->>react_i18next: useTranslation(undefined, { i18n })
react_i18next-->>VOTD: t()
VOTD->>i18n: t('verseOfTheDay')
i18n-->>VOTD: "Verse of The Day"
Reviews (9): Last reviewed commit: "fix: make i18next required" | Re-trigger Greptile
|
Tip: Greploops — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Cameron Llewellyn <cameron.b.llewellyn@gmail.com>
bmanquen
left a comment
There was a problem hiding this comment.
Looks good just one comment for clarification/learning.
|
@bmanquen I am going to wait and let @cameronapak take a look at this too and make sure it tracks with what he thinks is best. |
This PR puts i18next into place and sets up a single translation string to show how it will work.
The current implementation is to mark i18next as a peer dependency and make it required. This is to prevent conflicts with any implementation apps having their own i18next implementation.
Greptile Summary
This PR wires up i18next to the
@youversion/platform-react-uipackage using an isolated instance (createInstance()) so the SDK's translations can never collide with a host app's own i18next configuration. The only translated surface right now is the "Verse of The Day" label inVerseOfTheDay. Previous concerns (unified versioning, silent init failure, SSR-breakingLanguageDetector, and theoptional: true/ static-import contradiction) have all been addressed in this revision.Confidence Score: 5/5
Safe to merge — all previously raised blockers are resolved; only two minor P2 suggestions remain.
Unified versioning is now correct (all three packages bumped), the LanguageDetector SSR risk is gone, init errors are observable via .catch(), and the isolated-instance design is intentional and coherent. The remaining findings are P2: a changeset description that mischaracterises the deps as 'optional peer dependencies' and a missing explicit lng that may produce a dev-mode warning. Neither blocks correctness.
The changeset description in .changeset/add-i18next-support.md should be corrected before merge to avoid misleading changelog consumers.
Vulnerabilities
No security concerns identified. i18next resources are in-memory constants, no user-supplied input reaches the translation layer, and
escapeValue: falseis safe because React's JSX already escapes all output.Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["packages/ui/src/i18n/index.ts\n(module load)"] -->|"createInstance()"| B["Isolated i18n instance"] B -->|"use(initReactI18next)\n.init({ resources })"| C["In-memory EN resources loaded"] C --> D["i18n instance exported"] D --> E["verse-of-the-day.tsx\nimport i18n from '@/i18n'"] E -->|"useTranslation(undefined, { i18n })"| F["t() bound to SDK instance"] F -->|"t('verseOfTheDay')"| G["'Verse of The Day'"] H["Host app's i18next instance"] -.->|"Completely isolated\nno interference"| BPrompt To Fix All With AI
Reviews (10): Last reviewed commit: "fix: internalize i18next" | Re-trigger Greptile
Context used: