feat(i18n): add localization port contract#106
Merged
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 49 minutes and 8 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughAdds a LocalizationPort contract and runtime adapter, threads port-based localization through the docs app and DOGFOOD previews, implements block-tree rendering with Surface-backed nested previews, and introduces a backtick-toggled Perf HUD; includes tests, catalogs, and docs/changelog updates. ChangesLocalizationPort Implementation & Integration
Block-Tree Rendering & Standard Blocks
Perf HUD / App Frame
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7dd7c307d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/bijou-i18n/src/localization.ts (1)
117-140: 💤 Low value
freezeLocalizedValuehas a few quiet edge cases for arbitrary data payloads.Since
LocalizedObject<Value = unknown>is used fordata/resourcepayloads, the deep-freeze helper may want to be a bit more defensive:
- Plain-object reconstruction via
Object.entries(Object.getOwnPropertyDescriptors(value))silently drops symbol-keyed properties and demotes non-enumerable own properties to enumerable. For JSON-derived catalogs this is fine, but programmatically built data payloads will lose properties without any warning.- There is no cycle guard. A self-referencing data payload would recurse until the call stack blows.
If those shapes are intentionally out-of-scope, consider documenting the invariant in the JSDoc (e.g., "values must be JSON-shaped, no symbols, no cycles, no accessors") alongside the existing accessor-property throw at line 134, so adapters know what they are required to produce.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bijou-i18n/src/localization.ts` around lines 117 - 140, freezeLocalizedValue currently loses symbol-keyed and non-enumerable own properties and has no cycle protection; update freezeLocalizedValue to (1) accept a traversal context (e.g., a WeakSet seen) and throw on cycles or return already-frozen object to avoid infinite recursion, (2) iterate own keys using Object.getOwnPropertyNames + Object.getOwnPropertySymbols and copy property descriptors instead of using Object.entries so non-enumerable flags are preserved, applying freezeLocalizedValue recursively to descriptor.value while still throwing for accessor descriptors (preserve the existing accessor error), and (3) after defining all properties on a new plain object via Object.defineProperty, freeze and return it; alternatively, if you prefer to keep the simpler behavior, add a JSDoc on freezeLocalizedValue / LocalizedObject stating the JSON-shaped invariant (no symbols, no non-enumerable props, no cycles, no accessors) so callers know the requirements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/docs/app.ts`:
- Around line 3398-3403: The pages array built in createDocsExplorerApp
snapshots localized strings (calls like pageTitle(spec.id, localization) and
searchTitle(...)) at construction so labels never update after cycle-locale;
change pages metadata to compute titles lazily or recompute on locale change —
e.g., replace stored title/searchTitle with functions/getters that call
pageTitle(spec.id, currentLocalization) at render-time or add a locale change
listener in createDocsExplorerApp that maps DOCS_SITE_PAGES (including the
COMPONENTS_PAGE_ID branch and entries using componentsPageKeys) and updates the
pages array by re-running pageTitle/searchTitle using the new localization;
ensure any code that reads page.title or page.searchTitle invokes the resolver
or reads the refreshed value.
In `@examples/docs/content/guides-i18n-workflow.md`:
- Around line 99-109: The example assigns the LocalizedObject returned by
LocalizationPort.resolve<Value>() to label instead of using its .value; update
the snippet so the call to localization.resolve (from
createRuntimeLocalizationPort/runtime) returns a LocalizedObject (e.g.,
labelResult) and then read the optional localized string from labelResult.value
(handling undefined if needed) so consumers get the actual string rather than
the wrapper object.
In `@examples/docs/localization.ts`:
- Around line 44-48: formatLocalizedList currently calls localization.formatList
directly which can throw and bypass the fallback; wrap the call in a try/catch
like localizedText does: if localization is provided, call
localization.formatList(values) inside a try block, return the result on
success, and on any thrown error (or if localization is undefined) return
values.join(', '); reference the formatLocalizedList function and the
localization.formatList call to implement this safe fallback.
In `@packages/bijou-i18n/src/runtime.ts`:
- Around line 395-397: The bug: localize forwards options.missingMessage into
resolveLocalizedValueResult unconditionally, which causes entries with
entry.kind === 'resource'|'data' to end up with a message string as their value
(wrong kind). Fix: in localize skip passing options.missingMessage when
entry.kind is 'resource' or 'data' (mirror resource() behavior) so call
resolveLocalizedValueResult without missingMessage for those kinds; update the
invocation in localize and ensure resolveLocalizedValueResult is still used for
other kinds unchanged.
---
Nitpick comments:
In `@packages/bijou-i18n/src/localization.ts`:
- Around line 117-140: freezeLocalizedValue currently loses symbol-keyed and
non-enumerable own properties and has no cycle protection; update
freezeLocalizedValue to (1) accept a traversal context (e.g., a WeakSet seen)
and throw on cycles or return already-frozen object to avoid infinite recursion,
(2) iterate own keys using Object.getOwnPropertyNames +
Object.getOwnPropertySymbols and copy property descriptors instead of using
Object.entries so non-enumerable flags are preserved, applying
freezeLocalizedValue recursively to descriptor.value while still throwing for
accessor descriptors (preserve the existing accessor error), and (3) after
defining all properties on a new plain object via Object.defineProperty, freeze
and return it; alternatively, if you prefer to keep the simpler behavior, add a
JSDoc on freezeLocalizedValue / LocalizedObject stating the JSON-shaped
invariant (no symbols, no non-enumerable props, no cycles, no accessors) so
callers know the requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f08ae5b0-895c-4707-8ec3-0d934db12f16
⛔ Files ignored due to path filters (1)
examples/docs/i18n/source/dogfood-strings.csvis excluded by!**/*.csv
📒 Files selected for processing (20)
docs/BEARING.mddocs/CHANGELOG.mddocs/DOGFOOD.mddocs/design/LX-019-localization-port-contract.mddocs/method/legends/LX-localization-and-bidirectionality.mdexamples/docs/app.tsexamples/docs/content/guides-i18n-workflow.mdexamples/docs/i18n/catalogs/de/bijou.dogfood.jsonexamples/docs/i18n/catalogs/en/bijou.dogfood.jsonexamples/docs/i18n/catalogs/es/bijou.dogfood.jsonexamples/docs/i18n/catalogs/fr/bijou.dogfood.jsonexamples/docs/locale.tsexamples/docs/localization.tspackages/bijou-i18n/README.mdpackages/bijou-i18n/src/index.tspackages/bijou-i18n/src/localization.test.tspackages/bijou-i18n/src/localization.tspackages/bijou-i18n/src/runtime.tstests/cycles/DX-031/dogfood-blocks-section.test.tstests/cycles/LX-019/localization-port-dogfood.test.ts
Owner
Author
|
@codex Self-review findings from the Code Lawyer pass. I found two behavior-level issues that should be resolved before merge.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96ba79ccab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex Self-review findings from local diff against
No files were changed during this review pass. |
Summary
Testing
Also completed:
|
Owner
Author
|
@codex Addressed the self-review findings in
Validation after the fix:
|
Summary
Testing
|
Owner
Author
|
@codex Follow-up pushed in 0ebfa52.\n\n| Issue | Resolution | Regression |\n|---|---|---|\n| Blocks preview looked like raw printed/debug text | Selected block pages now show concise documentation, hide embedded lowering scrollbars, and keep raw metadata summaries/source paths out of the TUI surface. | tests/cycles/DX-031/dogfood-blocks-section.test.ts |\n| Backtick no longer opened the perf overlay | createFramedApp now owns a shell-level perf HUD on backtick, including active help/shell layers; DOGFOOD landing routes that key into the docs shell. | packages/bijou-tui/src/app-frame.test.ts; tests/cycles/DF-001/dogfood-coverage-progress.test.ts |\n\nValidation run before push: focused regressions, docs:inventory, dogfood:i18n:debt, typecheck:test, lint, git diff --check, npm test, verify:interactive-examples. Pre-push reran typecheck:test, full npm test, and interactive example smoke. |
|
No follow-up changes were required from this trigger: the branch is already clean, and the referenced follow-up commit ( Given there were no new modifications to commit, I did not create a commit and did not call Checks run
|
Owner
Author
|
@codex update pushed in 3b6ad96.\n\n| Area | Outcome | Coverage |\n|---|---|---|\n| Blocks side nav | Standard block rows now select their preview when focused, so the nav row, guide info pane, and main live preview describe the same block. | tests/cycles/DX-031/dogfood-blocks-section.test.ts |\n| Blocks live preview | Lower-mode output is now summarized as mode/fact data instead of rendering clipped nested mini-surfaces into the preview page. | tests/cycles/DX-031/dogfood-blocks-section.test.ts |\n| Nested visual blocks | Standard block visual sections fit oversized child surfaces to the parent section, preventing nested blocks from overrunning AppShell preview regions. | packages/bijou/src/core/standard-blocks.test.ts |\n\nValidation run before push:\n- npm test -- --run packages/bijou-tui/src/app-frame.test.ts tests/cycles/DF-001/dogfood-coverage-progress.test.ts packages/bijou/src/core/standard-blocks.test.ts tests/cycles/DX-031/dogfood-blocks-section.test.ts\n- npm run docs:inventory\n- npm run dogfood:i18n:debt\n- npm run typecheck:test\n- npm run lint\n- git diff --check\n- npm test\n- npm run verify:interactive-examples\n\nPre-push also reran typecheck:test, the full test suite, and scripted interactive example smoke. |
|
Summary
Testing
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/cycles/DX-031/dogfood-blocks-section.test.ts (1)
54-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheck every match on the row, not just the first one.
row.indexOf(needle)only inspects the first occurrence. If an unstyled copy appears before the styled one, this helper returnsfalseeven though the frame does contain a styled match.Suggested fix
- const start = row.indexOf(needle); - if (start === -1) continue; - - for (let x = start; x < Math.min(frame.width, start + needle.length); x++) { - const cell = frame.get(x, y); - if ( - cell.fg != null - || cell.fgRGB != null - || (cell.modifiers?.length ?? 0) > 0 - ) { - return true; - } - } + for (let start = row.indexOf(needle); start !== -1; start = row.indexOf(needle, start + 1)) { + for (let x = start; x < Math.min(frame.width, start + needle.length); x++) { + const cell = frame.get(x, y); + if ( + cell.fg != null + || cell.fgRGB != null + || (cell.modifiers?.length ?? 0) > 0 + ) { + return true; + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cycles/DX-031/dogfood-blocks-section.test.ts` around lines 54 - 66, The current logic only checks the first occurrence of needle on a row via row.indexOf(needle) and can miss later styled matches; change it to iterate over every occurrence by repeatedly calling row.indexOf(needle, fromIndex) (or a while loop that updates fromIndex = start + 1) and for each found start run the existing inner loop that checks frame.get(x, y) (preserving the Math.min(frame.width, start + needle.length) bounds); if any occurrence has styled cells return true, otherwise continue scanning further occurrences and rows.
♻️ Duplicate comments (2)
packages/bijou-i18n/src/runtime.ts (1)
365-367:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly pass
missingMessagefor message entries.
localize()still forwardsoptions.missingMessagefor every entry kind. When aresourceordataentry is missing in the selected locale, this can return a marker string whilekindstays non-message, which breaks theLocalizedObject<Value>contract.resource()already avoids this;localize()should mirror it here.Suggested fix
const resolved = resolveLocalizedValueResult<Value>(entry as I18nCatalogEntry<Value>, new Set<string>(), { - missingMessage: options.missingMessage, + missingMessage: entry.kind === 'message' ? options.missingMessage : undefined, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bijou-i18n/src/runtime.ts` around lines 365 - 367, The code currently always passes options.missingMessage into resolveLocalizedValueResult which causes non-message entries (resource/data) to get marker strings while retaining a non-message kind; update localize() so it only forwards options.missingMessage when the entry is a message entry (check entry.kind === 'message' or similar on the I18nCatalogEntry<Value>), mirroring the resource() behavior, so resolveLocalizedValueResult is called with missingMessage only for message entries and leaves resource/data entries untouched to preserve the LocalizedObject<Value> contract.examples/docs/app.ts (1)
3441-3446:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't snapshot localized page metadata at app construction.
These
title/searchTitlestrings are still resolved once while buildingpages. Aftercycle-locale, the runtime locale changes, but the header tabs and palette titles keep the old language until the app is recreated.Also applies to: 3514-3514, 3533-3533, 3589-3590
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/docs/app.ts` around lines 3441 - 3446, The pages array is currently capturing localized strings at construction (e.g., title: pageTitle(spec.id, localization)), so titles/searchTitles don't update after locale changes; change those entries (for the case when spec.id === COMPONENTS_PAGE_ID and the other noted spots) to defer resolution by storing either a function/closure or a key instead of the resolved string—for example replace title/searchTitle with a thunk (e.g., title: () => pageTitle(spec.id, localization)) or keep the locale-independent id and call pageTitle(spec.id, currentLocalization) at render/palette time; update handling code that reads pages to call the thunk or resolve the key to get current localized text (apply same change where DOCS_SITE_PAGES is mapped at the other referenced locations).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bijou-i18n/src/runtime.ts`:
- Around line 384-386: The status calculation uses resolved and issues but the
code drops the LocalizedResolution when encountering a $ref; update the
reference resolution path (where $ref entries are resolved) to propagate the
full LocalizedResolution object instead of replacing it with a raw value so that
resolved.status and resolved.issues reflect the referenced entry. Specifically,
inside the reference-resolution logic (the code that produces resolved for $ref
entries), return/assign the referenced LocalizedResolution (including its status
and issues) and merge or prefer its issues/status before the status calc that
uses issues.some(...) and resolved.status in localize()/LocalizedResolution
creation.
---
Outside diff comments:
In `@tests/cycles/DX-031/dogfood-blocks-section.test.ts`:
- Around line 54-66: The current logic only checks the first occurrence of
needle on a row via row.indexOf(needle) and can miss later styled matches;
change it to iterate over every occurrence by repeatedly calling
row.indexOf(needle, fromIndex) (or a while loop that updates fromIndex = start +
1) and for each found start run the existing inner loop that checks frame.get(x,
y) (preserving the Math.min(frame.width, start + needle.length) bounds); if any
occurrence has styled cells return true, otherwise continue scanning further
occurrences and rows.
---
Duplicate comments:
In `@examples/docs/app.ts`:
- Around line 3441-3446: The pages array is currently capturing localized
strings at construction (e.g., title: pageTitle(spec.id, localization)), so
titles/searchTitles don't update after locale changes; change those entries (for
the case when spec.id === COMPONENTS_PAGE_ID and the other noted spots) to defer
resolution by storing either a function/closure or a key instead of the resolved
string—for example replace title/searchTitle with a thunk (e.g., title: () =>
pageTitle(spec.id, localization)) or keep the locale-independent id and call
pageTitle(spec.id, currentLocalization) at render/palette time; update handling
code that reads pages to call the thunk or resolve the key to get current
localized text (apply same change where DOCS_SITE_PAGES is mapped at the other
referenced locations).
In `@packages/bijou-i18n/src/runtime.ts`:
- Around line 365-367: The code currently always passes options.missingMessage
into resolveLocalizedValueResult which causes non-message entries
(resource/data) to get marker strings while retaining a non-message kind; update
localize() so it only forwards options.missingMessage when the entry is a
message entry (check entry.kind === 'message' or similar on the
I18nCatalogEntry<Value>), mirroring the resource() behavior, so
resolveLocalizedValueResult is called with missingMessage only for message
entries and leaves resource/data entries untouched to preserve the
LocalizedObject<Value> contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68855073-fccc-4470-b289-cf57dd5b2340
⛔ Files ignored due to path filters (1)
examples/docs/i18n/source/dogfood-strings.csvis excluded by!**/*.csv
📒 Files selected for processing (31)
docs/CHANGELOG.mddocs/design-system/blocks.mddocs/design/DX-031-standard-bijou-blocks.mddocs/design/DX-031F-block-tree-rendering.mddocs/design/LX-019-localization-port-contract.mdexamples/docs/README.mdexamples/docs/app.tsexamples/docs/i18n/catalogs/de/bijou.dogfood.jsonexamples/docs/i18n/catalogs/en/bijou.dogfood.jsonexamples/docs/i18n/catalogs/es/bijou.dogfood.jsonexamples/docs/i18n/catalogs/fr/bijou.dogfood.jsonpackages/bijou-i18n/src/runtime.test.tspackages/bijou-i18n/src/runtime.tspackages/bijou-tui/GUIDE.mdpackages/bijou-tui/README.mdpackages/bijou-tui/src/app-frame-actions.tspackages/bijou-tui/src/app-frame-i18n.tspackages/bijou-tui/src/app-frame-types.tspackages/bijou-tui/src/app-frame-utils.tspackages/bijou-tui/src/app-frame.test.tspackages/bijou-tui/src/app-frame.tspackages/bijou-tui/src/layout-preset.test.tspackages/bijou/GUIDE.mdpackages/bijou/README.mdpackages/bijou/src/core/block-tree-render.test.tspackages/bijou/src/core/block-tree-render.tspackages/bijou/src/core/standard-blocks.test.tspackages/bijou/src/core/standard-blocks.tspackages/bijou/src/index.tstests/cycles/DF-001/dogfood-coverage-progress.test.tstests/cycles/DX-031/dogfood-blocks-section.test.ts
✅ Files skipped from review due to trivial changes (12)
- packages/bijou-tui/GUIDE.md
- docs/design/DX-031F-block-tree-rendering.md
- docs/design-system/blocks.md
- examples/docs/README.md
- packages/bijou-tui/README.md
- docs/design/DX-031-standard-bijou-blocks.md
- examples/docs/i18n/catalogs/fr/bijou.dogfood.json
- packages/bijou-tui/src/layout-preset.test.ts
- packages/bijou/README.md
- docs/design/LX-019-localization-port-contract.md
- examples/docs/i18n/catalogs/de/bijou.dogfood.json
- docs/CHANGELOG.md
Owner
Author
|
@codex follow-up pushed in ef285bb. The remaining screenshot was the parent |
Summary
Testing
|
Summary
Committed: Testing
|
Owner
Author
|
Addressed the self-review findings in
Validation run locally before push and again via pre-push where applicable:
✅ Addressed in commits: |
Owner
Author
|
@codex Self-review findings for PR #106. Please confirm or challenge these before we merge.
No merge recommended until these are resolved or explicitly ruled out with evidence. |
There was a problem hiding this comment.
💡 Codex Review
bijou/packages/bijou-i18n/src/runtime.ts
Line 472 in 31f6806
Return frozen clones from resource()
The new localization contract in this change introduces deep-frozen localized values via localize(), but resource() still returns the catalog-backed value by reference. When a caller reads an object/array resource and mutates it, that mutation leaks back into runtime state and changes later lookups for the same key, which breaks the immutability boundary this PR establishes for localized payloads and can cause hard-to-debug cross-request state corruption.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex Follow-up from the self-review findings is pushed.
Validation run:
✅ Addressed in commits: |
|
Summary
Testing
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/bijou-i18n/src/runtime.ts (2)
301-307:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStage loader activation before mutating live locale state.
setLocale()now waits for the loader, but it still assignscurrentLocalebefore activation andactivateLoaderCatalogs()mutatesloaderCatalogs/entriesin place. IfrebuildCatalogState()throws on a catalog merge conflict, the runtime is left half-switched.Also applies to: 449-457
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bijou-i18n/src/runtime.ts` around lines 301 - 307, activateLoaderCatalogs currently mutates loaderCatalogs and entries in place and setLocale assigns currentLocale before activation, which can leave the runtime half-switched if rebuildCatalogState throws; change activateLoaderCatalogs (and the duplicate logic around rememberCatalog at the other occurrence) to stage catalogs into a new temporary collection, run rememberCatalog and then call rebuildCatalogState against that staged data, and only on success swap the staged collection into the live loaderCatalogs and finally set currentLocale (move currentLocale assignment to after the successful swap) so the mutation and locale flip are atomic from the runtime's perspective.
257-276:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve missing referenced resolutions instead of throwing.
If the referenced entry exists but resolves to
status: 'missing'with novalue, this branch still throwsMissing i18n reference. That makes indirect misses crashlocalize()/resource()instead of surfacing a normal missing result orundefined.🛠️ Minimal fix
const resolved = resolveLocalizedValueResult<T>(referencedEntry as I18nCatalogEntry<T>, seen, options); seen.delete(refKey); - if (resolved.value !== undefined) { - return { - status: mergeResolutionStatus(candidateStatus, resolved.status), - value: resolved.value, - issues: resolved.issues, - }; - } - throw new Error(`Missing i18n reference: ${refKey}`); + return resolved.value === undefined + ? { + status: mergeResolutionStatus(candidateStatus, resolved.status), + issues: resolved.issues, + } + : { + status: mergeResolutionStatus(candidateStatus, resolved.status), + value: resolved.value, + issues: resolved.issues, + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bijou-i18n/src/runtime.ts` around lines 257 - 276, The code in the isReference branch throws `Missing i18n reference` even when the referenced entry exists but resolves with status 'missing' and no value; change the behavior in the block that handles `resolved` (the result of resolveLocalizedValueResult<T>(referencedEntry...)) so that instead of throwing when `resolved.value` is undefined you return a resolved result preserving the merged status and issues (i.e., return { status: mergeResolutionStatus(candidateStatus, resolved.status), value: undefined, issues: resolved.issues }); keep the existing seen handling and only throw when referencedEntry is actually undefined (entries.get(refKey) === undefined). References: isReference, keyToString, seen, entries, resolveLocalizedValueResult, mergeResolutionStatus, candidateStatus.
♻️ Duplicate comments (1)
examples/docs/app.ts (1)
3583-3589:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPage and search titles are still frozen in the initial locale.
These
pageTitle(...)/dogfoodText(...)calls run once while thepagesarray is built, so aftercycle-localethe tab labels and command-palette titles stay in the old language until the explorer is recreated.Also applies to: 3656-3656, 3673-3676, 3764-3771
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/docs/app.ts` around lines 3583 - 3589, The page/tab labels and command-palette titles are computed once during DOCS_SITE_PAGES.map (using pageTitle(...) and dogfoodText(...)) so they never update after locale changes; change the pages data to compute titles lazily or reactively instead of evaluating them upfront — e.g. replace static title/key strings with functions/getters that call pageTitle(spec.id, localization) or dogfoodText(...) at render/lookup time (update places that consume pages to call the title function), and ensure componentsPageKeys/other key maps likewise derive labels dynamically so cycling the locale recomputes the displayed text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bijou-i18n/src/localization.ts`:
- Around line 167-198: The array branch in cloneLocalizedValue currently only
maps indexed elements and silently drops non-indexed own properties (symbol
keys, non-enumerable, accessors, or self-referential properties); modify the
Array.isArray(value) branch in cloneLocalizedValue to inspect
Object.getOwnPropertyDescriptors(value) / Reflect.ownKeys(value) like the object
branch does: verify there are no symbol keys, no non-enumerable descriptors, no
accessor properties, and that all non-numeric own keys are rejected (or numeric
keys validated as indices), detect circular references using the seen set, and
throw the same error messages (using objectKind and path) for any unsupported
array-own-property case before returning the frozen mapped array so the
validation contract matches the object path.
---
Outside diff comments:
In `@packages/bijou-i18n/src/runtime.ts`:
- Around line 301-307: activateLoaderCatalogs currently mutates loaderCatalogs
and entries in place and setLocale assigns currentLocale before activation,
which can leave the runtime half-switched if rebuildCatalogState throws; change
activateLoaderCatalogs (and the duplicate logic around rememberCatalog at the
other occurrence) to stage catalogs into a new temporary collection, run
rememberCatalog and then call rebuildCatalogState against that staged data, and
only on success swap the staged collection into the live loaderCatalogs and
finally set currentLocale (move currentLocale assignment to after the successful
swap) so the mutation and locale flip are atomic from the runtime's perspective.
- Around line 257-276: The code in the isReference branch throws `Missing i18n
reference` even when the referenced entry exists but resolves with status
'missing' and no value; change the behavior in the block that handles `resolved`
(the result of resolveLocalizedValueResult<T>(referencedEntry...)) so that
instead of throwing when `resolved.value` is undefined you return a resolved
result preserving the merged status and issues (i.e., return { status:
mergeResolutionStatus(candidateStatus, resolved.status), value: undefined,
issues: resolved.issues }); keep the existing seen handling and only throw when
referencedEntry is actually undefined (entries.get(refKey) === undefined).
References: isReference, keyToString, seen, entries,
resolveLocalizedValueResult, mergeResolutionStatus, candidateStatus.
---
Duplicate comments:
In `@examples/docs/app.ts`:
- Around line 3583-3589: The page/tab labels and command-palette titles are
computed once during DOCS_SITE_PAGES.map (using pageTitle(...) and
dogfoodText(...)) so they never update after locale changes; change the pages
data to compute titles lazily or reactively instead of evaluating them upfront —
e.g. replace static title/key strings with functions/getters that call
pageTitle(spec.id, localization) or dogfoodText(...) at render/lookup time
(update places that consume pages to call the title function), and ensure
componentsPageKeys/other key maps likewise derive labels dynamically so cycling
the locale recomputes the displayed text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: baeba0bf-6205-43c1-9aac-31457b00a2de
⛔ Files ignored due to path filters (1)
examples/docs/i18n/source/dogfood-strings.csvis excluded by!**/*.csv
📒 Files selected for processing (20)
docs/CHANGELOG.mddocs/design/DX-031F-block-tree-rendering.mddocs/design/LX-019-localization-port-contract.mdexamples/docs/app.tsexamples/docs/content/guides-i18n-workflow.mdexamples/docs/counter-block-demo.tsexamples/docs/i18n/catalogs/de/bijou.dogfood.jsonexamples/docs/i18n/catalogs/en/bijou.dogfood.jsonexamples/docs/i18n/catalogs/es/bijou.dogfood.jsonexamples/docs/i18n/catalogs/fr/bijou.dogfood.jsonexamples/docs/localization.tspackages/bijou-i18n/src/localization.test.tspackages/bijou-i18n/src/localization.tspackages/bijou-i18n/src/runtime.test.tspackages/bijou-i18n/src/runtime.tspackages/bijou/src/core/block-tree-render.test.tspackages/bijou/src/core/block-tree-render.tstests/cycles/DX-031/dogfood-blocks-section.test.tstests/cycles/LX-011/dogfood-locale-ratchet.test.tstests/cycles/LX-019/localization-port-dogfood.test.ts
✅ Files skipped from review due to trivial changes (4)
- examples/docs/i18n/catalogs/fr/bijou.dogfood.json
- docs/design/LX-019-localization-port-contract.md
- examples/docs/i18n/catalogs/de/bijou.dogfood.json
- examples/docs/content/guides-i18n-workflow.md
Owner
Author
|
@codex Activity summary for the latest Code Lawyer pass:
Validation:
✅ Addressed in commits: |
|
Summary
Testing
|
Owner
Author
|
@codex Backlog-fuel follow-up pushed.
Validation:
|
Summary
Testing
|
Owner
Author
|
@codex Self-audit found one additional issue before merge.
|
Owner
Author
|
@codex Activity summary for the self-audit fix.
Validation:
|
|
Summary
Testing
|
Owner
Author
|
@codex Backlog-fuel implementation pushed.
Validation:
|
Summary
Testing
|
This was referenced Jun 1, 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
1 participant
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
Adds the LX-019 localization port contract and wires DOGFOOD ordinary text lookup through a runtime-backed
LocalizationPort.This keeps the concrete
I18nRuntimeat the app composition boundary for catalog loading, shell integration, and locale activation, while views/helpers resolve structured localized objects instead of callingruntime.t()directly.What changed
docs/design/LX-019-localization-port-contract.mdand updated BEARING/LX legend.LocalizationPort,LocalizationRequest,LocalizedObject, status/issue/fact types, andcreateRuntimeLocalizationPort()in@flyingrobots/bijou-i18n.I18nRuntime.localize()so runtime-backed adapters can return immutable structured localization results.examples/docs/localization.ts.@flyingrobots/bijou-i18nREADME, and changelog.Non-goals
Validation
npm test -- --run packages/bijou-i18n/src/runtime.test.ts packages/bijou-i18n/src/localization.test.ts tests/cycles/LX-019/localization-port-dogfood.test.ts tests/cycles/LX-018/runtime-loc-fallbacks.test.ts tests/cycles/LX-008/localized-shell-and-dogfood.test.tsnpm run dogfood:i18n:debtnpm run dogfood:i18n:checknpm run docs:inventorynpm run typecheck:testnpm run lintgit diff --checknpm test— 308 files, 3447 testsnpm run verify:interactive-examplestypecheck:test, fullnpm test, and interactive smoke.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests