Load routes and page copy only when needed - #1839
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces eager English locale merging with lazy message-group loading, adds route-scoped translation visibility, associates groups with application routes, updates admin rendering and test bootstrapping, and adds settings, routing, and wallet validation coverage. ChangesLazy i18n routing
Settings bundle safety
Wallet pass validation
Mutation attribution maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant routeMainApp
participant withMessageGroups
participant RouteHandler
Request->>routeMainApp: dispatch request
routeMainApp->>withMessageGroups: provide route message groups
withMessageGroups->>RouteHandler: load groups and execute handler
RouteHandler-->>Request: return rendered response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@src/features/admin/index.ts`:
- Around line 169-188: Both route dispatch tables must reject inherited
prototype properties for attacker-controlled keys. In
src/features/admin/index.ts lines 169-188, update the segmentRouters lookup in
the route dispatch flow to require Object.hasOwn(segmentRouters, segment) before
using segmentRouter; in src/features/index.ts lines 517-525, apply the
equivalent own-property guard to prefixHandlers[prefix] within routeMainApp.
Preserve the existing not-found behavior for missing entries.
In `@test/lib/i18n-route-loading.test.ts`:
- Around line 9-16: Consolidate the duplicated i18n cold-reset and
message-restoration logic by reusing the existing shared helper from
test/shared/i18n-loading.test.ts. Update withColdMessages and the related
withCommonLoader setup to delegate through that helper, preserving the
resetI18nForTest(true), try/finally restoration, and MESSAGE_GROUPS behavior
without retaining duplicate inline boilerplate.
- Line 1: Extract the repeated reset/restore flow into one exported shared test
helper in the existing `#test-utils` area, reusing resetI18nForTest and
ensureMessageGroups. Keep or move withColdMessages from
i18n-route-loading.test.ts to use this helper, and update withCommonLoader plus
the other cold-reset test sites in i18n-loading.test.ts to delegate to it while
retaining only their loader-specific setup and restoration.
In `@test/shared/i18n-loading.test.ts`:
- Around line 15-29: Refactor withCommonLoader to reuse the shared helper used
by the route-loading test’s withColdMessages instead of duplicating loader
replacement, cold-reset, restoration, and ensureMessageGroups cleanup. Preserve
the helper’s temporary ENGLISH_MESSAGE_LOADERS.common override and guaranteed
restoration behavior.
- Around line 32-49: Replace the duplicated reset-and-restore setup in the test
"loads only the requested message group" with the shared helper or fixture used
for the consolidated cold-reset/restore pattern. Preserve the test’s assertions
and ensure all message groups are restored after execution.
- Around line 51-64: Replace the duplicated reset-and-restore setup in the test
“keeps messages and compiled formats when another group loads” with the shared
cold-reset/restore helper or pattern referenced by the consolidated review
comment, while preserving the existing assertions and cleanup behavior.
In `@test/test-utils/i18n.ts`:
- Around line 12-17: Update the catalog aggregation in the i18n test utility
around ENGLISH_MESSAGE_LOADERS to track which group owns each message key and
throw immediately when a key appears in more than one loaded catalog, instead of
using Object.assign’s last-write-wins behavior. Preserve the existing merged
result for catalogs without duplicate keys.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8909b62e-7cf4-450b-8f03-fad03b7eba22
📒 Files selected for processing (29)
scripts/mutation/equivalent-mutants.txtscripts/mutation/runner.tsscripts/test-harness.tssrc/features/admin/guide.tssrc/features/admin/index.tssrc/features/index.tssrc/locales/en/common.jsonsrc/locales/en/guide.tssrc/locales/en/index.tssrc/locales/en/public.jsonsrc/locales/en/system.jsonsrc/locales/groups.tssrc/locales/manifest.tssrc/shared/guide-messages.tssrc/shared/i18n.tssrc/ui/templates/admin/admin-page.tsxsrc/ui/templates/admin/backup.tsxsrc/ui/templates/admin/builder.tsxsrc/ui/templates/admin/update.tsxtest/lib/i18n-coverage.test.tstest/lib/i18n-route-loading.test.tstest/shared/guide-messages.test.tstest/shared/i18n-loading.test.tstest/shared/i18n.test.tstest/shared/listing-defaults.test.tstest/shared/settings/forms.test.tstest/test-utils/i18n.tstest/test-utils/preload.tstest/ui/templates/admin/guide/schema.test.ts
💤 Files with no reviewable changes (5)
- src/locales/en/guide.ts
- src/shared/guide-messages.ts
- src/locales/en/index.ts
- test/shared/guide-messages.test.ts
- src/locales/en/public.json
![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: 93e3a41d95
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/shared/i18n.ts`:
- Around line 64-75: Update the catalog processing around the extra-entry loop
to validate all entries into temporary collections before mutating messages or
owners. Only commit the temporary entries after validation succeeds, ensuring
rejected catalogs leave no earlier keys registered; add a regression test
confirming previously encountered keys remain unavailable after a non-string or
duplicate entry causes rejection.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e214bcd5-41e0-4592-9af5-04555b0fb57a
📒 Files selected for processing (12)
scripts/mutation/equivalent-mutants.txtsrc/features/admin/index.tssrc/features/index.tssrc/features/settings-bundles.tssrc/shared/i18n.tstest/features/settings-bundles.test.tstest/lib/i18n-route-loading.test.tstest/lib/owner-footer.test.tstest/lib/server-misc-routing.test.tstest/lib/server-wallet-webservice.test.tstest/shared/i18n-loading.test.tstest/test-utils/i18n.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/mutation/equivalent-mutants.txt`:
- Around line 954-957: Update the survivor-count header for this i18n
mutation-attribution block from five to four, leaving all four listed entries
unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 01d053ee-55ec-4aaa-bd0c-83938e68021b
📒 Files selected for processing (3)
scripts/mutation/equivalent-mutants.txtsrc/shared/i18n.tstest/shared/i18n-loading.test.ts
stefan-burke
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5bf30af7f
ℹ️ 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".
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
Why
Every request previously imported most route code and built the complete English message catalog during startup. Most pages need only a small part of both. This avoids that unused work while keeping translated message lookup synchronous after a route loads its copy.
Results
/listingskeeps the same 28 cold and 23 warm database round trips.Tests
deno task precommitpasses with 100% coverage.Mutation-test hardening is intentionally separated into follow-up PR #1849.