♻️ (synced-prefs) separate metadata and local prefs out #3458
Conversation
actual-github-bot
bot
changed the title
WalkthroughThe pull request introduces a notification system to alert users about available application updates. It implements a local preference hook to track the last acknowledged version, ensuring notifications are shown only when necessary. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| await checkForUpdateNotification( | ||
| dispatch, | ||
| getIsOutdated, | ||
| getLatestVersion, | ||
| ); |
There was a problem hiding this comment.
Inlined the update checking logic. This makes it easier to use various other hooks in it (i.e. useLocalPref and t).
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 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
|
| // TODO: implement logic for fetching the pref exclusively from the | ||
| // metadata.json file (in follow-up PR) | ||
| return useLocalPref(prefName); | ||
| const dispatch = useDispatch(); |
There was a problem hiding this comment.
This is the same logic we previously had in useLocalPref. It accesses metadata.json prefs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
packages/desktop-client/src/hooks/useMetadataPref.ts (1)
Line range hint
1-26: Refactor looks good! The hook now integrates well with Redux for state management.The refactor of the
useMetadataPrefhook to use Redux for state management is a positive change. It promotes a more centralized approach to managing metadata preferences and removes the dependency on theuseLocalPrefhook.Key points:
- The usage of
useDispatchanduseSelectorhooks from React Redux is appropriate for interacting with the Redux store.- The
setLocalPreffunction is correctly memoized usinguseCallbackto avoid unnecessary re-creations on re-renders.- The
useSelectorhook is used to access the local preferences from the Redux state, ensuring the component receives the latest values.- The type annotations for the hook and related types are properly defined, providing type safety and clarity.
Overall, the refactor aligns with the best practices of using Redux for state management in a React application. It improves the integration with the Redux store and promotes a more centralized approach to handling metadata preferences.
Consider updating other parts of the codebase that rely on the
useMetadataPrefhook to handle the new Redux-based implementation consistently. This may involve dispatching actions or selecting values from the Redux state as needed.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- packages/desktop-client/src/components/FinancesApp.tsx (3 hunks)
- packages/desktop-client/src/hooks/useLocalPref.ts (1 hunks)
- packages/desktop-client/src/hooks/useMetadataPref.ts (2 hunks)
- packages/loot-core/src/client/actions/prefs.ts (3 hunks)
- packages/loot-core/src/client/update-notification.ts (0 hunks)
- packages/loot-core/src/server/sync/index.ts (2 hunks)
- packages/loot-core/src/types/prefs.d.ts (1 hunks)
- packages/loot-core/src/types/server-handlers.d.ts (2 hunks)
- upcoming-release-notes/3458.md (1 hunks)
Files not reviewed due to no reviewable changes (1)
- packages/loot-core/src/client/update-notification.ts
Additional comments not posted (16)
upcoming-release-notes/3458.md (2)
1-4: LGTM!The frontmatter section correctly categorizes the changes as "Maintenance" and attributes authorship to the PR author "MatissJanis".
6-6: Release notes align with the PR objectives.The release notes accurately summarize the changes introduced in this PR, which involve separating
MetadataPrefsandLocalPrefsinto different storage locations within theSyncedPrefssystem.This change aligns with the stated PR objectives of finalizing the separation of metadata and local preferences. As mentioned in the AI-generated summary, this separation can lead to benefits such as:
- Improved organization of preference data
- Enhanced data retrieval efficiency
- Increased clarity in preference management
- Potential reduction in conflicts and streamlined operations related to user settings and configurations
Great job capturing the essence of the changes and their benefits in the release notes!
packages/desktop-client/src/hooks/useLocalPref.ts (4)
1-3: LGTM!The imports are necessary for the hook implementation and look good.
7-8: LGTM!The import is necessary for the migration logic and looks good.
16-38: Great work on refactoring theuseLocalPrefhook!The new implementation using
useLocalStoragesimplifies the state management by removing the dependency on Redux. The migration process ensures a smooth transition for existing preferences stored inmetadata.json. The return value of the hook is now more intuitive, directly returning the local storage value and its setter function.Overall, these changes enhance the efficiency and clarity of the
useLocalPrefhook.
1-38: Excellent refactoring of theuseLocalPrefhook!The changes in this file align perfectly with the PR objective of separating metadata and local preferences. The refactored hook provides a cleaner and more efficient way to manage local preferences by leveraging local storage and removing the dependency on Redux.
The migration process demonstrates a thoughtful approach to backward compatibility and data preservation, ensuring a smooth transition for existing preferences stored in
metadata.json.Overall, the work done in this file is commendable and contributes to the overall improvement of the preference management system in the application.
packages/loot-core/src/client/actions/prefs.ts (2)
2-2: Improve code clarity by importing specific types.Importing specific types,
GlobalPrefsandMetadataPrefs, instead of all types under an alias enhances code clarity and maintainability. It ensures that the code explicitly uses the intended types, reducing the likelihood of errors related to incorrect type usage. The change aligns with the best practice of importing only the required entities from a module.
28-28: Enhance type specificity and clarity by updating parameter types.Updating the parameter types for the
savePrefsandsaveGlobalPrefsfunctions to use the specifically imported types,MetadataPrefsandGlobalPrefs, improves type specificity and clarity. It ensures that the functions are explicitly tied to the intended preferences types, enhancing type safety by leveraging the benefits of the updated import statement. The change maintains consistency with the updated import statement and does not alter the overall functionality of the code.Also applies to: 51-51
packages/desktop-client/src/components/FinancesApp.tsx (4)
3-3: LGTM!The
useTranslationhook is correctly imported from thereact-i18nextlibrary to enable internationalization.
13-13: LGTM!The
addNotificationandsyncactions, as well as theuseLocalPrefhook, are correctly imported and will likely be used in the component logic.Also applies to: 18-18
92-96: LGTM!The
useTranslationanduseLocalPrefhooks are correctly used to get the translation function and manage thelastUsedVersionstate respectively.
106-137: Excellent work on implementing the update notification feature!The code segment correctly checks for updates and shows a sticky notification with the update details and a button to open the changelog. The use of
lastUsedVersionstate ensures that the user is not repeatedly notified about the same update. The notification strings are also correctly translated using thetfunction.Overall, the implementation aligns with the PR summary and AI-generated summary, and there are no apparent issues with the code logic or syntax.
packages/loot-core/src/types/server-handlers.d.ts (2)
242-242: LGTM!The change in the return type of
load-prefsmethod is consistent with the objective of separatingMetadataPrefsandLocalPrefs.
20-20: Verify the usage ofload-prefsmethod.The change in the import statement and the return type of
load-prefsmethod is consistent with the objective of separatingMetadataPrefsandLocalPrefs.Run the following script to verify the usage of
load-prefsmethod:Verification successful
Verification complete:
load-prefsusage is consistent with new return typeThe usage of
load-prefsmethod has been verified across the codebase:
- The server-side implementation in
main.tsreturns the result ofprefs.getPrefs(), which is consistent with the new return type.- The client-side usage in
prefs.tshandles the returned value appropriately, checking for existence and theidproperty.- The type definition in
server-handlers.d.tscorrectly specifies the return type asPromise<MetadataPrefs | null>.No issues were found during the verification process. The changes appear to be correctly implemented and consistent throughout the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `load-prefs` method. # Test: Search for the method usage. Expect: Code consuming the method should handle the new return type. rg --type typescript -A 5 $'load-prefs'Length of output: 73
Script:
#!/bin/bash # Description: Find TypeScript files and search for `load-prefs` usage # Find TypeScript files and search for `load-prefs` fd -e ts -e tsx | xargs rg -n -A 5 'load-prefs'Length of output: 1489
packages/loot-core/src/server/sync/index.ts (2)
16-16: LGTM!The import statement for
MetadataPrefstype is syntactically correct and likely necessary for using the type in this file.
307-307: Verify usages ofprefsToSethave been updated.The type change from
LocalPrefstoMetadataPrefsforprefsToSetaligns with the PR objective of separating these two types of preferences. Please ensure that all usages ofprefsToSetin this file and other relevant files have been updated to handleMetadataPrefsinstead ofLocalPrefs.Run the following script to verify the usages of
prefsToSet:Verification successful
prefsToSetusage verified and consistent withMetadataPrefstypeThe type change from
LocalPrefstoMetadataPrefsforprefsToSethas been consistently applied throughout the codebase. All observed usages ofprefsToSetare compatible with theMetadataPrefstype:
- In sync/index.ts, the variable is correctly typed and used.
- prefs.ts explicitly uses the
MetadataPrefstype forprefsToSet.- main.ts and server-handlers.d.ts use
prefsToSetin a manner consistent withMetadataPrefs.No issues or inconsistencies were found in the usage of
prefsToSetafter the type change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `prefsToSet` handle `MetadataPrefs`. # Test: Search for usages of `prefsToSet`. Expect: Only usages handling `MetadataPrefs`. rg --type typescript $'prefsToSet' -A 5Length of output: 73
Script:
#!/bin/bash # Description: Verify all usages of `prefsToSet` handle `MetadataPrefs`. # Find all TypeScript files and search for `prefsToSet` usages fd -e ts -e tsx | xargs rg 'prefsToSet' -n -C 5Length of output: 6815
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/loot-core/src/client/state-types/prefs.d.ts (1 hunks)
Additional comments not posted (2)
packages/loot-core/src/client/state-types/prefs.d.ts (2)
11-11: Verify the impact of removingLocalPrefsfromSetPrefsAction.The change aligns with the PR objective of separating
MetadataPrefsandLocalPrefs. However, it may break existing code that relies on theLocalPrefstype in theprefsproperty ofSetPrefsAction.Run the following script to verify all usages of
SetPrefsActionare updated to handle the new type definition:Verification successful
Change to
SetPrefsActiontype is consistent and isolatedThe verification process confirms that the change to
SetPrefsActiontype, removingLocalPrefsfrom theprefsproperty, is consistent across the codebase. No other usages ofSetPrefsActionwere found that would be affected by this change.
- The type definition in
packages/loot-core/src/client/state-types/prefs.d.tscorrectly showsprefs: MetadataPrefs;- No other direct usages of
SetPrefsActionwere found in the codebaseThis change appears to be isolated and doesn't introduce any inconsistencies or potential issues in other parts of the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `SetPrefsAction` are updated to handle the new type definition. # Test: Search for usages of `SetPrefsAction`. Expect: No occurrences of `LocalPrefs` in the `prefs` property. rg --type typescript -A 5 $'SetPrefsAction'Length of output: 77
Script:
#!/bin/bash # Description: Search for SetPrefsAction usages in TypeScript files # Use fd to find TypeScript files and pipe them to rg for content searching echo "Searching for SetPrefsAction usages:" fd -e ts -e tsx | xargs rg -n -A 5 'SetPrefsAction' # Use ast-grep as an alternative method echo "Using ast-grep to search for SetPrefsAction:" ast-grep --lang typescript --pattern 'SetPrefsAction'Length of output: 989
17-17: Verify the impact of removingLocalPrefsfromMergeLocalPrefsAction.The change aligns with the PR objective of separating
MetadataPrefsandLocalPrefs. However, it may break existing code that relies on theLocalPrefstype in theprefsproperty ofMergeLocalPrefsAction.Run the following script to verify all usages of
MergeLocalPrefsActionare updated to handle the new type definition:Verification successful
Confirm the intentional separation of
MetadataPrefsandLocalPrefsThe change to
MergeLocalPrefsActionis correct and aligns with the recent refactoring effort to separate metadata and local preferences. The type still exists in 'packages/loot-core/src/client/state-types/prefs.d.ts' and has been updated to use onlyMetadataPrefs.However, to ensure consistency across the codebase:
- Review usages of
LocalPrefsin files like 'packages/desktop-client/src/hooks/useLocalPref.ts' to confirm they're using the correct type.- Verify that all components using
MergeLocalPrefsActionhave been updated to work with the new type definition.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `MergeLocalPrefsAction` are updated to handle the new type definition. # Test: Search for usages of `MergeLocalPrefsAction`. Expect: No occurrences of `LocalPrefs` in the `prefs` property. rg --type typescript -A 5 $'MergeLocalPrefsAction'Length of output: 84
Script:
#!/bin/bash # Description: Search for usages of MergeLocalPrefsAction in TypeScript files # Use ast-grep to find MergeLocalPrefsAction usages ast-grep --lang typescript --pattern 'MergeLocalPrefsAction' # If ast-grep doesn't find anything, fall back to a more generic search using fd and rg if [ $? -ne 0 ]; then echo "Falling back to generic search..." fd -e ts -e tsx | xargs rg 'MergeLocalPrefsAction' -C 5 fiLength of output: 78
Script:
#!/bin/bash # Description: Perform broader searches related to MergeLocalPrefsAction and LocalPrefs # Search for partial matches of MergeLocalPrefs and LocalPrefs echo "Searching for partial matches:" rg 'MergeLocalPrefs|LocalPrefs' -C 3 # Check if MergeLocalPrefsAction type still exists echo -e "\nChecking for MergeLocalPrefsAction type definition:" ast-grep --lang typescript --pattern 'type MergeLocalPrefsAction = $_' # Look for recent changes related to preferences or actions echo -e "\nChecking recent changes related to preferences or actions:" git log -n 5 --oneline --grep="pref\|action" # If no results from git log, show recent changes in relevant files if [ $? -ne 0 ]; then echo "No relevant commits found. Showing recent changes in preference-related files:" git log -n 3 --oneline -- $(fd -e ts -e tsx | rg 'pref|action') fiLength of output: 4446
MatissJanis
changed the title
|
Good catch! TIL: electron wipes localStorage for each session. While looking into this I realized there is one more bug here: the local-prefs should be scoped to the budget file. And global prefs are.. well.. global (across multiple files). -- So there's a couple of things we could do to make local prefs work.
I'm leaning more towards (2) since we already have this data store and wouldn't need to build a new one (solution 1). Thoughts? |
|
I also think some of our current local prefs should actually be global prefs (i.e. sidebar width). But that's a discussion for another PR :) |
It's doing it on the web too. I'm hoping if you fix it for web it'll also fix it for Electron - Localstorage is persisting across Electron restarts. |
|
@MikesGlitch good spot on the web version. Turns out the sidebar is only loading the initial local pref value, but not loading subsequent updates to the value. So that was making it appear broken. |
|
@MikesGlitch I suspect the JSON issue stems from concurrent preference save attempts. So save A partially overrides save B causing a broken JSON. (guessing) |
MatissJanis
force-pushed
the
matiss/synced-prefs-8
branch
from
248bb25
to
d6708a2
Compare
* marked files for translation * added release note * fixed linting warnings * 🐛 fix account filters being overridden (#3441) *Reduce package size (#3443) * reduce package size of all packages * release notes * Update beforePackHook.ts * [Maintenance] Cleanup react aria packages and dedupe (#3450) * Cleanup react aria packages and dedupe * Release notes * ♻️ (synced-prefs) moving the prefs from metadata.json to the db (#3423) *
This should be the last PR in the series.
Separating out
MetadataPrefs(metadata.json) andLocalPrefs(local storage).