Add updater dialog and preferences (#443, #449)#731
Conversation
Replace the QMessageBox update flow with a QML dialog that handles install-flavor states, package-manager guidance, and skip/remind actions. Add an Updates preferences tab with channel and startup settings, guarded by ENABLE_AUTO_UPDATER for package-managed builds. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
More reviews will be available in 44 minutes and 1 second. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a fully gated in-app auto-updater feature behind a new ChangesIn-app Auto-updater
Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant QQmlEngine as QQmlApplicationEngine
participant UpdaterDialog
participant UpdaterController
User->>MainWindow: click "Check for Updates..."
MainWindow->>MainWindow: showUpdaterDialog(true)
alt m_updaterWindow exists
MainWindow->>UpdaterDialog: invokeMethod("open", runCheck=true)
else first open
MainWindow->>QQmlEngine: new engine + register singletons
QQmlEngine->>UpdaterDialog: load qrc:/UpdaterDialog/UpdaterDialog.qml
MainWindow->>UpdaterDialog: force software rendering
MainWindow->>UpdaterDialog: open(runCheck=true)
end
UpdaterDialog->>UpdaterController: checkForUpdates()
UpdaterController->>UpdaterController: setState(Checking) + breadcrumb
UpdaterController-->>UpdaterDialog: state = Checking → BusyIndicator
UpdaterController->>UpdaterController: worker completes
alt update available
UpdaterController-->>UpdaterDialog: state = UpdateAvailable
UpdaterDialog->>User: show changelog + actions
User->>UpdaterDialog: "Download & Install" (wired but disabled)
else already latest
UpdaterController-->>UpdaterDialog: state = UpToDate
User->>UpdaterDialog: close
else package managed
UpdaterController-->>UpdaterDialog: state = PackageManaged
UpdaterDialog->>User: show update command + copy
else unknown install
UpdaterController-->>UpdaterDialog: state = UnknownInstall
User->>UpdaterDialog: "Continue" or "Cancel"
else error
UpdaterController-->>UpdaterDialog: state = Error
UpdaterDialog->>User: show error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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. ✨ 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 |
![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: 6fbfe637bd
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@qml/UpdaterDialog.qml`:
- Around line 36-42: The Keys.onPressed handler calls
UpdaterController.dismiss() which only removes UI state without canceling active
update operations like Checking, Downloading, or Verifying. This leaves
background work running and causes issues when the dialog is reopened. Replace
or augment the UpdaterController.dismiss() call with the appropriate method that
actively cancels ongoing update work (such as a cancel() or abort() method) to
ensure any active requests are properly stopped when the Escape key is pressed.
In `@qml/UpdaterSettingsPanel.qml`:
- Around line 42-140: Replace the custom mouse-only controls throughout
UpdaterSettingsPanel.qml with proper Qt Quick Controls that support keyboard
accessibility. Specifically: convert the channel selection section (the Repeater
with Rectangle and MouseArea for selecting between "stable" and "beta") to use
RadioButton components within a ButtonGroup; replace the two checkbox
implementations (the "Check for updates on startup" and "Auto-download in
background" Rows containing custom Rectangle and MouseArea) with CheckBox
controls; and replace the "Check now" button (the Rectangle with MouseArea for
triggering requestCheckDialog()) with a Button control. These Qt Quick Controls
provide built-in keyboard navigation, focus handling, and accessibility features
that the custom implementations lack.
In `@src/updater/UpdaterController_test.cpp`:
- Around line 13-26: The SetUp() method clears QSettings but leaves the
singleton alive with stale in-memory values, causing test order-dependency
issues. Before removing the settings entries in SetUp(), call the
UpdaterController singleton's kill() method on the main thread to destroy the
existing singleton instance, ensuring each test gets a fresh state and forces
the singleton to reload settings from the cleared QSettings during
initialization.
In `@src/updater/UpdaterController.cpp`:
- Around line 128-134: The loadSettings() method calls setChannel() before
loading the remaining settings (m_checkOnStartup, m_autoDownload,
m_lastCheckedAt), and since setChannel() now calls saveSettings(), it writes the
uninitialized member values back to QSettings before they are loaded,
overwriting the actual saved preferences. To fix this, either load all settings
into their respective member variables first before calling setChannel(), or
directly assign the channel value to the m_channel member variable without
calling setChannel() to avoid triggering saveSettings() prematurely.
In `@src/updater/UpdaterController.h`:
- Around line 82-83: The requestCheckDialog() method is a user-facing action
entrypoint but lacks breadcrumb logging for telemetry. Add a call to
SentryReporter::addBreadcrumb() with category "ui.action" in the implementation
of requestCheckDialog() before or alongside the existing
showDialogRequested(true) emission to ensure consistent action telemetry across
all user-facing actions, following the established coding guidelines for Sentry
breadcrumb logging.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7e67a94-94c1-49d3-b51c-7db49f972ee3
📒 Files selected for processing (13)
CMakeLists.txtqml/PreferencesDialog.qmlqml/UpdaterDialog.qmlqml/UpdaterSettingsPanel.qmlsrc/AppSettingsKeys.hsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/mainwindow.cppsrc/mainwindow.hsrc/qml_resources.qrcsrc/updater/UpdaterController.cppsrc/updater/UpdaterController.hsrc/updater/UpdaterController_test.cpp
Load all updater QSettings before persisting to avoid resetting toggles when the saved channel differs from the default. Wire check-on-startup, cancel in-flight work on Esc, use CheckBox/Button controls, reset singleton in tests, and add regression coverage for the load path. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
qml/UpdaterSettingsPanel.qml (1)
62-91: Use explicitidon CheckBox instances for consistency with codebase patterns.Lines 62 and 94 reference
control.*in custom CheckBox delegates (indicatorandcontentItem). While Qt Quick Controls 2 implicitly provides thecontrolvariable in delegate scopes, the codebase establishes an explicit pattern inThemedCheckBox.qml(which definesid: control). For consistency and clarity, add explicit ids to these CheckBox instances and update references accordingly.Suggested fix
CheckBox { + id: checkOnStartupCheckBox width: parent.width text: "Check for updates on startup" checked: UpdaterController.checkOnStartup onToggled: UpdaterController.checkOnStartup = checked font.pixelSize: 12 indicator: Rectangle { implicitWidth: 14 implicitHeight: 14 - x: control.leftPadding + x: checkOnStartupCheckBox.leftPadding y: parent.height / 2 - height / 2 radius: 2 border.color: borderColor border.width: 1 - color: control.checked ? highlightColor : "transparent" + color: checkOnStartupCheckBox.checked ? highlightColor : "transparent" Text { anchors.centerIn: parent - visible: control.checked + visible: checkOnStartupCheckBox.checked text: "\u2713" color: "white" font.pixelSize: 10 } } contentItem: Text { - text: control.text - font: control.font + text: checkOnStartupCheckBox.text + font: checkOnStartupCheckBox.font color: textColor verticalAlignment: Text.AlignVCenter - leftPadding: control.indicator.width + control.spacing + leftPadding: checkOnStartupCheckBox.indicator.width + checkOnStartupCheckBox.spacing } } CheckBox { + id: autoDownloadCheckBox width: parent.width text: "Auto-download in background (opt-in)" checked: UpdaterController.autoDownload onToggled: UpdaterController.autoDownload = checked font.pixelSize: 12 indicator: Rectangle { implicitWidth: 14 implicitHeight: 14 - x: control.leftPadding + x: autoDownloadCheckBox.leftPadding y: parent.height / 2 - height / 2 radius: 2 border.color: borderColor border.width: 1 - color: control.checked ? highlightColor : "transparent" + color: autoDownloadCheckBox.checked ? highlightColor : "transparent" Text { anchors.centerIn: parent - visible: control.checked + visible: autoDownloadCheckBox.checked text: "\u2713" color: "white" font.pixelSize: 10 } } contentItem: Text { - text: control.text - font: control.font + text: autoDownloadCheckBox.text + font: autoDownloadCheckBox.font color: textColor verticalAlignment: Text.AlignVCenter - leftPadding: control.indicator.width + control.spacing + leftPadding: autoDownloadCheckBox.indicator.width + autoDownloadCheckBox.spacing } }🤖 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 `@qml/UpdaterSettingsPanel.qml` around lines 62 - 91, The CheckBox instances on lines 62 and 94 reference `control.*` properties in their indicator and contentItem delegates using the implicit control variable, but lack an explicit `id: control` definition. To maintain consistency with the codebase pattern established in ThemedCheckBox.qml, add an explicit `id: control` property to each CheckBox element. This makes the pattern explicit and clear while all existing references to control properties will continue to work correctly.
🤖 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.
Nitpick comments:
In `@qml/UpdaterSettingsPanel.qml`:
- Around line 62-91: The CheckBox instances on lines 62 and 94 reference
`control.*` properties in their indicator and contentItem delegates using the
implicit control variable, but lack an explicit `id: control` definition. To
maintain consistency with the codebase pattern established in
ThemedCheckBox.qml, add an explicit `id: control` property to each CheckBox
element. This makes the pattern explicit and clear while all existing references
to control properties will continue to work correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03214ec1-4dc0-4478-86e0-8de8af74782e
📒 Files selected for processing (5)
qml/UpdaterDialog.qmlqml/UpdaterSettingsPanel.qmlsrc/mainwindow.cppsrc/updater/UpdaterController.cppsrc/updater/UpdaterController_test.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/updater/UpdaterController_test.cpp
- qml/UpdaterDialog.qml
- src/mainwindow.cpp
MainWindow tests construct many instances; firing GitHub update checks from each one added network load and contributed to the 45-minute unit-tests-linux timeout. Also name CheckBox ids explicitly per review. Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
Summary
UpdaterDialog.qmlwith state-driven UX for unknown install confirmation, package-manager guidance, update available (release notes + actions), up-to-date, and error states — replacing the oldQMessageBoxflow.UpdaterSettingsPanel.qml) with stable/beta channel, check-on-startup, auto-download opt-in, and Check now (opens the dialog).ENABLE_AUTO_UPDATER(default ON) to omit in-app updater UI from package-managed builds; wires Help → Check for Updates… throughMainWindow::showUpdaterDialog().Test plan
./build_local/bin/UnitTests --gtest_filter="UpdaterController*:GitHubReleaseParser*:UpdateVersion*"(21 tests pass)Closes #443, #449
Made with Cursor
Summary by CodeRabbit