added i18n support for plugins #10182
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
khavinshankar
requested review from
gigincg,
bodhish,
rithviknishad and
Jacobjeevan
Deploying care-fe with
|
| Latest commit: |
a5b3417
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ee505838.care-fe.pages.dev |
| Branch Preview URL: | https://plugs-i18n.care-fe.pages.dev |
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
plugs-i18n
|
| Run status |
|
| Run duration | 02m 56s |
| Commit |
|
| Committer | Khavin Shankar |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
5
|
| View all changes introduced in this branch ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/i18n.ts (2)
17-27: Consider validating or defaulting empty input
Ifinputis empty or whitespace only, the function returns"https://"—which may fail later. A fallback or validation for an empty string can help avoid such errors.
43-63: Optionally provide a fallback for missing translations
If translation fetch fails, the callback passes an error, causing the i18n load to break. Returning an empty object can keep the UI partially functional rather than halting.Possible fallback approach:
.catch((error) => { console.error( `Failed to load translations for ${language}/${namespace}:`, error, ); - callback(error, null); + callback(null, {}); });care.config.ts (1)
124-124: Safeguard against invalid string patterns
Splitting the string twice may fail if the app name doesn't followorganization/app@branch. Consider a fallback or error handling if a slash is missing.- name: app.split("@")[0].split("/")[1], + const [org, appName] = app.split("@")[0].split("/"); + name: appName || org,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
care.config.ts(1 hunks)package.json(2 hunks)src/i18n.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (6)
src/i18n.ts (4)
1-1: No issues with this import
Essential import for referencing thecareConfig.
5-5: No issues with the new dependency import
TheresourcesToBackendimport is properly integrated and aligns with the i18n enhancement.
66-66: Dynamic namespaces are well implemented
Adding...careConfig.careApps.map(app => app.name)ensures robust multi-app translation support.
73-73: Default namespace choice looks good
SettingdefaultNSto"care_fe"is consistent with your existing codebase.package.json (2)
103-103: New i18n resource dependency
Good addition for enabling dynamic resource loading.
142-142: Incremental type definition update
Updating@types/nodeto^22.10.10is generally safe.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/i18n.ts (2)
25-45: Enhance resource loading resilience.While the error handling is good, consider these improvements for production readiness:
- Add retry mechanism for failed requests
- Implement caching strategy
- Add request timeout
- Handle partial translation data
Here's a suggested implementation:
.use( resourcesToBackend((language, namespace, callback) => { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 5000); + + const retryFetch = (retries = 3) => fetch(`${namespaceToUrl(namespace)}/locale/${language}.json`, { + signal: controller.signal, + cache: 'default' }) .then((response) => { if (!response.ok) { throw new Error(`HTTP error! status: ${response.status}`); } return response.json(); }) .then((resources) => { + clearTimeout(timeoutId); + // Validate required translation keys + if (!resources || typeof resources !== 'object') { + throw new Error('Invalid translation data'); + } callback(null, resources); }) .catch((error) => { + if (retries > 0 && error.name !== 'AbortError') { + return retryFetch(retries - 1); + } console.error( `Failed to load translations for ${language}/${namespace}:`, error, ); callback(error, null); }); + + retryFetch(); }), )
46-55: Consider additional i18n options for production use.The current configuration is good, but consider these additional options for better production performance:
.init({ fallbackLng: "en", ns: ["care_fe", ...careConfig.careApps.map((app) => app.name)], load: "currentOnly", supportedLngs: Object.keys(LANGUAGES), interpolation: { escapeValue: false, skipOnVariables: false, }, defaultNS: "care_fe", + // Performance optimizations + partialBundledLanguages: true, + nonExplicitSupportedLngs: false, + // Debug options for non-production environments + debug: process.env.NODE_ENV !== 'production', });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care.config.ts(1 hunks)public/locale/en.json(0 hunks)src/i18n.ts(2 hunks)
💤 Files with no reviewable changes (1)
- public/locale/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- care.config.ts
🔇 Additional comments (2)
src/i18n.ts (2)
1-5: LGTM! Clean import structure.The imports are well-organized and include all necessary dependencies for i18n functionality.
27-27: Verify URL construction in careConfig.The code now uses pre-constructed URLs instead of building them from individual components, which should address the previous branch-related issues. However, let's verify the URL construction in careConfig.
✅ Verification successful
URL construction in careConfig is properly implemented
The URL construction in careConfig is robust and secure, with proper validation, protocol handling, and error checking. The code correctly handles both GitHub Pages and custom CDN URLs, with appropriate fallbacks and error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify URL construction in careConfig # Check URL construction logic rg -A 10 'careApps.*url' # Check for any remaining branch usage rg 'careApp.*branch'Length of output: 501
Script:
#!/bin/bash # Search for careConfig definition and initialization rg -A 5 "careConfig.*=" # Look for type definitions ast-grep --pattern "interface CareApp { $$$ }" # Check for any config files that might contain careConfig fd -e json -e ts -e js configLength of output: 1863
Script:
#!/bin/bash # Get complete careConfig content cat care.config.ts # Look for type definitions in the codebase ast-grep --pattern "type CareConfig = { $$$ }" # Search for careApps related code rg -A 5 "careApps.*="Length of output: 5889
| const namespaceToUrl = (namespace: string) => { | ||
| return careConfig.careApps.find((app) => app.name === namespace)?.url ?? ""; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and type safety in namespaceToUrl.
The current implementation has several potential issues:
- Returning an empty string on failure could lead to silent failures
- No validation of the URL structure
- Missing type safety for namespace parameter
Consider this improved implementation:
-const namespaceToUrl = (namespace: string) => {
- return careConfig.careApps.find((app) => app.name === namespace)?.url ?? "";
-};
+type ValidNamespace = typeof careConfig.careApps[number]['name'];
+
+const namespaceToUrl = (namespace: ValidNamespace): string => {
+ const app = careConfig.careApps.find((app) => app.name === namespace);
+ if (!app?.url) {
+ throw new Error(`Invalid namespace or missing URL: ${namespace}`);
+ }
+ return app.url;
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const namespaceToUrl = (namespace: string) => { | |
| return careConfig.careApps.find((app) => app.name === namespace)?.url ?? ""; | |
| }; | |
| type ValidNamespace = typeof careConfig.careApps[number]['name']; | |
| const namespaceToUrl = (namespace: ValidNamespace): string => { | |
| const app = careConfig.careApps.find((app) => app.name === namespace); | |
| if (!app?.url) { | |
| throw new Error(`Invalid namespace or missing URL: ${namespace}`); | |
| } | |
| return app.url; | |
| }; |
|
@khavinshankar Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
Summary by CodeRabbit
Release Notes
New Features
Dependencies
i18next-resources-to-backendpackage@types/nodeto latest versionImprovements
Localization Updates