feat(updater): background check + telemetry funnel (#450, #451) - #737
Merged
Conversation
Add silent startup checks with 24h rate limiting, portable-only gating, update toast, and --no-update-check. Centralize privacy-safe Sentry breadcrumbs in UpdaterTelemetry and document funnel queries. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds background update checking with rate-limiting and session-disable gating ( ChangesBackground Update Check, Toast UI, and Telemetry Funnel
Sequence Diagram(s)sequenceDiagram
participant main.cpp
participant MainWindow
participant UpdaterController
participant UpdaterTelemetry
participant SentryReporter
participant UpdateToast
main.cpp->>UpdaterController: setSessionBackgroundChecksDisabled(true) [if --no-update-check]
UpdaterTelemetry->>SentryReporter: breadcrumb("updater.background.skip", ...)
MainWindow->>UpdaterController: checkForUpdatesInBackground() [after 5s QTimer]
UpdaterController->>UpdaterController: shouldRunBackgroundCheck() [rate-limit / session / flavor checks]
UpdaterController->>UpdaterTelemetry: breadcrumb("updater.check.background.start", ...)
UpdaterTelemetry->>SentryReporter: isEnabled()?
SentryReporter-->>UpdaterTelemetry: true/false
UpdaterTelemetry->>SentryReporter: addBreadcrumb(...) [if enabled and message allowed]
UpdaterController->>UpdaterController: checkForUpdates() [m_silentCheck=true]
UpdaterController-->>MainWindow: backgroundUpdateAvailable(version)
MainWindow->>UpdateToast: showForVersion(version) [via QQmlApplicationEngine]
UpdateToast->>UpdateToast: reposition() [bottom-right screen]
UpdateToast->>UpdaterController: openUpdateDialog() [on "View update" click]
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 docstrings
🧪 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: 2f4be09f10
ℹ️ 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".
| border.color: PropertiesPanelController.borderColor | ||
| border.width: 1 | ||
|
|
||
| RowLayout { |
There was a problem hiding this comment.
When a background check finds an update, showUpdateToast loads this QML file, but RowLayout and the attached Layout.* properties are defined by QtQuick.Layouts, which is not imported here. In that path the QML engine fails to create the toast (RowLayout is not a type), so users get no non-modal notification for available background updates.
Useful? React with 👍 / 👎.
Comment on lines
+106
to
+107
| finishSilentCheck(); | ||
| return; |
There was a problem hiding this comment.
When the 5s startup background check hits a network error, this branch only clears m_silentCheck and returns. checkForUpdates() has already set the controller state to Checking, so the updater stays stuck in Checking; a later Help > Check for Updates in the same session immediately returns on the existing Checking state and the dialog spins forever instead of retrying or reporting the error.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (1)
src/updater/UpdaterController_test.cpp (1)
85-111: ⚡ Quick winAdd a regression test for background call while controller is already in
Checking.A dedicated case here would lock in the expected behavior that silent mode is not left latched when
checkForUpdatesInBackground()is invoked during an active check.🤖 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 `@src/updater/UpdaterController_test.cpp` around lines 85 - 111, Add a new test function in the UpdaterControllerTestEnv test class following the pattern of BackgroundCheckSkippedWhenRateLimited and BackgroundCheckSkippedWhenSessionDisabled. The test should first set the controller to the Checking state, then call checkForUpdatesInBackground() and verify that the controller remains in the Checking state (similar to the existing tests using stateBefore comparison). This regression test will ensure that invoking checkForUpdatesInBackground() while an active check is in progress does not leave silent mode latched.
🤖 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 `@src/main.cpp`:
- Around line 189-195: The --no-update-check command-line argument handling in
the code block starting at line 189 calls
UpdaterController::setSessionBackgroundChecksDisabled(true) but does not emit a
breadcrumb for this significant operation. Add a call to
SentryReporter::addBreadcrumb() immediately after the
setSessionBackgroundChecksDisabled(true) call to log this action with an
appropriate category and message that documents the --no-update-check flag being
applied and background checks being disabled for the session.
In `@src/mainwindow.cpp`:
- Around line 681-687: The updater initialization code in MainWindow does not
emit breadcrumbs to track the new updater startup and toast notification flows
as required by the coding guidelines. Add SentryReporter::addBreadcrumb calls
with appropriate categories (such as "ui.action") within the lambda functions
connected to UpdaterController signals: in the lambda for
backgroundUpdateAvailable (which calls showUpdateToast), in the lambda for
checkForUpdatesAvailable (which calls showUpdaterDialog), and in the
QTimer::singleShot lambda that invokes checkForUpdatesInBackground. Each
breadcrumb should include a descriptive message indicating what updater
operation is being performed. Apply the same breadcrumb tracking to the related
updater code mentioned at lines 4524-4569.
In `@src/updater/UpdaterController.cpp`:
- Around line 430-443: The m_silentCheck flag in the checkForUpdatesInBackground
function is set to true before calling checkForUpdates, but if checkForUpdates
returns early (such as due to an already-checking guard inside that function),
the flag will remain true and can suppress or alter a foreground check that may
be running. Move the m_silentCheck assignment to immediately before the
checkForUpdates call, and ensure it is reset to false immediately after the call
completes, so the silent mode only applies to the specific background check
being initiated and does not leak into other concurrent checks. Apply the same
fix pattern to the other location mentioned at lines 485-487.
- Around line 142-149: The UpdaterTelemetry::breadcrumb call emitting
"updater.check.success" is executed unconditionally after
applyCheckResult(result), but it should only be emitted when the comparison
result is valid. Add a conditional check before calling
UpdaterTelemetry::breadcrumb to verify that result.comparison represents a valid
comparison outcome (not an error state). Only emit the success breadcrumb when
the comparison is valid, ensuring error conditions are not incorrectly reported
as successful checks.
In `@src/updater/UpdaterTelemetry.cpp`:
- Around line 8-14: The breadcrumb() function forwards all messages to
SentryReporter::addBreadcrumb() without filtering them first, violating the
privacy-safe wrapper contract. Add a check using isAllowedTelemetryMessage()
before the SentryReporter::addBreadcrumb() call to verify that the message
parameter is permitted according to the telemetry filtering rules, and only
forward to the reporter if the message passes this check.
---
Nitpick comments:
In `@src/updater/UpdaterController_test.cpp`:
- Around line 85-111: Add a new test function in the UpdaterControllerTestEnv
test class following the pattern of BackgroundCheckSkippedWhenRateLimited and
BackgroundCheckSkippedWhenSessionDisabled. The test should first set the
controller to the Checking state, then call checkForUpdatesInBackground() and
verify that the controller remains in the Checking state (similar to the
existing tests using stateBefore comparison). This regression test will ensure
that invoking checkForUpdatesInBackground() while an active check is in progress
does not leave silent mode latched.
🪄 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: fff12714-9f34-44ab-949c-f6fed4947ee4
📒 Files selected for processing (13)
docs/AUTO_UPDATER_DESIGN.mdqml/UpdateToast.qmlsrc/main.cppsrc/mainwindow.cppsrc/mainwindow.hsrc/qml_resources.qrcsrc/updater/CMakeLists.txtsrc/updater/UpdaterController.cppsrc/updater/UpdaterController.hsrc/updater/UpdaterController_test.cppsrc/updater/UpdaterTelemetry.cppsrc/updater/UpdaterTelemetry.hsrc/updater/UpdaterTelemetry_test.cpp
Comment on lines
+681
to
+687
| this, [this](bool runCheck) { showUpdaterDialog(runCheck); }); | ||
| connect(UpdaterController::instance(), &UpdaterController::backgroundUpdateAvailable, | ||
| this, [this](const QString& version) { showUpdateToast(version); }); | ||
| #ifndef QTMESH_UNIT_TESTS | ||
| if (UpdaterController::instance()->checkOnStartup()) { | ||
| QTimer::singleShot(3000, this, []() { | ||
| UpdaterController::instance()->checkForUpdates(); | ||
| }); | ||
| } | ||
| QTimer::singleShot(5000, this, []() { | ||
| UpdaterController::instance()->checkForUpdatesInBackground(); | ||
| }); |
There was a problem hiding this comment.
Track the new updater startup/toast flow with breadcrumbs.
Line 685 and Line 4524 introduce significant updater operations and user-visible behavior, but this new path does not emit breadcrumbs in MainWindow.
Suggested patch
@@
`#ifdef` ENABLE_AUTO_UPDATER
connect(UpdaterController::instance(), &UpdaterController::showDialogRequested,
this, [this](bool runCheck) { showUpdaterDialog(runCheck); });
connect(UpdaterController::instance(), &UpdaterController::backgroundUpdateAvailable,
this, [this](const QString& version) { showUpdateToast(version); });
`#ifndef` QTMESH_UNIT_TESTS
QTimer::singleShot(5000, this, []() {
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("Updater: startup background check triggered"));
UpdaterController::instance()->checkForUpdatesInBackground();
});
`#endif`
`#endif`
@@
void MainWindow::showUpdateToast(const QString& version)
{
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("Updater toast requested"));
if (m_updateToastEngine) {
if (auto* toast = m_updateToastWindow) {
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("Updater toast shown"));
QMetaObject::invokeMethod(toast, "showForVersion", Q_ARG(QVariant, version));
}
return;
}
@@
[this, version](QObject* obj, const QUrl&) {
if (!obj) {
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("Updater toast: QML load failed"));
m_updateToastEngine->deleteLater();
m_updateToastEngine = nullptr;
return;
}As per coding guidelines, “All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message) using categories like ui.action…”.
Also applies to: 4524-4569
🤖 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 `@src/mainwindow.cpp` around lines 681 - 687, The updater initialization code
in MainWindow does not emit breadcrumbs to track the new updater startup and
toast notification flows as required by the coding guidelines. Add
SentryReporter::addBreadcrumb calls with appropriate categories (such as
"ui.action") within the lambda functions connected to UpdaterController signals:
in the lambda for backgroundUpdateAvailable (which calls showUpdateToast), in
the lambda for checkForUpdatesAvailable (which calls showUpdaterDialog), and in
the QTimer::singleShot lambda that invokes checkForUpdatesInBackground. Each
breadcrumb should include a descriptive message indicating what updater
operation is being performed. Apply the same breadcrumb tracking to the related
updater code mentioned at lines 4524-4569.
Source: Coding guidelines
Add QtQuick.Layouts import for the toast, reset Idle state after silent check failures, clear m_silentCheck on early returns, filter telemetry payloads in UpdaterTelemetry, and skip check.success on invalid compare. Limit build-wrapper to test targets and raise unit-tests-linux cap to 90m. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/deploy.yml:
- Around line 1048-1055: The build-wrapper make command explicitly includes
qtmesh_ps1core_libretro as a hard target, but this target is optional in CMake
and may not be generated depending on configuration. This causes the CI job to
fail before tests can run if the target doesn't exist. Remove the
qtmesh_ps1core_libretro line from the explicit make targets list since UnitTests
already depends on it when the target is available, making the explicit
inclusion unnecessary and preventing CI failures when the target is not
generated.
🪄 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: 6162c11e-cad7-4e1e-aa01-e7a986e78ca8
📒 Files selected for processing (7)
.github/workflows/deploy.ymlqml/UpdateToast.qmlsrc/main.cppsrc/mainwindow.cppsrc/updater/UpdaterController.cppsrc/updater/UpdaterTelemetry.cppsrc/updater/UpdaterTelemetry_test.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/updater/UpdaterTelemetry_test.cpp
- src/main.cpp
- src/updater/UpdaterTelemetry.cpp
- src/mainwindow.cpp
- src/updater/UpdaterController.cpp
Comment on lines
+1048
to
+1055
| build-wrapper-linux-x86-64 --out-dir build-wrapper-output \ | ||
| make -C build -j$(nproc) \ | ||
| UnitTests \ | ||
| qtmesh_test_common \ | ||
| qtmesh_updater \ | ||
| qtmesh_ps1core_stub \ | ||
| qtmesh_ps1core_libretro \ | ||
| MaterialEditorQML_test \ |
There was a problem hiding this comment.
Guard optional target usage in the build-wrapper make list.
Line 1054 adds qtmesh_ps1core_libretro as a hard make target, but this target is optional in CMake (if(TARGET qtmesh_ps1core_libretro)). If it is not generated, this step can fail the whole CI job before tests/sonar run. UnitTests already depends on it when available, so this explicit target should be removed (or conditionally added).
Suggested fix
build-wrapper-linux-x86-64 --out-dir build-wrapper-output \
make -C build -j$(nproc) \
UnitTests \
qtmesh_test_common \
qtmesh_updater \
qtmesh_ps1core_stub \
- qtmesh_ps1core_libretro \
MaterialEditorQML_test \
MaterialEditorQML_qml_test \
MaterialEditorQML_perf_test \🤖 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 @.github/workflows/deploy.yml around lines 1048 - 1055, The build-wrapper
make command explicitly includes qtmesh_ps1core_libretro as a hard target, but
this target is optional in CMake and may not be generated depending on
configuration. This causes the CI job to fail before tests can run if the target
doesn't exist. Remove the qtmesh_ps1core_libretro line from the explicit make
targets list since UnitTests already depends on it when the target is available,
making the explicit inclusion unnecessary and preventing CI failures when the
target is not generated.
|
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
--no-update-checksession opt-out, no UI on up-to-date/errors.UpdaterTelemetryhelper for privacy-safeupdater.*Sentry breadcrumbs and documents funnel queries indocs/AUTO_UPDATER_DESIGN.md.Test plan
./build_local/bin/UnitTests --gtest_filter="Updater*"(15/15 pass)unit-tests-linux--no-update-checksuppresses checkEpic note
Epic #439 MVP still needs package-build
ENABLE_AUTO_UPDATER=OFFwiring and any remaining #449 UX polish before calling the auto-updater fully shipped.Made with Cursor
Summary by CodeRabbit
New Features
--no-update-checkto disable background update checking for the session.Improvements
Documentation
Tests / Chores