Repository navigation
Conversation
Model updates as three tracks — stable, RC (-rc.N), and perf-RC (-rc.N.perf) — classified by getReleaseChannel. A running build is always offered stable plus its own track; Shift-clicking "Check for Updates" additively opts into the RC track and Cmd (macOS) / Ctrl (Win/Linux)+Shift opts into the perf-RC track. So an RC build sees stable+RC (perf-RC only with the key), and a perf-RC build sees stable+perf-RC (RC only with the key). Auto/background checks use only the running build's own track + stable. - getReleaseChannel + ReleaseChannel classifier in updater-fallback - feed filtering switched from an includePrerelease boolean to eligibleChannels - resolveEligibleReleaseChannels combines the current channel with sticky opt-ins - native menu + both Settings "Check for Updates" surfaces detect the modifiers - sidebar item now reads a real onClick MouseEvent (Radix onSelect delivers a modifier-less CustomEvent, so Shift/Cmd-Shift never registered there) Co-authored-by: Orca <help@stably.ai>
605f1f4 to
8e4cccb
Compare
📝 WalkthroughWalkthroughChangesThis PR adds perf-RC release channel support to update checks. A new Sequence Diagram(s)Included in the hidden review stack artifact above. Related PRs: None specified. Suggested labels: updater, enhancement Suggested reviewers: None specified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/updater-fallback.ts (1)
130-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLogic is correct; comment exceeds guideline length.
getReleaseChannelcorrectly classifies stable/rc/perf-rc via exact'perf'identifier matching (avoids substring false positives). The explanatory block above it is 5 lines; as per coding guidelines, comments explaining non-obvious behavior should be "one or two lines, capturing only the non-obvious reason."Source: Coding guidelines
src/main/updater.ts (2)
1020-1033: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueIPC contract allows
includePerfPrereleasewithoutincludePrerelease.
checkForUpdatesFromMenuderivesrc/perfRcindependently fromoptions, so a caller passing onlyincludePerfPrerelease: trueyieldseligibleChannels: ['stable', 'perf-rc'](no'rc'). The PR's UX contract assumes Cmd/Ctrl+Shift always carries Shift too (per theupdater.test.tscomment "Cmd/Ctrl+Shift-click always carries Shift too, so both opt-ins fire"), but nothing in this file enforces that pairing — it relies entirely on the menu/renderer callers (not in this review batch) always sending both flags together.Consider normalizing here for defense-in-depth, e.g. treating
includePerfPrereleaseas implyingincludePrerelease:🛡️ Optional normalization
const wantsPrereleaseOptIn = Boolean(options?.includePrerelease || options?.includePerfPrerelease) if (wantsPrereleaseOptIn) { clearPrereleaseFallbackContext() enablePrereleaseChannels({ - rc: Boolean(options?.includePrerelease), + rc: Boolean(options?.includePrerelease || options?.includePerfPrerelease), perfRc: Boolean(options?.includePerfPrerelease) }) }
782-785: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeveral new "Why" comments exceed the one-to-two-line guideline.
Blocks at lines 782-785, 853-857, and 1005-1009 are 4-5 lines each. As per coding guidelines,
**/*.{ts,tsx,js,jsx}comments explaining non-obvious behavior should be "short — one or two lines, capturing only the non-obvious reason." Not blocking, but worth trimming for consistency.Also applies to: 853-857, 1005-1009
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5dcc7116-60f0-4ef7-87e1-149e34f96c39
📥 Commits
Reviewing files that changed from the base of the PR and between 8070e65 and 605f1f479bf25ba4cb997c5a4d76898bc6b87afa.
📒 Files selected for processing (14)
src/main/menu/register-app-menu.test.tssrc/main/menu/register-app-menu.tssrc/main/updater-fallback.tssrc/main/updater-prerelease-feed-readiness.test.tssrc/main/updater-prerelease-feed.test.tssrc/main/updater-prerelease-feed.tssrc/main/updater.fallback.test.tssrc/main/updater.test.tssrc/main/updater.tssrc/main/window/attach-main-window-services.tssrc/preload/api-types.tssrc/renderer/src/components/settings/GeneralUpdateSettingsSection.tsxsrc/renderer/src/components/sidebar/SidebarSettingsHelpMenu.tsxsrc/shared/types.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/sidebar/SidebarSettingsHelpMenu.test.tsx (1)
268-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only exercises one platform branch for perf-RC modifier.
perfModifieris derived usingnavigator.userAgent.includes('Mac')— the exact same expression the SUT (SidebarSettingsHelpMenu.tsxline 151) uses to pickmetaKeyvsctrlKey. Since both expected and actual values are computed from the same runtimenavigator.userAgent, this test only ever exercises whichever branch matches the CI/test-runner's jsdom environment; the other platform's modifier-detection path is never independently verified. It also doesn't cover the case of holding the platform modifier without Shift (should not enable any prerelease flag), which would catch a regression in theshiftKey && (...)logic.Consider stubbing
navigator.userAgent(e.g. viavi.stubGlobalorObject.defineProperty) to explicitly assert both the Mac (metaKey) and non-Mac (ctrlKey) branches, plus a modifier-only-without-shift case.♻️ Example of explicit branch coverage
- it('routes Check for Updates modifiers to the RC and perf-RC channels', async () => { - const container = await renderMenu() - const item = findMenuItem(container, 'Check for Updates') - // Why: Cmd (macOS) / Ctrl (Win/Linux) with Shift is the perf-RC escalation. - const perfModifier = navigator.userAgent.includes('Mac') ? { metaKey: true } : { ctrlKey: true } - - const clickWith = async (init: MouseEventInit): Promise<void> => { - await act(async () => { - item.dispatchEvent(new MouseEvent('click', { bubbles: true, ...init })) - }) - } - - await clickWith({}) - await clickWith({ shiftKey: true }) - await clickWith({ shiftKey: true, ...perfModifier }) - - expect(mocks.updaterCheck.mock.calls).toEqual([ - [{ includePrerelease: false, includePerfPrerelease: false }], - [{ includePrerelease: true, includePerfPrerelease: false }], - [{ includePrerelease: true, includePerfPrerelease: true }] - ]) - }) + it.each([ + ['Macintosh', { metaKey: true }], + ['Windows', { ctrlKey: true }] + ])('routes Check for Updates modifiers to the RC and perf-RC channels (%s)', async (ua, perfModifier) => { + vi.stubGlobal('navigator', { userAgent: ua }) + const container = await renderMenu() + const item = findMenuItem(container, 'Check for Updates') + + const clickWith = async (init: MouseEventInit): Promise<void> => { + await act(async () => { + item.dispatchEvent(new MouseEvent('click', { bubbles: true, ...init })) + }) + } + + await clickWith({}) + await clickWith(perfModifier) // modifier alone, no shift + await clickWith({ shiftKey: true }) + await clickWith({ shiftKey: true, ...perfModifier }) + + expect(mocks.updaterCheck.mock.calls).toEqual([ + [{ includePrerelease: false, includePerfPrerelease: false }], + [{ includePrerelease: false, includePerfPrerelease: false }], + [{ includePrerelease: true, includePerfPrerelease: false }], + [{ includePrerelease: true, includePerfPrerelease: true }] + ]) + vi.unstubAllGlobals() + })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 351daff2-eb5a-49ad-9f02-d9b2083f0e18
📥 Commits
Reviewing files that changed from the base of the PR and between 605f1f479bf25ba4cb997c5a4d76898bc6b87afa and 8e4cccb.
📒 Files selected for processing (15)
src/main/menu/register-app-menu.test.tssrc/main/menu/register-app-menu.tssrc/main/updater-fallback.tssrc/main/updater-prerelease-feed-readiness.test.tssrc/main/updater-prerelease-feed.test.tssrc/main/updater-prerelease-feed.tssrc/main/updater.fallback.test.tssrc/main/updater.test.tssrc/main/updater.tssrc/main/window/attach-main-window-services.tssrc/preload/api-types.tssrc/renderer/src/components/settings/GeneralUpdateSettingsSection.tsxsrc/renderer/src/components/sidebar/SidebarSettingsHelpMenu.test.tsxsrc/renderer/src/components/sidebar/SidebarSettingsHelpMenu.tsxsrc/shared/types.ts
✅ Files skipped from review due to trivial changes (1)
- src/shared/types.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/main/updater.fallback.test.ts
- src/main/menu/register-app-menu.ts
- src/main/updater-fallback.ts
- src/renderer/src/components/sidebar/SidebarSettingsHelpMenu.tsx
- src/main/updater-prerelease-feed.test.ts
- src/preload/api-types.ts
- src/main/updater-prerelease-feed-readiness.test.ts
- src/main/menu/register-app-menu.test.ts
- src/main/window/attach-main-window-services.ts
- src/main/updater-prerelease-feed.ts
- src/renderer/src/components/settings/GeneralUpdateSettingsSection.tsx
- src/main/updater.test.ts
- src/main/updater.ts
|
Closing as superseded after scope audit: although primarily release-channel feature work, it also fixed lost Shift/Cmd/Ctrl modifiers in the updater menu. Equivalent modifier capture landed via #7278, so there is no remaining bug-fix delta to rebase. |
What
Reworks the updater into a three-channel model so power users can steer which release track they're offered, keyed off modifier keys on Check for Updates.
Release tracks (classified by the new
getReleaseChannel):v1.4.122v1.4.122-rc.3v1.4.122-rc.3.perfBehavior
A running build is always offered stable + its own track. Modifier keys additively opt into higher tracks for that check:
Which matches the requested rules:
"Update to latest" then pins the generic feed to the newest eligible tag (perf-rc outranks the same-numbered plain rc by semver precedence).
How
getReleaseChannel(version)inupdater-fallback.tsclassifies a version by track (perfidentifier ⇒ perf-rc, other prerelease ⇒ rc, else stable).resolveEligibleReleaseChannels()inupdater.tscombines the running build's channel with sticky Shift / Cmd-Shift opt-ins into a canonicalstable → rc → perf-rcset.fetchNewerReleaseTagsWithReadiness) now takeseligibleChannelsand filters candidate tags by track instead of a binaryincludePrerelease.UpdateCheckOptions({ includePrerelease?, includePerfPrerelease? }) is threaded through the native menu, both Settings "Check for Updates" surfaces, and theupdater:checkIPC. Modifier detection is platform-correct (metaKeyon macOS,ctrlKeyelsewhere).Sidebar fix (from review)
The sidebar
…Help-menu "Check for Updates" item was wired to Radix'sonSelect, which delivers a syntheticCustomEventwith no modifier state — soShift/Cmd-Shiftnever registered there (the RC half was a pre-existing bug onmain). Switched it toonClick, which Radix composes with a realMouseEventbefore its own select/close, and which keyboard activation also dispatches (as a modifier-less click → plain stable check). The native menu and Settings-section button already received real events and were unaffected.Tests
getReleaseChannelunit tests + channel-filtering feed tests (rc-only hides perf, perf-only hides rc, all-tracks picks semver-newest).updater.tsintegration tests for all four modifier / current-version combinations.ShiftandCmd/Ctrl+Shiftand asserts the emittedUpdateCheckOptions.{ includePrerelease, includePerfPrerelease }shape.includePrereleasetoeligibleChannels.All updater/menu/sidebar suites (188 tests),
typecheck,oxlint, andoxfmtpass. Adversarially reviewed end-to-end against the four rules via a multi-lens workflow; it surfaced one real defect (the sidebar RadixonSelectissue above), which is fixed and covered by a test.Notes
updater.checkremains a no-op stub — the widened options type is compatible.