translation support for packages/loot-core/src/server/budget/template-nodes #3840
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
AlexMuellr
changed the title
WalkthroughThe pull request introduces changes to the Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint (1.23.1)
packages/loot-core/src/server/budget/template-notes.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/server/budget/template-notes.ts (1)
Translation implementation needs attention
The verification reveals several issues that need to be addressed:
- The
useTranslationhook from 'react-i18next' is being used in a non-React context (server-side code)- The translation keys are not found in any locale files
- The test file doesn't account for translations
Required changes:
- Replace
useTranslationwith a server-side compatible translation solution- Add the new translation keys to the locale files
- Update tests to handle translations
🔗 Analysis chain
Line range hint
38-69: Verify translation implementation completenessLet's ensure all necessary parts of the translation system are in place.
Please ensure that:
- Translation files exist and contain the new keys
- All calls to
checkTemplatesare updated to handle the translation function parameter if you choose that approach
Let me gather more information about the translation implementation.
Let me check the actual translation files in the locale directory to verify if the new translation keys are present.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for translation key files and usage of checkTemplates echo "Checking for translation files..." fd -e json -e yaml "translation" -x cat {} \; echo "Checking for checkTemplates usage..." rg "checkTemplates\(" --type tsLength of output: 547
Script:
#!/bin/bash # Check the implementation of translation in related files echo "Checking translation setup in template-notes.ts..." rg "useTranslation|import.*i18n" packages/loot-core/src/server/budget/template-notes.ts -A 5 echo "Checking translation setup in goaltemplates.ts..." rg "useTranslation|import.*i18n" packages/loot-core/src/server/budget/goaltemplates.ts -A 5 echo "Checking for i18n configuration..." fd -e ts "i18n" -x cat {} \;Length of output: 1796
Script:
#!/bin/bash # Check translation files in locale directory echo "Checking locale files..." fd -e json "en.json" -x cat {} \; # Check if the translation is used correctly in the test file echo "Checking test implementation..." rg -A 10 "checkTemplates" packages/loot-core/src/server/budget/template-notes.test.tsLength of output: 975
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loot-core/src/server/budget/template-notes.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/loot-core/src/server/budget/template-notes.ts
[warning] 12-12:
react-i18next import should occur before import of ../../client/state-types/notifications
[failure] 39-39:
React Hook "useTranslation" is called in function "checkTemplates" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use"
| resetCategoryGoalDefsWithNoTemplates, | ||
| } from './statements'; | ||
| import { Template } from './types/templates'; | ||
| import { useTranslation } from 'react-i18next'; |
There was a problem hiding this comment.
Critical: Incorrect usage of React hooks in server-side code
The useTranslation hook cannot be used in server-side code as it's specifically designed for React components. This will cause runtime errors since React hooks can only be used within React components or custom hooks.
Consider these alternatives for server-side translation:
- Use i18next directly instead of react-i18next:
-import { useTranslation } from 'react-i18next';
+import i18next from 'i18next';
export async function checkTemplates(): Promise<Notification> {
- const { t } = useTranslation();
+ const t = i18next.t.bind(i18next);
// ... rest of the function- Or inject the translation function as a parameter:
-import { useTranslation } from 'react-i18next';
-export async function checkTemplates(): Promise<Notification> {
+export async function checkTemplates(t: (key: string, options?: any) => string): Promise<Notification> {
- const { t } = useTranslation();
// ... rest of the functionAlso applies to: 39-39
🧰 Tools
🪛 GitHub Check: lint
[warning] 12-12:
react-i18next import should occur before import of ../../client/state-types/notifications
| @@ -48,22 +50,22 @@ | |||
| template.type === 'schedule' && | |||
| !scheduleNames.includes(template.name) | |||
| ) { | |||
| errors.push(`${id}: Schedule “${template.name}” does not exist`); | |||
| errors.push(t('{{id}}: Schedule {{template.name}} does not exist')); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve translation key structure and accessibility
The current translation implementation could be improved in several ways:
- Use proper i18n key format instead of template literals:
- errors.push(t('{{id}}: Schedule {{template.name}} does not exist'));
+ errors.push(t('errors.schedule.not_found', { id, templateName: template.name }));- Use structured translation keys for better organization:
- message: t('There were errors interpreting some templates:'),
+ message: t('templates.validation.error_header'),
- message: t('All templates passed! 🎉'),
+ message: t('templates.validation.success'),- Consider adding aria-label for emoji content or moving emoji to CSS for better accessibility.
Also applies to: 61-61, 68-68
translation support for
packages/loot-core/src/server/budget/template-nodes.ts