Auto-reload on app updates #3693
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
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
|
|
Tested with |
jfdoming
force-pushed
the
jfdoming/reload-on-updates
branch
from
e1dec97 to
22eaa25
Compare
| const compose = window['__REDUX_DEVTOOLS_EXTENSION_COMPOSE__'] || (f => f); | ||
| const store = createStore( | ||
| rootReducer, | ||
| undefined, | ||
| compose(applyMiddleware(thunk)), | ||
| ); | ||
| const boundActions = bindActionCreators( |
There was a problem hiding this comment.
Enabling Redux devtools to work. Let me know if I should remove the change, I found it useful when debugging.
jfdoming
changed the title
| @@ -152,7 +152,7 @@ export default defineConfig(async ({ mode }) => { | |||
| mode === 'desktop' | |||
| ? undefined | |||
| : VitePWA({ | |||
| registerType: 'autoUpdate', | |||
| registerType: 'prompt', | |||
There was a problem hiding this comment.
'prompt' is misleading here. It's just needed so that we can hook into the "update ready" event. We only show a prompt if an update is available later
WalkthroughThe pull request introduces several enhancements to the application's update management system across multiple files. In 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 (
|
Remove some old updater code![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/index.tsx (1)
Line range hint
70-95: LGTM with a security consideration.The addition of the
inputFocusedfunction and its exposure to the main process via__actionsForMenuimproves the interaction between the renderer and the main process. This is particularly useful for managing menu items based on the active element's type.However, consider the security implications of exposing functionality to the global scope. While this approach is valid for inter-process communication in Electron, it's important to ensure that only necessary functions are exposed and that proper input validation is implemented in the main process when using these exposed functions.
packages/desktop-client/src/components/FinancesApp.tsx (1)
104-121: Add error handling to the async functionThe async function
rundoes not include error handling for the awaited calls. If an error occurs duringawait global.Actual.waitForUpdateReadyForDownload()or while dispatching the notification, it may go unhandled. Consider adding atry-catchblock to handle potential errors gracefully.useEffect(() => { async function run() { + try { await global.Actual.waitForUpdateReadyForDownload(); dispatch( addNotification({ // ... notification details ... }), ); + } catch (error) { + console.error('Error checking for updates:', error); + // Optionally, dispatch an error notification or handle the error appropriately. + } } run(); }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3693.mdis excluded by!**/*.md
📒 Files selected for processing (6)
- packages/desktop-client/src/browser-preload.browser.js (3 hunks)
- packages/desktop-client/src/components/App.tsx (3 hunks)
- packages/desktop-client/src/components/FinancesApp.tsx (1 hunks)
- packages/desktop-client/src/index.tsx (1 hunks)
- packages/desktop-client/vite.config.mts (1 hunks)
- packages/loot-core/src/client/actions/app.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/App.tsx
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
packages/loot-core/src/client/actions/app.ts (1)
21-22: Excellent improvement to the update process!The addition of
awaittoglobal.Actual.applyAppUpdate()is a significant enhancement. This change ensures that the app update is fully applied before updating the app state, which prevents potential race conditions and improves the reliability of the update process. This modification aligns well with the PR objectives and the changes made in other files.packages/desktop-client/src/index.tsx (1)
61-66: LGTM: Redux DevTools integration looks good.The integration of Redux DevTools enhances debugging capabilities and follows Redux best practices. This change aligns with the previous comment about its usefulness during debugging.
packages/desktop-client/vite.config.mts (1)
155-155: Approved: Improved update mechanism aligns with PR objectivesThe change from
registerType: 'autoUpdate'toregisterType: 'prompt'in the VitePWA plugin configuration is a positive improvement. This modification aligns well with the PR objectives and addresses the user experience issues reported in the PR comments. By prompting users for updates instead of applying them automatically, it provides more control over the update process and prevents unexpected delays during app loading.This change works in conjunction with the new update handling mechanisms implemented in other components, as mentioned in the AI summary. It contributes to a more user-friendly update experience, allowing users to choose an appropriate time to perform updates.
packages/desktop-client/src/browser-preload.browser.js (3)
43-50: Deferred update readiness promise is correctly implementedThe implementation of
isUpdateReadyForDownloadPromiseandmarkUpdateReadyForDownloadeffectively creates a promise that resolves when an update is ready, allowing asynchronous waiting on the update status.
51-55: Service worker registration with update handler
registerSWis correctly called withimmediate: trueandonNeedRefresh: markUpdateReadyForDownload, setting up the service worker to check for updates and mark them as ready when necessary.
157-158: Exposing update readiness methodsThe methods
isUpdateReadyForDownloadandwaitForUpdateReadyForDownloadare properly added to theglobal.Actualobject, giving external access to the update readiness state.packages/desktop-client/src/components/App.tsx (3)
54-64: Well-implemented 'maybeUpdate' utility functionThe
maybeUpdatefunction is correctly designed to check for updates and apply them before proceeding with the provided callback. The use of generics ensures type safety, and wrapping asynchronous operations enhances code reuse and maintainability.
74-74: Usage of 'await' with 'maybeUpdate' is appropriateThe asynchronous call to
initConnection(socketName)is properly wrapped withmaybeUpdateand awaited, ensuring that updates are applied before initializing the connection.
90-90:⚠️ Potential issueEnsure 'await' is used with 'maybeUpdate'
At this line,
maybeUpdatewraps an asynchronous operation but is not awaited. This might causeget-last-opened-backupto execute without ensuring that updates have been applied first.Apply this diff to fix the issue:
- const budgetId = await maybeUpdate(() => send('get-last-opened-backup')); + const budgetId = await maybeUpdate(() => send('get-last-opened-backup'));Likely invalid or redundant comment.
|
^ for electron changes |
What electron changes 😅 |
Ah sorry, I was just referring to the changes to preload.js/ts to make sure I didn't miss something obvious that needs to be implemented there. I briefly tested out the Electron app and it seemed to work as expected, but not 100% versed in these things yet. |
jfdoming
force-pushed
the
jfdoming/reload-on-updates
branch
from
25020c1 to
cf28241
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/browser-preload.browser.js (1)
157-164: Add error handling to applyAppUpdate.While the infinite Promise is intentionally used to prevent subsequent async calls from resolving (as clarified in previous discussions), consider adding error handling for
updateSW().Apply this diff to improve error handling:
applyAppUpdate: async () => { - updateSW(); + try { + await updateSW(); + } catch (error) { + console.error('Failed to update service worker:', error); + throw error; + } // Wait for the app to reload await new Promise(() => {}); },packages/desktop-client/src/components/App.tsx (2)
54-64: Consider handling the no-callback case more explicitly.The function returns
undefinedwhen no callback is provided. Consider making this more explicit in the return type.- const maybeUpdate = async <T,>(cb?: () => T): Promise<T> => { + const maybeUpdate = async <T,>(cb?: () => T): Promise<T | undefined> => {
104-105: Simplify condition with optional chaining.The condition can be simplified using optional chaining.
- const remoteFile = files.find(f => f.fileId === cloudFileId); - if (remoteFile && remoteFile.deleted) { + if (files.find(f => f.fileId === cloudFileId)?.deleted) {🧰 Tools
🪛 Biome
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3693.mdis excluded by!**/*.md
📒 Files selected for processing (7)
packages/desktop-client/src/browser-preload.browser.js(3 hunks)packages/desktop-client/src/components/App.tsx(2 hunks)packages/desktop-client/src/components/FinancesApp.tsx(1 hunks)packages/desktop-client/src/index.tsx(1 hunks)packages/desktop-client/vite.config.mts(1 hunks)packages/desktop-electron/preload.ts(1 hunks)packages/loot-core/src/client/actions/app.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/desktop-client/src/components/FinancesApp.tsx
- packages/desktop-client/src/index.tsx
- packages/desktop-client/vite.config.mts
- packages/loot-core/src/client/actions/app.ts
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/browser-preload.browser.js (1)
Learnt from: jfdoming
PR: actualbudget/actual#3693
File: packages/desktop-client/src/browser-preload.browser.js:159-164
Timestamp: 2024-10-20T19:11:51.750Z
Learning: In the `applyAppUpdate` function, the use of `await new Promise(() => {});` is intentional to ensure that any subsequent async calls don't resolve until the app reloads.
🪛 Biome
packages/desktop-client/src/components/App.tsx
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
packages/desktop-electron/preload.ts (1)
69-71: Verify the update mechanism integration.
Let's verify how these methods are used in the renderer process to ensure the no-op implementations won't cause issues.
✅ Verification successful
No-op update implementations are safe to use
The verification shows these methods are properly handled in the codebase:
- In
packages/desktop-client/src/browser-preload.browser.js, there's a web-based implementation that manages updates properly - In
packages/desktop-client/src/components/App.tsxandFinancesApp.tsx, the methods are used defensively:isUpdateReadyForDownload()is checked before taking any actionwaitForUpdateReadyForDownload()is awaited properly
The no-op implementations in the desktop electron preload are intentional and won't cause issues since the code gracefully handles the "no updates available" case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how these update-related methods are used in the renderer
# Test 1: Find direct usage of these methods
echo "Searching for direct method usage..."
rg "isUpdateReadyForDownload|waitForUpdateReadyForDownload" --type ts --type tsx
# Test 2: Look for update-related error handling
echo "Checking for update-related error handling..."
rg "catch.*update" --type ts --type tsx -A 3
Length of output: 369
Script:
#!/bin/bash
# Let's try a different approach without file type restrictions
# Search for direct usage of these methods
echo "Searching for direct method usage..."
rg "isUpdateReadyForDownload|waitForUpdateReadyForDownload"
# Look for any update-related code patterns
echo "Checking for update-related patterns..."
rg "update.*ready|ready.*update|checkForUpdates"
# Check the interface or type definitions for these methods
echo "Checking interface definitions..."
rg "interface.*\{[\s\S]*?(isUpdateReadyForDownload|waitForUpdateReadyForDownload)"
Length of output: 1621
packages/desktop-client/src/browser-preload.browser.js (1)
43-54:
Consider removing immediate: true flag based on PR feedback.
Based on the PR comments, setting immediate: true resulted in poor user experience due to update download times. Consider removing this flag to align with the proposed "reload to update" approach.
Apply this diff to improve the user experience:
const updateSW = registerSW({
- immediate: true,
onNeedRefresh: markUpdateReadyForDownload,
});packages/desktop-client/src/components/App.tsx (1)
67-67: LGTM: Strategic placement of update checks.
The implementation with two update checks (start and end of initialization) provides a good balance between update availability and user experience, as discussed in the PR comments.
Also applies to: 110-110
| // No auto-updates in the desktop app | ||
| isUpdateReadyForDownload: () => false, | ||
| waitForUpdateReadyForDownload: () => new Promise<void>(() => {}), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the update-related implementations and documentation.
The current implementation has a few areas for improvement:
- The never-resolving promise in
waitForUpdateReadyForDownloadcould lead to memory leaks - The comment could be more descriptive about why auto-updates are disabled
Consider applying these changes:
- // No auto-updates in the desktop app
+ // Auto-updates are handled differently in the desktop app through Electron's built-in
+ // auto-update mechanism rather than through the web app's service worker updates
isUpdateReadyForDownload: () => false,
- waitForUpdateReadyForDownload: () => new Promise<void>(() => {}),
+ waitForUpdateReadyForDownload: () => Promise.resolve(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // No auto-updates in the desktop app | |
| isUpdateReadyForDownload: () => false, | |
| waitForUpdateReadyForDownload: () => new Promise<void>(() => {}), | |
| // Auto-updates are handled differently in the desktop app through Electron's built-in | |
| // auto-update mechanism rather than through the web app's service worker updates | |
| isUpdateReadyForDownload: () => false, | |
| waitForUpdateReadyForDownload: () => Promise.resolve(), |
Definitely not expected! I will try to repro |
|
hmm @MikesGlitch, I wasn't able to repro with the updates from merging master. Do you still see this issue? |
Huh, weird. I'll merge and wait to see if others run into this at all. |
I couldn't find a way to synchronously check for updates, so the best I figured out is:
This is a little hard to test without pushing an update to this PR. Please let me know if you'd like me to do that so you can see what it looks like. Here's what a notification looks like:
Unfortunately couldn't get a good screenshot of the automatic updates since they happen pretty quick