feat(desktop): redesign update UX#459
Merged
Merged
Conversation
✅ Deploy Preview for devsydev canceled.
|
|
Warning Review limit reached
More reviews will be available in 17 minutes and 19 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
📝 WalkthroughWalkthroughThis PR implements a complete desktop update system for the Electron app, spanning main-process updater state management with persistent settings, renderer UI components for user-facing update flows, IPC bridging for main/renderer communication, and build-time tooling for manifest validation and rewriting. It refactors update handling into a structured event model with progress tracking and error classification. ChangesUpdate System Core & Renderer Integration
Test Coverage (Main Process & Renderer)
Build Tools: Manifest Management & Release Pipeline
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skevetter
force-pushed
the
improve-update-ux
branch
from
073a86c to
fe90f01
Compare
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
skevetter
force-pushed
the
improve-update-ux
branch
from
fe90f01 to
5887adc
Compare
skevetter
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
hack/smoke_test_manifests/main_test.go (1)
39-75: ⚡ Quick winAssert that the probe stays on
HEAD.These tests validate status handling, but they would still pass if
checkURLregressed toGET. Since the tool’s contract is specifically HEAD-based, make the handlers reject other methods.Suggested test hardening
func TestRunSucceedsWhenAllReachable(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodHead { + t.Fatalf("expected HEAD request, got %s", r.Method) + } w.WriteHeader(200) })) defer srv.Close() @@ func TestRunFailsOn404(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodHead { + t.Fatalf("expected HEAD request, got %s", r.Method) + } w.WriteHeader(404) })) defer srv.Close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/smoke_test_manifests/main_test.go` around lines 39 - 75, The tests currently accept any HTTP method, hiding regressions if checkURL uses GET; update the handlers inside TestRunSucceedsWhenAllReachable and TestRunFailsOn404 to assert r.Method == "HEAD" (or return 405 for non-HEAD) and only then respond with the intended status code so the tests fail if the probe switches from HEAD to GET; reference the test functions TestRunSucceedsWhenAllReachable and TestRunFailsOn404 and the server handlers to locate where to add the method check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 213-218: The run step that calls "go run
hack/rewrite_manifest_urls/main.go desktop/release/ \"${{ github.repository }}\"
\"${{ inputs.tag || github.ref_name }}\"" interpolates
inputs.tag/github.ref_name directly into the shell and risks command injection;
change the step to pass the tag/ref via the job environment (env:) and reference
the env variable inside run (e.g., use an ENV var like INPUT_TAG or REF_NAME) so
the value is treated as data, not shell code—update the workflow step that
invokes the rewrite_manifest_urls program to set the tag via env and use that
env var in the run command instead of the direct ${{ ... }} expression.
In `@desktop/package.json`:
- Around line 24-26: Move the "`@types/dompurify`" entry out of the dependencies
block and into devDependencies in desktop/package.json: remove the
"`@types/dompurify`": "^3.0.5" line from the "dependencies" object and add the
same version under "devDependencies"; after updating package.json, run your
package manager (npm/yarn/pnpm) to update the lockfile so the change is
reflected in installs.
In `@desktop/src/main/__tests__/updater.test.ts`:
- Around line 42-54: The test mutates shared state electron.app.isPackaged; wrap
the mutation and test logic in a try/finally so isPackaged is always restored.
Specifically, in the "emits dev-mode status when app is not packaged" test, set
(electron.app as { isPackaged: boolean }).isPackaged = false, then run the
initAutoUpdater() call and assertions inside a try block, and in the finally
block reset (electron.app as { isPackaged: boolean }).isPackaged = true to avoid
leaking state across tests (references: initAutoUpdater,
electron.app.isPackaged, send/webContents.send).
In `@desktop/src/main/ipc.ts`:
- Around line 793-795: Handler persists the new release channel before awaiting
checkForUpdatesWithChannel, which can leave persisted state out of sync if the
check fails; change the flow to call await
checkForUpdatesWithChannel(args.channel) first and only call
setReleaseChannel(channel) after that succeeds, or wrap the current sequence in
try/catch and on failure revert persistence by calling
setReleaseChannel(previousChannel); refer to the handler's use of args.channel,
setReleaseChannel and checkForUpdatesWithChannel to implement the reorder or the
rollback logic so UI and persisted channel never diverge.
In `@desktop/src/main/updater.ts`:
- Around line 177-186: The download-progress handler currently calls setStatus
with a new object that overwrites existing metadata (version, releaseName,
releaseNotes); change it to merge into the existing status instead of replacing
it — e.g. in the autoUpdater.on("download-progress", ...) callback use a
functional update to setStatus(prev => ({ ...prev, state: "downloading",
progress: { percent: info.percent, bytesPerSecond: info.bytesPerSecond,
transferred: info.transferred, total: info.total } })) so
version/releaseName/releaseNotes are preserved during download.
- Around line 116-119: setAutoDownloadEnabled currently only updates the
in-memory flag and persisted settings but does not change the live updater
instance; after initAutoUpdater runs the autoUpdater.autoDownload remains stale
until restart. Modify setAutoDownloadEnabled(enabled: boolean) to also set the
running updater's autoDownload property when available (e.g., if a module-level
autoUpdater or the object returned by initAutoUpdater exists), so call something
like autoUpdater.autoDownload = enabled (guarded by a null/undefined check) in
addition to updating autoDownloadEnabled and saveSettings({ autoDownload:
enabled }). Ensure you reference the existing symbols setAutoDownloadEnabled,
autoDownloadEnabled, saveSettings, and the runtime autoUpdater/initAutoUpdater
variable so the change applies immediately when toggled.
In `@desktop/src/renderer/src/lib/components/update/UpdateBadge.svelte`:
- Around line 19-26: The badge currently treats any non-ready state as
"downloading", causing the pulsing Download icon and a 0% progress when s.state
=== "available"; update the rendering logic in UpdateBadge.svelte to distinguish
s.state === "downloading" from s.state === "available" (and other states), e.g.
use the existing ready boolean plus explicit checks on s.state to: 1) show the
Download icon and progress only when s.state === "downloading", 2) show a
separate icon/text (e.g. an "available" label without progress or pulsing) when
s.state === "available", and 3) update the title string (title={...}) to reflect
the specific state ("available" vs "downloading") so tooltips are accurate.
In `@desktop/src/renderer/src/lib/stores/settings.ts`:
- Around line 159-165: The renderer updates localStorage and the autoUpdate
store before the IPC write in setAutoDownload completes, causing the UI to show
a value the main process may not have applied; change setAutoUpdate to await
setAutoDownload (or handle its Promise) and only set userTouchedAutoUpdate,
localStorage.setItem(AUTO_UPDATE_KEY, String(value)) and autoUpdate.set(value)
after setAutoDownload resolves successfully; on rejection, revert any
provisional UI changes (or keep prior state) and surface/log the error so the
renderer and main process stay consistent; reference the setAutoUpdate function,
the setAutoDownload call, userTouchedAutoUpdate, AUTO_UPDATE_KEY, and autoUpdate
store when making the change.
In `@desktop/src/renderer/src/lib/stores/updates.svelte.ts`:
- Around line 39-45: subscribe currently only registers future listeners, so any
update stored in state.current before a subscriber (e.g., initUpdateToasts after
initUpdateStore) never gets replayed; modify subscribe (the function that pushes
into listeners) to immediately invoke the new listener with the current cached
state (state.current) after adding it so subscribers receive the latest status
right away, while keeping the existing unsubscribe closure logic unchanged.
In `@hack/rewrite_manifest_urls/main_test.go`:
- Around line 10-39: The test TestRewriteRewritesRelativeURLs only checks that
files[].url were rewritten but not the top-level path; update the test to
explicitly assert that the top-level "path" field was rewritten to the absolute
GitHub release URL after calling rewriteFile (use the same expected URL pattern
as for files[].url). Locate TestRewriteRewritesRelativeURLs and after reading
got (from os.ReadFile) add an assertion that string(got) contains the expected
rewritten path URL (constructed the same way as want) so the rewriteFile(path,
repo, tag) behavior for the top-level path is verified.
---
Nitpick comments:
In `@hack/smoke_test_manifests/main_test.go`:
- Around line 39-75: The tests currently accept any HTTP method, hiding
regressions if checkURL uses GET; update the handlers inside
TestRunSucceedsWhenAllReachable and TestRunFailsOn404 to assert r.Method ==
"HEAD" (or return 405 for non-HEAD) and only then respond with the intended
status code so the tests fail if the probe switches from HEAD to GET; reference
the test functions TestRunSucceedsWhenAllReachable and TestRunFailsOn404 and the
server handlers to locate where to add the method check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77c7f655-4418-472b-91d6-a653564fb524
⛔ Files ignored due to path filters (1)
desktop/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
.github/workflows/release.ymldesktop/package.jsondesktop/src/main/__tests__/tray.test.tsdesktop/src/main/__tests__/updater.test.tsdesktop/src/main/ipc.tsdesktop/src/main/tray.tsdesktop/src/main/updater.tsdesktop/src/renderer/src/App.sveltedesktop/src/renderer/src/lib/components/update/UpdateBadge.sveltedesktop/src/renderer/src/lib/components/update/UpdateDialog.sveltedesktop/src/renderer/src/lib/components/update/UpdateDialog.test.tsdesktop/src/renderer/src/lib/components/update/UpdatesSection.sveltedesktop/src/renderer/src/lib/components/update/update-toasts.test.tsdesktop/src/renderer/src/lib/components/update/update-toasts.tsdesktop/src/renderer/src/lib/ipc/commands.tsdesktop/src/renderer/src/lib/ipc/events.tsdesktop/src/renderer/src/lib/stores/settings.tsdesktop/src/renderer/src/lib/stores/updates.svelte.tsdesktop/src/renderer/src/pages/SettingsPage.sveltehack/merge_mac_metadata/main.gohack/merge_mac_metadata/main_test.gohack/rewrite_manifest_urls/main.gohack/rewrite_manifest_urls/main_test.gohack/smoke_test_manifests/main.gohack/smoke_test_manifests/main_test.go
Comment on lines
+42
to
+54
| it("emits dev-mode status when app is not packaged", async () => { | ||
| const electron = await import("electron") | ||
| ;(electron.app as { isPackaged: boolean }).isPackaged = false | ||
| const { initAutoUpdater } = await import("../updater.js") | ||
| const send = vi.fn() | ||
| const win = { isDestroyed: () => false, webContents: { send } } as never | ||
| await initAutoUpdater(() => win) | ||
| expect(send).toHaveBeenCalledWith( | ||
| "update-status", | ||
| expect.objectContaining({ state: "not-available", code: "dev-mode" }), | ||
| ) | ||
| ;(electron.app as { isPackaged: boolean }).isPackaged = true | ||
| }) |
There was a problem hiding this comment.
Guard mutable mock restoration with try/finally.
Line 44 mutates shared mock state; if the test exits early, isPackaged may stay false and contaminate later tests. Wrap the mutation in try/finally to guarantee restoration.
Suggested fix
it("emits dev-mode status when app is not packaged", async () => {
const electron = await import("electron")
- ;(electron.app as { isPackaged: boolean }).isPackaged = false
- const { initAutoUpdater } = await import("../updater.js")
- const send = vi.fn()
- const win = { isDestroyed: () => false, webContents: { send } } as never
- await initAutoUpdater(() => win)
- expect(send).toHaveBeenCalledWith(
- "update-status",
- expect.objectContaining({ state: "not-available", code: "dev-mode" }),
- )
- ;(electron.app as { isPackaged: boolean }).isPackaged = true
+ ;(electron.app as { isPackaged: boolean }).isPackaged = false
+ try {
+ const { initAutoUpdater } = await import("../updater.js")
+ const send = vi.fn()
+ const win = { isDestroyed: () => false, webContents: { send } } as never
+ await initAutoUpdater(() => win)
+ expect(send).toHaveBeenCalledWith(
+ "update-status",
+ expect.objectContaining({ state: "not-available", code: "dev-mode" }),
+ )
+ } finally {
+ ;(electron.app as { isPackaged: boolean }).isPackaged = true
+ }
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/main/__tests__/updater.test.ts` around lines 42 - 54, The test
mutates shared state electron.app.isPackaged; wrap the mutation and test logic
in a try/finally so isPackaged is always restored. Specifically, in the "emits
dev-mode status when app is not packaged" test, set (electron.app as {
isPackaged: boolean }).isPackaged = false, then run the initAutoUpdater() call
and assertions inside a try block, and in the finally block reset (electron.app
as { isPackaged: boolean }).isPackaged = true to avoid leaking state across
tests (references: initAutoUpdater, electron.app.isPackaged,
send/webContents.send).
Comment on lines
+39
to
+45
| export function subscribe(fn: Listener): () => void { | ||
| listeners.push(fn) | ||
| return () => { | ||
| const i = listeners.indexOf(fn) | ||
| if (i >= 0) listeners.splice(i, 1) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate and inspect the update store implementation
fd -a "updates\.svelte\.ts" -0 . | xargs -0 -I{} bash -lc 'echo "---- {}"; sed -n "1,120p" {}'
# 2) Find where subscribe is used and where initUpdateToasts / initUpdateStore are called
rg -n "initUpdateToasts|initUpdateStore|subscribe\(" desktop/src/renderer/src -S || true
# 3) Inspect App.svelte around the update wiring
fd -a "App\.svelte" desktop/src/renderer/src -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; rg -n "initUpdateToasts|initUpdateStore|update" {} -n -S; sed -n "1,220p" {}' || true
# 4) Inspect update-toasts implementation
fd -a "update-toasts\.ts" desktop/src/renderer/src -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; sed -n "1,240p" {}' || true
# 5) Search for state.current usage and Listener type
rg -n "state\.current|type Listener|interface Listener|Listener" desktop/src/renderer/src/lib/stores -S || trueRepository: devsy-org/devsy
Length of output: 15440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect update-toasts tests
fd -a "update-toasts\.test\.ts" desktop/src/renderer/src/lib/components/update -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; sed -n "1,220p" {}'
# Inspect UpdateDialog tests that might cover initialization ordering
fd -a "UpdateDialog\.test\.ts" desktop/src/renderer/src/lib/components/update -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; sed -n "1,240p" {}'
# Inspect mock subscribe in update-toasts.test
rg -n "subscribe\\(fn" desktop/src/renderer/src/lib/components/update/update-toasts.test.tsRepository: devsy-org/devsy
Length of output: 7007
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect update-toasts tests
fd -a "update-toasts\.test\.ts" desktop/src/renderer/src/lib/components/update -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; sed -n "1,240p" {}'
# Inspect UpdateDialog tests
fd -a "UpdateDialog\.test\.ts" desktop/src/renderer/src/lib/components/update -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; sed -n "1,220p" {}'
# Search for other subscribe usages of updates store
rg -n "subscribe\\(" desktop/src/renderer/src/lib -S | rg -n "stores/updates\\.svelte" || trueRepository: devsy-org/devsy
Length of output: 7077
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect update-toasts tests
fd -a "update-toasts\.test\.ts" desktop/src/renderer/src/lib/components/update -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; sed -n "1,260p" {}'
# Inspect UpdateDialog tests
fd -a "UpdateDialog\.test\.ts" desktop/src/renderer/src/lib/components/update -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; sed -n "1,240p" {}'
# Search for other subscribers to updates store
rg -n "from \"\\$lib/stores/updates\\.svelte" desktop/src/renderer/src/lib -S || true
rg -n "subscribe\\(" desktop/src/renderer/src/lib -S | rg -n "updates\\.svelte" || trueRepository: devsy-org/devsy
Length of output: 7006
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Update-toasts tests
fd -a "update-toasts\.test\.ts" desktop/src/renderer/src/lib/components/update -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; sed -n "1,260p" {}'
# UpdateDialog tests
fd -a "UpdateDialog\.test\.ts" desktop/src/renderer/src/lib/components/update -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; sed -n "1,240p" {}'
# Any other subscribers using updates.svelte
rg -n "stores/updates\.svelte" desktop/src/renderer/src/lib -S || trueRepository: devsy-org/devsy
Length of output: 7734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Update-toasts tests
fd -a "update-toasts\.test\.ts" desktop/src/renderer/src/lib/components/update -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; sed -n "1,260p" {}'
# UpdateDialog tests
fd -a "UpdateDialog\.test\.ts" desktop/src/renderer/src/lib/components/update -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; sed -n "1,240p" {}'
# Any other subscribers using updates.svelte
rg -n "stores/updates\.svelte" desktop/src/renderer/src/lib -S || trueRepository: devsy-org/devsy
Length of output: 7734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect components that read updateStatus
for f in desktop/src/renderer/src/lib/components/update/UpdateBadge.svelte \
desktop/src/renderer/src/lib/components/update/UpdatesSection.svelte \
desktop/src/renderer/src/lib/components/update/UpdateDialog.svelte; do
echo "---- $f"
sed -n '1,220p' "$f" | nl -ba | head -n 220
done
# Inspect UpdateDialog.svelte to see how it uses updateStatus
sed -n '1,260p' desktop/src/renderer/src/lib/components/update/UpdateDialog.svelte | nl -ba | head -n 260Repository: devsy-org/devsy
Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect components that read updateStatus
for f in \
desktop/src/renderer/src/lib/components/update/UpdateBadge.svelte \
desktop/src/renderer/src/lib/components/update/UpdatesSection.svelte \
desktop/src/renderer/src/lib/components/update/UpdateDialog.svelte; do
echo "---- $f"
sed -n '1,240p' "$f" | cat -n | head -n 260
doneRepository: devsy-org/devsy
Length of output: 12670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print ipc/events.js (or specific onUpdateStatus implementation)
fd -a "events\.js$" desktop/src/renderer/src -0 | head -n 20 | cat
fd -a "events\.js" desktop/src/renderer/src/lib/ipc -0 | xargs -0 -I{} bash -lc 'echo "---- {}"; rg -n "onUpdateStatus" -n {} && sed -n "1,220p" {}'
# Also search for onUpdateStatus usage
rg -n "onUpdateStatus" desktop/src/renderer/src -SRepository: devsy-org/devsy
Length of output: 490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' desktop/src/renderer/src/lib/ipc/events.ts | cat -n
# Print only onUpdateStatus region
rg -n "export function onUpdateStatus" -n desktop/src/renderer/src/lib/ipc/events.ts
start=$(rg -n "export function onUpdateStatus" desktop/src/renderer/src/lib/ipc/events.ts | head -n1 | cut -d: -f1)
# Grab 120 lines after start
sed -n "$start,$((start+160))p" desktop/src/renderer/src/lib/ipc/events.ts | cat -nRepository: devsy-org/devsy
Length of output: 3819
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "subscribe\\([^\\)]*\\)" desktop/src/renderer/src/lib -S | rg -n "stores/updates\\.svelte\\.js|stores/updates\\.svelte" || true
rg -n "\\bsubscribe\\b" desktop/src/renderer/src/lib | rg -n "updates\\.svelte" || true
rg -n "from \"\\$lib/stores/updates\\.svelte\\.js\"" desktop/src/renderer/src/lib -S || trueRepository: devsy-org/devsy
Length of output: 470
Replay cached update status to new subscribers in updates.svelte.ts
subscribe() only notifies future set() calls. With App.svelte awaiting initUpdateStore() before wiring initUpdateToasts(), an "available"/"downloaded" event received in that gap updates state.current but never reaches the toast subscriber, so the one-shot toast/restart UX can be missed.
♻️ Proposed fix
export function subscribe(fn: Listener): () => void {
listeners.push(fn)
+ fn(state.current)
return () => {
const i = listeners.indexOf(fn)
if (i >= 0) listeners.splice(i, 1)
}
}📝 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.
Suggested change
| export function subscribe(fn: Listener): () => void { | |
| listeners.push(fn) | |
| return () => { | |
| const i = listeners.indexOf(fn) | |
| if (i >= 0) listeners.splice(i, 1) | |
| } | |
| } | |
| export function subscribe(fn: Listener): () => void { | |
| listeners.push(fn) | |
| fn(state.current) | |
| return () => { | |
| const i = listeners.indexOf(fn) | |
| if (i >= 0) listeners.splice(i, 1) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/renderer/src/lib/stores/updates.svelte.ts` around lines 39 - 45,
subscribe currently only registers future listeners, so any update stored in
state.current before a subscriber (e.g., initUpdateToasts after initUpdateStore)
never gets replayed; modify subscribe (the function that pushes into listeners)
to immediately invoke the new listener with the current cached state
(state.current) after adding it so subscribers receive the latest status right
away, while keeping the existing unsubscribe closure logic unchanged.
Comment on lines
+10
to
+39
| func TestRewriteRewritesRelativeURLs(t *testing.T) { | ||
| dir := t.TempDir() | ||
| in := filepath.Join(dir, "beta-mac.yml") | ||
| yaml := `version: 1.10.0-beta.12 | ||
| files: | ||
| - url: Devsy_mac_arm64.zip | ||
| sha512: abc== | ||
| size: 100 | ||
| - url: Devsy_mac_x64.dmg | ||
| sha512: def== | ||
| size: 200 | ||
| path: Devsy_mac_arm64.zip | ||
| sha512: abc== | ||
| size: 100 | ||
| ` | ||
| if err := os.WriteFile(in, []byte(yaml), 0o644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := rewriteFile(in, "devsy-ai/devsy", "v1.10.0-beta.12"); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| got, _ := os.ReadFile(in) | ||
| want := "https://github.com/devsy-ai/devsy/releases/download/v1.10.0-beta.12/Devsy_mac_arm64.zip" | ||
| if !strings.Contains(string(got), want) { | ||
| t.Fatalf("missing rewritten url. got:\n%s", got) | ||
| } | ||
| if !strings.Contains(string(got), "Devsy_mac_x64.dmg") { | ||
| t.Fatal("second url missing") | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert the top-level path rewrite explicitly.
This test currently passes if only files[].url is rewritten, but the updater path regression also depends on path becoming absolute. Add a direct assertion for the path: field so the main failure mode stays covered.
Suggested test tightening
got, _ := os.ReadFile(in)
want := "https://github.com/devsy-ai/devsy/releases/download/v1.10.0-beta.12/Devsy_mac_arm64.zip"
if !strings.Contains(string(got), want) {
t.Fatalf("missing rewritten url. got:\n%s", got)
}
+ if !strings.Contains(string(got), "path: "+want) {
+ t.Fatalf("top-level path was not rewritten. got:\n%s", got)
+ }
if !strings.Contains(string(got), "Devsy_mac_x64.dmg") {
t.Fatal("second url missing")
}📝 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.
Suggested change
| func TestRewriteRewritesRelativeURLs(t *testing.T) { | |
| dir := t.TempDir() | |
| in := filepath.Join(dir, "beta-mac.yml") | |
| yaml := `version: 1.10.0-beta.12 | |
| files: | |
| - url: Devsy_mac_arm64.zip | |
| sha512: abc== | |
| size: 100 | |
| - url: Devsy_mac_x64.dmg | |
| sha512: def== | |
| size: 200 | |
| path: Devsy_mac_arm64.zip | |
| sha512: abc== | |
| size: 100 | |
| ` | |
| if err := os.WriteFile(in, []byte(yaml), 0o644); err != nil { | |
| t.Fatal(err) | |
| } | |
| if err := rewriteFile(in, "devsy-ai/devsy", "v1.10.0-beta.12"); err != nil { | |
| t.Fatal(err) | |
| } | |
| got, _ := os.ReadFile(in) | |
| want := "https://github.com/devsy-ai/devsy/releases/download/v1.10.0-beta.12/Devsy_mac_arm64.zip" | |
| if !strings.Contains(string(got), want) { | |
| t.Fatalf("missing rewritten url. got:\n%s", got) | |
| } | |
| if !strings.Contains(string(got), "Devsy_mac_x64.dmg") { | |
| t.Fatal("second url missing") | |
| } | |
| } | |
| func TestRewriteRewritesRelativeURLs(t *testing.T) { | |
| dir := t.TempDir() | |
| in := filepath.Join(dir, "beta-mac.yml") | |
| yaml := `version: 1.10.0-beta.12 | |
| files: | |
| - url: Devsy_mac_arm64.zip | |
| sha512: abc== | |
| size: 100 | |
| - url: Devsy_mac_x64.dmg | |
| sha512: def== | |
| size: 200 | |
| path: Devsy_mac_arm64.zip | |
| sha512: abc== | |
| size: 100 | |
| ` | |
| if err := os.WriteFile(in, []byte(yaml), 0o644); err != nil { | |
| t.Fatal(err) | |
| } | |
| if err := rewriteFile(in, "devsy-ai/devsy", "v1.10.0-beta.12"); err != nil { | |
| t.Fatal(err) | |
| } | |
| got, _ := os.ReadFile(in) | |
| want := "https://github.com/devsy-ai/devsy/releases/download/v1.10.0-beta.12/Devsy_mac_arm64.zip" | |
| if !strings.Contains(string(got), want) { | |
| t.Fatalf("missing rewritten url. got:\n%s", got) | |
| } | |
| if !strings.Contains(string(got), "path: "+want) { | |
| t.Fatalf("top-level path was not rewritten. got:\n%s", got) | |
| } | |
| if !strings.Contains(string(got), "Devsy_mac_x64.dmg") { | |
| t.Fatal("second url missing") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/rewrite_manifest_urls/main_test.go` around lines 10 - 39, The test
TestRewriteRewritesRelativeURLs only checks that files[].url were rewritten but
not the top-level path; update the test to explicitly assert that the top-level
"path" field was rewritten to the absolute GitHub release URL after calling
rewriteFile (use the same expected URL pattern as for files[].url). Locate
TestRewriteRewritesRelativeURLs and after reading got (from os.ReadFile) add an
assertion that string(got) contains the expected rewritten path URL (constructed
the same way as want) so the rewriteFile(path, repo, tag) behavior for the
top-level path is verified.
Fixes the broken auto-updater (manifests on the CDN pointed to relative paths but binaries lived only on GitHub Releases) and ships a redesigned, coordinated update UX. Publish pipeline: - New hack/rewrite_manifest_urls helper rewrites manifest files[].url and path to absolute https://github.com/<repo>/releases/download/<tag>/<file> URLs so binaries stay on GitHub Releases (download stats preserved) while manifests are served from dl.devsy.sh. - hack/merge_mac_metadata moved into its own subdir for layout consistency and now dedupes files[] by URL with last-write-wins (fixes stale entries that caused electron-updater to advertise mismatched sha512 hashes). - Dropped the fetch-existing-from-live-site step that was the source of the stale entries. - New hack/smoke_test_manifests helper HEAD-checks every URL in every deployed manifest before Netlify deploy (with GITHUB_TOKEN auth for github.com hosts via url.Parse + Hostname comparison). - release.yml updated to invoke all three Go helpers in the correct order. Main process (desktop/src/main/updater.ts): - Extended UpdateStatus with progress, code (dev-mode/unsupported/network/ feed-error/verification), and an idle state. - Wired download-progress events to the renderer. - Replaced the misleading "not code-signed" message with real error codes. - Removed the native dialog.showMessageBox on update-downloaded; the renderer now owns the post-download UX. - Honors the user's auto-download setting end-to-end via new set_auto_download / get_auto_download IPC. - Settings file write is atomic (write-to-temp + rename) and error-wrapped. - Exposes getLastStatus for the tray. IPC (desktop/src/main/ipc.ts): - New handlers: download_update, get_app_version, get_auto_download, set_auto_download. set_release_channel narrows the channel via an explicit ReleaseChannel local for clarity. Renderer: - New global Svelte 5 store at lib/stores/updates.svelte.ts subscribed once at App root. - New UpdateDialog.svelte renders all six states (checking, available, downloading with progress bar, downloaded with restart CTA, not-available, error). Release notes sanitized through DOMPurify in a $derived value. - New update-toasts.ts fires svelte-sonner toasts for available, downloaded, error, and not-available, gated on user-initiated checks so background errors stay silent. Dedupes by (state, version). - New UpdateBadge.svelte passive header indicator visible only when an update is in flight or ready. - New UpdatesSection.svelte extracted from SettingsPage so the Updates + Version + Release Channel controls are self-contained; SettingsPage embeds a single <UpdatesSection /> tag. - New tray "Install Update v<version>" item via the pure buildUpdateMenuItems helper. - localStorage autoUpdate is now a fast cache; main-process JSON is the canonical source. syncAutoUpdateFromMain seeds the renderer store at boot and respects a userTouchedAutoUpdate flag so a fast user toggle during boot is not clobbered. Tests: - desktop/src/main/__tests__/updater.test.ts (dev-mode emission, progress propagation, autoDownload honor, no native dialog on update-downloaded). - desktop/src/main/__tests__/tray.test.ts (buildUpdateMenuItems across all states, click wiring, missing-version fallback). - desktop/src/renderer/.../update/UpdateDialog.test.ts (per-state rendering). - desktop/src/renderer/.../update/update-toasts.test.ts (dedupe, user-initiated gating, dev-mode silence, auto-download-on/off branches, available->downloading->available transition). - hack/rewrite_manifest_urls/main_test.go (relative rewrite, absolute skip, non-map entries, filename filter). - hack/merge_mac_metadata/main_test.go (URL dedupe last-write-wins, empty files, missing-url entries, single-source no-op). - hack/smoke_test_manifests/main_test.go (200 success, 404 fail, relative URL fail, missing dir tolerated). 163 desktop tests pass, 13 Go tests pass across 4 packages, svelte-check clean, electron-builder build succeeds.
skevetter
force-pushed
the
improve-update-ux
branch
from
5887adc to
c1f52e5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the broken auto-updater (manifest URLs pointed to a CDN that never had binaries) and ships a redesigned, coordinated update UX. Delivered as one PR per request.
Root cause of the failed update check
The error users were seeing — "Auto-update is not available (app is not code-signed)" — was a misleading catch-all. Code signing was fine. Two real bugs were causing the failures:
dl.devsy.shlisted relative URLs likeDevsy_mac_arm64.zip, but the binaries lived only on GitHub Releases.electron-updaterfetched the manifest fine, then 404'd on every binary download.hack/merge_mac_metadata.goaccumulated stale duplicatefiles[]entries because each release re-merged the deployed manifest with the new one. The deployedbeta-mac.ymllistedDevsy_mac_arm64.zipthree times with three different sha512 hashes;electron-updaterhappened to pick a stale one.What changed
Publish pipeline (
hack/+.github/workflows/release.yml)hack/rewrite_manifest_urls/Go helper rewrites manifestfiles[].urlandpathto absolutehttps://github.com/<owner>/<repo>/releases/download/<tag>/<file>URLs. Binaries stay on GitHub Releases (download stats preserved); manifests on the CDN now point to them.hack/merge_mac_metadata/(moved into subdir for layout consistency) now dedupes by URL with last-write-wins.hack/smoke_test_manifests/Go helper HEAD-checks every URL in every deployed manifest before Netlify deploy, withGITHUB_TOKENauth for github.com hosts. Future regressions of this class fail the release job loudly instead of silently breaking users.Main process (
desktop/src/main/updater.ts)UpdateStatuswithprogress,code(dev-mode/unsupported/network/feed-error/verification), and anidlestate.download-progressevents to the renderer.dialog.showMessageBoxonupdate-downloaded— the renderer owns this UX now.set_auto_download/get_auto_downloadIPC.Renderer UX
$lib/stores/updates.svelte.ts; subscribed once at App root.UpdateDialog.sveltemodal renders all six states (checking / available / downloading-with-progress-bar / downloaded-with-restart-CTA / not-available / error). Built on bits-ui Dialog.update-toasts.tsfires svelte-sonner toasts for available / downloaded / error / not-available, gated on user-initiated checks so background errors stay silent.UpdateBadge.sveltepassive header indicator (appears only when an update is in flight).app.getVersion(), delegated inline status to the new dialog via a sharedopenUpdateDialog()helper.Summary by CodeRabbit
New Features
Tests