Migrations out of sync modal
#3600
Conversation
MikesGlitch
changed the title
migrations out of sync modal Migrations out of sync modal
✅ 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 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
|
…lectron/client-version-out-of-sync
…lectron/client-version-out-of-sync
| @@ -591,15 +592,8 @@ export function Modals() { | |||
| return <ImportYNAB5Modal key={name} />; | |||
| case 'import-actual': | |||
| return <ImportActualModal key={name} />; | |||
| case 'manager-load-backup': | |||
There was a problem hiding this comment.
This isn't used anywhere, cleaning it up
actual-github-bot
bot
changed the title
Migrations out of sync modal Migrations out of sync modal
WalkthroughThe pull request introduces a new modal component, Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (7)
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: 1
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/modals/OutOfSyncMigrationsModal.tsx (2)
22-83: LGTM: Modal structure and content are well-implemented, with a suggestion for improvement.The modal structure is well-organized, using appropriate components for layout and styling. The content effectively communicates the out-of-sync issue to the user and provides a solution. The use of the
Transcomponent suggests good internationalization support.Consider adding an
aria-labelto the "Close Budget" button to improve accessibility. For example:<Button variant="primary" style={{ padding: '10px 30px', }} onPress={() => closeBudgetAndModal(close)} + aria-label="Close Budget and return to budget selection" > <Trans>Close Budget</Trans> </Button>
1-85: LGTM: Well-implemented component with suggestions for enhancements.The
OutOfSyncMigrationsModalcomponent is well-structured, following React best practices and effectively utilizing Redux for state management. It handles the specific use case of out-of-sync migrations clearly and concisely.Consider the following enhancements to improve user experience and error handling:
- Add an option for users to attempt to sync their local data with the server, if possible.
- Implement error logging to track occurrences of out-of-sync migrations.
- Provide more detailed information about the nature of the out-of-sync issue, if available.
Example implementation for point 1:
<Button variant="secondary" onPress={() => attemptSync()} style={{ marginRight: 10 }} > <Trans>Attempt Sync</Trans> </Button>These suggestions could provide users with more options and help in diagnosing and resolving sync issues.
packages/loot-core/src/client/actions/budgets.ts (1)
62-62: Add a comment explaining the removal of 'out-of-sync-data' handling.To improve code maintainability and provide context for future developers, consider adding a comment explaining why the 'out-of-sync-data' error handling was removed.
Here's a suggested comment:
// 'out-of-sync-data' handling removed as part of the migration warning system update. // Users will now be notified via a general error alert instead of the specific backup loading prompt.packages/loot-core/src/client/state-types/modals.d.ts (1)
89-90: LGTM! Consider adding a comment for clarity.The addition of the 'out-of-sync-migrations' modal type is appropriate and aligns with the PR objectives. It follows the existing pattern for modals without options.
For consistency and clarity, consider adding a brief comment explaining the purpose of this modal, similar to other modal definitions in the file. For example:
// Modal to warn users about out-of-sync migrations 'out-of-sync-migrations': null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3600.mdis excluded by!**/*.md
📒 Files selected for processing (4)
- packages/desktop-client/src/components/Modals.tsx (2 hunks)
- packages/desktop-client/src/components/modals/OutOfSyncMigrationsModal.tsx (1 hunks)
- packages/loot-core/src/client/actions/budgets.ts (1 hunks)
- packages/loot-core/src/client/state-types/modals.d.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
packages/desktop-client/src/components/modals/OutOfSyncMigrationsModal.tsx (2)
1-12: LGTM: Imports are well-organized and appropriate.The imports cover all necessary dependencies for the component, including React hooks, Redux dispatch, and various UI components. The import of
closeBudgetaction from 'loot-core/client/actions' is particularly noteworthy as it indicates integration with the core application logic.
14-20: LGTM: Component structure and hook usage are well-implemented.The
OutOfSyncMigrationsModalis correctly implemented as a functional component. The use ofuseDispatchhook is appropriate for managing state actions. ThecloseBudgetAndModalfunction effectively combines closing the budget and the modal, ensuring a smooth user experience.packages/loot-core/src/client/actions/budgets.ts (1)
60-61: LGTM: Handling of 'out-of-sync-migrations' error.The handling of the 'out-of-sync-migrations' error remains unchanged and aligns with the PR objectives of introducing a warning modal for this scenario.
packages/desktop-client/src/components/Modals.tsx (4)
54-54: LGTM: New modal component imported correctly.The
OutOfSyncMigrationsModalcomponent is imported correctly, following the existing import style in the file.
595-596: LGTM: New modal case added correctly.The
out-of-sync-migrationscase has been added correctly to the switch statement, rendering theOutOfSyncMigrationsModalcomponent with a uniquekeyprop.Please verify if the
OutOfSyncMigrationsModalcomponent requires any additional props. If it does, consider passing relevant data from theoptionsobject:- return <OutOfSyncMigrationsModal key={name} />; + return <OutOfSyncMigrationsModal key={name} {...options} />;
Line range hint
1-605: Summary: Changes align with PR objectivesThe modifications to the
Modals.tsxfile successfully implement the newOutOfSyncMigrationsModalto address the scenario where a user's budget has been migrated using an up-to-date client, but the current client is outdated. This aligns well with the PR objectives of preventing users from continuing to use an outdated client to avoid potential data issues.Key changes:
- Import of the new
OutOfSyncMigrationsModalcomponent.- Addition of the
out-of-sync-migrationscase in the switch statement.- Removal of the
manager-load-backupcase (to be verified).These changes contribute to the implementation of the warning modal described in the PR objectives, informing users about the out-of-date client and suggesting the use of the web application as an alternative.
594-594: Verify the intentional removal of themanager-load-backupcase.The
manager-load-backupcase has been removed from the switch statement. Please confirm if this removal is intentional and if the functionality for loading backups is now handled differently or no longer needed.To check for any remaining references to the removed case, run the following script:
✅ Verification successful
Removal of
manager-load-backupcase verified.All references to
manager-load-backuphave been successfully removed from the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to 'manager-load-backup' rg --type typescript --type javascript 'manager-load-backup'Length of output: 95
Script:
#!/bin/bash # Search for any remaining references to 'manager-load-backup' rg --type ts --type js 'manager-load-backup'Length of output: 44
packages/desktop-client/src/components/modals/OutOfSyncMigrationsModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/OutOfSyncMigrationsModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/OutOfSyncMigrationsModal.tsx
Outdated
Show resolved
Hide resolved
When will this display?
When you have multiple clients using one server. One client has been updated, the other hasn't. The updated client runs on the budget and migrates the data to the latest. Then the out-of-date client opens the budget.
We shouldn't let the user continue using the client in case they end up with bad data. It's not worth the risk. We should warn the user about it, prompt them to update their client, and let them know it's possible to use the web app from app.actualbudget.org.
Original intention
This was intended to be a warning for when server is a different version from client. I couldn't do it because
I don't think we should do that so I had to bin the idea.