🐛 Fix translations failing #3830
Conversation
✅ 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 No assets were bigger Smaller
Unchanged No assets were unchanged |
|
@awaisalee Can you update the spreadsheet with this reversion? There's some setup needed before we can get the translations working with the server files in loot-core. |
WalkthroughThe pull request introduces changes across multiple files in 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
packages/loot-core/src/server/sheet.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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
packages/loot-core/src/server/budget/goaltemplates.ts (3)
197-199: Ensure consistent error message formattingThe error message structure using
prefield for formatted text is maintained, which is good for consistency. However, consider extracting these error messages to a centralized location for better maintainability.Consider creating an error constants file:
+// error-messages.ts +export const TEMPLATE_ERRORS = { + TEMPLATE_UP_TO_DATE: 'Everything is up to date', + TEMPLATE_PARSING_ERROR: 'There were errors interpreting some templates:' +}; - message: 'There were errors interpreting some templates:', + message: TEMPLATE_ERRORS.TEMPLATE_PARSING_ERROR,
248-248: Type-safe template string usageThe template literal is correctly used for dynamic content. However, consider adding type safety for the
catObjects.lengthinterpolation.Add type assertion or validation:
- message: `Successfully applied templates to ${catObjects.length} categories`, + message: `Successfully applied templates to ${catObjects.length.toString()} categories`,
Line range hint
1-1: Consider removing @ts-strict-ignoreThe file uses TypeScript but bypasses strict type checking. Consider addressing type safety issues to remove the
@ts-strict-ignoredirective.Key areas to address:
- Add proper type annotations for function parameters
- Handle potential null/undefined values explicitly
- Define interfaces for template and category objects
packages/loot-core/src/server/budget/cleanup-template.ts (2)
323-335: Standardize notification message structureThe notification messages in this block and throughout the function could benefit from a more structured approach:
- Consider using an enum or constant for message types
- Create a centralized message catalog that can support future internationalization
Example structure:
enum NotificationType { ERROR = 'error', WARNING = 'warning', MESSAGE = 'message' } interface NotificationMessage { type: NotificationType; messageKey: string; params?: Record<string, any>; } const MESSAGES = { TEMPLATE_ERRORS: 'There were errors interpreting some templates:', FUNDS_NOT_AVAILABLE: 'Global: Funds not available:', CATEGORIES_UPDATED: 'All categories were up to date.' } as const;
345-351: Improve template string handlingThe template string construction is correct, but consider extracting the pluralization logic into a helper function for better maintainability and future internationalization support.
Example:
function pluralize(count: number, singular: string, plural: string): string { return count === 1 ? singular : plural; } const applied = `Successfully returned funds from ${num_sources} ${ pluralize(num_sources, 'source', 'sources') } and funded ${num_sinks} sinking ${ pluralize(num_sinks, 'fund', 'funds') }.`;packages/loot-core/src/server/main.ts (1)
1096-1097: Consider adding error code for better error handling.While removing the translation function is correct, consider adding an error code to this generic error message to help with troubleshooting and potential future internationalization.
message: - 'There was an internal error. Please get in touch https://actualbudget.org/contact for support.', + { code: 'INTERNAL_SYNC_ERROR', message: 'There was an internal error. Please get in touch https://actualbudget.org/contact for support.' },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3830.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/loot-core/src/server/budget/cleanup-template.ts(5 hunks)packages/loot-core/src/server/budget/goaltemplates.ts(2 hunks)packages/loot-core/src/server/main.ts(1 hunks)packages/loot-core/src/server/mutators.ts(1 hunks)packages/loot-core/src/server/sheet.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/loot-core/src/server/mutators.ts
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/server/main.ts (1)
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/client/actions/account.ts:180-194
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/client/actions/account.ts`, within the `syncAccounts` function, the batch sync request for SimpleFin accounts handles errors by returning error objects instead of throwing exceptions. Therefore, wrapping this block in a try-catch is unnecessary.
🔇 Additional comments (5)
packages/loot-core/src/server/sheet.ts (1)
135-137: Verify impact of removing translations
The removal of translations for these breadcrumb messages aligns with the PR objectives to fix translation issues. However, we should verify that these messages are intended for debugging/monitoring only and don't need to be user-facing or localized.
Also applies to: 165-167
✅ Verification successful
Breadcrumb messages are for server-side debugging only
Based on the code context, these breadcrumb messages are strictly server-side debugging/monitoring messages:
- They are captured in the server-side code (
server/sheet.ts) - The messages are only used with
captureBreadcrumbfor error tracking withcategory: 'server' - There are no exports or UI components consuming these messages
- They are logged during spreadsheet loading/initialization process, which is a server-side operation
The removal of translations for these messages is appropriate since they are not user-facing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these messages are used in any UI components or user-facing contexts
# Search for any UI components that might use these messages
rg -l "loading spreadsheet|loaded spreadsheet" --type ts --type tsx
# Check if these breadcrumb messages are used anywhere else
ast-grep --pattern 'captureBreadcrumb({
message: $_,
category: "server"
})'
Length of output: 179
Script:
#!/bin/bash
# Search for any UI components that might use these messages, fixing the file type flags
rg -l "loading spreadsheet|loaded spreadsheet" -t typescript -t javascriptreact -t typescriptreact
# Check for breadcrumb usage in server code
rg -B2 -A2 "captureBreadcrumb.*category.*server" -t typescript -t javascript
# Look for any imports or exports of these messages that might indicate UI usage
rg "export.*loading.*spreadsheet|import.*loading.*spreadsheet" -t typescript -t javascript
Length of output: 377
Script:
#!/bin/bash
# Search for any UI components that might use these messages
rg -l "loading spreadsheet|loaded spreadsheet"
# Check for breadcrumb usage in server code
rg -B2 -A2 "captureBreadcrumb.*category.*server"
# Look for any imports or exports of these messages that might indicate UI usage
rg "export.*loading.*spreadsheet|import.*loading.*spreadsheet"
# Check the specific file for context around these breadcrumbs
cat packages/loot-core/src/server/sheet.ts
Length of output: 7241
packages/loot-core/src/server/budget/goaltemplates.ts (1)
191-191: Consider maintaining internationalization support
While removing the translation function resolves the immediate issue, this change might impact future internationalization needs. Consider implementing a centralized message handling system that can support translations when needed.
Let's check if other files still use translations:
packages/loot-core/src/server/budget/cleanup-template.ts (1)
Line range hint 1-391: Implement proper translation infrastructure
The current changes remove translations entirely, which is a temporary fix for the translation failures. Based on the PR objectives, a more appropriate solution would be to:
- Set up proper JSON-based translations in the worker process
- Implement a translation service that works within the server context
- Maintain internationalization support for all user-facing messages
This would provide a more maintainable and user-friendly solution than removing translations entirely.
Let's check for any translation-related configuration or implementation:
packages/loot-core/src/server/main.ts (2)
1090-1091: Template literal replacement looks good.
The replacement of the t function with a template literal maintains the same functionality while removing the translation dependency. The error message format is preserved correctly.
1090-1097: Verify error handling consistency.
The removal of translations from error messages should be consistent across all sync-related error handling. Let's verify this:
✅ Verification successful
Let me gather more information about error message patterns in the codebase to make a comprehensive verification.
Let me try one more search pattern to find error message handling patterns in the codebase.
Based on the comprehensive search results, I can now provide a final response regarding the error handling consistency.
Error handling is consistent with non-translated messages.
The codebase shows a consistent pattern of using non-translated error messages, particularly in sync and account-related functionality. The changes in the review are aligned with the broader codebase where:
- Direct string messages are used for error handling in sync operations
- Account-related errors use plain strings without translation wrappers
- Internal errors consistently use direct string messages
The only exception found is in the desktop client UI component (GoCardlessExternalMsgModal.tsx) which is expected as it's a user-facing component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for remaining translation usage in error handling
# Look for remaining translation calls in error handling code
rg "t\(['\"].*error" --type ts
# Look for similar error handling patterns
ast-grep --pattern 'message: t($$$)'
Length of output: 1386
Script:
#!/bin/bash
# Search for error message patterns in sync-related files
rg "message: ['\"].*error" --type ts packages/loot-core/src/server/sync/
rg "message: ['\"].*Error" --type ts packages/loot-core/src/server/sync/
# Search for error message patterns in account-related files
rg "message: ['\"].*error" --type ts packages/loot-core/src/server/accounts/
rg "message: ['\"].*Error" --type ts packages/loot-core/src/server/accounts/
# Look for translation usage in error messages specifically in sync and account modules
rg "message:\s*t\(['\"]" --type ts packages/loot-core/src/server/sync/
rg "message:\s*t\(['\"]" --type ts packages/loot-core/src/server/accounts/
Length of output: 469
Script:
#!/bin/bash
# Search for error message patterns more broadly
rg "message.*?['\"].*?error" -i --type ts -A 2 packages/loot-core/src/server/
rg "throw new Error\(" --type ts -A 2 packages/loot-core/src/server/
# Look for any translation usage in error contexts
rg "t\(['\"].*?(error|Error)" --type ts -A 2 packages/loot-core/src/server/
Length of output: 29200
@MikesGlitch I updated the spreadsheet. |
Translations aren't working in the lootcore server files. It'll be because they run in a worker process and aren't setup with the json.
Related to: #3829 #3827