Upload QtMesh Cloud scan reports after file complete - #748
Conversation
…ponsive. Add uploadFileReport() and call it after completeUpload in the CLI, session, and MCP paths so analysis shows on the website. Move scan, manifest prep, and project listing off the main thread to prevent the app from freezing during upload. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
More reviews will be available in 26 minutes and 43 seconds. 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 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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. 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 (6)
📝 WalkthroughWalkthroughAdds a ChangesCloud upload scan report and session refactor
Sequence Diagram(s)sequenceDiagram
participant MainWindow
participant QtMeshCloudSession
participant DependencyResolver
participant QtMeshCloudClient
participant CloudAPI
MainWindow->>QtMeshCloudSession: uploadPackageFromAssets(CloudPackageUploadRequest)
QtMeshCloudSession->>DependencyResolver: detect(filePath) → selectedAbsolutePaths
QtMeshCloudSession->>QtMeshCloudClient: createProject / getUploadUrls
QtMeshCloudClient->>CloudAPI: POST /v1/projects, POST /files/upload-urls
CloudAPI-->>QtMeshCloudClient: uploadUrls
QtMeshCloudSession->>QtMeshCloudClient: uploadFiles (per-file binary PUT)
QtMeshCloudClient->>CloudAPI: PUT /binary/...
QtMeshCloudSession->>QtMeshCloudClient: completeUpload → mainFileId
QtMeshCloudClient->>CloudAPI: POST /files/complete
CloudAPI-->>QtMeshCloudClient: mainFileId
QtMeshCloudSession->>QtMeshCloudClient: uploadFileReport(mainFileId, scanSummary)
QtMeshCloudClient->>CloudAPI: PUT /files/:fileId/report
CloudAPI-->>QtMeshCloudClient: ok / error
QtMeshCloudSession-->>MainWindow: uploadFinished(ok, error, projectUrl, reportWarning)
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: ef4f30d407
ℹ️ 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".
| [self, current, total, label]() { | ||
| if (self) | ||
| emit self->uploadProgress(current, total, label); |
There was a problem hiding this comment.
When the upload session is deleted while a worker still has queued callbacks pending (for example, the user signs out or closes the window during an upload), this lambda keeps a raw QtMeshCloudSession*; the if (self) check does not become false after the QObject is destroyed, so the queued emit can dereference freed memory. The helpers are called with self.data() from a QPointer, so capture a QPointer<QtMeshCloudSession> through to the queued lambda instead of converting it to a raw pointer.
Useful? React with 👍 / 👎.
Avoid use-after-free when the session is destroyed while worker callbacks are still queued. Co-authored-by: Cursor <cursoragent@cursor.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CloudUploadDialog.cpp (1)
179-195:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAlways keep the main asset in the filtered manifest.
Line 205 returns only checked dependency paths, so Line 180 builds
selectedwithoutmainCanonical. The filter at Lines 191-195 then removes the main file thatProjectPackager::buildManifest()just added, causing main-only uploads to become empty and multi-file uploads to lose themainrole.Keep the main asset selected and normalize comparisons
const QString mainCanonical = QFileInfo(mainAssetPath).absoluteFilePath(); QSet<QString> selected; + if (!mainCanonical.isEmpty()) + selected.insert(mainCanonical); for (const QString& path : selectedAbsolutePaths) selected.insert(QFileInfo(path).absoluteFilePath()); @@ std::remove_if(metadata.files.begin(), metadata.files.end(), [&](const PackageEntry& entry) { - return !selected.contains(entry.absolutePath); + return !selected.contains(QFileInfo(entry.absolutePath).absoluteFilePath()); }), metadata.files.end());Also applies to: 205-214
🤖 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/CloudUploadDialog.cpp` around lines 179 - 195, The main asset file is being incorrectly filtered out of the manifest because mainCanonical is never added to the selected set before the filter is applied. The selected set is built only from user-checked dependency paths, but then the filter at the erase/remove_if block removes any entries not in selected, which removes the main file that buildManifest() added. Fix this by adding mainCanonical to the selected set immediately after it is computed, ensuring the main asset is always included in the filtered manifest regardless of whether dependencies are selected.
🧹 Nitpick comments (2)
src/QtMeshCloudClient_test.cpp (1)
393-425: ⚡ Quick winAdd a payload-limit regression test for
uploadFileReport.The implementation enforces a 5 MB cap, but this validation path is not covered here. Add one test that sends a
QJsonObjectjust over 5 MB and asserts a local error (without relying on mock HTTP behavior).Suggested test shape
+TEST(QtMeshCloudClientUploadFileReport, OversizedReportReturnsErrorImmediately) +{ + QJsonObject report; + report.insert(QStringLiteral("version"), QStringLiteral("3.0.0")); + report.insert(QStringLiteral("blob"), QString(QString(5 * 1024 * 1024, QLatin1Char('a')))); + + const auto result = QtMeshCloudClient::uploadFileReport( + QStringLiteral("token"), QStringLiteral("me"), QStringLiteral("project"), + QStringLiteral("file-1"), report, /*timeoutMs=*/100); + + EXPECT_FALSE(result.ok); + EXPECT_TRUE(result.errorString.contains("5 MB", Qt::CaseInsensitive)); +}🤖 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/QtMeshCloudClient_test.cpp` around lines 393 - 425, Add a new test to the QtMeshCloudClientUploadFileReport test suite that validates the 5 MB payload limit for uploadFileReport. Create a test function that constructs a QJsonObject exceeding 5 MB (you can populate it with large data), call QtMeshCloudClient::uploadFileReport with valid token, owner, project, and fileId parameters but with the oversized report payload, then assert that result.ok is false and that result.errorString contains an appropriate error message related to payload size or file size limit. This test should validate the local validation without requiring mock HTTP behavior.src/QtMeshCloudSession.cpp (1)
291-298: ⚡ Quick winUse the required Sentry category for upload I/O breadcrumbs.
These upload/report breadcrumbs use
cloud.upload; the repository convention requires file upload/export operations to usefile.export.As per coding guidelines,
src/**/*.cpp: “Use 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import'/'file.export' for I/O operations.”Align breadcrumb categories
- SentryReporter::addBreadcrumb(QStringLiteral("cloud.upload"), + SentryReporter::addBreadcrumb(QStringLiteral("file.export"), reportWarning, QStringLiteral("warning")); @@ - SentryReporter::addBreadcrumb(QStringLiteral("cloud.upload"), + SentryReporter::addBreadcrumb(QStringLiteral("file.export"), QStringLiteral("QtMesh Cloud package upload completed")); @@ - SentryReporter::addBreadcrumb(QStringLiteral("cloud.upload"), + SentryReporter::addBreadcrumb(QStringLiteral("file.export"), reportWarning, QStringLiteral("warning")); @@ - SentryReporter::addBreadcrumb(QStringLiteral("cloud.upload"), + SentryReporter::addBreadcrumb(QStringLiteral("file.export"), QStringLiteral("QtMesh Cloud package upload completed"));Also applies to: 424-431
🤖 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/QtMeshCloudSession.cpp` around lines 291 - 298, The SentryReporter::addBreadcrumb calls in QtMeshCloudSession.cpp are using the category "cloud.upload" for file upload operations, but the repository coding guidelines require file upload/export operations to use the "file.export" category instead. Replace all instances of the category parameter QStringLiteral("cloud.upload") with QStringLiteral("file.export") in the SentryReporter::addBreadcrumb calls related to QtMesh Cloud package uploads, which occur at the specified line ranges (291-298 and 424-431).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 `@src/mainwindow.cpp`:
- Around line 2755-2803: The projectsListed signal handler lambda captures a
token that may become stale if multiple upload requests are triggered or if the
auth state changes before the signal arrives. Add a guard mechanism to prevent
overlapping requests by checking if an upload is already in flight, and then
verify that the captured token still matches the current stored token before
showing the CloudUploadDialog and before calling startCloudPackageUpload. This
prevents stale token usage and duplicate dialogs when async operations complete
after the user has moved to a different account or triggered another upload.
In `@src/MCPServer.cpp`:
- Around line 5280-5297: The code populates selectedPaths with all entries from
DependencyResolver::detect(filePath) without filtering, but should only include
entries that are valid and user-selected. Filter the dependency entries to keep
only those where both the exists field and checkedByDefault field evaluate to
true, then build selectedPaths from only these filtered entries. This ensures
uploadedFileCount reflects the actual number of files being uploaded and
prevents missing or unchecked dependencies from being included in the
CloudPackageUploadRequest.
- Around line 5291-5314: The `loop.exec()` call in the QtMeshCloudSession upload
handling can block indefinitely if the uploadFinished or uploadCanceled signals
are never emitted due to network issues or dropped connections. Add a QTimer
with a single-shot timeout before calling loop.exec() that will cancel the
session and quit the loop if the timeout fires. Additionally, check if uploadOk
is already set to true before calling loop.exec() to handle cases where the
signals are emitted synchronously before exec() is invoked. Make sure to start
the timer after connecting all signals but before calling
session.uploadPackageFromAssets(request), and ensure the timeout duration is
reasonable for typical upload scenarios.
In `@src/QtMeshCloudSession_test.cpp`:
- Around line 6-18: The test contains a nested event loop around lines 202-213
that waits for the uploadFinished signal without any timeout mechanism, causing
it to hang indefinitely if the signal is never emitted. Add a QTimer instance
before the event loop execution that emits a timeout after a reasonable duration
(e.g., a few seconds), connect its timeout signal to quit the event loop, start
the timer, and then add an assertion or check after the event loop exits to
verify that uploadFinished was actually emitted and the upload completed
successfully rather than timing out.
In `@src/QtMeshCloudSession.cpp`:
- Line 121: Replace the raw pointer capture of `&m_canceled` in the lambda
closures at lines 121 and 314-315 (in the QThread::create calls) with a
thread-safe mechanism such as a std::shared_ptr<std::atomic_bool> to prevent
use-after-free when QtMeshCloudSession is destroyed while worker threads are
still running and calling canceled->load(). Alternatively, add an explicit
destructor to QtMeshCloudSession that waits for all active worker threads to
complete before the object is destroyed. Additionally, update the breadcrumb
category parameter from "cloud.upload" to "file.export" in all four Sentry
breadcrumb calls at lines 291, 297, 424, and 430 to align with coding guidelines
for file upload I/O operations.
---
Outside diff comments:
In `@src/CloudUploadDialog.cpp`:
- Around line 179-195: The main asset file is being incorrectly filtered out of
the manifest because mainCanonical is never added to the selected set before the
filter is applied. The selected set is built only from user-checked dependency
paths, but then the filter at the erase/remove_if block removes any entries not
in selected, which removes the main file that buildManifest() added. Fix this by
adding mainCanonical to the selected set immediately after it is computed,
ensuring the main asset is always included in the filtered manifest regardless
of whether dependencies are selected.
---
Nitpick comments:
In `@src/QtMeshCloudClient_test.cpp`:
- Around line 393-425: Add a new test to the QtMeshCloudClientUploadFileReport
test suite that validates the 5 MB payload limit for uploadFileReport. Create a
test function that constructs a QJsonObject exceeding 5 MB (you can populate it
with large data), call QtMeshCloudClient::uploadFileReport with valid token,
owner, project, and fileId parameters but with the oversized report payload,
then assert that result.ok is false and that result.errorString contains an
appropriate error message related to payload size or file size limit. This test
should validate the local validation without requiring mock HTTP behavior.
In `@src/QtMeshCloudSession.cpp`:
- Around line 291-298: The SentryReporter::addBreadcrumb calls in
QtMeshCloudSession.cpp are using the category "cloud.upload" for file upload
operations, but the repository coding guidelines require file upload/export
operations to use the "file.export" category instead. Replace all instances of
the category parameter QStringLiteral("cloud.upload") with
QStringLiteral("file.export") in the SentryReporter::addBreadcrumb calls related
to QtMesh Cloud package uploads, which occur at the specified line ranges
(291-298 and 424-431).
🪄 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: 701555cd-01c3-48db-9d0b-1f0405a16998
📒 Files selected for processing (14)
src/CLIPipeline.cppsrc/CloudCLIPipeline.cppsrc/CloudUploadDialog.cppsrc/CloudUploadDialog.hsrc/CloudUploadProgress.cppsrc/MCPServer.cppsrc/QtMeshCloudClient.cppsrc/QtMeshCloudClient.hsrc/QtMeshCloudClient_test.cppsrc/QtMeshCloudSession.cppsrc/QtMeshCloudSession.hsrc/QtMeshCloudSession_test.cppsrc/mainwindow.cppsrc/mainwindow.h
Use shared cancel flags so worker threads outlive session teardown, guard GUI uploads against overlapping project lists and stale tokens, tighten MCP dependency selection with a timeout, and add a test watchdog against hangs. Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
Addressed all review feedback in 208c36d:
CI is green on the latest push (linux/macos/windows builds, unit-tests-linux, SonarCloud, scan-assets, verify-doc-versions). |
Sync pinned doc refs via scripts/sync-doc-versions-from-cmake.sh. Covers the features merged since 3.7.0: - AI PBR map synthesis from albedo (ONNX) + multi-slot PBR texture UI (#404, #738) - QtMesh Cloud scan-report upload (#748) - In-app isometric sprite export UI (#724, #742) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
QtMeshCloudClient::uploadFileReport()(PUT/v1/u/{owner}/p/{project}/files/{fileId}/report) and call it aftercompleteUploadin the GUI session, CLIcloud upload, and MCPcloud_uploadtool.ScanEngine::scanReportToJsonObjectformat) tomainFileIdonly; report upload failure is non-fatal with a user-visible warning.uploadPackageFromAssets()so the editor stays responsive during upload.Test plan
UnitTests --gtest_filter="QtMeshCloudClient*:QtMeshCloudSession*:CloudUpload*"(64 tests)uploadFileReport(PUT path, auth, ordering after complete)QtMeshCloudSessionUploadReportTest.ReportFailureDoesNotFailBinaryUploadMade with Cursor
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation