[WIP] Translation: desktop-electron #3267
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 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
|
|
Still needs work... 
|
|
Trying an alternate approach. Seems to get farther, but getting an error loading one of the modules (which breaks the main window's thread): 
One other issue. I couldn't figure out how to access localStorage, so for now it is hard coding to 'en' locale. |
|
👋 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. |
|
👋 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. |
|
👋 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. |
WalkthroughThe pull request introduces internationalization (i18n) support in the Electron application by adding the Changes
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: 11
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
upcoming-release-notes/3267.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (7)
- .eslintignore (1 hunks)
- packages/desktop-client/i18next-parser.config.js (1 hunks)
- packages/desktop-electron/i18n.ts (1 hunks)
- packages/desktop-electron/index.ts (4 hunks)
- packages/desktop-electron/menu.ts (11 hunks)
- packages/desktop-electron/package.json (2 hunks)
- packages/desktop-electron/preload.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- .eslintignore
Additional context used
GitHub Check: lint
packages/desktop-electron/i18n.ts
[warning] 27-27:
Prefer named exports
Additional comments not posted (9)
packages/desktop-electron/preload.ts (1)
19-19: Review security implications of exposingprocessto the rendererExposing the
processobject viabackend.preloadBindings(ipcRenderer, process)can introduce security risks. Verify that this exposure is necessary and that it complies with Electron's security best practices to prevent potential vulnerabilities.Consider whether all required functionalities can be achieved without exposing the entire
processobject to the renderer process.packages/desktop-electron/package.json (1)
93-94: Internationalization dependencies added successfullyThe addition of
"i18next"and"i18next-electron-fs-backend"is appropriate for implementing internationalization support in the Electron application.packages/desktop-electron/menu.ts (3)
264-281: Ensure Reliability When Modifying the 'Window' MenuWhen modifying the
'Window'menu, you are locating it usingtemplate.findIndex(t => t.role === 'window'), which is reliable due to the static role. However, ensure that any additional modifications do not rely on translated labels to prevent similar issues.
9-9: Verify Proper Initialization of thei18nModuleEnsure that the
i18nmodule imported from'./i18n'is properly initialized before being used. An uninitialized translation module could lead to untranslated labels or runtime errors.
209-209: Confirm Conditional Inclusion of the 'Screenshot' Menu ItemThe
'Screenshot'menu item is conditionally included based on theisDevflag. Verify that this item is appropriately excluded from production builds to prevent unintended exposure of development tools.packages/desktop-electron/index.ts (2)
118-118: Integration ofbackend.mainBindingsis correctThe call to
backend.mainBindings(ipcMain, win, fs);successfully sets up the i18n backend in the main process, enabling translation functionality within the Electron application.
277-277: Proper cleanup withbackend.clearMainBindingsCalling
backend.clearMainBindings(ipcMain);ensures that i18n bindings are properly cleared when all windows are closed on macOS, preventing potential memory leaks or dangling references.packages/desktop-electron/i18n.ts (2)
6-7: Verify Relative Paths for Translation FilesThe
loadPathandaddPathare using relative paths that may not resolve correctly in the Electron environment, especially when packaging the application. Ensure that these paths correctly point to your translation files at runtime.Consider modifying the paths to use
__dirnamefor accurate resolution:- loadPath: '../desktop-client/locale/{{lng}}.json', - addPath: '../desktop-client/locale/{{lng}}.missing.json', + loadPath: path.join(__dirname, 'locale', '{{lng}}.json'), + addPath: path.join(__dirname, 'locale', '{{lng}}.missing.json'),Don't forget to import the
pathmodule at the beginning of your file:+import path from 'path';
23-23: Re-evaluate Disabling Value Escaping in InterpolationsSetting
escapeValue: falsedisables HTML escaping during interpolation, which can lead to security risks like XSS attacks if any untrusted data is interpolated.Ensure that all interpolated values in your translations are from trusted sources. If there's any chance untrusted data could be used, consider setting
escapeValuetotrue:interpolation: { - escapeValue: false, + escapeValue: true, },
| input: [ | ||
| 'src/**/*.{js,jsx,ts,tsx}', | ||
| '../loot-core/src/**/*.{js,jsx,ts,tsx}', | ||
| '../desktop-electron/*.{js,ts}', |
There was a problem hiding this comment.
Include subdirectories in the input path for comprehensive localization
The current input path '../desktop-electron/*.{js,ts}' only includes JavaScript and TypeScript files located directly in the ../desktop-electron/ directory. To ensure that all relevant files, including those in subdirectories, are processed for translation keys, consider updating the pattern to include subdirectories.
Apply this diff to update the input path:
- '../desktop-electron/*.{js,ts}',
+ '../desktop-electron/**/*.{js,ts}',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.
| '../desktop-electron/*.{js,ts}', | |
| '../desktop-electron/**/*.{js,ts}', |
| @@ -6,6 +6,8 @@ import { | |||
| SaveFileDialogPayload, | |||
| } from './index'; | |||
|
|
|||
| const backend = require('i18next-electron-fs-backend'); | |||
There was a problem hiding this comment.
Use ES6 import instead of CommonJS require
For consistency and better TypeScript integration, consider importing i18next-electron-fs-backend using ES6 import syntax. This enhances type checking and aligns the code with modern standards.
Suggested change:
-const backend = require('i18next-electron-fs-backend');
+import backend from 'i18next-electron-fs-backend';Ensure that the module exports support this syntax, and adjust the import statement if necessary.
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 backend = require('i18next-electron-fs-backend'); | |
| import backend from 'i18next-electron-fs-backend'; |
| @@ -90,6 +90,9 @@ | |||
| "dependencies": { | |||
| "electron-is-dev": "2.0.0", | |||
| "electron-log": "4.4.8", | |||
| "i18next": "^23.12.6", | |||
| "i18next-electron-fs-backend": "^3.0.2", | |||
| "lodash": "^4.17.21", | |||
There was a problem hiding this comment.
Consider optimizing lodash usage
Adding "lodash" introduces a large dependency that can increase the application's size. If only a few utility functions are needed, consider importing specific modules (e.g., "lodash.merge") or using native JavaScript alternatives to minimize the bundle size.
| @@ -98,6 +101,7 @@ | |||
| "@electron/notarize": "2.4.0", | |||
| "@electron/rebuild": "3.6.0", | |||
| "@types/copyfiles": "^2", | |||
| "@types/lodash": "^4", | |||
There was a problem hiding this comment.
Align TypeScript definitions with lodash version
The @types/lodash package is specified with "^4". To ensure compatibility and prevent potential type mismatches, consider specifying the exact version to match the "lodash" version "^4.17.21".
Apply this diff to update the @types/lodash version:
"@types/copyfiles": "^2",
- "@types/lodash": "^4",
+ "@types/lodash": "^4.17.21",
"copyfiles": "^2.4.1",Committable suggestion was skipped due to low confidence.
| @@ -238,13 +240,13 @@ export function getMenu( | |||
| ], | |||
| }); | |||
| // Edit menu. | |||
| const editIdx = template.findIndex(t => t.label === 'Edit'); | |||
| const editIdx = template.findIndex(t => t.label === i18n.t('Edit')); | |||
There was a problem hiding this comment.
Potential Issue with Using Translated Labels in findIndex
Using i18n.t('Edit') within template.findIndex may lead to issues if the translation of 'Edit' changes or differs across languages. This could result in editIdx being -1, causing a runtime error when accessing template[editIdx].
Consider assigning a unique identifier to the 'Edit' menu item to reliably locate it, regardless of language.
Apply this diff to fix the issue:
{
label: i18n.t('Edit'),
+ id: 'edit-menu',
submenu: [
// submenu items...
],
},
// Later in the code, find the 'Edit' menu using the id
-const editIdx = template.findIndex(t => t.label === i18n.t('Edit'));
+const editIdx = template.findIndex(t => t.id === 'edit-menu');Committable suggestion was skipped due to low confidence.
| const tools = menu.items.filter(item => item.label === i18n.t('Tools'))[0]; | ||
| tools.submenu?.items.forEach(item => { |
There was a problem hiding this comment.
Avoid using translated labels for menu item identification
Filtering menu items using i18n.t('Tools') can cause identification issues when the language changes. Assign unique identifiers to menu items to ensure reliable access regardless of the current language setting.
Suggested refactor:
Assign an id to the "Tools" menu item:
{
label: i18n.t('Tools'),
id: 'menu-tools',
// ...
},Update the reference in your code:
-const tools = menu.items.filter(item => item.label === i18n.t('Tools'))[0];
+const tools = menu.getMenuItemById('menu-tools');| const edit = menu.items.filter(item => item.label === i18n.t('Edit'))[0]; | ||
| const editItems = edit.submenu?.items || []; | ||
| editItems | ||
| .filter(item => item.label === 'Undo' || item.label === 'Redo') | ||
| .filter( | ||
| item => item.label === i18n.t('Undo') || item.label === i18n.t('Redo'), | ||
| ) |
There was a problem hiding this comment.
Avoid using translated labels for menu item identification
Using i18n.t('Edit'), i18n.t('Undo'), and i18n.t('Redo') for filtering can become unreliable with different languages. Assign unique identifiers to these menu items to maintain consistent behavior.
Suggested refactor:
Assign ids to the relevant menu items:
{
label: i18n.t('Edit'),
id: 'menu-edit',
submenu: [
{
label: i18n.t('Undo'),
id: 'menu-undo',
// ...
},
{
label: i18n.t('Redo'),
id: 'menu-redo',
// ...
},
// ...
],
},Update your filtering logic:
-const edit = menu.items.filter(item => item.label === i18n.t('Edit'))[0];
+const edit = menu.getMenuItemById('menu-edit');
- editItems.filter(
- item => item.label === i18n.t('Undo') || item.label === i18n.t('Redo'),
- )
+ editItems.filter(
+ item => item.id === 'menu-undo' || item.id === 'menu-redo',
+ )| import { getMenu } from './menu'; | ||
| import { | ||
| get as getWindowState, | ||
| listen as listenToWindowState, | ||
| } from './window-state'; | ||
|
|
||
| import './security'; | ||
| const backend = require('i18next-electron-fs-backend'); |
There was a problem hiding this comment.
Use ES6 import instead of require for module consistency
To maintain consistency and leverage TypeScript's features, consider replacing require with an import statement. This ensures better type checking and aligns with modern ES6 module syntax.
Apply this diff:
-const backend = require('i18next-electron-fs-backend');
+import backend from 'i18next-electron-fs-backend';If i18next-electron-fs-backend does not have a default export or if there are compatibility issues, you might need to adjust the import:
-const backend = require('i18next-electron-fs-backend');
+import * as backend from 'i18next-electron-fs-backend';Ensure that your tsconfig.json has "esModuleInterop": true if you're using default imports with CommonJS modules.
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 backend = require('i18next-electron-fs-backend'); | |
| import backend from 'i18next-electron-fs-backend'; |
| lng: 'en', // FIXME localStorage undefined | ||
| // lng: localStorage.getItem('language') || 'en', |
There was a problem hiding this comment.
Cannot Access localStorage in the Main Process
localStorage is a Web API available in the renderer process, not in the main process of an Electron application. Attempting to access it in the main process results in undefined.
To resolve this, consider the following options:
-
Option 1: Use Electron's
ipcMainandipcRenderermodules to communicate the language preference from the renderer process to the main process. -
Option 2: Store the language preference in a configuration file using Electron's
app.getPath('userData')directory.
Here's how you might implement Option 1:
-
In your renderer process, send the language preference to the main process:
// renderer.js const { ipcRenderer } = require('electron'); ipcRenderer.send('set-language', localStorage.getItem('language') || 'en');
-
In the main process, listen for the language setting and initialize
i18naccordingly:+import { ipcMain } from 'electron'; let language = 'en'; // default language +ipcMain.on('set-language', (event, lng) => { + language = lng; + i18n.changeLanguage(language); +}); i18n.use(backend).init({ // ... - lng: 'en', // FIXME localStorage undefined + lng: language, // ... });
| }, | ||
| }); | ||
|
|
||
| export default i18n; |
There was a problem hiding this comment.
Prefer Named Exports Over Default Export
Using named exports can improve maintainability and enable easier refactoring and code navigation.
Change the default export to a named export:
-export default i18n;
+export { i18n };And update any import statements accordingly:
-import i18n from './i18n';
+import { i18n } from './i18n';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.
| export default i18n; | |
| export { i18n }; |
Tools
GitHub Check: lint
[warning] 27-27:
Prefer named exports
Translation support for
packages/desktop-electron.