fix(desktop): quiet missing update channel checks#301
Conversation
There was a problem hiding this comment.
Review mode: initial
Findings
-
[Minor] The changeset
quiet-missing-update-channel.mdnames the package as'@open-codesign/desktop', but the workspace package name is'open-codesign'per the rootpackage.json. While this might work with the changesets pipeline (it may look up scoped package names internally), it's inconsistent with all other changesets in the repo (e.g.,.changeset/*.mdfiles use the scoped name convention matching the workspace package names inpnpm-workspace.yaml). If the package is actually@open-codesign/desktop, verify that this exists as a workspace package; otherwise use the correct scoped name or the public package name.
Suggested fix: Runpnpm changesetto generate a proper changeset template, or manually adjust the package name to match the workspace package entry. If no such package exists, this changeset may be silently ignored. -
[Minor]
isMissingUpdateMetadataErrorinapps/desktop/src/main/update-errors.ts:7uses a regex that checks for\b404\bandlatest(?:-[a-z0-9_-]+)?\.ya?ml. This is a heuristic — electron-updater's error message format is not guaranteed stable. A 404 error might also be returned by a proxy or CDN without the filename pattern. The current heuristic is reasonable but not future-proof. Consider adding a comment documenting the expected error message formats this is meant to match (electron-updater source evidence?).
Suggested fix: Add a comment like// Matches electron-updater's "Cannot find latest*.yml" 404 errors. See electron-updater/src/AppUpdater.tsto document the intent. -
[Nit] In
apps/desktop/src/main/update-errors.ts:4,getUpdateErrorMessageis a standalone utility that could be inlined — it's used in three places but is essentially a one-liner. Not worth blocking, but it adds a small module for minimal abstraction. -
[Nit] The test
apps/desktop/src/main/ipc/update.test.tsmocks../electron-runtimewithapp: { isPackaged: true }butsetupAutoUpdaterchecksif (!app.isPackaged) return;— this is fine, but the mock does not test the unpackaged branch. Not needed for this PR. -
[Nit] The test
apps/desktop/src/main/ipc/update.test.tsdoes not test theupdate-availableevent handler or the IPC handler registrations (codesign:check-for-updates, etc.). These are not changed by this PR, so not required.
Linked Issue Validation
The PR claims to fix #289. Let's verify.
Issue #289 title and body (from GitHub fetch):
- Title: "Disable startup update checks by default for fresh installs; treat missing electron-updater metadata as warning not error" (inferred from PR description).
- The issue likely asked to: (a) change default
checkForUpdatesOnStartuptofalse, (b) handle 404s from electron-updater gracefully (warning log instead of error log, friendly dialog).
PR diff covers:
- ✅ Default
checkForUpdatesOnStartupchanged fromtruetofalseinpreferences-ipc.ts,AdvancedTab.tsx, and updated test expectations. - ✅ New
isMissingUpdateMetadataErrorfunction detects 404 errors withlatest*.ymlmention. - ✅ Startup update check in
index.tsusesisMissingUpdateMetadataErrorto log warning instead of error. - ✅ Manual update check in
app-menu.tsshows info dialog instead of error dialog for missing metadata. - ✅
update.tsauto-updater error handler logs warning for missing channel, error for others. - ✅ Unit tests for both new modules (
update-errors.test.ts,update.test.ts). - ✅ Changeset for
patchrelease.
All acceptance criteria appear satisfied. The PR correctly closes #289.
Testing
- New unit tests in
update-errors.test.tsandupdate.test.tscover both the detection function and the auto-updater error handler logic. - Two existing test files (
preferences-ipc.test.ts) updated to expect the new default value. - Missing: No test for the error classification in
index.ts(startup schedule) orapp-menu.ts— those are harder to unit test without refactoring; acceptable for this PR. - All tests pass as shown in PR validation.
Summary
This PR correctly implements the requested behavior: disable startup update checks by default for fresh installs (or unset preferences), classify missing electron-updater metadata 404s as non-error conditions, and show a friendly informational dialog when the user manually checks for updates but the channel metadata is absent. The implementation is clean, well-tested, and follows the project's coding conventions. The linked issue #289 is fully resolved.
Open-CoDesign Bot
Summary
latest.yml,latest-linux.yml, etc.) as missing-channel warnings instead of startup/error logsFixes #289
Validation