Use useNavigate instead of accessing window.__navigate #3904
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
joel-jeremy
changed the title
WalkthroughThe pull request introduces several changes across multiple components in the desktop client application. The primary modification involves the integration of the Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/components/manager/ConfigServer.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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
|
![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 (3)
packages/desktop-client/src/components/LoggedInUser.tsx (1)
Line range hint
49-67: Consider adding error handling for async operationsWhile the navigation implementation is correct, consider adding try-catch blocks for the async operations to handle potential failures gracefully.
Here's a suggested improvement:
async function onMenuSelect(type) { setMenuOpen(false); switch (type) { case 'change-password': - onChangePassword(); + try { + await onChangePassword(); + } catch (error) { + console.error('Failed to change password:', error); + // Handle error appropriately + } break; case 'sign-in': - await closeBudget(); - navigate('/login'); + try { + await closeBudget(); + navigate('/login'); + } catch (error) { + console.error('Failed to close budget:', error); + // Handle error appropriately + } break; // Similar changes for other cases } }packages/desktop-client/src/components/modals/manager/ImportActualModal.tsx (1)
Line range hint
1-1: Consider enabling TypeScript strict modeThe file currently ignores strict TypeScript checks. Consider addressing type safety issues to remove the
@ts-strict-ignoredirective.Would you like help identifying and fixing the TypeScript strict mode issues?
packages/desktop-client/src/components/modals/manager/ImportYNAB5Modal.tsx (1)
Line range hint
36-51: Consider adding success feedback before navigationThe implementation correctly replaces window.__navigate with the useNavigate hook. However, consider adding a success message or brief feedback before navigating away, as immediate navigation might be jarring for users.
try { await dispatch(importBudget(res[0], 'ynab5')); + // Consider showing a success message + await new Promise(resolve => setTimeout(resolve, 1000)); // Brief delay to show feedback navigate('/budget'); } catch (err) { setError(err.message);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3904.mdis excluded by!**/*.md
📒 Files selected for processing (6)
packages/desktop-client/src/components/LoggedInUser.tsx(3 hunks)packages/desktop-client/src/components/manager/ConfigServer.tsx(1 hunks)packages/desktop-client/src/components/modals/manager/ImportActualModal.tsx(3 hunks)packages/desktop-client/src/components/modals/manager/ImportYNAB4Modal.tsx(3 hunks)packages/desktop-client/src/components/modals/manager/ImportYNAB5Modal.tsx(3 hunks)packages/loot-core/src/client/actions/budgets.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/loot-core/src/client/actions/budgets.ts
🔇 Additional comments (10)
packages/desktop-client/src/components/LoggedInUser.tsx (3)
9-9: LGTM! Clean import of useNavigate hook
The import is properly organized with other hooks and follows the established pattern.
42-42: LGTM! Proper hook initialization
The useNavigate hook is correctly initialized at the component level, following React hooks rules.
44-47: LGTM! Proper navigation flow in onChangePassword
The function maintains the correct sequence: closing the budget before navigation.
Let's verify all navigation paths are consistently updated:
✅ Verification successful
Navigation implementation is consistent across the codebase
The verification shows that:
- The
/change-passwordroute is properly defined inManagementApp.tsx - All references to 'change-password' in
LoggedInUser.tsxare using the new navigation approach - There's an ongoing migration from
window.__navigatetouseNavigate(as noted in release notes), and this change aligns with that migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining window.__navigate usage
rg "window.__navigate" --type ts --type tsx
# Search for all navigation paths to /change-password to ensure consistency
rg "(/change-password|'change-password')" --type ts --type tsx -A 2
Length of output: 175
Script:
#!/bin/bash
# Search for any remaining window.__navigate usage without file type restrictions
rg "window.__navigate"
# Search for all navigation paths to /change-password
rg "(/change-password|'change-password')" -A 2
Length of output: 1666
packages/desktop-client/src/components/modals/manager/ImportYNAB4Modal.tsx (3)
8-8: LGTM: Import statement for useNavigate is correctly added
The import statement is properly placed and aligns with the PR objective to use the useNavigate hook.
27-27: LGTM: Hook initialization follows React best practices
The useNavigate hook is properly initialized at the component level, consistent with other hooks usage.
41-43: Verify navigation timing after budget import
While the navigation placement is logically correct, we should verify that the budget import is fully processed before navigation occurs. Consider adding a verification to ensure the imported budget is ready for viewing.
Consider implementing a callback or promise resolution from importBudget to confirm the budget is ready before navigation.
✅ Verification successful
Let's also check the Redux action and its implementation:
Based on the search results, I can now generate the final response:
Navigation after budget import is implemented correctly
The implementation is safe because:
- The
importBudgetaction is properlyawaited before navigation occurs - The action internally handles errors and only resolves after the budget is fully imported
- The same pattern is consistently used across all import handlers (YNAB4, YNAB5, and Actual)
- The action closes the modal only after successful import, indicating proper completion handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns in other import handlers to verify consistent behavior
rg -A 5 "importBudget.*navigate.*budget" --type typescript
Length of output: 95
Script:
#!/bin/bash
# Search for similar patterns in other import handlers to verify consistent behavior, without file type restriction
rg -A 5 "importBudget.*navigate.*budget"
# Also search for the importBudget action definition
ast-grep --pattern 'const importBudget = $_'
Length of output: 3076
Script:
#!/bin/bash
# Search for importBudget implementation
rg -A 10 "function importBudget"
# Look for dispatch calls with importBudget
rg "dispatch.*importBudget"
Length of output: 3159
packages/desktop-client/src/components/modals/manager/ImportActualModal.tsx (2)
8-8: LGTM: Proper hook implementation
The useNavigate hook is correctly imported and initialized at the component level, following React hooks best practices.
Also applies to: 35-35
49-51: Verify navigation timing after budget import
While the navigation implementation is functionally correct, let's verify that the navigation timing doesn't cause any race conditions or UI/UX issues.
Let's check how other components handle post-import navigation:
Consider adding a small delay or ensuring the import is fully processed before navigation:
await dispatch(importBudget(res[0], 'actual'));
+await new Promise(resolve => setTimeout(resolve, 100)); // Small delay for UI feedback
navigate('/budget');✅ Verification successful
Navigation after budget import is correctly implemented
The implementation is safe and follows the proper pattern. The importBudget action is fully asynchronous and the navigation only occurs after:
- The server-side import is complete (
await send('import-budget')) - The modal is closed (
dispatch(closeModal())) - Preferences are reloaded (
await dispatch(loadPrefs()))
This pattern is consistently used across all import modals (YNAB4, YNAB5, and Actual) and is verified by e2e tests that check for the budget table's visibility after import.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns of navigation after importBudget
rg -A 5 "importBudget.*navigate.*budget" --type typescript
# Check if there are any existing tests covering this scenario
rg -l "importBudget.*test" --type typescript
Length of output: 177
Script:
#!/bin/bash
# Search for similar patterns of navigation after importBudget without file type restriction
rg -A 5 "importBudget.*navigate.*budget"
# Look for importBudget usage patterns
rg -A 5 "importBudget"
# Find test files related to budget import
fd ".*test.*" | rg -i "budget|import"
Length of output: 9108
Script:
#!/bin/bash
# Check the implementation of importBudget action to understand its async behavior
ast-grep --pattern 'export function importBudget($$$)'
# Look for the server-side implementation of import-budget handler
rg -A 10 "'import-budget'"
Length of output: 2805
packages/desktop-client/src/components/modals/manager/ImportYNAB5Modal.tsx (2)
8-8: LGTM: Clean import of useNavigate hook
The import statement correctly brings in the useNavigate hook from the local hooks directory, aligning with the PR's objective to standardize navigation.
30-30: LGTM: Proper hook initialization
The useNavigate hook is correctly initialized at the component's top level, following React hooks rules and conventions.
packages/desktop-client/src/components/manager/ConfigServer.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
No description provided.