feat: add i18n extraction server and some i18n #379
Conversation
WalkthroughThe pull request introduces several changes to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
frontend/public/locales/zh/translation.json (1)
42-46: LGTM: Transaction-related translations are accurate and well-structured.The new translations for transaction-related UI elements are clear and consistent with common Chinese UI terminology. They accurately convey the meaning of their English counterparts.
For consistency with other dialog-related translations in the file, consider changing "NEW_TRANSACTION_DIALOG_TITLE" to "NEW_TRANSACTION_MODAL_TITLE". This aligns with the naming convention used for other modal-related keys (e.g., "TRANSACTION_PREVIEW_MODAL_TITLE").
frontend/public/locales/en/translation.json (2)
55-57: LGTM! Consider adding a period for consistency.The addition of the "hide_closed_accounts" entry is well-structured and clear. It enhances the functionality for managing account visibility in the UI.
For consistency with other similar entries in the file, consider adding a period at the end of the value:
"accounts": { - "hide_closed_accounts": "Hide closed accounts" + "hide_closed_accounts": "Hide closed accounts." }
Line range hint
1-57: Consider improving consistency and completeness of translations.While the file is well-structured, there are a few suggestions for improvement:
Punctuation consistency: Some entries end with a period, while others don't. Consider standardizing this across all entries.
Common UI elements: The file seems to be missing translations for some common UI elements. Consider adding translations for:
- "Yes"
- "No"
- "OK"
- "Cancel" (there's a specific "NEW_TRANSACTION_CANCEL", but a generic one might be useful)
Grouping: Consider grouping related items under nested objects for better organization. For example, group all navigation-related items under a "nav" object.
Would you like assistance in implementing these suggestions?
frontend/package.json (1)
12-13: LGTM! Consider using double quotes for consistency.The changes to the scripts section look good. The addition of the "i18n-server" script aligns with the PR's objective of adding i18n extraction capabilities.
For consistency with other scripts in this file, consider using double quotes instead of single quotes in the "prettier:fix" script:
- "prettier:fix": "prettier --write 'src/**/*.{js,jsx,ts,tsx,json,css,scss,md}'", + "prettier:fix": "prettier --write \"src/**/*.{js,jsx,ts,tsx,json,css,scss,md}\"",frontend/src/pages/Accounts.tsx (2)
71-71: Excellent internationalization improvement!The change from a hardcoded string to a translatable one using the
tfunction is a great step towards better internationalization support. This allows for easy translation of the "Hide closed accounts" label into different languages.Consider using a more specific key for this translation. Instead of
'accounts.hide_closed_accounts', you might want to use something like'accounts.toggle_label.hide_closed_accounts'. This can help organize translations better, especially as the project grows.- {t('accounts.hide_closed_accounts')} + {t('accounts.toggle_label.hide_closed_accounts')}
Line range hint
1-101: Suggestions for component improvementsWhile the component functions well, here are some suggestions to enhance its structure and maintainability:
Consider extracting the account filtering logic into a custom hook or utility function. This would improve readability and make the logic reusable across other components if needed.
The component uses a mix of Mantine hooks and custom UI components. For consistency, consider standardizing on one set of UI components throughout the application.
The
refreshAccountsfunction is called directly in the onClick handler. For larger applications, it's often beneficial to wrap such calls in useCallback to prevent unnecessary re-renders and improve performance.Here's an example of how you could implement these suggestions:
// In a new file, e.g., useAccountFiltering.ts import { useState, useMemo } from 'react'; import { AccountStatus } from '../rest-model'; import AccountTrie from '../utils/AccountTrie'; export function useAccountFiltering(accounts, hideClosedAccount, filterKeyword) { return useMemo(() => { let trie = new AccountTrie(); for (let account of accounts.filter((it) => (hideClosedAccount ? it.status === AccountStatus.Open : true))) { let trimmedKeyword = filterKeyword.trim().toLowerCase(); if (trimmedKeyword === '' || account.name.toLowerCase().includes(trimmedKeyword) || (account.alias?.toLowerCase() ?? '').includes(trimmedKeyword)) { trie.insert(account); } } return trie; }, [accounts, hideClosedAccount, filterKeyword]); } // In Accounts.tsx import { useCallback } from 'react'; import { useAccountFiltering } from './useAccountFiltering'; export default function Accounts() { // ... other code ... const accounts = useAtomValue(accountAtom); const accountTrie = useAccountFiltering(accounts, hideClosedAccount, filterKeyword); const refreshAccountsCallback = useCallback(() => refreshAccounts(), [refreshAccounts]); // ... rest of the component ... return ( // ... other JSX ... <Button variant="outline" onClick={refreshAccountsCallback}> {t('REFRESH')} </Button> // ... other JSX ... ); }These changes would make the component more modular, consistent, and potentially more performant.
frontend/src/components/ErrorBox.tsx (2)
57-57: Approve the change with a minor suggestion for improvement.The modification enhances the robustness of the error type rendering by handling the case where
selectError?.error_typeis undefined. This prevents potential runtime errors and improves the component's reliability.For improved readability, consider using a nullish coalescing operator:
-<p>{selectError?.error_type === undefined ? '' : t(`ERROR.${selectError.error_type}`)}</p> +<p>{t(`ERROR.${selectError?.error_type ?? ''}`)}</p>This change maintains the same functionality while making the code more concise and easier to read.
Line range hint
44-51: Address commented-out code insaveErrorModifyDataThe
saveErrorModifyDatafunction contains commented-out code that appears to be related to modifying a file. This might indicate incomplete functionality or outdated code.Consider one of the following actions:
- If the functionality is needed, implement it and remove the comments.
- If the functionality is no longer required, remove the commented-out code entirely.
- If the functionality is planned for future implementation, add a TODO comment explaining the plan and timeline.
Keeping the codebase clean of unused commented code improves maintainability and reduces confusion for other developers.
frontend/src/server.js (2)
35-48: Handle concurrent write operations to prevent data corruptionMultiple clients might attempt to write to the same translation file simultaneously, leading to race conditions and potential data loss or corruption.
Consider implementing file write queuing, locks, or using asynchronous file operations with proper concurrency control to handle simultaneous write requests. Alternatively, using a database or a dedicated key-value store like Redis could help manage concurrent access more effectively.
16-18: Consolidate logging statements for clarityThe logs at lines 16 and 18 both print the contents of
req.bodywith messages about missing and inserting translations. Since both log statements output the same data, consider consolidating them or differentiating their purposes more clearly.For example:
- console.log(`Missing translations for ${req.params.lng}/${req.params.ns}:`, req.body); ... - console.log(`inserting translations for ${req.params.lng}/${req.params.ns}:`, req.body); + console.log(`Processing translations for ${req.params.lng}/${req.params.ns}:`, req.body);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
- frontend/package.json (4 hunks)
- frontend/public/locales/en/translation.json (1 hunks)
- frontend/public/locales/zh/translation.json (1 hunks)
- frontend/src/components/ErrorBox.tsx (1 hunks)
- frontend/src/i18n.ts (1 hunks)
- frontend/src/pages/Accounts.tsx (1 hunks)
- frontend/src/server.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
frontend/src/i18n.ts (3)
4-4: LGTM: Improved code readabilityThe addition of an empty line after the import statements improves code readability by clearly separating the imports from the rest of the code.
Line range hint
1-35: Summary of i18n configuration changesThe changes to the i18n configuration in this file are generally positive, enhancing the internationalization capabilities of the application. Key points from the review:
- Improved code readability with better spacing.
- Changed fallbackLng behavior, which needs verification to ensure it doesn't negatively impact user experience.
- Added saveMissing option, which is good for development purposes.
- Introduced backend configuration, which is a step in the right direction but may need refinement for better portability across environments.
Please address the suggestions in the previous comments, particularly regarding the use of environment variables for the backend URL. This will make the configuration more flexible and easier to manage across different environments.
12-13: Verify the impact of fallbackLng change and consider saveMissing implicationsThe changes to the i18n configuration have significant implications:
Setting
fallbackLng: falsemeans no fallback language will be used when a translation is missing. This could lead to blank spots or keys being displayed instead of translated text in the UI. Ensure this is the intended behavior and that all necessary translations are in place.The
saveMissing: isDevelopmentoption is a good practice. It allows saving missing translation keys in development, which can help identify missing translations, but prevents this in production.To ensure all necessary translations are in place, run the following script:
This script will help identify any missing translations across all locale files.
✅ Verification successful
Re-attempting to verify missing translations with updated search parameters
The previous script failed to locate the
localesdirectory. Let's search the entire repository for JSON files that may contain translation keys.
All translations are complete;
fallbackLngset tofalseis safeThe verification shows that there are no missing translations in the locale files:
frontend/public/locales/en/translation.jsonfrontend/public/locales/zh/translation.jsonTherefore, setting
fallbackLng: falsewill not result in untranslated keys being displayed in the UI. Additionally, thesaveMissing: isDevelopmentconfiguration ensures that any future missing translations are tracked during development.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for missing translations in all locale files # Find all locale JSON files locale_files=$(fd -e json . locales) # Loop through each file and check for empty string values for file in $locale_files; do echo "Checking $file for missing translations:" jq -r 'paths as $p | select(getpath($p) == "") | $p | join(".")' "$file" echo "---" doneLength of output: 134
Script:
#!/bin/bash # Description: Locate all potential locale JSON files and check for missing translations # Find all JSON files that likely contain locale translations locale_files=$(fd -e json .) # Filter files that are likely to be locale files based on common naming conventions locale_files=$(echo "$locale_files" | grep -E '/(locales|i18n|translations)/|/en\.json$|/fr\.json$|/es\.json$') if [ -z "$locale_files" ]; then echo "No locale JSON files found." exit 1 fi # Loop through each locale file and check for empty string values for file in $locale_files; do echo "Checking $file for missing translations:" jq -r 'paths as $p | select(getpath($p) == "") | $p | join(".")' "$file" echo "---" doneLength of output: 5664
frontend/public/locales/zh/translation.json (3)
38-41: LGTM: New translations are consistent and clear.The new translations for ledger health status and account visibility options are well-structured and consistent with the existing translations. They provide clear Chinese equivalents for the English phrases.
53-54: LGTM: Transaction edit modal translations are accurate and complete the UI set.The new translations for the transaction edit modal description and title are clear, consistent with the existing style, and accurately convey the intended meanings in Chinese. These additions complete the set of UI elements for comprehensive transaction management.
38-54: Overall: Excellent additions to Chinese localizationThe new translations added in this file significantly enhance the internationalization support for Chinese users. They provide clear and accurate translations for new UI elements related to transaction management, account visibility, and journal operations. The additions are well-structured, consistent with existing translations, and maintain the established JSON format.
A few minor suggestions have been made for naming consistency, but overall, these changes greatly improve the user experience for Chinese-speaking users of the application.
frontend/package.json (3)
94-94: LGTM! Improved formatting.The addition of a trailing comma after the last devDependency is a good practice. It improves consistency and makes future additions to the list easier and cleaner in version control diffs.
74-75: Verify the necessity of new server-related devDependencies.The addition of new devDependencies (body-parser, cors, express, i18next-http-middleware) aligns with the PR's objective of adding an i18n extraction server. However:
- Consider if these server-related packages should be regular dependencies instead of devDependencies, especially if they're used in the i18n-server script.
- Ensure that the i18n extraction server is only used during development and not in production.
Please run the following script to check the usage of these new dependencies:
#!/bin/bash # Description: Check usage of new server-related dependencies # Test: Search for usage of new dependencies in the codebase echo "Searching for usage of new dependencies:" rg --type typescript --type javascript 'from [\'"]express[\'"]|from [\'"]body-parser[\'"]|from [\'"]cors[\'"]|from [\'"]i18next-http-middleware[\'"]' -A 5 # Note: Review the output to ensure these dependencies are used appropriately and only in development contexts.Also applies to: 80-80, 83-83
48-48: Verify compatibility with the updated react-i18next version.The dependency updates look good and align with the PR's i18n focus. However, please note:
- The update to react-i18next (^15.0.2) is a major version bump from the previous ^14.1.1. This may introduce breaking changes.
- The new "translation-check" dependency is a welcome addition for i18n support.
Please run the following script to check for any breaking changes in the react-i18next update:
Also applies to: 56-56
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary by CodeRabbit
New Features
Bug Fixes
Chores