Fix loading of number format on initial app startup #4038
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
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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
jfdoming
force-pushed
the
jfdoming/fix-number-format-loading
branch
from
13950ca to
d35f61c
Compare
jfdoming
changed the title
|
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/desktop-client/src/components/spreadsheet/useFormat.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. WalkthroughThe pull request introduces modifications to number formatting across multiple files. In Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (2)
packages/desktop-client/src/components/spreadsheet/useFormat.ts (1)
6-6: **Use descriptive imports to maintain clarity **The introduction of
parseNumberFormatis sound. However, consider grouping or commenting imports to keep context clear for future contributors. For example, grouping “number format” related imports under a dedicated comment section can reduce confusion as the file grows.packages/loot-core/src/client/actions/prefs.ts (1)
35-42: **Prevent global side effects **Storing number formatting in a global state (
setNumberFormat) can result in side effects outside the React tree. If possible, prefer a reactive context or a shared store to reduce hidden dependencies. This is functional, but can be improved by gradually removing reliance on the global variable insetNumberFormat().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4038.mdis excluded by!**/*.md
📒 Files selected for processing (3)
packages/desktop-client/src/components/spreadsheet/useFormat.ts(2 hunks)packages/loot-core/src/client/actions/prefs.ts(2 hunks)packages/loot-core/src/shared/util.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/client/actions/prefs.ts (2)
Learnt from: MatissJanis
PR: actualbudget/actual#3544
File: packages/loot-core/src/client/state-types/prefs.d.ts:13-18
Timestamp: 2024-11-10T16:45:31.225Z
Learning: When action types like `SetPrefsAction` are updated with new properties, ensure that reducers are correctly handling these properties before suggesting a potential issue.
Learnt from: MatissJanis
PR: actualbudget/actual#3458
File: packages/loot-core/src/client/state-types/prefs.d.ts:5-5
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In future reviews, ensure that changes related to `PrefsState` in `prefs.d.ts` do not incorrectly assume necessary updates in other parts of the codebase. Verify the impact thoroughly before making suggestions.
🔇 Additional comments (6)
packages/desktop-client/src/components/spreadsheet/useFormat.ts (1)
67-67: **Validate fallback handling of ‘hideFraction’ and ‘numberFormat’ **
While calling parseNumberFormat({ format: numberFormat, hideFraction }), ensure that the defaults for numberFormat and hideFraction behave as intended if the user or environment does not supply values. The logic itself appears consistent, but testing with null/undefined values might help confirm this is robust to edge cases.
✅ Verification successful
Let me check the implementation of parseNumberFormat to understand how it handles defaults.
Let me check the isNumberFormat function to complete the verification.
Let me try a different pattern to find the isNumberFormat implementation.
Let me check what the NUMBER_FORMATS constant contains.
Fallback handling for 'hideFraction' and 'numberFormat' is properly implemented
The implementation has robust fallback handling:
numberFormat: If the format is undefined or invalid, it defaults to 'comma-dot' (which is a valid format in NUMBER_FORMATS)hideFraction: Converts input to string and checks against 'true', effectively handling undefined/null by defaulting to false
The implementation in parseNumberFormat is safe and handles all edge cases appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Looking for references to parseNumberFormat usage.
# We want to see if there's any other usage that needs consistent defaults.
rg -A 5 "parseNumberFormat"
Length of output: 2390
Script:
#!/bin/bash
# Let's look at the implementation of parseNumberFormat in util.ts
ast-grep --pattern 'export function parseNumberFormat($_) {
$$$
}'
Length of output: 828
Script:
#!/bin/bash
# Let's look at the implementation of isNumberFormat
ast-grep --pattern 'function isNumberFormat($_) {
$$$
}'
Length of output: 60
Script:
#!/bin/bash
# Looking for isNumberFormat definition with rg
rg "isNumberFormat" -A 5
Length of output: 867
Script:
#!/bin/bash
# Looking for NUMBER_FORMATS definition
rg "NUMBER_FORMATS.*=" -A 5
Length of output: 364
packages/loot-core/src/client/actions/prefs.ts (3)
2-2: **Imports look good **
The import of parseNumberFormat and setNumberFormat from ../../shared/util properly references the new logic for number formatting. No further action required.
31-32: **Clean assignment of global and synced preferences **
Storing globalPrefs and syncedPrefs within the Redux store is coherent and consistent with the overall pattern. Ensure subsequent reducers handle these new properties appropriately, but it looks fine here.
23-27: **Handle load errors gracefully **
Here, Promise.all loads globalPrefs and syncedPrefs. The code is correct for happy paths, but consider how to handle or log partial failures if one call fails while another succeeds. This ensures the user’s number format preferences are consistently loaded or that fallback logic is implemented.
✅ Verification successful
Let me search for error handling patterns and the implementation of the preference loading functions.
Let me search for the implementation of the preference loading handlers.
Let me search for error handling in the server-side implementation.
Let me search for the implementation of the preferences handlers.
Based on the code analysis, I can now provide a final response:
**Existing error handling is sufficient **
The current implementation already has appropriate error handling:
-
The
Promise.allcall is used within an async function, so any errors from eitherload-global-prefsorpreferences/getwill be automatically caught and propagated up the promise chain. -
The server-side implementations show:
load-global-prefsusesasyncStorage.multiGetwhich has built-in error handlingpreferences/getuses database queries through a robust db abstraction layer that handles errors
-
If either call fails, the error will propagate to the Redux action's error handling middleware, which is the appropriate level for handling such failures in a React/Redux application.
The current implementation follows best practices by:
- Letting errors propagate up to be handled by the application's global error boundaries
- Avoiding unnecessary try-catch blocks that would complicate the code without adding value
- Relying on the existing error handling in the underlying storage and database layers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Attempting to locate error handling or fallback within the codebase
rg -A 5 "Promise.all"
Length of output: 31317
Script:
#!/bin/bash
# Search for error handling in prefs.ts and related files
rg -A 5 "catch|throw|error" packages/loot-core/src/client/actions/prefs.ts
# Search for implementation of load-global-prefs and preferences/get handlers
ast-grep --pattern 'send($_, $_)'
Length of output: 57608
Script:
#!/bin/bash
# Search for the implementation of load-global-prefs and preferences/get handlers
rg -A 10 "load-global-prefs|preferences/get" packages/loot-core/src/
# Search for error handling in the preferences module
rg -A 5 "try|catch|throw" packages/loot-core/src/client/actions/prefs.ts
Length of output: 3570
Script:
#!/bin/bash
# Search for error handling in the server-side preferences implementation
rg -A 10 "try|catch|throw" packages/loot-core/src/server/preferences/
rg -A 10 "try|catch|throw" packages/loot-core/src/server/main.ts
# Look for the implementation of getPreferences
ast-grep --pattern 'function getPreferences() {
$$$
}'
Length of output: 19043
Script:
#!/bin/bash
# Search for the preferences implementation
rg -A 10 "getPreferences" packages/loot-core/src/server/preferences/
rg -A 10 "preferences/get" packages/loot-core/src/server/preferences/
Length of output: 1368
packages/loot-core/src/shared/util.ts (2)
238-238: **Convert to private function isNumberFormat **
You made isNumberFormat private, removing the export. Good for encapsulation. This ensures external modules rely on parseNumberFormat for validation, simplifying the overall interface.
262-273: **Ensure correct typing of ‘hideFraction’ **
The new parseNumberFormat function casts hideFraction to a string and compares it to 'true'. Verify that all possible truthy/falsey values are handled correctly (e.g., an empty string or uppercase strings). If any such values slip in, they might produce unexpected behavior. You may want to parse booleans more robustly or restrict the type to ensure predictable outcomes.
✅ Verification successful
Let me gather more information about the usage of hideFraction in the codebase.
Let me try a different approach to gather information.
Based on the comprehensive search results, I can now provide a final response about the hideFraction typing concern.
**The current boolean parsing is appropriate for the codebase's usage **
The implementation of parseNumberFormat is actually well-aligned with how hideFraction is used throughout the codebase:
- The type definition is properly constrained as
hideFraction?: string | booleanin the function parameters, which matches its usage patterns. - The codebase consistently uses
hideFractionin two ways:- As a boolean when setting number formats internally (e.g., in tests and utils)
- As a string 'true'/'false' when coming from user preferences (via
useSyncedPref)
The current implementation String(hideFraction) === 'true' is appropriate because:
- It handles both boolean and string inputs consistently
- The preferences system always saves the value as a string through
preferences/save - The test files demonstrate that the conversion works correctly in all number format scenarios
- The UI components (like Format.tsx) properly handle the boolean conversion when setting preferences
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching all references to parseNumberFormat usage outside this file
rg -A 5 "parseNumberFormat"
Length of output: 2390
Script:
#!/bin/bash
# Search for hideFraction usage and type definitions
rg -A 3 "hideFraction" --type typescript
# Search for SyncedPrefs type definition to understand the type of hideFraction
ast-grep --pattern 'type SyncedPrefs = {
$$$
}'
Length of output: 127
Script:
#!/bin/bash
# Search for hideFraction usage without file type restriction
rg -A 3 "hideFraction"
# Look for type definitions in types directory
fd "prefs.ts" --exec cat {} \;
Length of output: 26978
Fixes #4037. The bug occurs because the React component renders without using
useFormatat all. An easy fix would be to just call that hook inside the component, but then we risk this bug cropping up on other pages. Instead I changed the logic at preference load time so it's valid for all componentsRequesting Matiss since this relates to #3397 but feel free to request someone else instead