Translate calendar related strings #4211
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 (largest 100 files by percent change)
View detailed bundle breakdownAdded No assets were added Removed No assets were removed 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 (largest 100 files by percent change)
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
|
@joel-jeremy , any way to access the redux store from loot-core? actual/packages/loot-core/src/server/budget/actions.ts Lines 554 to 556 in 1442662 and actual/packages/loot-core/src/shared/schedules.ts Lines 62 to 194 in 1442662 There both cases of translations AND dates needing locale (from the redux store) |
WalkthroughThis pull request introduces comprehensive localization enhancements across multiple components in the desktop client and core libraries. The primary focus is on implementing a new The implementation adds an optional Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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 (
|
![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 comments (1)
packages/desktop-client/src/components/schedules/ScheduleDetails.tsx (1)
Add unit tests for locale-specific date formatting functionality
The codebase lacks test coverage for locale-specific date formatting. Specifically:
- No tests verify the correct formatting of dates with different locales in
monthUtils.format- Edge cases like DST transitions with different locales are not tested
- The
nameForMonthfunction's locale-specific behavior is untestedThe implementation in
months.tsproperly supports locale-specific formatting through date-fns, but without tests, we cannot ensure reliable behavior across different locales and date format patterns.🔗 Analysis chain
Line range hint
1-941: Add unit tests for locale-specific date formatting.The changes introduce locale-dependent behavior that should be thoroughly tested.
Would you like me to generate comprehensive unit tests for the locale-specific date formatting functionality?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing tests covering date formatting rg -g '*test*.{ts,tsx}' "monthUtils.format|dateFormat|locale" packages/desktop-client/src/Length of output: 364
Script:
#!/bin/bash # Check for tests in loot-core package echo "=== Checking for monthUtils tests ===" rg -g '*test*.{ts,tsx,js}' "monthUtils.format|dateFormat|locale" packages/loot-core/src/ echo -e "\n=== Checking monthUtils implementation ===" cat packages/loot-core/src/shared/months.{ts,js} 2>/dev/null || trueLength of output: 14174
🧹 Nitpick comments (15)
packages/desktop-client/src/components/reports/reports/CustomReport.tsx (1)
125-125: Consider adding type safety for Redux state.While the Redux selector implementation is correct, consider adding type safety to prevent potential runtime errors:
-const locale = useSelector(state => state.app.locale); +const locale = useSelector((state: RootState) => state.app.locale);packages/desktop-client/src/components/reports/reports/Spending.tsx (1)
Line range hint
89-119: Consider adding cleanup for the async effect.While the locale integration is correct, the effect could benefit from proper cleanup to handle potential race conditions when the locale changes rapidly.
Consider this improvement:
useEffect(() => { + let mounted = true; async function run() { const trans = await send('get-earliest-transaction'); + if (!mounted) return; let earliestMonth = trans ? monthUtils.monthFromDate(d.parseISO(fromDateRepr(trans.date))) : monthUtils.currentMonth(); // ... rest of the code ... setAllIntervals(allMonths); } run(); + return () => { + mounted = false; + }; }, [locale]);packages/desktop-client/src/components/reports/reports/NetWorth.tsx (2)
115-115: Consider using date-fns locale integration.The month formatting looks good, but since date-fns is already imported, consider using its built-in locale support for consistent date formatting across the application.
- pretty: monthUtils.format(month, 'MMMM, yyyy', locale), + pretty: d.format(d.parseISO(month), 'MMMM, yyyy', { locale: require(`date-fns/locale/${locale}`) }),
122-122: Add error handling to the async effect.The effect dependency update is correct, but consider adding error handling for the async operations.
useEffect(() => { async function run() { + try { const trans = await send('get-earliest-transaction'); const currentMonth = monthUtils.currentMonth(); let earliestMonth = trans ? monthUtils.monthFromDate(d.parseISO(fromDateRepr(trans.date))) : currentMonth; // ... rest of the code setAllMonths(allMonths); + } catch (error) { + console.error('Failed to fetch transaction data:', error); + // Consider showing user-friendly error notification + } } run(); }, [locale]);packages/desktop-client/src/components/reports/DateRange.tsx (1)
47-48: LGTM! Consider extracting date format strings as constants.The locale integration for date formatting is well implemented. Consider extracting the format strings ('MMM yyyy' and 'MMMM yyyy') as named constants to improve maintainability and reusability.
+const DATE_FORMAT = { + SHORT: 'MMM yyyy', + LONG: 'MMMM yyyy', +} as const; + export function DateRange({ start, end, type }: DateRangeProps): ReactElement { const locale = useSelector(state => state.app.locale); // ... - const formattedStartDate = d.format(startDate, 'MMM yyyy', { locale }); - const formattedEndDate = d.format(endDate, 'MMM yyyy', { locale }); + const formattedStartDate = d.format(startDate, DATE_FORMAT.SHORT, { locale }); + const formattedEndDate = d.format(endDate, DATE_FORMAT.SHORT, { locale }); // ... - content = d.format(endDate, 'MMMM yyyy', { locale }); + content = d.format(endDate, DATE_FORMAT.LONG, { locale });Also applies to: 84-84
packages/desktop-client/src/components/reports/ReportSummary.tsx (1)
40-40: LGTM! Comprehensive locale support implementation.The locale integration is thorough and well-implemented:
- Correctly retrieves locale from Redux store
- Consistently applies locale to all date formatting calls
- Properly handles translation of the "to" connector
Consider simplifying the date range formatting logic.
The current implementation has multiple repeated calls to
monthUtils.formatwith identical parameters. Consider extracting this into a helper function to reduce code duplication and improve maintainability.const { t } = useTranslation(); +const formatDate = (date: string) => + monthUtils.format(date, ReportOptions.intervalFormat.get(interval) || '', locale); + return ( // ... <Text> - {monthUtils.format( - startDate, - ReportOptions.intervalFormat.get(interval) || '', - locale, - )} - {monthUtils.format( - startDate, - ReportOptions.intervalFormat.get(interval) || '', - locale, - ) !== - monthUtils.format( - endDate, - ReportOptions.intervalFormat.get(interval) || '', - locale, - ) && - ` ${t('to')} ` + - monthUtils.format( - endDate, - ReportOptions.intervalFormat.get(interval) || '', - locale, - )} + {formatDate(startDate)} + {formatDate(startDate) !== formatDate(endDate) && + ` ${t('to')} ${formatDate(endDate)}`} </Text>Also applies to: 77-77, 82-82, 87-87, 89-89, 93-93
packages/loot-core/src/client/app/appSlice.ts (3)
130-134: Add type safety for locale imports.The current implementation lacks type checking for imported locales.
Consider adding type guards:
function isLocale(value: unknown): value is Locale { return value !== null && typeof value === 'object' && 'code' in value && 'formatDistance' in value; }
136-136: Remove commented out code.Remove the commented line as it's not being used.
137-142: Improve error handling.The error handling could be more specific and provide better debugging information.
Consider this improvement:
} catch (error) { const errorMessage = error instanceof Error ? error.message : 'Unknown error'; console.error( `Failed to load locale for language ${language}: ${errorMessage}. Falling back to default.` ); return enUS; }packages/desktop-client/src/components/reports/graphs/CashFlowGraph.tsx (1)
62-64: Consider extracting repeated date formatting logic.The date formatting logic is duplicated across multiple components. Consider extracting it into a reusable function.
Consider this improvement:
const formatDate = (date: Date, isConcise: boolean, locale: Locale) => { return d.format( date, isConcise ? "MMM ''yy" : 'MMM d', { locale } ); };Then use it consistently:
// In XAxis tickFormatter={x => formatDate(x, isConcise, locale)} // In Tooltip labelFormatter={x => formatDate(x, isConcise, locale)}Also applies to: 143-145, 163-165
packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts (1)
122-123: Enhance error handling for date formatting.While the locale integration is correct, consider wrapping the date formatting in a try-catch block to handle potential locale-related formatting errors gracefully.
- fromRange: d.format(startDay, 'MMM yy', { locale }), - toRange: d.format(endDay, 'MMM yy', { locale }), + fromRange: (() => { + try { + return d.format(startDay, 'MMM yy', { locale }); + } catch (error) { + console.error('Error formatting date with locale:', error); + return d.format(startDay, 'MMM yy'); + } + })(), + toRange: (() => { + try { + return d.format(endDay, 'MMM yy', { locale }); + } catch (error) { + console.error('Error formatting date with locale:', error); + return d.format(endDay, 'MMM yy'); + } + })(),packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetSummary.tsx (1)
31-31: Consider creating a custom hook for locale access.The pattern of accessing locale with
useSelector(state => state.app.locale)is repeated across multiple components. Consider creating a custom hook likeuseLocaleto:
- Centralize the locale access logic
- Make it easier to modify the locale access pattern in the future
- Reduce code duplication
- Make the code more maintainable
Example implementation:
// hooks/useLocale.ts import { useSelector } from '../redux'; export function useLocale() { return useSelector(state => state.app.locale); }This would simplify the components to:
const locale = useLocale();Also applies to: 82-82, 98-98, 1976-1976
packages/desktop-client/src/components/schedules/ScheduleDetails.tsx (2)
745-745: Consider extracting date formatting logic.While the locale integration is correct, the date formatting logic could be moved to a custom hook or utility function to promote reusability and maintain consistency across the application.
+// In a new file: hooks/useDateFormatter.ts +export function useDateFormatter() { + const locale = useSelector(state => state.app.locale); + const dateFormat = useDateFormat() || 'MM/dd/yyyy'; + + return useCallback((date: string, format?: string) => { + return monthUtils.format(date, format || `${dateFormat} EEEE`, locale); + }, [locale, dateFormat]); +} // In ScheduleDetails.tsx -{monthUtils.format(date, `${dateFormat} EEEE`, locale)} +{dateFormatter(date)}
Line range hint
1-941: Consider adding error boundary for date formatting.The component should gracefully handle invalid dates or locales to prevent rendering failures.
+// Add error boundary component +class DateFormattingErrorBoundary extends React.Component { + componentDidCatch(error, errorInfo) { + console.error('Date formatting error:', error, errorInfo); + } + + render() { + return this.props.children; + } +} // Wrap date formatting section +<DateFormattingErrorBoundary> <Stack direction="column" spacing={1} style={{ marginTop: 10, color: theme.pageTextLight }}> {state.upcomingDates.map(date => ( <View key={date}> {monthUtils.format(date, `${dateFormat} EEEE`, locale)} </View> ))} </Stack> +</DateFormattingErrorBoundary>packages/desktop-client/src/components/reports/reports/CashFlow.tsx (1)
Line range hint
67-132: Consider extracting locale-dependent formatting logicThe implementation looks good, but consider these improvements:
- Extract the locale-dependent date formatting logic into a custom hook (e.g.,
useDateFormat) to promote reusability and consistency across components- Add tests specifically for locale-dependent functionality to ensure proper localization
Example custom hook:
function useDateFormat() { const locale = useSelector(state => state.app.locale); return useCallback((date: string, format: string) => { return monthUtils.format(date, format, locale); }, [locale]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4211.mdis excluded by!**/*.md
📒 Files selected for processing (33)
packages/desktop-client/src/components/App.tsx(2 hunks)packages/desktop-client/src/components/budget/MonthPicker.tsx(3 hunks)packages/desktop-client/src/components/budget/envelope/budgetsummary/BudgetSummary.tsx(4 hunks)packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetSummary.tsx(4 hunks)packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx(3 hunks)packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx(3 hunks)packages/desktop-client/src/components/mobile/budget/index.tsx(3 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx(3 hunks)packages/desktop-client/src/components/modals/EnvelopeBudgetMonthMenuModal.tsx(3 hunks)packages/desktop-client/src/components/modals/EnvelopeBudgetSummaryModal.tsx(2 hunks)packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx(3 hunks)packages/desktop-client/src/components/modals/TrackingBudgetMonthMenuModal.tsx(3 hunks)packages/desktop-client/src/components/reports/DateRange.tsx(4 hunks)packages/desktop-client/src/components/reports/Header.tsx(3 hunks)packages/desktop-client/src/components/reports/ReportSummary.tsx(3 hunks)packages/desktop-client/src/components/reports/graphs/CashFlowGraph.tsx(6 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(4 hunks)packages/desktop-client/src/components/reports/reports/CashFlow.tsx(4 hunks)packages/desktop-client/src/components/reports/reports/CustomReport.tsx(4 hunks)packages/desktop-client/src/components/reports/reports/NetWorth.tsx(4 hunks)packages/desktop-client/src/components/reports/reports/NetWorthCard.tsx(3 hunks)packages/desktop-client/src/components/reports/reports/Spending.tsx(6 hunks)packages/desktop-client/src/components/reports/reports/Summary.tsx(4 hunks)packages/desktop-client/src/components/reports/reports/SummaryCard.tsx(3 hunks)packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet.tsx(4 hunks)packages/desktop-client/src/components/reports/spreadsheets/net-worth-spreadsheet.ts(4 hunks)packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts(2 hunks)packages/desktop-client/src/components/schedules/ScheduleDetails.tsx(3 hunks)packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx(3 hunks)packages/desktop-client/src/components/settings/LanguageSettings.tsx(1 hunks)packages/desktop-client/src/hooks/useSyncLocale.ts(1 hunks)packages/loot-core/src/client/app/appSlice.ts(5 hunks)packages/loot-core/src/shared/months.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4041
File: packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx:518-544
Timestamp: 2024-12-26T01:16:39.339Z
Learning: A <Trans> inside another <Trans> does not necessarily produce two distinct strings. Instead, it merges them into a single translation entry, allowing for object literals to be used within the same translation string.
🔇 Additional comments (46)
packages/desktop-client/src/components/reports/reports/CustomReport.tsx (2)
29-29: LGTM!The Redux selector import is correctly placed and follows the project's import conventions.
323-323: LGTM!The locale parameter is correctly integrated with the monthUtils.format function, enabling proper date localization.
packages/desktop-client/src/components/reports/reports/Spending.tsx (3)
17-17: LGTM! Clean Redux integration for locale support.The Redux setup for locale management follows best practices with proper selector implementation.
Also applies to: 63-63
451-458: LGTM! Consistent locale implementation across date formatting.The date formatting is consistently implemented with proper locale support throughout the UI elements.
Also applies to: 479-480, 500-501
Line range hint
451-501: Verify date format accessibility across different locales.While the date formatting implementation is correct, it's important to verify that the chosen format ('MMM, yyyy') remains accessible and readable across different locales, especially for screen readers.
Run this script to check the date format implementation across the codebase:
✅ Verification successful
Date format implementation is accessible and properly localized
The date format implementation is verified to be accessible and consistent across the codebase. The format uses full month names and properly handles localization through the locale parameter, making it screen-reader friendly across different languages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent date format usage and potential accessibility issues # Check for date format patterns in the codebase echo "Checking date format patterns..." rg -g '*.{ts,tsx}' "'MMM,?\s*yyyy'" # Check for other monthUtils.format usage that might need locale echo -e "\nChecking other monthUtils.format usage..." ast-grep --pattern 'monthUtils.format($_, $_, $_)' --lang tsLength of output: 1270
packages/desktop-client/src/components/reports/reports/NetWorth.tsx (2)
18-18: LGTM: Clean Redux integration for locale management.The Redux integration follows best practices with proper typing and state access.
Also applies to: 56-56
83-92: Verify spreadsheet function's locale handling.The locale integration looks good, but let's verify that the netWorthSpreadsheet function properly handles the locale parameter.
✅ Verification successful
Locale integration properly implemented
The netWorthSpreadsheet function correctly uses the locale parameter for date formatting in the generated spreadsheet, ensuring consistent date representation according to the user's locale preferences.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if netWorthSpreadsheet function is updated to handle locale ast-grep --pattern 'function netWorthSpreadsheet($_) { $$$ }'Length of output: 65
Script:
#!/bin/bash # Find file containing netWorthSpreadsheet definition rg "netWorthSpreadsheet" -l # Try different patterns to locate the function ast-grep --pattern 'export function netWorthSpreadsheet($_) { $$$ }' ast-grep --pattern 'export const netWorthSpreadsheet = $_' # Get the actual implementation with context rg "netWorthSpreadsheet" -A 10Length of output: 7933
Script:
#!/bin/bash # Find and examine the net-worth-spreadsheet implementation fd "net-worth-spreadsheet" --type f # Get the content of the spreadsheet file rg -A 20 "createSpreadsheet" packages/desktop-client/src/components/reports/spreadsheets/net-worth-spreadsheet.ts || rg -A 20 "createSpreadsheet" packages/desktop-client/src/components/reports/spreadsheets/net-worth-spreadsheet.tsxLength of output: 949
Script:
#!/bin/bash # Get the entire content of net-worth-spreadsheet.ts to see locale usage cat packages/desktop-client/src/components/reports/spreadsheets/net-worth-spreadsheet.tsLength of output: 4881
packages/desktop-client/src/hooks/useSyncLocale.ts (1)
9-18: LGTM! Well-structured custom hook implementation.The hook follows React's best practices:
- Correctly imports
useDispatchfromsrc/redux- Properly includes all dependencies in the useEffect dependency array
- Efficiently dispatches the
fetchLocaleaction only when language changespackages/desktop-client/src/components/reports/reports/SummaryCard.tsx (1)
36-36: LGTM! Clean implementation of locale support.The locale integration is well-implemented:
- Correctly retrieves locale from Redux store
- Properly passes locale to summarySpreadsheet
- Updates useMemo dependencies appropriately
Also applies to: 67-67, 69-69
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx (1)
37-37: LGTM! Locale support properly implemented.The locale integration follows the established pattern across the application.
Verify date format pattern compatibility.
The format string
'MMMM 'yy'contains single quotes which might cause issues in some locales. Consider testing with various locales to ensure consistent formatting.Also applies to: 112-112
✅ Verification successful
Format pattern 'MMMM 'yy' is correctly implemented and widely used
The date format pattern is consistently used across budget-related components and follows the established convention in the codebase. The single quotes are correctly used for escaping special characters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all monthUtils.format usage to compare format patterns rg -l "monthUtils\.format" | xargs rg "monthUtils\.format\([^)]+\)" -A 1Length of output: 7404
packages/desktop-client/src/components/reports/reports/NetWorthCard.tsx (1)
43-43: LGTM! Consistent implementation of locale support.The locale integration matches the pattern used in other components:
- Correctly retrieves locale from Redux store
- Properly passes locale to netWorthSpreadsheet
- Updates useMemo dependencies appropriately
Also applies to: 62-62, 64-64
packages/desktop-client/src/components/modals/EnvelopeBudgetSummaryModal.tsx (1)
11-11: LGTM! Clean implementation of locale support.The changes correctly integrate Redux for locale management and properly apply the locale to date formatting.
Also applies to: 30-30, 32-32
packages/desktop-client/src/components/reports/spreadsheets/net-worth-spreadsheet.ts (1)
30-31: LGTM! Good propagation of locale parameter.The locale parameter is correctly propagated through the function chain, enabling localized date formatting.
Also applies to: 82-82, 94-94
packages/desktop-client/src/components/budget/MonthPicker.tsx (2)
9-9: LGTM: Redux integration for locale management.The Redux store integration is properly implemented to access the locale state.
Also applies to: 31-31
106-106: LGTM: Locale-aware month formatting.The month formatting now correctly uses the locale from Redux state, ensuring proper internationalization.
packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts (1)
19-19: LGTM: Added locale parameter.The locale parameter is properly typed and positioned as the last parameter, maintaining backward compatibility.
packages/desktop-client/src/components/reports/Header.tsx (1)
11-11: LGTM: Consistent locale integration.The locale integration follows the same pattern as other components, ensuring consistent internationalization across the application.
Also applies to: 70-70, 127-127
packages/desktop-client/src/components/modals/TrackingBudgetMonthMenuModal.tsx (1)
13-13: LGTM: Redux integration for locale.The Redux store integration is properly implemented to access the locale state.
Also applies to: 32-32
packages/desktop-client/src/components/App.tsx (1)
30-30: LGTM! Good placement of the locale synchronization hook.The
useSyncLocalehook is correctly placed at the root level of the application, ensuring that locale synchronization is available throughout the component tree.Also applies to: 164-164
packages/desktop-client/src/components/modals/EnvelopeBudgetMonthMenuModal.tsx (1)
13-13: LGTM! Consistent locale usage for month formatting.The locale is correctly retrieved from the Redux store and consistently used across all month formatting calls.
Also applies to: 32-32, 61-61
packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet.tsx (2)
211-213: LGTM! Proper locale usage in date formatting.The locale is correctly passed to the date formatting function, ensuring consistent date localization.
76-76: Verify function calls after parameter changes.The addition of the
localeparameter tocashFlowByDateandrecalculatefunctions is a breaking change. Ensure all callers of these functions are updated to provide the locale parameter.Run the following script to find all usages of these functions:
Also applies to: 137-137, 152-152
✅ Verification successful
All function calls are properly updated with the locale parameter
The verification shows that:
- The only usage of
cashFlowByDatein CashFlow.tsx already includes the locale parameter- The
recalculatefunction in cash-flow-spreadsheet.tsx and its usage is properly updated with the locale parameter- Other
recalculatefunctions found in the codebase are unrelated implementations with different signatures🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of cashFlowByDate function to ensure they're updated with the locale parameter # Search for function calls ast-grep --pattern 'cashFlowByDate($$$)' # Also check for any direct imports and usages rg -A 5 'cashFlowByDate'Length of output: 2180
Script:
#!/bin/bash # Search for recalculate function calls and usages ast-grep --pattern 'recalculate($$$)' rg -A 5 '\brecalculate\b'Length of output: 14838
packages/desktop-client/src/components/budget/envelope/budgetsummary/BudgetSummary.tsx (2)
11-11: LGTM! Proper Redux integration for locale.The locale is correctly retrieved from the Redux store using the
useSelectorhook.Also applies to: 29-29
49-53: LGTM! Consistent locale usage across month formatting.The locale is consistently applied to all month formatting calls, ensuring uniform localization throughout the component.
Also applies to: 59-59, 133-133
packages/loot-core/src/shared/months.ts (1)
396-397: LGTM! Well-structured locale integration.The changes appropriately add locale support to date formatting functions while maintaining backward compatibility through optional parameters. The implementation correctly utilizes date-fns' locale support.
Also applies to: 400-405
packages/desktop-client/src/components/mobile/budget/index.tsx (1)
26-26: LGTM! Clean Redux integration for localization.The changes properly integrate locale support through Redux, with correct dependency management in the useCallback hook. The implementation follows React best practices.
Also applies to: 40-40, 442-442, 447-447
packages/desktop-client/src/components/reports/reports/Summary.tsx (1)
25-25: LGTM! Comprehensive locale integration in reports.The changes properly integrate locale support throughout the component, with appropriate handling of locale changes in useEffect and correct propagation to the summary spreadsheet.
Also applies to: 68-68, 125-125, 133-133, 178-178, 185-185
packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx (1)
22-22: LGTM! Clean locale integration for schedule preview.The changes properly integrate locale support for day name formatting in the schedule preview, maintaining consistency with the application's localization approach.
Also applies to: 243-243, 263-263
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (3)
31-31: LGTM: Import statement added correctly.The addition of
useSelectorfrom redux is appropriate for accessing the locale state.
82-82: LGTM: Locale state accessed correctly.The locale is properly retrieved from the Redux store using useSelector.
173-173: LGTM: Date formatting updated with locale support.The
monthUtils.formatcall now correctly includes the locale parameter for proper date localization.packages/desktop-client/src/components/reports/reports/Calendar.tsx (3)
50-50: LGTM: Import statement added correctly.The addition of
useSelectorfrom redux is appropriate for accessing the locale state.
98-98: LGTM: Locale state accessed correctly.The locale is properly retrieved from the Redux store using useSelector.
270-270: Verify the effect dependency array update.The locale has been added to the useEffect dependency array, which is correct. However, let's verify that all dependencies are properly listed to prevent stale closure issues.
Also applies to: 280-280
✅ Verification successful
Dependencies are correctly specified
The useEffect dependency array correctly includes only
localeas it's the only value that needs to trigger a re-render when changed. All other values used within the effect are either static imports or stable references that don't need to be included in the dependency array.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for variables used within the useEffect ast-grep --pattern 'useEffect(async function run() { $$$ monthUtils.format($_, $_, locale) $$$ }, [$$$])'Length of output: 113
Script:
#!/bin/bash # Get the useEffect containing monthUtils.format ast-grep --pattern 'useEffect($$$)' packages/desktop-client/src/components/reports/reports/Calendar.tsx # Also get the file content around line 270 for context rg -A 10 -B 10 "monthUtils.format" packages/desktop-client/src/components/reports/reports/Calendar.tsxLength of output: 12116
packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (3)
33-33: LGTM: Import statement added correctly.The addition of
useSelectorfrom redux is appropriate for accessing the locale state.
1976-1976: LGTM: Locale state accessed correctly.The locale is properly retrieved from the Redux store using useSelector.
2021-2021: LGTM: Date formatting updated with locale support.The
monthUtils.formatcall now correctly includes the locale parameter for proper date localization.packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetSummary.tsx (3)
12-12: LGTM: Import statement added correctly.The addition of
useSelectorfrom redux is appropriate for accessing the locale state.
31-31: LGTM: Locale state accessed correctly.The locale is properly retrieved from the Redux store using useSelector.
56-56: LGTM: Date formatting updated with locale support.Both instances of
monthUtils.formatnow correctly include the locale parameter for proper date localization.Also applies to: 125-125
packages/desktop-client/src/components/schedules/ScheduleDetails.tsx (2)
24-24: LGTM! Locale integration looks good.The Redux integration for locale management is properly implemented. The locale is correctly retrieved from the Redux store using
useSelector.Also applies to: 109-109
Line range hint
1-941: Verify i18n coverage for new date formats.The component handles date formatting with locale support, but we should verify that all date-related strings and formats are properly internationalized.
✅ Verification successful
Date formatting is properly internationalized ✅
The component follows the established i18n patterns by:
- Using the
useDateFormat()hook with proper fallback- Formatting dates through locale-aware utilities (
monthUtils.format)- Consistently passing the locale parameter for all date formatting operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for hardcoded date formats and non-translated date-related strings rg -g '*.{ts,tsx}' "MM/dd/yyyy|EEEE|dddd|MMMM" packages/desktop-client/src/Length of output: 6045
packages/desktop-client/src/components/reports/reports/CashFlow.tsx (4)
21-21: LGTM: Clean Redux integration for locale managementThe Redux integration follows best practices with proper use of hooks and selector pattern.
Also applies to: 67-67
132-132: Verify completeness of locale-dependent effectsThe locale dependency has been added to this useEffect. Let's verify if there are other effects that might need locale updates.
✅ Verification successful
All locale-dependent operations are properly handled
The locale dependency is correctly implemented across all relevant operations:
- Month formatting in useEffect
- Cash flow parameters in useMemo
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other useEffect hooks in the file that might need locale rg "useEffect.*\{" "packages/desktop-client/src/components/reports/reports/CashFlow.tsx" # Search for date formatting in effects rg -A 5 -B 5 "useEffect.*\{.*\bformat\b" "packages/desktop-client/src/components/reports/reports/CashFlow.tsx"Length of output: 219
Script:
#!/bin/bash # Get more context around the useEffect rg -A 10 -B 10 "useEffect" "packages/desktop-client/src/components/reports/reports/CashFlow.tsx" # Look for imports of formatting utilities rg "import.*format" "packages/desktop-client/src/components/reports/reports/CashFlow.tsx" # Search for any usage of formatting functions or locale rg -A 2 -B 2 "\b(format|locale)\b" "packages/desktop-client/src/components/reports/reports/CashFlow.tsx"Length of output: 2129
108-110: Verify locale handling in cashFlowByDate functionThe locale parameter has been added to the cashFlowByDate function call. Let's verify its implementation.
✅ Verification successful
Locale parameter is properly implemented ✅
The locale parameter is correctly passed through to the date formatting function, ensuring proper date localization in the cash flow report.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the cashFlowByDate function definition to verify locale parameter handling rg -A 10 "export function cashFlowByDate" "packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet" # Search for any date formatting within the function that might need locale rg "format|parseISO|parse|toString" "packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet"Length of output: 489
Script:
#!/bin/bash # Find the import statement for cashFlowByDate rg "import.*cashFlowByDate" packages/desktop-client # Find all files that might contain the cashFlowByDate function fd -e ts -e tsx | xargs rg "export.*function cashFlowByDate"Length of output: 374
Script:
#!/bin/bash # Get the full implementation of cashFlowByDate and surrounding context rg -A 50 "export function cashFlowByDate" packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet.tsx # Look for date formatting utilities usage in the same file rg "format|parseISO|parse|toString|Intl" packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet.tsxLength of output: 1940
Script:
#!/bin/bash # Get more context around the date formatting code rg -B 5 -A 5 "format\(x, isConcise" packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet.tsxLength of output: 422
125-125: Verify monthUtils.format localization implementationThe locale parameter has been added to monthUtils.format. Let's verify its implementation.
✅ Verification successful
Locale support properly implemented in monthUtils.format
The monthUtils.format function correctly implements locale support by passing the locale parameter through to date-fns, which has robust internationalization capabilities built-in.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the monthUtils.format function definition rg -A 10 "export function format" "packages/loot-core/src/shared/months" # Check if date-fns format is being used with locale rg "date-fns|format" "packages/loot-core/src/shared/months"Length of output: 286
Script:
#!/bin/bash # Find files containing monthUtils fd -e ts -e js monthUtils # Search for monthUtils format function definition across all files rg -g '*.{ts,js}' -A 10 "monthUtils.*format|export.*format.*month"Length of output: 9591
Script:
#!/bin/bash # Find monthUtils definition ast-grep --pattern 'export const monthUtils = $_' # Find monthUtils imports to trace its source rg "import.*monthUtils" # Find potential date-fns usage with format rg "from 'date-fns'"Length of output: 16642
Script:
#!/bin/bash # Find the implementation of format in months.ts rg -A 10 "export.*format.*=" "packages/loot-core/src/shared/months.ts" # Also check if there are any locale-related imports from date-fns rg "from 'date-fns/locale" "packages/loot-core/src/shared/months.ts"Length of output: 1732
Script:
#!/bin/bash # Try different patterns to find format implementation ast-grep --pattern 'function format($_) { $$$ }' # Search for any line containing format function definition rg "^(export )?function format|^export const format" "packages/loot-core/src/shared/months.ts" # Get the entire months.ts file content as it might be using a different export pattern rg -l "import.*from 'date-fns'" "packages/loot-core/src/shared/months.ts" | xargs catLength of output: 13577
packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/spreadsheets/net-worth-spreadsheet.ts
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/TrackingBudgetMonthMenuModal.tsx
Show resolved
Hide resolved
You can actually import the store the call Why do we need to put the locale in the store? Why don't we store the locale as a global pref (same as how the language global pref is being stored) and get the locale from there? |
I guess I can try serializing the locale object to see what happens. |
Global prefs are stored via |
|
@joel-jeremy, I tried to use asyncStorage with |
|
Also, anyone knows why |
We shouldn't be storing the entire Locale object IMO. Instead, we should store the locale key on the global pref and retrieve the Locale instance based on that key. It is also not recommended to store Locale in the store if it contains non-serializable properties such as functions: https://redux.js.org/style-guide/#do-not-put-non-serializable-values-in-state-or-actions |
lelemm
changed the title
lelemm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/desktop-client/src/components/reports/reports/Spending.tsx (1)
Hardcoded strings 'Spent' and 'MTD' need translation
The review comment is correct. Found untranslated strings at:
- Lines 480-483:
SpentandMTDhardcoded- Lines 501-504: Same untranslated strings
The suggested fix using
t()function is appropriate as the translation infrastructure is already set up and used consistently for other strings in the file.🔗 Analysis chain
Line range hint
452-463: Verify translations in all date-formatted strings.The locale is consistently applied to all
monthUtils.formatcalls, which is good. However, ensure that the surrounding text strings like "Spent" and "MTD" are also properly translated using thetfunction fromuseTranslation.- Spent{' '} + {t('Spent')}{' '} {monthUtils.format(compare, 'MMM, yyyy', locale)} - {compare === monthUtils.currentMonth() && ' MTD'} + {compare === monthUtils.currentMonth() && t(' MTD')}Also applies to: 480-483, 501-504
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check for translation imports and usage echo "=== Checking translation imports ===" rg "useTranslation|import.*i18n" packages/desktop-client/src/components/reports/reports/Spending.tsx echo -e "\n=== Checking existing translations of 'Spent' and 'MTD' ===" rg "'Spent'|\"Spent\"|'MTD'|\"MTD\"" packages/desktop-client/src/components/reports/reports/Spending.tsx -A 2 -B 2 echo -e "\n=== Looking for translation patterns ===" rg "t\(['\"]" packages/desktop-client/src/components/reports/reports/Spending.tsxLength of output: 1165
Script:
#!/bin/bash # Check the specific line ranges mentioned in the review echo "=== Checking line range 449-462 ===" rg -A 13 -B 2 "label={monthUtils\.format\(compare," packages/desktop-client/src/components/reports/reports/Spending.tsx echo -e "\n=== Checking line ranges 480-483 and 501-504 ===" sed -n '480,483p;501,504p' packages/desktop-client/src/components/reports/reports/Spending.tsxLength of output: 1001
🧹 Nitpick comments (12)
packages/loot-core/src/shared/rules.ts (1)
143-158: LGTM! Consider using translation key constants.The translation implementation is correct and consistent. However, to improve maintainability and prevent typos, consider defining these translation keys as constants.
Example implementation:
const TRANSLATION_KEYS = { ACCOUNT: 'account', DATE: 'date', CATEGORY: 'category', // ... other keys } as const; // Usage in mapField case 'account': return t(TRANSLATION_KEYS.ACCOUNT);packages/desktop-client/src/components/reports/reports/CashFlow.tsx (1)
126-126: Consider memoizing the month formatting function.While the locale integration is correct, the
runfunction insideuseEffectis recreated on every render. Consider extracting and memoizing it to optimize performance.+ const formatMonth = useMemo( + () => (month: string) => ({ + name: month, + pretty: monthUtils.format(month, 'MMMM, yyyy', locale), + }), + [locale] + ); useEffect(() => { async function run() { const trans = await send('get-earliest-transaction'); const earliestMonth = trans ? monthUtils.monthFromDate(d.parseISO(trans.date)) : monthUtils.currentMonth(); const allMonths = monthUtils .rangeInclusive(earliestMonth, monthUtils.currentMonth()) - .map(month => ({ - name: month, - pretty: monthUtils.format(month, 'MMMM, yyyy', locale), - })) + .map(formatMonth) .reverse(); setAllMonths(allMonths); } run(); }, [locale, formatMonth]);Also applies to: 133-133
packages/desktop-client/src/components/reports/reports/Summary.tsx (1)
179-179: Consider caching the month formatting function.While the implementation is correct, the month formatting operation could be optimized by memoizing the format function to prevent unnecessary re-renders, especially since it's used within a map operation.
Consider this optimization:
+ const formatMonth = useMemo( + () => (month: string) => monthUtils.format(month, 'MMMM, yyyy', locale), + [locale] + ); useEffect(() => { async function run() { // ... const allMonths = monthUtils .rangeInclusive(earliestMonth, monthUtils.currentMonth()) .map(month => ({ name: month, - pretty: monthUtils.format(month, 'MMMM, yyyy', locale), + pretty: formatMonth(month), })) .reverse(); // ... } run(); }, [locale, formatMonth]);Also applies to: 186-186
packages/desktop-client/src/components/rules/Value.tsx (1)
Line range hint
1-1: Consider addressing TypeScript technical debt.The file has several TypeScript-related issues that should be addressed:
@ts-strict-ignoreat the top of the file disables strict type checking- Multiple
@ts-expect-errorcomments throughout the code suppress type errorsConsider creating follow-up tasks to:
- Enable strict type checking
- Fix the underlying type issues instead of suppressing them
Would you like me to help create issues to track these TypeScript improvements?
packages/desktop-client/src/components/reports/reports/Spending.tsx (1)
Line range hint
91-120: Consider memoizing the month formatting logic.The effect runs on every locale change and recreates the entire months array. Consider memoizing this computation to optimize performance.
+ const formattedMonths = useMemo( + () => + monthUtils + .rangeInclusive(earliestMonth, monthUtils.currentMonth()) + .map(month => ({ + name: month, + pretty: monthUtils.format(month, 'MMMM, yyyy', locale), + })) + .reverse(), + [earliestMonth, locale] + ); + useEffect(() => { async function run() { const trans = await send('get-earliest-transaction'); let earliestMonth = trans ? monthUtils.monthFromDate(d.parseISO(fromDateRepr(trans.date))) : monthUtils.currentMonth(); // Make sure the month selects are at least populates with a // year's worth of months. We can undo this when we have fancier // date selects. const yearAgo = monthUtils.subMonths(monthUtils.currentMonth(), 12); if (earliestMonth > yearAgo) { earliestMonth = yearAgo; } - const allMonths = monthUtils - .rangeInclusive(earliestMonth, monthUtils.currentMonth()) - .map(month => ({ - name: month, - pretty: monthUtils.format(month, 'MMMM, yyyy', locale), - })) - .reverse(); - setAllIntervals(allMonths); + setAllIntervals(formattedMonths); } run(); }, [locale, formattedMonths]);packages/loot-core/src/shared/locale.ts (1)
3-11: Consider a more scalable approach for locale management.The current implementation might become difficult to maintain as more languages are added. Consider:
- Using dynamic imports for locales to reduce bundle size
- Implementing error handling for unsupported languages
- Creating a mapping of language codes to their respective locales
import { enUS, ptBR } from 'date-fns/locale'; + +const LOCALE_MAP = { + 'en-US': enUS, + 'pt-BR': ptBR, +} as const; export function getLocale(language: string) { - switch (language) { - case 'pt-BR': - return ptBR; - - default: - return enUS; - } + return LOCALE_MAP[language] || enUS; }packages/loot-core/src/shared/schedules.ts (2)
67-74: Consider using i18next's built-in day name translations.Instead of maintaining a manual mapping of day names, consider using i18next's built-in day translations or date-fns' formatting.
- const days = { - SU: t('Sunday'), - MO: t('Monday'), - TU: t('Tuesday'), - WE: t('Wednesday'), - TH: t('Thursday'), - FR: t('Friday'), - SA: t('Saturday'), - }; - return days[day]; + const dayMap = { SU: 0, MO: 1, TU: 2, WE: 3, TH: 4, FR: 5, SA: 6 }; + return monthUtils.format(new Date(2020, 0, 5 + dayMap[day]), 'EEEE', locale);
86-94: Add translation contexts for proper pluralization.Some translations might need context for proper pluralization in different languages. Consider using i18next's context feature for translations that involve numbers.
Example for interval translations:
-t(`{{interval}} days`, { interval }) +t('days_interval', { interval, count: interval })Add to your translation file:
{ "days_interval": "{{interval}} days", "days_interval_1": "{{interval}} day" }Also applies to: 106-114, 118-120, 181-182, 188-192
packages/loot-core/src/shared/schedules.test.ts (1)
8-54: Consider extracting translations to a separate file.The i18next initialization with English translations is correctly implemented. However, for better maintainability and reusability, consider moving these translations to a dedicated translation file.
Example structure:
// en.ts export const en = { translation: { Every: 'Every', day: 'day', // ... rest of the translations } }; // schedules.test.ts import { en } from './locales/en'; i18next.init({ lng: 'en', fallbackLng: 'en', resources: { en }, interpolation: { escapeValue: false, }, });packages/desktop-client/src/components/select/DateSelect.tsx (2)
92-115: Consider adding JSDoc comments and improving error handling.The
createPikadayLocalefunction is well-implemented but could benefit from better documentation and error handling./** * Creates a Pikaday locale object from a date-fns locale. * @param dateFnsLocale The date-fns locale object * @returns A PikadayI18n object with localized month and weekday names * @throws {Error} If the locale is invalid or missing required formats */ function createPikadayLocale(dateFnsLocale: Locale): PikadayI18n { if (!dateFnsLocale) { throw new Error('Invalid locale: locale object is required'); } const months = Array.from({ length: 12 }, (_, i) => format(new Date(2023, i, 1), 'MMMM', { locale: dateFnsLocale }), ); const weekdays = Array.from({ length: 7 }, (_, i) => format(new Date(2023, 0, i + 1), 'EEEE', { locale: dateFnsLocale }), ); const weekdaysShort = Array.from({ length: 7 }, (_, i) => format(new Date(2023, 0, i + 1), 'EEE', { locale: dateFnsLocale }).slice( 0, 3, ), ); return { previousMonth: 'Previous', nextMonth: 'Next', months, weekdays, weekdaysShort, }; }
130-130: Consider memoizing the locale object.The locale object is correctly used, but consider memoizing it to prevent unnecessary re-renders.
const locale = useLocale(); const pikadayLocale = useMemo( () => createPikadayLocale(locale), [locale] ); useLayoutEffect(() => { picker.current = new Pikaday({ // ...other options i18n: pikadayLocale, }); // ... }, [pikadayLocale]); // Update effect dependencyAlso applies to: 169-186
packages/desktop-client/src/components/budget/MonthPicker.tsx (1)
7-7: Consider memoizing formatted month names.The locale is correctly used for month formatting, but consider memoizing the formatted month names to prevent unnecessary recalculations.
const locale = useLocale(); const formattedMonths = useMemo( () => range.map(month => ({ month, name: monthUtils.format(month, 'MMM', locale) })), [range, locale] ); // In the render loop: {formattedMonths.map(({ month, name: monthName }, idx) => { // ... rest of the render logic })}Also applies to: 31-31, 106-106
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
packages/desktop-client/src/components/budget/MonthPicker.tsx(3 hunks)packages/desktop-client/src/components/budget/envelope/budgetsummary/BudgetSummary.tsx(4 hunks)packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetSummary.tsx(4 hunks)packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx(3 hunks)packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx(3 hunks)packages/desktop-client/src/components/mobile/budget/index.tsx(3 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx(3 hunks)packages/desktop-client/src/components/modals/EnvelopeBudgetMonthMenuModal.tsx(3 hunks)packages/desktop-client/src/components/modals/EnvelopeBudgetSummaryModal.tsx(2 hunks)packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx(3 hunks)packages/desktop-client/src/components/modals/TrackingBudgetMonthMenuModal.tsx(3 hunks)packages/desktop-client/src/components/reports/DateRange.tsx(4 hunks)packages/desktop-client/src/components/reports/Header.tsx(3 hunks)packages/desktop-client/src/components/reports/ReportSummary.tsx(3 hunks)packages/desktop-client/src/components/reports/graphs/CashFlowGraph.tsx(6 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(4 hunks)packages/desktop-client/src/components/reports/reports/CashFlow.tsx(4 hunks)packages/desktop-client/src/components/reports/reports/CustomReport.tsx(4 hunks)packages/desktop-client/src/components/reports/reports/NetWorth.tsx(4 hunks)packages/desktop-client/src/components/reports/reports/NetWorthCard.tsx(3 hunks)packages/desktop-client/src/components/reports/reports/Spending.tsx(6 hunks)packages/desktop-client/src/components/reports/reports/Summary.tsx(4 hunks)packages/desktop-client/src/components/reports/reports/SummaryCard.tsx(3 hunks)packages/desktop-client/src/components/rules/Value.tsx(3 hunks)packages/desktop-client/src/components/schedules/DiscoverSchedules.tsx(2 hunks)packages/desktop-client/src/components/schedules/ScheduleDetails.tsx(3 hunks)packages/desktop-client/src/components/select/DateSelect.tsx(5 hunks)packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx(5 hunks)packages/desktop-client/src/hooks/useLocale.ts(1 hunks)packages/loot-core/src/server/budget/actions.ts(2 hunks)packages/loot-core/src/shared/locale.ts(1 hunks)packages/loot-core/src/shared/rules.ts(2 hunks)packages/loot-core/src/shared/schedules.test.ts(23 hunks)packages/loot-core/src/shared/schedules.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetSummary.tsx
- packages/desktop-client/src/components/reports/DateRange.tsx
- packages/desktop-client/src/components/reports/reports/SummaryCard.tsx
- packages/desktop-client/src/components/reports/reports/NetWorthCard.tsx
- packages/desktop-client/src/components/modals/TrackingBudgetMonthMenuModal.tsx
- packages/desktop-client/src/components/budget/envelope/budgetsummary/BudgetSummary.tsx
- packages/desktop-client/src/components/modals/EnvelopeBudgetMonthMenuModal.tsx
- packages/desktop-client/src/components/schedules/ScheduleDetails.tsx
- packages/desktop-client/src/components/reports/graphs/CashFlowGraph.tsx
- packages/desktop-client/src/components/reports/reports/NetWorth.tsx
- packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx
- packages/desktop-client/src/components/modals/EnvelopeBudgetSummaryModal.tsx
- packages/desktop-client/src/components/reports/reports/Calendar.tsx
- packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx
- packages/desktop-client/src/components/reports/Header.tsx
- packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx
- packages/desktop-client/src/components/reports/ReportSummary.tsx
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4041
File: packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx:518-544
Timestamp: 2024-12-26T01:16:39.339Z
Learning: A <Trans> inside another <Trans> does not necessarily produce two distinct strings. Instead, it merges them into a single translation entry, allowing for object literals to be used within the same translation string.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Visual regression
- GitHub Check: Functional
🔇 Additional comments (24)
packages/loot-core/src/shared/rules.ts (1)
223-227: LGTM! Translation keys are appropriate.The translations for logical operations and budget status are well-implemented and consistent with the existing patterns.
packages/desktop-client/src/components/reports/reports/CashFlow.tsx (3)
19-19: LGTM: Import statement properly organized.The
useLocalehook import is correctly placed alongside other custom hooks.
68-68: LGTM: Proper hook initialization.The
useLocalehook is correctly initialized at the component level, making the locale available for date formatting throughout the component.
109-111: LGTM: Proper memoization with locale dependency.The
localeparameter is correctly integrated into the cash flow calculation and dependency array, ensuring proper recalculation when the locale changes.packages/desktop-client/src/components/reports/reports/Summary.tsx (3)
19-19: LGTM! Clean import of the useLocale hook.The import follows the project's import organization pattern.
69-69: LGTM! Proper integration of locale in the component.The locale is correctly:
- Retrieved using the hook
- Passed to
summarySpreadsheet- Added to the dependency array
Also applies to: 126-126, 134-134
Line range hint
1-486: Verify the impact of locale changes on the Summary report.The Summary report now includes locale-aware date formatting. Please ensure that:
- The report updates correctly when the locale changes
- The date formats are consistent across different locales
- The calculations remain accurate regardless of the locale
Let's verify the locale handling implementation:
✅ Verification successful
Locale handling in Summary report is properly implemented
The Summary report correctly handles locale changes:
- All date formatting calls include the locale parameter
- The report updates automatically when locale changes through dependency tracking
- Calculations use locale-independent ISO date strings internally
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent locale usage across the codebase # and verify that all date formatting calls include locale parameter # Check for date formatting calls without locale echo "Checking for potential missing locale in date formatting calls..." rg "monthUtils\.format\([^,]+,[^,]+\)" --type ts # Check for consistent locale prop passing in Summary-related components echo "Checking for consistent locale handling in Summary components..." rg "Summary.*props.*locale" --type ts # Look for other date formatting utilities that might need locale echo "Checking other date formatting utilities..." rg "format.*date" --type tsLength of output: 7695
packages/desktop-client/src/components/rules/Value.tsx (3)
14-14: LGTM!Clean addition of the
useLocalehook import, following the existing import organization pattern.
49-49: LGTM!The
useLocalehook is properly initialized following React hooks naming convention and placed appropriately with other hook calls.
80-80: Verify the updated function signature.The locale parameter has been added to the
getRecurringDescriptioncall. The change looks good, but let's verify that the function signature inschedules.tshas been updated to accept this parameter.✅ Verification successful
Function signature verification successful
The
getRecurringDescriptionfunction inschedules.tscorrectly accepts the locale parameter, and all calls to this function across the codebase have been updated accordingly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that getRecurringDescription function accepts locale parameter # Search for the function definition ast-grep --pattern 'export function getRecurringDescription($_) { $$$ }' # Also check for any other calls to this function that might need updating rg "getRecurringDescription\(" --type tsLength of output: 2411
Script:
#!/bin/bash # Check the complete function call in DiscoverSchedules.tsx rg "getRecurringDescription\(" packages/desktop-client/src/components/schedules/DiscoverSchedules.tsx -A 3Length of output: 211
packages/desktop-client/src/components/reports/reports/Spending.tsx (2)
64-64: LGTM! Good practice initializing locale at the component level.The locale hook is correctly initialized at the component level, making it available throughout the component.
16-16: LGTM! Clean import of the useLocale hook.The import is correctly placed with other hook imports.
packages/desktop-client/src/hooks/useLocale.ts (1)
7-11: LGTM! Clean and efficient implementation.The hook is well-implemented with proper memoization and fallback handling.
packages/desktop-client/src/components/schedules/DiscoverSchedules.tsx (1)
12-12: LGTM! Clean integration of locale support.The locale integration is properly implemented and maintains the existing functionality.
Also applies to: 45-45, 50-54
packages/loot-core/src/shared/schedules.ts (1)
60-63: Consider edge cases in makeNumberSuffix implementation.The function uses a hardcoded date (2020) which might cause issues in certain locales or with certain date formats.
Consider testing with various locales and validating the output format:
packages/loot-core/src/shared/schedules.test.ts (2)
1-2: LGTM! Required imports for localization.The imports for
enUSlocale andi18nextare necessary for the localization enhancements.
119-119: LGTM! Consistent locale parameter usage.The
enUSlocale parameter is consistently added to allgetRecurringDescriptioncalls, ensuring proper localization of date formats.Also applies to: 131-131, 141-141, 153-153, 165-165, 178-178, 191-191, 204-204, 217-217, 229-229, 248-248, 264-264, 281-281, 299-299, 309-309, 321-321, 337-337, 351-351, 365-365, 379-379, 395-395, 409-409
packages/desktop-client/src/components/select/DateSelect.tsx (2)
29-29: LGTM! Required import for localization.The import for
useLocalehook is necessary for the localization enhancements.
84-90: LGTM! Well-defined type for Pikaday localization.The
PikadayI18ntype correctly defines the structure required for Pikaday's localization interface.packages/loot-core/src/server/budget/actions.ts (1)
3-4: LGTM! Required imports for localization.The imports for
asyncStorageandgetLocaleare necessary for the localization enhancements.packages/desktop-client/src/components/mobile/budget/index.tsx (1)
23-23: LGTM! The localization changes are well implemented.The changes correctly integrate the locale for date formatting:
- The
useLocalehook is properly imported- The
localeis correctly passed tomonthUtils.format- The dependency array is properly updated to include the
localeAlso applies to: 41-41, 443-444, 448-448
packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx (1)
5-5: LGTM! The localization changes are well implemented with proper memoization.The changes correctly integrate the locale for date formatting and descriptions:
- The
useLocalehook is properly imported and used- Date formatting correctly uses the locale parameter
- The
recurringDescriptionis properly memoized withlocalein its dependency arrayAlso applies to: 22-22, 244-245, 264-264, 596-606, 615-615
packages/desktop-client/src/components/reports/reports/CustomReport.tsx (2)
25-25: LGTM! The localization changes are well implemented.The changes correctly integrate the locale for date formatting:
- The
useLocalehook is properly imported and used- Date formatting correctly uses the locale parameter
- The dependency array is properly updated to include the
localeAlso applies to: 125-125, 323-323, 350-350
323-323: Consider handling potential race conditions in the useEffect.The effect includes async operations and state updates. When the locale changes, there might be race conditions if the effect is triggered multiple times in quick succession.
Also applies to: 350-350
There was a problem hiding this comment.
@lelemm I tested your branch and I discovered one issue 🐛 related to default language recognition (HTTP Request header Accept-Language). I am not sure if the previous reviews already covered it.
When I have the following configuration (default):
- Language: System default // which is default.
And I access the Actual with Accept-Language: pt-BR, I got:
- The texts are translated to my preferred language, but the dates not.
Expected result: Everything (including the dates/months) must be translated to the default language (if a translation for it exists).
PS: If you want a way to quickly change the preferred language, you can use this: https://github.com/locale-switcher/locale-switcher/
@xthiago try now |
|
@lelemm yes, it works. Thanks for the fix. 🎉 Actually, I located another issue not directly related to your PR (also happens with the regular texts), so I can open a separated issue for it. Browsers offer user's the ability to specify more than one preferable language and allow a prioritization. Like this one:
I did one test setting: 1. Netherlands, Spanish, Brazilian Portuguese and then US English: Expected result: Actual should check the available translations accordingly with the priority list and use it. Actual result: If more than one preferable language is set, the UI ignores it and always sets to English (no matter if there is a translation for the chosen languages or not). Update: the separated issue is here: #4229 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loot-core/src/shared/locale.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (2)
packages/loot-core/src/shared/locale.ts (2)
1-1: LGTM! Import strategy is appropriate for dynamic locale access.While importing all locales might increase bundle size, it's necessary here for dynamic locale access based on user preferences.
1-11: Verify locale handling across the application.Based on the PR objectives, please verify:
- Integration with HTTP Accept-Language header
- Usage in schedule-related date formatting
- Proper fallback behavior
Run this script to analyze locale handling:
✅ Verification successful
Locale handling implementation is correct and consistent
The implementation follows best practices with:
- Browser language detection on client-side
- Stored language preferences on server-side
- Proper locale parameter usage in date formatting
- Appropriate fallback to en-US
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze locale handling across the codebase echo "Checking Accept-Language header handling..." rg -l "Accept-Language" echo "\nChecking getLocale usage..." rg "getLocale\(" --type ts echo "\nChecking date formatting in schedules..." rg "format\(" packages/loot-core/src/shared/schedules.ts echo "\nChecking other date-fns locale usage..." rg "date-fns/locale" --type tsLength of output: 1346
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/loot-core/src/shared/locale.ts (1)
1-1: Consider using dynamic imports for better performance.Instead of importing all locales at once, consider using dynamic imports to load only the required locale on demand. This can significantly reduce the initial bundle size.
Example implementation:
const loadLocale = async (key: string) => { try { return (await import(`date-fns/locale/${key}`)).default; } catch { return (await import('date-fns/locale/enUS')).default; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loot-core/src/shared/locale.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (4)
packages/loot-core/src/shared/locale.ts (4)
4-6: Enhance input validation for edge cases.While the basic validation is good, consider handling more edge cases:
- Empty strings
- Whitespace-only strings
- Invalid language codes
Example implementation:
if (!language?.trim() || typeof language !== 'string' || !/^[a-zA-Z]{2}(-[a-zA-Z]{2,4})?$/.test(language)) { return locales.enUS; }
8-19:⚠️ Potential issueImprove locale key handling and add logging.
The current implementation has several limitations:
- Only handles single hyphen
- Case sensitivity issues
- Limited support for complex language codes
- No error logging for debugging
Apply this diff to improve the implementation:
- let localeKey = language.replace('-', '') as keyof typeof locales; + // Normalize and try full locale key first + let localeKey = language.toLowerCase().replace(/-/g, '') as keyof typeof locales; + console.debug(`Attempting to load locale for: ${language}, normalized to: ${localeKey}`); if (localeKey in locales) { return locales[localeKey]; } - //if language was not found with four letters, try with two - localeKey = language.replace('-', '').substring(0, 2) as keyof typeof locales; + // If not found, try base language + const baseKey = language.toLowerCase().split('-')[0] as keyof typeof locales; + console.debug(`Falling back to base language: ${baseKey}`); - if (localeKey in locales) { - return locales[localeKey]; + if (baseKey in locales) { + return locales[baseKey]; }Likely invalid or redundant comment.
3-22: 🛠️ Refactor suggestionAdd JSDoc documentation and improve error handling.
The function would benefit from comprehensive documentation and better error handling for debugging purposes.
Apply this diff to add documentation and improve error handling:
+/** + * Returns a date-fns locale object for the given language code. + * Used for formatting calendar-related strings throughout the application. + * + * @param language - ISO language code (e.g., 'en-US', 'fr-FR', 'zh-Hant-TW') + * @returns date-fns locale object + * @throws {Error} If the locale module cannot be loaded + * @example + * getLocale('en-US') // returns enUS locale + * getLocale('fr-FR') // returns frFR locale + * getLocale('zh-TW') // returns zhTW locale + * getLocale('invalid') // returns enUS locale + */ export function getLocale(language: string) { if (!language || typeof language !== 'string') { + console.warn(`Invalid language provided: ${language}, falling back to enUS`); return locales.enUS; } // ... rest of the implementation ... + console.warn(`Locale not found for language: ${language}, falling back to enUS`); return locales.enUS; }Likely invalid or redundant comment.
1-22: Verify integration with global preferences.The implementation aligns with the PR objectives by avoiding Redux store dependency. However, ensure that:
- The language code is correctly retrieved from global preferences
- The locale is properly applied to all calendar-related strings
- The implementation handles the
Accept-Languageheader priority correctlyRun this script to verify the integration:
✅ Verification successful
Implementation verified: Locale handling is properly integrated ✓
The locale implementation correctly:
- Integrates with global preferences through the
useLocalehook- Provides appropriate fallbacks (global pref → browser language → en-US)
- Applies consistently across calendar-related components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify locale handling implementation # Check for direct Redux store imports echo "Checking for Redux store imports..." rg -l "store.getState|useSelector" "packages/loot-core" # Check calendar-related string usage echo "Checking calendar string formatting..." rg -l "getLocale|format.*date|format.*time" "packages" # Check Accept-Language header handling echo "Checking language header handling..." rg -l "Accept-Language" "packages"Length of output: 2554
Script:
#!/bin/bash # Check useLocale hook implementation echo "Checking useLocale hook implementation..." cat "packages/desktop-client/src/hooks/useLocale.ts" # Check for language preference handling echo -e "\nChecking language preference handling..." rg -g '!*.test.*' -g '!*.spec.*' "language.*preference|setLanguage" packagesLength of output: 898
|
Judging by the time ran to try to do E2E, this might broke playwright. Any ideas? |
|
/update-vrt |
|
It's running now, but there's a test or two to fix |
ty, I will check it |
|
@matt-fidd where are stored the reference images? I don't remember |
packages/desktop-client/e2e |
|
|
|
Looks like it's not happy about there being two alerts, not sure why that didn't fail on an earlier PR because it's not a change in here that's done it 
|
@xthiago Please could you open a separate feature request for that? |
|
@lelemm sorted :) |
|
ty @matt-fidd |
Translate calendar related strings