fix: improve locale handling and tree-shaking#8
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds a tree-shaking validation task and Bun checker script for ParaglideJS translations, refactors Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Mise as "Task runner (Mise)"
participant Build as "Webapp build (.var/dist/webapp)"
participant Checker as "Bun script\nscripts/check-tree-shaking.ts"
Mise->>Build: run `build:webapp` (depends for task)
Mise->>Checker: run `bun scripts/check-tree-shaking.ts`
Checker->>Build: glob and read `assets/*.js`
Checker->>Checker: scan files for sentinel strings
alt sentinel found
Checker->>Mise: exit non-zero (fail CI/validation)
else no sentinel
Checker->>Mise: report success (tree-shaking OK)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves ParaglideJS tree-shaking by eliminating dynamic message access patterns and refining the reatomLoc API. The changes ensure that unused translations are properly removed from the production bundle, reducing bundle size.
Changes:
- Introduced
localeAtom.label()method with static locale label mapping to replace dynamicm[language_${locale}]()access - Updated
reatomLocAPI signature to remove themparameter, requiring direct static calls tom.*()inside callbacks - Added automated tree-shaking verification script with canary messages and CI integration
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared/model/locale.ts | Added localeLabels static mapping and label() method; updated reatomLoc signature to take zero-argument callback; updated documentation |
| src/widgets/layout/ui/AppShell.tsx | Replaced dynamic m[language_${locale}]() with localeAtom.label(locale)() |
| src/pages/settings/ui/SettingsPage.tsx | Updated all reatomLoc callbacks to remove m parameter; refactored language collection to use localeAtom.label() |
| src/pages/items/ui/ItemsPage.tsx | Updated all reatomLoc callbacks to remove m parameter |
| scripts/check-tree-shaking.ts | New script that verifies canary messages are not present in production bundle |
| messages/en.json | Added reatomloc_tree_shake_canary message for verification |
| messages/es.json | Added reatomloc_tree_shake_canary message for verification |
| docs/localization.md | Updated documentation to reflect new API patterns and tree-shaking best practices |
| .config/mise/conf.d/tasks-quality.toml | Added lint:paraglide-tree-shaking task to validation and CI pipelines |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/localization.md`:
- Around line 78-80: Replace the problematic inline code span that currently
uses escaped backticks (the segment referencing message lookup like m[`...`])
with a quoting style that avoids backticks inside code spans (for example use
m['...'] or m["..."]) and ensure there are no leading/trailing spaces inside the
code span; update the text around reatomLoc / localeAtom / m.* to use that new
inline form so the MD038 lint error is resolved.
In `@scripts/check-tree-shaking.ts`:
- Around line 34-44: Add a guard that verifies the dist directory exists and
that the Glob('*.js').scan({ cwd: distDir }) yields at least one file before
declaring success: check the filesystem for distDir (use the same distDir
identifier) and count the results of new Glob('*.js').scan(...) (or collect
files into an array) and if the directory is missing or the file count is zero,
print a descriptive error message referencing distDir and set failed = true (and
exit non-zero) so the check cannot silently pass when no JS files are present;
integrate this check near the top of the current scan loop that uses Glob,
sentinels, and failed.
In `@src/shared/model/locale.ts`:
- Around line 66-70: The computed returned by the label property doesn't
subscribe to locale changes when localeName is passed because target() is only
invoked conditionally; to fix it, always read target() inside the computed to
establish the dependency (e.g. call const current = target(); then const name =
localeName ?? current) before resolving localeLabels[name as keyof typeof
localeLabels](), keeping the computed('localeLabel') and existing lookup logic
intact so the computed tracks localeAtom via target().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.config/mise/conf.d/tasks-quality.tomldocs/localization.mdmessages/en.jsonmessages/es.jsonscripts/check-tree-shaking.tssrc/pages/items/ui/ItemsPage.tsxsrc/pages/settings/ui/SettingsPage.tsxsrc/shared/model/locale.tssrc/widgets/layout/ui/AppShell.tsx
![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: 7a8a39c429
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/localization.md (1)
160-160: Document the failure mode for partial locale registration.The two-step requirement (message file +
localeLabelsmap) creates a dual-maintenance surface. The doc doesn't state what silently breaks when only one step is done — e.g., doeslocaleAtom.label(locale)()returnundefined, an empty string, or throw? Adding a note on the observable symptom (and whether the Bun checker script in this PR validateslocaleLabelscompleteness, not just message-key existence) would help developers diagnose missing registrations quickly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/localization.md` at line 160, Update the docs to explicitly state the failure mode when only the message file is added but the locale is not registered in the localeLabels map: call out that localeAtom.label(locale)() will not return the expected localized label (it will resolve to a missing/undefined label or fallback value used by the app) and that UI text will appear blank or fallback rather than throwing; also document whether the Bun validation script in this PR checks both message keys and localeLabels completeness — if it doesn't, update the script to verify that every locale_xx message has a corresponding entry in localeLabels (referencing localeLabels and localeAtom.label) and mention this check in the docs so developers can quickly diagnose missing registrations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/localization.md`:
- Line 13: Update the table description for localeAtom.label(locale) to
accurately state that it returns a thunk (a zero-argument function) that yields
the display label rather than the label itself; reference the existing usage
pattern localeAtom.label(locale)() and change the text to something like
"Returns a zero-argument function () => string that produces the display label
for a locale code" so readers know to call the returned function.
---
Nitpick comments:
In `@docs/localization.md`:
- Line 160: Update the docs to explicitly state the failure mode when only the
message file is added but the locale is not registered in the localeLabels map:
call out that localeAtom.label(locale)() will not return the expected localized
label (it will resolve to a missing/undefined label or fallback value used by
the app) and that UI text will appear blank or fallback rather than throwing;
also document whether the Bun validation script in this PR checks both message
keys and localeLabels completeness — if it doesn't, update the script to verify
that every locale_xx message has a corresponding entry in localeLabels
(referencing localeLabels and localeAtom.label) and mention this check in the
docs so developers can quickly diagnose missing registrations.
Guria
force-pushed
the
paraglide-tree-shake
branch
from
c8917ac to
57f774b
Compare
|
Summary by CodeRabbit
New Features
Documentation
Refactor