Add updater download and verification (#444, #445)#733
Conversation
Wire downloadAndInstall through ArtifactResolver, a resumable worker-thread downloader with retry/cancel, and SHA-256 + minisign verification before ReadyToInstall. Enables the dialog download button and auto-download path. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughImplements the full auto-updater download-verify-install pipeline: platform-aware artifact resolution via ChangesAuto-updater download, verify, and install pipeline
Sequence Diagram(s)sequenceDiagram
participant User
participant UpdaterDialog as UpdaterDialog (QML)
participant Controller as UpdaterController
participant Worker as UpdaterWorker
participant Network as QNetworkAccessManager
participant Verifier as UpdateVerifier
rect rgba(100, 149, 237, 0.5)
Note over Controller,Worker: Update detected
Controller->>Controller: beginDownloadIfNeeded(false)
Controller->>Worker: downloadUpdate(DownloadRequest)
end
rect rgba(144, 238, 144, 0.5)
Note over Worker,Network: Resumable artifact download
Worker->>Network: GET artifact (Range header)
Network-->>Worker: progress / data chunks
Worker-->>Controller: downloadProgress(received, total)
Controller-->>UpdaterDialog: progressChanged
Worker->>Network: downloadUrlBlocking(signature)
Worker->>Network: downloadUrlBlocking(SHA256SUMS)
Worker-->>Controller: downloadFinished(DownloadOutcome)
end
rect rgba(255, 200, 100, 0.5)
Note over Controller,Verifier: Verification stage
Controller->>Worker: verifyDownload(VerifyRequest)
Worker->>Verifier: verifyDownloadedArtifact(artifact, sig, manifest, name)
Verifier-->>Worker: Outcome
Worker-->>Controller: verifyFinished(VerifyOutcome)
Controller->>Controller: state = ReadyToInstall
Controller-->>UpdaterDialog: stateChanged
end
User->>UpdaterDialog: sees "Ready to install" screen
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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: 143050f5ab
ℹ️ 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".
|
|
||
| const qint64 resumeOffset = reply->property("resumeOffset").toLongLong(); | ||
| const bool append = resumeOffset > 0; | ||
| const QByteArray payload = reply->readAll(); |
There was a problem hiding this comment.
Because the artifact body is read only after finished and the .part file is written only after a no-error reply, bytes received before a dropped connection are discarded; the retry Range header therefore resumes from the old on-disk size, normally 0, instead of the point of failure. Large zip/dmg/tarball assets are also buffered entirely in memory. This breaks the resumable download path for the network failures it is meant to recover; stream readyRead chunks to the .part file during the transfer.
Useful? React with 👍 / 👎.
| QByteArray("bytes=" + QByteArray::number(existingSize) + '-')); | ||
| } | ||
|
|
||
| QNetworkReply* reply = m_network->get(httpRequest); |
There was a problem hiding this comment.
downloadUrlBlocking keeps the sidecar reply in a local variable while m_activeReply is null, so pressing Escape/cancel during the .minisig or SHA256SUMS fetch calls cancelActiveRequest() but has no reply to abort. The nested event loop continues, and if the sidecar downloads finish the controller can proceed to verify/ReadyToInstall after the user cancelled or closed the dialog; store this reply or check cancellation after the blocking call so cancellation takes effect.
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: 10
🧹 Nitpick comments (1)
src/updater/UpdaterWorker.cpp (1)
110-344: ⚡ Quick winAdd breadcrumbs for download/verify I/O operations in this worker path.
This new flow introduces significant user-visible I/O stages (artifact download, sidecar download, verification) without breadcrumb instrumentation in this file. Add
SentryReporter::addBreadcrumb()at start/retry/success/failure points usingfile.importfor downloads and a consistent category for verify stage events.As per coding guidelines, “Add Sentry breadcrumbs for all user-facing actions and significant operations using SentryReporter::addBreadcrumb() with categories: 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tools, 'file.import'/'file.export' for I/O.”
🤖 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/UpdaterWorker.cpp` around lines 110 - 344, Add Sentry breadcrumbs to instrument the download and verification workflow in UpdaterWorker. Insert SentryReporter::addBreadcrumb() calls with category "file.import" at the following points: at the start of downloadUpdate() to mark the beginning of the artifact download, at the beginning of startArtifactDownloadAttempt() to track each retry attempt with the attempt number, in onDownloadReplyFinished() to record successful artifact download completion and again before downloading each sidecar file (signature and sha256Sums), and in verifyDownload() at the start and upon completion to track the verification stage. Use descriptive messages that include relevant context like attempt numbers and file names to aid debugging.Source: Coding guidelines
🤖 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 `@cmake/Libsodium.cmake`:
- Around line 16-30: The source-build fallback path in this CMake file uses a
make invocation with the -j flag that lacks a numeric argument, which is
incompatible with BSD make (the default on macOS). Locate the make command
invocation (referenced as being around line 67) in the source-build fallback
section and modify it to specify a concrete number of parallel jobs or use a
CMake-compatible approach that works across both GNU make and BSD make. Ensure
the parallel build flag is portable by providing an explicit job count or using
a CMake function that handles this portability concern.
In `@docs/AUTO_UPDATER_DESIGN.md`:
- Around line 45-47: The documentation contains contradictory statements about
platform support for MinisignVerify. Line 45 states the feature is "Linux-only"
while line 46 indicates that MinisignVerify with QTMESH_MINISIGN_VERIFY=1
supports both Linux and macOS via libsodium. Resolve this contradiction by
choosing one consistent statement across both lines. Either revise line 45 to
accurately reflect that Linux and macOS are supported, or revise line 46 to
clarify the actual platform scope. Ensure the final wording clearly states which
platforms are currently supported under QTMESH_MINISIGN_VERIFY.
In `@src/updater/ArtifactResolver.cpp`:
- Around line 79-80: The error message string at lines 79-80 in the
ArtifactResolver.cpp file currently mentions only AppImage and .tar.gz as
accepted portable Linux package formats, but the resolver pattern actually
accepts .tar.xz as well. Update the error message string to include .tar.xz in
the list of supported extensions alongside AppImage and .tar.gz to ensure users
are provided accurate information about what package formats are accepted.
- Around line 58-60: The install-flavor validation logic in ArtifactResolver.cpp
has an inverted condition that allows package-managed installs to bypass the
error check. The current condition at the if statement rejects only when the
flavor is NOT package-manager-managed, NOT portable, and NOT unknown, which
means package-managed flavors pass through without triggering the error. Since
the error message states "In-app download is only supported for portable
installs", you need to invert the logic by removing the
InstallFlavor::isPackageManagerManaged() check and keeping only the checks that
ensure the flavor is either Portable or Unknown; if it's any other flavor type
(especially package-managed), the error should be triggered.
In `@src/updater/MinisignVerify.h`:
- Around line 14-15: The documentation comment in MinisignVerify.h incorrectly
states that only Linux links libsodium when QTMESH_MINISIGN_VERIFY is set, but
this PR enables the backend on macOS as well. Update the comment at lines 14-15
to reflect that both Linux and macOS support the minisign verify backend with
libsodium, either by explicitly listing both platforms or by using
platform-agnostic language such as "supported platforms" instead of specifically
mentioning only Linux.
In `@src/updater/UpdaterController.cpp`:
- Around line 435-492: Both handleDownloadFinished and handleVerifyFinished
methods need to validate that the outcome is still relevant for the current
state before processing it. Add a check at the beginning of each method to
ensure the current state matches the expected state for that operation. In
handleDownloadFinished, verify the state is in an active download state before
processing the outcome. In handleVerifyFinished, verify the state is in the
Verifying state before processing the outcome. If the state has changed (e.g.,
due to cancellation), return early without updating the state or error message.
This prevents stale signals from resurrecting canceled operations.
- Around line 367-369: In the downloadAndInstall() method, change the breadcrumb
category passed to SentryReporter::addBreadcrumb() from "updater.download.start"
to "ui.action" on line 367, since this action is triggered by a user button
click and should follow the coding guideline of using the "ui.action" category
for toolbar and button-triggered user-facing actions.
- Around line 403-408: The QDir().mkpath(stagingRoot) call does not check its
return value, which is a boolean indicating success or failure. Capture the
return value from mkpath() and add an explicit check: if it returns false, log
an appropriate error message and return early from the function or throw an
exception to prevent the workflow from continuing with a non-existent staging
directory. This will ensure failures are caught immediately with proper error
context rather than failing later in the update process.
In `@src/updater/UpdaterWorker.cpp`:
- Around line 177-203: The downloadUrlBlocking() method creates a local
QNetworkReply pointer but never assigns it to the m_activeReply member variable,
preventing cancelActiveRequest() from actually canceling the blocking download
since it returns early when m_activeReply is null. To fix this, assign the reply
returned from m_network->get(httpRequest) to m_activeReply immediately after
creation, then clear it back to nullptr after the event loop completes (after
loop.exec()). Additionally, ensure the propagation of the cancelled state
through finishDownloadWithError() calls is consistent by passing the appropriate
cancellation flag to maintain proper controller state handling.
- Around line 256-278: The code does not handle the case where a server ignores
a Range header request during resume operations and responds with 200 OK (full
body) instead of 206 Partial Content, causing data corruption when the full
payload is appended to an existing partial file. Reorder the logic so the 416
status check (for Range Not Satisfiable) executes before the generic httpOk
error check. Additionally, add a new condition that checks if httpStatus equals
200 while append is true (indicating the server ignored the Range header), and
in that case, remove the partial file at m_downloadRequest.artifactPartPath and
restart the download attempt from the beginning using
startArtifactDownloadAttempt. This prevents blindly appending full responses to
partial files.
---
Nitpick comments:
In `@src/updater/UpdaterWorker.cpp`:
- Around line 110-344: Add Sentry breadcrumbs to instrument the download and
verification workflow in UpdaterWorker. Insert SentryReporter::addBreadcrumb()
calls with category "file.import" at the following points: at the start of
downloadUpdate() to mark the beginning of the artifact download, at the
beginning of startArtifactDownloadAttempt() to track each retry attempt with the
attempt number, in onDownloadReplyFinished() to record successful artifact
download completion and again before downloading each sidecar file (signature
and sha256Sums), and in verifyDownload() at the start and upon completion to
track the verification stage. Use descriptive messages that include relevant
context like attempt numbers and file names to aid debugging.
🪄 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: 09aa5e2f-aebf-4610-a552-90f2e88b5b27
📒 Files selected for processing (17)
cmake/Libsodium.cmakedocs/AUTO_UPDATER_DESIGN.mdqml/UpdaterDialog.qmlsrc/CMakeLists.txtsrc/updater/ArtifactResolver.cppsrc/updater/ArtifactResolver.hsrc/updater/ArtifactResolver_test.cppsrc/updater/MinisignVerify.cppsrc/updater/MinisignVerify.hsrc/updater/MinisignVerify_test.cppsrc/updater/UpdateVerifier.cppsrc/updater/UpdateVerifier.hsrc/updater/UpdateVerifier_test.cppsrc/updater/UpdaterController.cppsrc/updater/UpdaterController.hsrc/updater/UpdaterWorker.cppsrc/updater/UpdaterWorker.h
| if(UNIX AND NOT APPLE) | ||
| find_package(PkgConfig QUIET) | ||
| if(PkgConfig_FOUND) | ||
| pkg_check_modules(QTMESH_LIBSODIUM libsodium) | ||
| endif() | ||
| if(QTMESH_LIBSODIUM_FOUND) | ||
| message(STATUS "libsodium: using system package (pkg-config)") | ||
| add_library(qtmesh_sodium INTERFACE) | ||
| target_include_directories(qtmesh_sodium SYSTEM INTERFACE ${QTMESH_LIBSODIUM_INCLUDE_DIRS}) | ||
| target_link_libraries(qtmesh_sodium INTERFACE ${QTMESH_LIBSODIUM_LIBRARIES}) | ||
| target_compile_options(qtmesh_sodium INTERFACE ${QTMESH_LIBSODIUM_CFLAGS_OTHER}) | ||
| target_compile_definitions(qtmesh_sodium INTERFACE QTMESH_MINISIGN_VERIFY=1) | ||
| return() | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does BSD make (the default /usr/bin/make on macOS) accept -j without a numeric argument?
💡 Result:
Yes, BSD make (the default make on macOS) accepts the -j flag without a numeric argument, but its behavior differs significantly from GNU make. In GNU make, -j without an argument allows an unlimited number of concurrent jobs [1][2][3]. In contrast, BSD make (as implemented in macOS, FreeBSD, NetBSD, and OpenBSD) requires an argument for -j [4][5][6][7]. If you provide -j without a following number, it is generally treated as an invalid usage or can cause the command to fail, as it expects a numeric value to specify the maximum number of jobs [4][5][7]. Consequently, users on macOS must specify a number (e.g., make -j8) or use a command substitution to dynamically provide a number (e.g., make -j$(sysctl -n hw.ncpu)) [8][9]. Attempting to run it without an argument may result in errors or unexpected behavior depending on the specific shell environment and version [10][9].
Citations:
- 1: https://www.gnu.org/s/make/manual/html_node/Parallel.html
- 2: http://gnu.ist.utl.pt/software/make/manual/html_node/Options-Summary.html
- 3: https://manpages.debian.org/bookworm/make/make.1.en.html
- 4: http://man.netbsd.org/make.1
- 5: https://manned.org/man/freebsd-13.0/make.1
- 6: https://man.openbsd.org/OpenBSD-7.6/make.1
- 7: https://manned.org/man/netbsd-10.1/make.1
- 8: https://stackoverflow.com/questions/72344183/how-many-cores-can-i-use-when-using-make-j
- 9: https://blog.yossarian.net/2019/04/23/Make-is-probably-fine
- 10: Builtin make on macOS often does not work with any -j besides -j1 (Permission denied) spack/spack#26669
🏁 Script executed:
cat -n cmake/Libsodium.cmake | head -100Repository: fernandotonon/QtMeshEditor
Length of output: 4166
Use a portable parallel-build invocation in the source-build path.
Line 16 routes macOS into the source-build fallback (since the condition excludes Apple), and line 67 runs make -j. This flag form is not portable: BSD make (the default on macOS) requires a numeric argument and will fail when invoked without one, blocking builds on macOS.
Proposed fix
include(FetchContent)
+include(ProcessorCount)
+
+ProcessorCount(QTMESH_LIBSODIUM_JOBS)
+if(NOT QTMESH_LIBSODIUM_JOBS OR QTMESH_LIBSODIUM_JOBS LESS 1)
+ set(QTMESH_LIBSODIUM_JOBS 1)
+endif()
@@
execute_process(
- COMMAND make -j
+ COMMAND make -j${QTMESH_LIBSODIUM_JOBS}
WORKING_DIRECTORY "${qtmesh_libsodium_src_SOURCE_DIR}"
RESULT_VARIABLE qtmesh_sodium_build_result
)🤖 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 `@cmake/Libsodium.cmake` around lines 16 - 30, The source-build fallback path
in this CMake file uses a make invocation with the -j flag that lacks a numeric
argument, which is incompatible with BSD make (the default on macOS). Locate the
make command invocation (referenced as being around line 67) in the source-build
fallback section and modify it to specify a concrete number of parallel jobs or
use a CMake-compatible approach that works across both GNU make and BSD make.
Ensure the parallel build flag is portable by providing an explicit job count or
using a CMake function that handles this portability concern.
| - Linux-only in this spike; Windows/macOS return `Unsupported` until #445 wires cross-platform static libsodium. | ||
| - **#445:** `MinisignVerify` builds wherever `QTMESH_MINISIGN_VERIFY=1` (Linux + macOS via libsodium). Windows MinGW verify is deferred — downloads fail closed at the signature step until a follow-up wires libsodium there. | ||
| - Tests: `MinisignVerify_test` verifies good/tampered/bad-key against `tests/fixtures/updater/release-3.5.3-readme.md` (content from GitHub tag `3.5.3`). |
There was a problem hiding this comment.
Resolve contradictory platform-support wording in PoC status.
Line 45 says “Linux-only,” while Line 46 says QTMESH_MINISIGN_VERIFY=1 supports Linux + macOS. Please keep one consistent statement.
🤖 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 `@docs/AUTO_UPDATER_DESIGN.md` around lines 45 - 47, The documentation contains
contradictory statements about platform support for MinisignVerify. Line 45
states the feature is "Linux-only" while line 46 indicates that MinisignVerify
with QTMESH_MINISIGN_VERIFY=1 supports both Linux and macOS via libsodium.
Resolve this contradiction by choosing one consistent statement across both
lines. Either revise line 45 to accurately reflect that Linux and macOS are
supported, or revise line 46 to clarify the actual platform scope. Ensure the
final wording clearly states which platforms are currently supported under
QTMESH_MINISIGN_VERIFY.
| if (!InstallFlavor::isPackageManagerManaged(flavor) && flavor != InstallFlavor::Flavor::Portable | ||
| && flavor != InstallFlavor::Flavor::Unknown) { | ||
| result.errorMessage = QStringLiteral("In-app download is only supported for portable installs"); |
There was a problem hiding this comment.
Invert the install-flavor gate; package-managed installs are currently allowed through.
The current predicate rejects only non-package-managed/non-portable flavors, which means package-managed flavors pass validation and can reach download resolution. That contradicts the “portable-only” behavior and can trigger incorrect updater flows.
Suggested fix
- if (!InstallFlavor::isPackageManagerManaged(flavor) && flavor != InstallFlavor::Flavor::Portable
- && flavor != InstallFlavor::Flavor::Unknown) {
+ if (InstallFlavor::isPackageManagerManaged(flavor)
+ || (flavor != InstallFlavor::Flavor::Portable
+ && flavor != InstallFlavor::Flavor::Unknown)) {
result.errorMessage = QStringLiteral("In-app download is only supported for portable installs");
return result;
}🤖 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/ArtifactResolver.cpp` around lines 58 - 60, The install-flavor
validation logic in ArtifactResolver.cpp has an inverted condition that allows
package-managed installs to bypass the error check. The current condition at the
if statement rejects only when the flavor is NOT package-manager-managed, NOT
portable, and NOT unknown, which means package-managed flavors pass through
without triggering the error. Since the error message states "In-app download is
only supported for portable installs", you need to invert the logic by removing
the InstallFlavor::isPackageManagerManaged() check and keeping only the checks
that ensure the flavor is either Portable or Unknown; if it's any other flavor
type (especially package-managed), the error should be triggered.
| QStringLiteral("No portable Linux update package found in this release " | ||
| "(expected AppImage or .tar.gz; .deb installs use apt)"); |
There was a problem hiding this comment.
Keep Linux guidance text aligned with accepted extensions.
The resolver pattern accepts .tar.xz, but the error string mentions only AppImage and .tar.gz. This can mislead users when a valid .tar.xz artifact is expected.
Suggested fix
- QStringLiteral("No portable Linux update package found in this release "
- "(expected AppImage or .tar.gz; .deb installs use apt)");
+ QStringLiteral("No portable Linux update package found in this release "
+ "(expected AppImage, .tar.gz, or .tar.xz; .deb installs use apt)");🤖 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/ArtifactResolver.cpp` around lines 79 - 80, The error message
string at lines 79-80 in the ArtifactResolver.cpp file currently mentions only
AppImage and .tar.gz as accepted portable Linux package formats, but the
resolver pattern actually accepts .tar.xz as well. Update the error message
string to include .tar.xz in the list of supported extensions alongside AppImage
and .tar.gz to ensure users are provided accurate information about what package
formats are accepted.
| * Linux builds link libsodium for verify when @c QTMESH_MINISIGN_VERIFY is set. | ||
| * Builds without the backend return @ref Result::Unsupported. |
There was a problem hiding this comment.
Update the platform note to include macOS backend support.
The comment says only Linux links libsodium when QTMESH_MINISIGN_VERIFY is set, but this PR’s platform gating enables that backend on macOS too. Please make the doc platform-agnostic or list both platforms.
Proposed fix
- * Linux builds link libsodium for verify when `@c` QTMESH_MINISIGN_VERIFY is set.
+ * Builds with `@c` QTMESH_MINISIGN_VERIFY set link libsodium for verify
+ * (currently Linux and macOS).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Linux builds link libsodium for verify when @c QTMESH_MINISIGN_VERIFY is set. | |
| * Builds without the backend return @ref Result::Unsupported. | |
| * Builds with `@c` QTMESH_MINISIGN_VERIFY set link libsodium for verify | |
| * (currently Linux and macOS). | |
| * Builds without the backend return `@ref` Result::Unsupported. |
🤖 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/MinisignVerify.h` around lines 14 - 15, The documentation comment
in MinisignVerify.h incorrectly states that only Linux links libsodium when
QTMESH_MINISIGN_VERIFY is set, but this PR enables the backend on macOS as well.
Update the comment at lines 14-15 to reflect that both Linux and macOS support
the minisign verify backend with libsodium, either by explicitly listing both
platforms or by using platform-agnostic language such as "supported platforms"
instead of specifically mentioning only Linux.
| SentryReporter::addBreadcrumb(QStringLiteral("updater.download.start"), | ||
| QStringLiteral("version=%1").arg(m_latestVersion)); | ||
| beginDownloadIfNeeded(true); |
There was a problem hiding this comment.
Use ui.action breadcrumb category for the button-triggered download action.
downloadAndInstall() is triggered by a user button click, but Line 367 logs under updater.download.start instead of ui.action.
Suggested fix
void UpdaterController::downloadAndInstall()
{
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("Updater dialog: Download & install"));
SentryReporter::addBreadcrumb(QStringLiteral("updater.download.start"),
QStringLiteral("version=%1").arg(m_latestVersion));
beginDownloadIfNeeded(true);
}As per coding guidelines: "Add Sentry breadcrumbs for all user-facing actions and significant operations using SentryReporter::addBreadcrumb() with categories: 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tools, 'file.import'/'file.export' for I/O".
🤖 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.cpp` around lines 367 - 369, In the
downloadAndInstall() method, change the breadcrumb category passed to
SentryReporter::addBreadcrumb() from "updater.download.start" to "ui.action" on
line 367, since this action is triggered by a user button click and should
follow the coding guideline of using the "ui.action" category for toolbar and
button-triggered user-facing actions.
Source: Coding guidelines
| const QString stagingRoot = | ||
| QStandardPaths::writableLocation(QStandardPaths::AppDataLocation) | ||
| + QStringLiteral("/updater/staging/") | ||
| + m_latestVersion; | ||
| QDir().mkpath(stagingRoot); | ||
|
|
There was a problem hiding this comment.
Handle staging directory creation failure explicitly.
QDir().mkpath(stagingRoot) on Line 407 is unchecked. If it fails, the workflow continues and fails later with a less precise error path.
Suggested fix
- QDir().mkpath(stagingRoot);
+ if (!QDir().mkpath(stagingRoot)) {
+ setError(tr("Could not create updater staging directory: %1").arg(stagingRoot));
+ setState(State::Error);
+ SentryReporter::addBreadcrumb(QStringLiteral("updater.download.error"),
+ QStringLiteral("mkpath failed: %1").arg(stagingRoot),
+ QStringLiteral("error"));
+ logDialogStateBreadcrumb();
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const QString stagingRoot = | |
| QStandardPaths::writableLocation(QStandardPaths::AppDataLocation) | |
| + QStringLiteral("/updater/staging/") | |
| + m_latestVersion; | |
| QDir().mkpath(stagingRoot); | |
| const QString stagingRoot = | |
| QStandardPaths::writableLocation(QStandardPaths::AppDataLocation) | |
| QStringLiteral("/updater/staging/") | |
| m_latestVersion; | |
| if (!QDir().mkpath(stagingRoot)) { | |
| setError(tr("Could not create updater staging directory: %1").arg(stagingRoot)); | |
| setState(State::Error); | |
| SentryReporter::addBreadcrumb(QStringLiteral("updater.download.error"), | |
| QStringLiteral("mkpath failed: %1").arg(stagingRoot), | |
| QStringLiteral("error")); | |
| logDialogStateBreadcrumb(); | |
| return; | |
| } |
🤖 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.cpp` around lines 403 - 408, The
QDir().mkpath(stagingRoot) call does not check its return value, which is a
boolean indicating success or failure. Capture the return value from mkpath()
and add an explicit check: if it returns false, log an appropriate error message
and return early from the function or throw an exception to prevent the workflow
from continuing with a non-existent staging directory. This will ensure failures
are caught immediately with proper error context rather than failing later in
the update process.
| void UpdaterController::handleDownloadFinished(const DownloadOutcome& outcome) | ||
| { | ||
| if (outcome.cancelled) { | ||
| SentryReporter::addBreadcrumb(QStringLiteral("updater.download.cancel"), | ||
| outcome.fileName); | ||
| setState(State::UpdateAvailable); | ||
| setProgressPercent(0); | ||
| logDialogStateBreadcrumb(); | ||
| return; | ||
| } | ||
|
|
||
| if (!outcome.ok) { | ||
| SentryReporter::addBreadcrumb(QStringLiteral("updater.download.error"), | ||
| outcome.errorMessage, | ||
| QStringLiteral("error")); | ||
| setError(outcome.errorMessage); | ||
| setState(State::Error); | ||
| logDialogStateBreadcrumb(); | ||
| return; | ||
| } | ||
|
|
||
| SentryReporter::addBreadcrumb(QStringLiteral("updater.download.complete"), | ||
| outcome.fileName); | ||
| setProgressPercent(100); | ||
| setState(State::Verifying); | ||
| logDialogStateBreadcrumb(); | ||
|
|
||
| VerifyRequest verifyRequest; | ||
| verifyRequest.artifactPath = outcome.artifactPath; | ||
| verifyRequest.signaturePath = outcome.signaturePath; | ||
| verifyRequest.sha256SumsPath = outcome.sha256SumsPath; | ||
| verifyRequest.fileName = outcome.fileName; | ||
|
|
||
| QMetaObject::invokeMethod(m_worker, | ||
| "verifyDownload", | ||
| Qt::QueuedConnection, | ||
| Q_ARG(VerifyRequest, verifyRequest)); | ||
| } | ||
|
|
||
| void UpdaterController::handleVerifyFinished(const VerifyOutcome& outcome) | ||
| { | ||
| if (!outcome.ok) { | ||
| SentryReporter::addBreadcrumb( | ||
| QStringLiteral("updater.verify.failure"), | ||
| QStringLiteral("%1: %2").arg(outcome.failedStage, outcome.errorMessage), | ||
| QStringLiteral("error")); | ||
| setError(outcome.errorMessage.isEmpty() | ||
| ? tr("Download verification failed.") | ||
| : outcome.errorMessage); | ||
| setState(State::Error); | ||
| logDialogStateBreadcrumb(); | ||
| return; | ||
| } | ||
|
|
||
| SentryReporter::addBreadcrumb(QStringLiteral("updater.verify.success"), m_latestVersion); | ||
| setState(State::ReadyToInstall); | ||
| logDialogStateBreadcrumb(); | ||
| } |
There was a problem hiding this comment.
Ignore stale worker outcomes after cancellation/state change.
After cancel() (Line 510) moves state away from active jobs, late downloadFinished/verifyFinished signals can still transition to Verifying, ReadyToInstall, or Error. This causes canceled flows to “resurrect.”
Suggested fix
void UpdaterController::handleDownloadFinished(const DownloadOutcome& outcome)
{
+ if (m_state != State::Downloading) {
+ return;
+ }
+
if (outcome.cancelled) {
SentryReporter::addBreadcrumb(QStringLiteral("updater.download.cancel"),
outcome.fileName);
@@
void UpdaterController::handleVerifyFinished(const VerifyOutcome& outcome)
{
+ if (m_state != State::Verifying) {
+ return;
+ }
+
if (!outcome.ok) {Also applies to: 504-519
🤖 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.cpp` around lines 435 - 492, Both
handleDownloadFinished and handleVerifyFinished methods need to validate that
the outcome is still relevant for the current state before processing it. Add a
check at the beginning of each method to ensure the current state matches the
expected state for that operation. In handleDownloadFinished, verify the state
is in an active download state before processing the outcome. In
handleVerifyFinished, verify the state is in the Verifying state before
processing the outcome. If the state has changed (e.g., due to cancellation),
return early without updating the state or error message. This prevents stale
signals from resurrecting canceled operations.
| bool UpdaterWorker::downloadUrlBlocking(const QString& url, | ||
| const QString& destPath, | ||
| bool resume, | ||
| QString* errorMessage) | ||
| { | ||
| qint64 existingSize = 0; | ||
| if (resume && QFile::exists(destPath)) { | ||
| existingSize = QFileInfo(destPath).size(); | ||
| } | ||
|
|
||
| QNetworkRequest httpRequest = makeRequest(QUrl(url)); | ||
| if (existingSize > 0) { | ||
| httpRequest.setRawHeader("Range", | ||
| QByteArray("bytes=" + QByteArray::number(existingSize) + '-')); | ||
| } | ||
|
|
||
| QNetworkReply* reply = m_network->get(httpRequest); | ||
| QEventLoop loop; | ||
| connect(reply, &QNetworkReply::finished, &loop, &QEventLoop::quit); | ||
| loop.exec(); | ||
|
|
||
| const bool wasCancelled = reply->error() == QNetworkReply::OperationCanceledError; | ||
| const bool ok = reply->error() == QNetworkReply::NoError; | ||
| const QByteArray payload = reply->readAll(); | ||
| const QString errorString = reply->errorString(); | ||
| reply->deleteLater(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the UpdaterWorker files
fd -t f "UpdaterWorker" --extension cpp --extension hRepository: fernandotonon/QtMeshEditor
Length of output: 128
🏁 Script executed:
# Read the specific lines mentioned in the review comment
cat -n src/updater/UpdaterWorker.cpp | sed -n '170,230p'Repository: fernandotonon/QtMeshEditor
Length of output: 2419
🏁 Script executed:
# Check what's at lines 304-318 (also applies to)
cat -n src/updater/UpdaterWorker.cpp | sed -n '300,325p'Repository: fernandotonon/QtMeshEditor
Length of output: 1120
🏁 Script executed:
# Search for cancelActiveRequest implementation
rg -n "cancelActiveRequest" src/updater/UpdaterWorker.cpp -A 10Repository: fernandotonon/QtMeshEditor
Length of output: 1202
🏁 Script executed:
# Search for m_activeReply usage
rg -n "m_activeReply" src/updater/UpdaterWorker.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1144
🏁 Script executed:
# Search for finishDownloadWithError
rg -n "finishDownloadWithError" src/updater/UpdaterWorker.cppRepository: fernandotonon/QtMeshEditor
Length of output: 688
Cancellation does not reliably interrupt blocking sidecar downloads.
downloadUrlBlocking() uses a local reply and event loop but never assigns that reply to m_activeReply. During sidecar fetches, cancelActiveRequest() becomes a no-op because it checks if (!m_activeReply) and returns early, so cancellation cannot abort the blocking network request.
Suggested fix
bool UpdaterWorker::downloadUrlBlocking(const QString& url,
const QString& destPath,
bool resume,
QString* errorMessage)
{
qint64 existingSize = 0;
if (resume && QFile::exists(destPath)) {
existingSize = QFileInfo(destPath).size();
}
QNetworkRequest httpRequest = makeRequest(QUrl(url));
if (existingSize > 0) {
httpRequest.setRawHeader("Range",
QByteArray("bytes=" + QByteArray::number(existingSize) + '-'));
}
QNetworkReply* reply = m_network->get(httpRequest);
+ m_activeReply = reply;
QEventLoop loop;
connect(reply, &QNetworkReply::finished, &loop, &QEventLoop::quit);
loop.exec();
+ if (m_activeReply == reply) {
+ m_activeReply = nullptr;
+ }
const bool wasCancelled = reply->error() == QNetworkReply::OperationCanceledError;Also propagate the cancelled state through finishDownloadWithError(sidecarError, true) at lines 308 and 317 so that cancellation is properly tracked during sidecar download failures, keeping controller state handling consistent.
🤖 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/UpdaterWorker.cpp` around lines 177 - 203, The
downloadUrlBlocking() method creates a local QNetworkReply pointer but never
assigns it to the m_activeReply member variable, preventing
cancelActiveRequest() from actually canceling the blocking download since it
returns early when m_activeReply is null. To fix this, assign the reply returned
from m_network->get(httpRequest) to m_activeReply immediately after creation,
then clear it back to nullptr after the event loop completes (after
loop.exec()). Additionally, ensure the propagation of the cancelled state
through finishDownloadWithError() calls is consistent by passing the appropriate
cancellation flag to maintain proper controller state handling.
| const qint64 resumeOffset = reply->property("resumeOffset").toLongLong(); | ||
| const bool append = resumeOffset > 0; | ||
| const QByteArray payload = reply->readAll(); | ||
| const bool httpOk = reply->error() == QNetworkReply::NoError; | ||
| const int httpStatus = | ||
| reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); | ||
| reply->deleteLater(); | ||
|
|
||
| if (!httpOk) { | ||
| if (m_downloadAttempt + 1 < kMaxDownloadAttempts) { | ||
| startArtifactDownloadAttempt(m_downloadAttempt + 1); | ||
| return; | ||
| } | ||
| finishDownloadWithError(QStringLiteral("Download failed after %1 attempts") | ||
| .arg(kMaxDownloadAttempts)); | ||
| return; | ||
| } | ||
|
|
||
| if (httpStatus == 416 && append) { | ||
| QFile::remove(m_downloadRequest.artifactPartPath); | ||
| startArtifactDownloadAttempt(0); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "UpdaterWorker.cpp" -type fRepository: fernandotonon/QtMeshEditor
Length of output: 102
🏁 Script executed:
cat -n src/updater/UpdaterWorker.cpp | sed -n '250,300p'Repository: fernandotonon/QtMeshEditor
Length of output: 2303
🏁 Script executed:
cat -n src/updater/UpdaterWorker.cpp | grep -A 30 "startArtifactDownloadAttempt"Repository: fernandotonon/QtMeshEditor
Length of output: 3209
🏁 Script executed:
sed -n '1,256p' src/updater/UpdaterWorker.cpp | tail -100Repository: fernandotonon/QtMeshEditor
Length of output: 3162
🏁 Script executed:
rg -i "206|416|range|partial.*content" src/updater/ --type cppRepository: fernandotonon/QtMeshEditor
Length of output: 283
🏁 Script executed:
rg "QNetworkReply" src/updater/UpdaterWorker.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 1043
🏁 Script executed:
cat -n src/updater/UpdaterWorker.cpp | sed -n '200,230p'Repository: fernandotonon/QtMeshEditor
Length of output: 1090
🏁 Script executed:
rg "HttpStatusCodeAttribute|httpStatus" src/updater/UpdaterWorker.cpp -B 2 -A 5Repository: fernandotonon/QtMeshEditor
Length of output: 631
🏁 Script executed:
cat -n src/updater/UpdaterWorker.cpp | sed -n '218,230p'Repository: fernandotonon/QtMeshEditor
Length of output: 554
Check for server responses that ignore Range header and prevent data corruption.
When resuming from a partial .part file with a Range header, if the server responds with 200 OK (ignoring the Range request) instead of 206 Partial Content, the payload contains the full body. The current code blindly appends this full content to the existing .part file, duplicating all bytes and corrupting the download. Additionally, the 416 recovery check should be processed before generic error handling to ensure it's never bypassed. Reorder the 416 check before the error handler and add explicit detection for 200 OK responses when resuming to reset the download:
Suggested fix
- const qint64 resumeOffset = reply->property("resumeOffset").toLongLong();
- const bool append = resumeOffset > 0;
+ const qint64 resumeOffset = reply->property("resumeOffset").toLongLong();
+ bool append = resumeOffset > 0;
@@
- if (!httpOk) {
+ if (httpStatus == 416 && append) {
+ QFile::remove(m_downloadRequest.artifactPartPath);
+ startArtifactDownloadAttempt(0);
+ return;
+ }
+
+ if (!httpOk) {
if (m_downloadAttempt + 1 < kMaxDownloadAttempts) {
startArtifactDownloadAttempt(m_downloadAttempt + 1);
return;
}
finishDownloadWithError(QStringLiteral("Download failed after %1 attempts")
.arg(kMaxDownloadAttempts));
return;
}
- if (httpStatus == 416 && append) {
- QFile::remove(m_downloadRequest.artifactPartPath);
- startArtifactDownloadAttempt(0);
- return;
+ if (append && httpStatus == 200) {
+ // Server ignored Range; restart from scratch to avoid duplicate payload.
+ QFile::remove(m_downloadRequest.artifactPartPath);
+ append = false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const qint64 resumeOffset = reply->property("resumeOffset").toLongLong(); | |
| const bool append = resumeOffset > 0; | |
| const QByteArray payload = reply->readAll(); | |
| const bool httpOk = reply->error() == QNetworkReply::NoError; | |
| const int httpStatus = | |
| reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); | |
| reply->deleteLater(); | |
| if (!httpOk) { | |
| if (m_downloadAttempt + 1 < kMaxDownloadAttempts) { | |
| startArtifactDownloadAttempt(m_downloadAttempt + 1); | |
| return; | |
| } | |
| finishDownloadWithError(QStringLiteral("Download failed after %1 attempts") | |
| .arg(kMaxDownloadAttempts)); | |
| return; | |
| } | |
| if (httpStatus == 416 && append) { | |
| QFile::remove(m_downloadRequest.artifactPartPath); | |
| startArtifactDownloadAttempt(0); | |
| return; | |
| } | |
| const qint64 resumeOffset = reply->property("resumeOffset").toLongLong(); | |
| bool append = resumeOffset > 0; | |
| const QByteArray payload = reply->readAll(); | |
| const bool httpOk = reply->error() == QNetworkReply::NoError; | |
| const int httpStatus = | |
| reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); | |
| reply->deleteLater(); | |
| if (httpStatus == 416 && append) { | |
| QFile::remove(m_downloadRequest.artifactPartPath); | |
| startArtifactDownloadAttempt(0); | |
| return; | |
| } | |
| if (!httpOk) { | |
| if (m_downloadAttempt + 1 < kMaxDownloadAttempts) { | |
| startArtifactDownloadAttempt(m_downloadAttempt + 1); | |
| return; | |
| } | |
| finishDownloadWithError(QStringLiteral("Download failed after %1 attempts") | |
| .arg(kMaxDownloadAttempts)); | |
| return; | |
| } | |
| if (append && httpStatus == 200) { | |
| // Server ignored Range; restart from scratch to avoid duplicate payload. | |
| QFile::remove(m_downloadRequest.artifactPartPath); | |
| append = false; | |
| } |
🤖 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/UpdaterWorker.cpp` around lines 256 - 278, The code does not
handle the case where a server ignores a Range header request during resume
operations and responds with 200 OK (full body) instead of 206 Partial Content,
causing data corruption when the full payload is appended to an existing partial
file. Reorder the logic so the 416 status check (for Range Not Satisfiable)
executes before the generic httpOk error check. Additionally, add a new
condition that checks if httpStatus equals 200 while append is true (indicating
the server ignored the Range header), and in that case, remove the partial file
at m_downloadRequest.artifactPartPath and restart the download attempt from the
beginning using startArtifactDownloadAttempt. This prevents blindly appending
full responses to partial files.
1 participant
Summary
.partstaging under AppData).ArtifactResolver(platform-specific release asset pick) andUpdateVerifier(SHA-256 manifest + minisign signature).UpdaterController::downloadAndInstall(), auto-download when enabled, and dialog UI throughReadyToInstall(install step remains Update: macOS installer — replace .app bundle atomically #446–448).Test plan
UnitTests --gtest_filter="ArtifactResolver*:UpdateVerifier*:MinisignVerify*:UpdaterController*"passes locally on Linuxunit-tests-linuxgreenbuild-windows/build-macosgreen (Windows verify backend still stubbed — fail-closed at signature step)Made with Cursor
Summary by CodeRabbit
New Features
Chores