(feat): Add setting for toggling auto-downloading updates#2106
(feat): Add setting for toggling auto-downloading updates#2106KieranBond wants to merge 14 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e23b1f0fd9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/code/src/renderer/features/settings/components/sections/UpdatesSettings.tsx:136-153
Analytics fires before the mutation succeeds. If the mutation fails and the UI is rolled back, the event is already sent with the new (incorrect) value, polluting analytics data. Move `track` into `onSuccess` so it only fires once the setting change is actually persisted.
```suggestion
setAutoDownloadEnabled(checked);
setAutoDownloadMutation.mutate(
{ enabled: checked },
{
onError: () => {
setAutoDownloadEnabled(previous);
},
onSuccess: (result) => {
setAutoDownloadEnabled(result.enabled);
track(ANALYTICS_EVENTS.SETTING_CHANGED, {
setting_name: "auto_download_updates",
old_value: previous,
new_value: result.enabled,
});
},
},
);
```
### Issue 2 of 3
apps/code/src/renderer/features/settings/components/sections/UpdatesSettings.tsx:24
The switch is initialized as `true` regardless of the persisted value, so users who have disabled auto-download will briefly see it toggle from "on" to "off" once the query resolves. Initializing to `undefined` and deferring render until the value is known avoids this flash.
```suggestion
const [autoDownloadEnabled, setAutoDownloadEnabled] = useState<boolean | undefined>(undefined);
```
### Issue 3 of 3
apps/code/src/main/services/updates/service.test.ts:427-435
These two tests each only exercise one value (`false`). The project rule prefers parameterised tests, so covering both `true` and `false` in a single `it.each` is preferred here.
```suggestion
it.each([true, false])(
"reads auto-download toggle from settings store (%s)",
(value) => {
mockAutoDownloadUpdatesEnabled.mockReturnValue(value);
expect(service.autoDownloadUpdatesEnabled).toBe(value);
},
);
it.each([true, false])(
"persists auto-download toggle changes (%s)",
(value) => {
service.setAutoDownloadUpdatesEnabled(value);
expect(mockSetAutoDownloadUpdatesEnabled).toHaveBeenCalledWith(value);
},
);
```
Reviews (1): Last reviewed commit: "Merge branch 'main' into codex-auto-down..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Reviews (2): Last reviewed commit: "Address auto-update settings review feed..." | Re-trigger Greptile |
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/code/src/main/services/updates/schemas.ts:20-26
**Missing TypeScript type exports for new schemas**
The file already exports derived types for every existing schema (`IsEnabledOutput`, `CheckForUpdatesOutput`, `InstallUpdateOutput`), but `autoDownloadUpdatesOutput` and `setAutoDownloadUpdatesInput` have no corresponding `export type` declarations. When callers outside tRPC's inference chain (e.g. the service or test helpers) need the types directly, they'd have to re-derive them themselves, breaking the OnceAndOnlyOnce pattern.
### Issue 2 of 2
apps/code/src/renderer/features/settings/components/sections/UpdatesSettings.tsx:84-88
**Stale query cache causes no visual feedback after enabling auto-download from UI**
The startup-check effect fires only when `autoDownload?.enabled` (the *query* value) transitions to `true`. Because `setAutoDownloadMutation` never invalidates the `getAutoDownload` query, the query cache stays `false` after the user enables the toggle — even though the setting is persisted and the backend starts checking immediately. As a result, the frontend never calls `handleCheckForUpdates()`, `checkingForUpdates` stays `false`, and no spinner is shown while the backend check is in flight. The subscription will eventually surface the result (e.g. "You're on the latest version"), but there is no intermediate visual feedback.
Calling `queryClient.invalidateQueries(trpcReact.updates.getAutoDownload.queryKey())` in the mutation's `onSuccess` would keep the two data sources in sync and allow the effect to retrigger correctly.
Reviews (3): Last reviewed commit: "Merge branch 'main' into codex-auto-down..." | Re-trigger Greptile |
|
Reviews (4): Last reviewed commit: "Merge branch 'main' into codex-auto-down..." | Re-trigger Greptile |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/renderer/features/settings/components/sections/UpdatesSettings.tsx:76-83
**Startup check re-triggers after toggling auto-download on**
When the app starts with auto-download already persisted as `false`, the startup effect exits early and leaves `hasCheckedRef.current === false`. If the user then enables auto-download via the switch, the mutation's `onSuccess` invalidates the query, `autoDownload.enabled` changes from `false` → `true`, and this effect fires again — calling `handleCheckForUpdates()` a second time. The service's `setAutoDownloadUpdatesEnabled(true)` path has already called `startAutoDownloadChecks()` (and thus `checkForUpdates("periodic")`), so you end up with two concurrent check attempts. The service's `already_checking` guard absorbs the duplicate in practice, but the intent of `hasCheckedRef` — fire at most once on mount — is violated for this re-enable path.
A simple guard is to also set `hasCheckedRef.current = true` in the mutation's `onSuccess` when auto-download is being enabled, so the startup effect doesn't consider it an "initial load" check.
Reviews (5): Last reviewed commit: "Handle auto-update settings follow-up re..." | Re-trigger Greptile |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/renderer/features/settings/components/sections/UpdatesSettings.tsx:76-83
When auto-download is **disabled on mount** then enabled mid-session, `hasCheckedRef.current` is never set (the early return fires on initial load), so it remains `false`. When `autoDownload?.enabled` later changes to `true` the effect fires again and calls `handleCheckForUpdates()` — but the service's `startAutoDownloadChecks()` (triggered by the tRPC mutation) already started a background check. The service returns `already_checking`, but not before `handleCheckForUpdates` has called `setCheckingForUpdates(true)`, causing the "Check for updates" button to flash the spinner briefly. The fix is to mark the ref as consumed on the **first received boolean value**, regardless of whether it is `true` or `false`, so subsequent setting changes don't re-trigger the UI check.
```suggestion
useEffect(() => {
if (typeof autoDownload?.enabled !== "boolean") {
return; // Still loading
}
if (hasCheckedRef.current) {
return; // Already ran initial check
}
hasCheckedRef.current = true;
if (autoDownload.enabled) {
handleCheckForUpdates();
}
}, [autoDownload?.enabled, handleCheckForUpdates]);
```
Reviews (6): Last reviewed commit: "Merge branch 'main' into codex-auto-down..." | Re-trigger Greptile |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/renderer/features/settings/components/sections/UpdatesSettings.tsx:151-163
The `if (result.enabled !== previous)` guard is always true and can never suppress the `track` call. The `onCheckedChange` handler already returns early when `previous === checked`, so `checked !== previous`. The server echoes back `input.enabled` unchanged, so `result.enabled === checked !== previous`. The condition is dead code — simplify it away so the analytics intent is clearly unconditional here.
```suggestion
onSuccess: (result) => {
setAutoDownloadEnabled(result.enabled);
void queryClient.invalidateQueries(
trpcReact.updates.getAutoDownload.queryFilter(),
);
track(ANALYTICS_EVENTS.SETTING_CHANGED, {
setting_name: "auto_download_updates",
old_value: previous,
new_value: result.enabled,
});
},
```
Reviews (7): Last reviewed commit: "Fix auto-update startup check guard" | Re-trigger Greptile |
|
Reviews (8): Last reviewed commit: "Simplify auto-update analytics tracking" | Re-trigger Greptile |
|
I think we need to keep the behavior that checks for updates and lets them know one is available to download in the bottom of the sidebar. Instead of "restart" to update it will just say "Download" and let them know an update is available. |
I agree, as it is a much better UX, but I think that's difficult without some refactor first. Checking for updates is directly tied to downloading it at the moment if I remember correctly (sorry, not looked for a few days - will confirm tomorrow/later). I was thinking to follow up this change with another PR to enable things like that, but if you like @charlesvien I'm happy to do that first. I wanted to keep them as separate changes to first enable this feature and then improve it, as well as keep the changes more concise and easy to review. Thoughts? |
We could (should?) move to a different update module to support this but that change needs to be heavily tested both manually and with unit/integration tests. autoUpdater is kind of the out of the box solution we used with electron but there are better packages that allow you to query update information without downloading it. |
Yeah, agreed. I think the ideal UX is:
I don’t think this PR should swap update modules or split check/download behaviour, though. Electron's My preference would be to keep this PR focused on adding the setting around the current behaviour, then follow up with a separate update-module PR for the better “check first, download later” UX. I'm happy to rename the setting I've added here if needed, so it’s clear that it controls the current auto-check+download behaviour? Or, if you'd prefer, I'm happy to take a look at that change first, and we can block this PR until it's in. What's your pref @charlesvien ? |
|
Reviews (9): Last reviewed commit: "Merge branch 'main' into codex-auto-down..." | Re-trigger Greptile |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/main/services/updates/service.test.ts:454-466
**Mock doesn't reflect store write, leaving stale getter value**
`mockSetAutoDownloadUpdatesEnabled` is a no-op `vi.fn()` — calling `service.setAutoDownloadUpdatesEnabled(true)` records the call but does not update what `mockAutoDownloadUpdatesEnabled` returns. So `service.autoDownloadUpdatesEnabled` still returns `false` after the setter is called in this test. The tests pass today only because the service branches on the `enabled` argument rather than re-reading the getter, but any future code that reads the getter after the setter will silently see the stale value and the test won't catch it. Adding `mockAutoDownloadUpdatesEnabled.mockReturnValue(true)` after the `setAutoDownloadUpdatesEnabled(true)` call (and symmetrically `mockReturnValue(false)` in the "stops" test) would keep the mock consistent with real store behaviour.
Reviews (10): Last reviewed commit: "Merge branch 'main' into codex-auto-down..." | Re-trigger Greptile |
|
Reviews (11): Last reviewed commit: "Keep auto-update settings mock in sync" | Re-trigger Greptile |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
2 participants
Problem
This is for anybody who wants to avoid auto-updating. Users may not have spare bandwidth to automatically pull the latest updates, or may be waiting for larger/more stable releases before updating, so this allows them to opt out of automatic downloads of the latest changes until they're ready.
Contributes to #1930 - More to come, but this solves the immediate need.
Changes
Caveats
UX isn't the best when new updates are available.
The current logic around updates being available is tied to downloading/installing, too.
How did you test this?
Publish to changelog?
yes