Fix: update toolbar server status on sync #4075
Conversation
…n sync status changed, so 'userData.offline' is up to date and server status displayed is valid
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. |
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
|
p-payet
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/LoggedInUser.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. WalkthroughThe pull request modifies the Assessment against linked issues
Suggested labels
Suggested reviewers
📜 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 (
|
![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
🧹 Nitpick comments (1)
packages/desktop-client/src/components/LoggedInUser.tsx (1)
56-59: Potential error handling enhancement.
initializeUserDataimmediately setsloadingtofalseaftergetUserData()completes. IfgetUserData()fails, it could lead to inconsistentloadingstates or unhandled exceptions. Consider wrappinggetUserData()in a try/catch and handling errors accordingly.const initializeUserData = async () => { - await dispatch(getUserData()); - setLoading(false); + try { + await dispatch(getUserData()); + } catch (e) { + console.error('Failed to fetch user data:', e); + } finally { + setLoading(false); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4075.mdis excluded by!**/*.md
📒 Files selected for processing (1)
packages/desktop-client/src/components/LoggedInUser.tsx(2 hunks)
🧰 Additional context used
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/LoggedInUser.tsx
[failure] 74-74:
'userData' is possibly 'null'.
[failure] 75-75:
'userData' is possibly 'null'.
[failure] 83-83:
'userData' is possibly 'null'.
🔇 Additional comments (2)
packages/desktop-client/src/components/LoggedInUser.tsx (2)
8-8: Import path verification.The import path for
listenlooks correct. Ensure that this import doesn't inadvertently bring in additional code or dependencies that aren't required, and confirm that there's no duplication of event listeners elsewhere.
61-62: Initialization approach looks good.Calling
initializeUserData()upon mounting meets the PR objective of ensuring the user data is loaded initially. Just be mindful of whether or not you also want to re-invoke this logic in other relevant scenarios.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/desktop-client/src/components/LoggedInUser.tsx (1)
61-62: Consider handling cleanup for initialization.While the initialization is correct, consider handling cleanup to prevent state updates on unmounted component:
useEffect(() => { - initializeUserData(); + let mounted = true; + const initialize = async () => { + try { + await dispatch(getUserData()); + if (mounted) { + setLoading(false); + } + } catch (error) { + if (mounted) { + setLoading(false); + console.error('Failed to initialize user data:', error); + } + } + }; + initialize(); + return () => { + mounted = false; + }; }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/components/LoggedInUser.tsx(2 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/LoggedInUser.tsx (2)
8-8: LGTM!The import of the
listenfunction is correctly placed and necessary for the new sync event functionality.
74-75: Null-safety properly implemented.The code now correctly uses optional chaining for
userData?.offlinechecks, addressing the null-safety concerns raised in the previous review.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/desktop-client/src/components/LoggedInUser.tsx (2)
56-59:⚠️ Potential issueAdd error handling for the async operation.
The async operation could fail, and currently, there's no error handling. This could lead to the loading state being stuck.
Apply this diff to add proper error handling:
const initializeUserData = async () => { + try { await dispatch(getUserData()); setLoading(false); + } catch (error) { + setLoading(false); + // Consider showing an error message to the user + console.error('Failed to initialize user data:', error); + } };
65-83:⚠️ Potential issueImprove sync event handling robustness.
- The null safety issue with
userDataaccess persists.- The loading state management in the sync event handler could be improved.
Apply these improvements:
useEffect(() => { return listen('sync-event', ({ type }) => { if (type === 'start') { setLoading(true); return; } const shouldReinitialize = - (type === 'success' && userData?.offline) || - (type === 'error' && !userData?.offline); + userData && + ((type === 'success' && userData.offline) || + (type === 'error' && !userData.offline)); if (shouldReinitialize) { - initializeUserData(); + initializeUserData().catch(() => { + // Ensure loading state is reset even if initialization fails + setLoading(false); + }); } else { setLoading(false); } }); }, [userData]);This change:
- Adds proper null check for
userData- Ensures loading state is reset even if initialization fails
🧹 Nitpick comments (1)
packages/desktop-client/src/components/LoggedInUser.tsx (1)
61-62: Add cleanup handling for component unmount.The effect should handle cleanup to prevent setting state on an unmounted component.
useEffect(() => { - initializeUserData(); + let mounted = true; + initializeUserData().finally(() => { + if (mounted) { + setLoading(false); + } + }); + return () => { + mounted = false; + }; }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/components/LoggedInUser.tsx(2 hunks)
🔇 Additional comments (1)
packages/desktop-client/src/components/LoggedInUser.tsx (1)
8-8: LGTM!The import of the
listenfunction is correctly placed and necessary for implementing the sync-event listener.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/desktop-client/src/components/LoggedInUser.tsx (1)
78-80:⚠️ Potential issueEnsure null-safety for userData checks.
The conditional checks on
userData?.offlinecould lead to runtime issues ifuserDatais null.const shouldReinitialize = - (type === 'success' && userData?.offline) || - (type === 'error' && !userData?.offline); + userData && + ((type === 'success' && userData.offline) || + (type === 'error' && !userData.offline));
🧹 Nitpick comments (1)
packages/desktop-client/src/components/LoggedInUser.tsx (1)
56-64: Consider adding user feedback for initialization errors.While the error handling is good, users might benefit from visual feedback when initialization fails.
const initializeUserData = async () => { try { await dispatch(getUserData()); } catch (error) { console.error('Failed to initialize user data:', error); + // Consider showing a user-friendly error message + // Example: dispatch(showErrorNotification('Failed to connect to server. Please try again.')); } finally { setLoading(false); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/components/LoggedInUser.tsx(2 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/LoggedInUser.tsx (2)
8-8: LGTM: Import statement is correctly placed.The new import of the
listenfunction is necessary for implementing the sync event functionality.
66-67: LGTM: Initial data loading is properly implemented.The useEffect hook correctly initializes user data on component mount.
| @@ -52,14 +53,41 @@ export function LoggedInUser({ | |||
| const currentFile = remoteFiles.find(f => f.cloudFileId === cloudFileId); | |||
| const hasSyncedPrefs = useSelector((state: State) => state.prefs.synced); | |||
|
|
|||
| useEffect(() => { | |||
| async function init() { | |||
| const initializeUserData = async () => { | |||
There was a problem hiding this comment.
This should be defined inside the useEffect hook
There was a problem hiding this comment.
There is now 2 different useEffect. The first one is calling initializeUserData after first rendering, and the second one is used to listen to sync-event and call initializeUserData when needed, and is dependant to userData.
Both are using the initializeUserData method, it is for this reason that initializeUserData is defined outside the useEffect hooks.
Can you explain in more detail how initializeUserData definition should be done?
Fixes #3128
Listen to
sync-eventto update user data when sync status changed, souserData.offlineis up to date and server status displayed is valid.