feat(update): add manual release checking - #313
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe Electron app now checks the latest official GitHub release, compares semantic versions, displays localized update results, opens validated release links, and exposes the action in the tray menu. Tests cover version ordering, response validation, URL security, and failure handling. ChangesUpdate checking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TrayMenu
participant ElectronMain
participant UpdateChecker
participant GitHub
participant Dialog
TrayMenu->>ElectronMain: invoke Check for Updates
ElectronMain->>UpdateChecker: checkLatestRelease(currentVersion, signal)
UpdateChecker->>GitHub: request latest release
GitHub-->>UpdateChecker: release response
UpdateChecker-->>ElectronMain: current or available result
ElectronMain->>Dialog: show localized update status
ElectronMain->>Dialog: open validated release link when selected
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review The earlier automated review was rate-limited; requesting a fresh substantive review now that the cooldown has elapsed. |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@electron/update-checker.test.ts`:
- Around line 46-88: Expand the update-checker tests around checkLatestRelease
to cover equal-version results, forwarding an AbortSignal to fetchLatest,
rejection of draft and prerelease releases, and rejection of GitHub URLs with
invalid paths, tags, queries, or hashes. Keep each case in
electron/update-checker.test.ts and assert the documented result or error for
every validation and cancellation contract.
In `@electron/update-checker.ts`:
- Around line 26-39: Update parseVersion to store major, minor, and patch as
bigint values, and build normalized core identifiers from the original validated
numeric components without precision loss. Reject leading-zero core identifiers
while preserving valid zero values, and add boundary tests covering oversized
identifiers and leading-zero inputs.
In `@src/i18n/locales/vi/common.json`:
- Around line 42-46: Update the Vietnamese `updates.current` translation to
state that the installed OpenScreen version is the latest/current version,
rather than saying OpenScreen has been updated; preserve the existing
`{{currentVersion}}` placeholder.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ef3436bb-9ad0-4117-a312-c8051b845b0e
📒 Files selected for processing (16)
electron/main.tselectron/update-checker.test.tselectron/update-checker.tssrc/i18n/locales/ar/common.jsonsrc/i18n/locales/en/common.jsonsrc/i18n/locales/es/common.jsonsrc/i18n/locales/fr/common.jsonsrc/i18n/locales/it/common.jsonsrc/i18n/locales/ja-JP/common.jsonsrc/i18n/locales/ko-KR/common.jsonsrc/i18n/locales/pt-BR/common.jsonsrc/i18n/locales/ru/common.jsonsrc/i18n/locales/tr/common.jsonsrc/i18n/locales/vi/common.jsonsrc/i18n/locales/zh-CN/common.jsonsrc/i18n/locales/zh-TW/common.json
|
Addressed all three fresh review findings in |
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Reviewed at the PR head (0250f1d8). Verdict: ship with nits, with one fix I'd want before merge (§1 — Store builds).
This is careful work, and I went at it hard because it's main-process code making an outbound request. The things that usually go wrong in a hand-rolled update checker are all done right here:
- Version comparator is correct. I bundled
update-checker.tswith esbuild and ran 29 cases standalone.1.9.1 < 1.10.0✓ (the classic string-compare trap),v1.2.3 == 1.2.3✓,1.2.3-rc.1 < 1.2.3✓,1.9.0-beta.2 < 1.9.0-beta.11✓ (numeric, not lexical),1.9.0-alpha < 1.9.0-alpha.1✓,1.0.0+build == 1.0.0✓, and1.2/""/abc/1.2.3.4/01.0.0all throw and surface as "Could not check". No case gave a wrong answer. The regex is fully anchored, no unescaped metachars. - The URL allowlist holds.
officialReleaseUrlchecks protocol + hostname + empty port + the exact/getopenscreen/openscreen/releases/tag/prefix +decodeURIComponent(tail) === tag_name+ empty search + empty hash. I probedhttps://evil.com/…,…/releases/download/…, path traversal,//github.com//getopenscreen/…, trailing slash, and?/#variants — all rejected. Stricter than the repo's existing protocol-onlyopen-external-urlhandler. - Failure is not reported as "up to date." This is the single most dangerous bug in this class and it's absent:
null,[], a__proto__payload and a throwingjson()all reject and land in thecatchshowing "Could not check for updates". Verified by probing each. AbortSignal.timeout(10_000)present. Nothing downloads or executes anything from the response. No IPC channel added, so no preload/channel-collision surface.- I confirmed neither
electron-updaternorsemveris a dependency andgrep -rn "autoUpdater|electron-updater|checkForUpdates" electron/ src/ package.jsonreturns zero hits onmain— so this is not re-implementing something already installed. Given both Windows CI artifacts are unsigned (electron-updater's NSIS path needs a signed installer), hand-rolling is defensible here. - 17/17 tests pass in 1.10s, network fully mocked via injected
vi.fn()— no realapi.github.comcall, so no CI flake or rate-limit hazard. Both typecheck configs, biome, i18n and docs checks clean. All 13 locales have all 5 keys, genuinely translated, placeholders consistent (zh-TW correctly distinct from zh-CN; the vi wording fix in commit 2 was right).
1. Store builds shouldn't offer this
main.ts:422 — the tray item is added unconditionally, including in MSIX/appx builds where the Store owns updates.
We ship a Store build (npm run build:win:store → electron-builder --win appx), and the .appx is the only signed Windows artifact, so it's the recommended install path. A Store-installed user on 1.9.2 clicks tray → Check for Updates → "OpenScreen 1.9.5 is available" → View Release → downloads OpenScreen-Setup.exe from GitHub, and now has two installations with two userData paths, while the Store copy keeps auto-updating underneath.
grep -rn "windowsStore" electron/ returns nothing today. process.windowsStore is available in Electron 41:
if (process.windowsStore) return; // in checkForUpdates()
...(process.windowsStore ? [] : [{ label: …, click: … }]) // and omit the menu entry2. The voided promise can take down the main process
main.ts:425 — void checkForUpdates() starts a 10-second round trip with no tie to app lifetime, and the failure branch itself awaits a dialog.
User clicks Check for Updates, then Quit in the same tray session. app.quit() runs while the fetch is pending; when it settles, dialog.showMessageBox is called on a quitting app — best case a stray modal after the app should be gone, worst case that call rejects and the rejection escapes entirely, because nothing handles the voided promise. electron/main-process-errors.ts:26-31 re-throws every non-EPIPE unhandledRejection, i.e. it kills the main process.
Keep the AbortController in module scope with app.on("before-quit", () => controller.abort()), and attach a terminal handler: checkForUpdates().catch((e) => console.error("[updates]", e)).
3. Tray is the only entry point
main.ts:415-435 — the application menu built two functions away (setupApplicationMenu, :323-324) is untouched. On stock GNOME 40+ with no StatusNotifier/AppIndicator host, new Tray() succeeds but nothing renders — and we ship AppImage/deb/pacman/nix/AUR, so those users have no path to the feature at all. On macOS the convention is the app menu's first section ("Check for Updates…"), which is also empty. The same { label: mainT("common", "actions.checkForUpdates"), click: … } entry in the app-menu template would cover both.
4. The wiring has no test
All 168 test lines target the pure module; checkForUpdates() in main.ts:349-394 — the part that calls shell.openExternal, keys off choice.response === 0, and owns the updateCheckInFlight guard — has zero coverage. Swap defaultId/cancelId, or reorder buttons so Cancel becomes index 0, and the Cancel button silently opens the browser with nothing failing. AGENTS.md asks for a test for every new behavior in the same package.
Hoisting the ~8 lines that turn an UpdateCheckResult into {message, buttons, onConfirm} into update-checker.ts and asserting on that would leave main.ts as pure Electron glue.
5. Some of commit 2's hardening is aimed at inputs that can't occur
update-checker.ts:26-49 — the BigInt conversion and leading-zero validator cover 9007199254740993.0.0 and 01.0.0. The only two version strings this function ever sees are app.getVersion() (from our own package.json) and tag_name from our own releases, and git tag shows a uniform vX.Y.Z / vX.Y.Z-rc.N history. The cost is a permanently harder-to-read parser, plus we're now stricter than GitHub: a maintainer who ever types v1.9.01 gets "Could not check for updates" with no clue why. Not wrong, just unearned — reverting parseVersion to Number() and keeping the three real ordering tests would read better. Your call.
6. Worth naming: 483 lines for "tell me a version exists"
The shortest correct alternative is one tray entry with no network code, no comparator, no JSON validation, no URL allowlist, and 1 i18n key instead of 5:
{ label: mainT("common", "actions.checkForUpdates"),
click: () => void shell.openExternal("https://github.com/getopenscreen/openscreen/releases/latest") }GitHub's own page already shows the latest version. What the other ~480 lines buy is the "you are already up to date" answer without a browser trip — which is a real product decision, not free. If that answer is wanted, the code as written is a reasonable way to get it and I'm happy to keep it; I'd just like it to be an explicit trade rather than an accident. If it isn't wanted, the 3-liner deletes update-checker.ts, its test, and 4 of the 5 keys.
Nits
update-checker.ts:80-99—officialReleaseUrlnever checksurl.username/url.password, sohttps://user:pass@github.com/getopenscreen/openscreen/releases/tag/v9.9.9passes the allowlist and reachesopenExternalverbatim (I probed it). Impact is capped since the host is still github.com, but it defeats the intent of an otherwise airtight check. Add|| url.username !== "" || url.password !== "".main.ts:355—net.fetchissues fromsession.defaultSession, so the default cookie jar and full Electron UA go to api.github.com on every check. Nothing secret leaks today, but any future in-app OAuth window on the default session would start silently attaching cookies.credentials: "omit"costs nothing.update-checker.ts:133-134— thedraft !== false || prerelease !== falserejection is unreachable:/releases/latestis documented to exclude both. And if one ever did arrive, a prerelease surfaces as an error dialog rather than the truthful "you're up to date". Twoit.eachcases exist purely to pin unreachable code. If kept as defense, return{kind:"current"}instead of throwing.update-checker.ts:71-78+:141—checkLatestReleaseparses both versions, thencompareVersions(latest.normalized, current.normalized)parses both again: fourparseVersioncalls per check where two suffice. It also makes the normalized round-trip an unstated invariant — ifnormalizedever emitted something the regex rejects, the checker would throw on its own output. A sharedcompareParsed(a, b)fixes both.main.ts:359,369,385— all three dialogs are parentless, andupdateCheckInFlightstaystruefor their whole lifetime. On Windows the user right-clicks the tray (app in background), the parentless dialog may only flash a taskbar button; they re-click Check for Updates and get nothing, because the guard is held by a dialog they can't see. Pass the HUD as parent when it exists.update-checker.ts:130— a 403 rate-limit (60 req/hr/IP, shared NAT in an office or university) is indistinguishable from any other failure. Low value given the trigger is manual; mentioning for completeness.- Pre-existing, newly user-visible:
electron/i18n.ts:29-56omitspt-BRfrom itsLocaleunion andmessagesmap (nocommonPtBrimport) whilei18n:checkvalidates 13 locales — so the pt-BR strings this PR adds can never render. I'll open a separate issue.
§1 is the one I'd like before merge; §2 is cheap and worth taking at the same time. Everything else can be follow-ups.
|
Filed the pt-BR main-process locale gap from my review as #354 — pre-existing, not blocking this PR. |
|
Opened #358 on top of this branch — it turns this manual check into a real in-place updater on all three platforms, and adds the install-channel detection that properly fixes the Microsoft Store point from my review (Store/Flathub/Snap/Nix get no update affordance at all, rather than a one-off #358 keeps this PR's release-page path rather than replacing it: every macOS install up to v1.9.0-rc.1 is ad-hoc signed, and Squirrel validates against the installed app's designated requirement, so those users can never be reached by an updater. Your fallback is the only thing that covers them. Merge this one first. |
2 participants
Summary
Check for Updatesaction to the idle system-tray menugetopenscreen/openscreenrelease-tag page only after explicit user confirmationThis is intentionally the safe manual-checking slice of #301. It does not download, install, execute, or automatically poll for updates; automatic update policy/settings remain future work.
Related issue
Part of #301
Type of change
Release impact
Desktop impact
Screenshots / video
Not included; this adds a native tray item and native result dialogs.
Testing
npx vitest --run electron/update-checker.test.ts(5 passed)npm run test(1,682 passed, 1 skipped across 141 files)npx tsc --noEmitnpx tsc -p tsconfig.test.json --noEmitnpx biome checkon the Electron, updater, test, and locale filesnpm run docs:checknpm run i18n:checknpm run build-vitev1.9.0andhttps://github.com/getopenscreen/openscreen/releases/tag/v1.9.0, matching the guarded contractAuthored with Codex assistance and manually security-reviewed to ensure this path cannot open a non-official download URL.
Summary by CodeRabbit