feat(cloud): package editor assets for QtMesh Cloud upload (#684) - #719
Merged
Conversation
Add dependency-aware upload from the open model, project picker, cloud projects dialog, CLI/MCP cloud commands, and run pre-upload scans in an isolated subprocess so the live Ogre scene is not cleared. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds end-to-end QtMesh Cloud support: new ChangesQtMesh Cloud Integration
Sequence Diagram(s)sequenceDiagram
participant User
participant CloudCLIPipeline
participant QtMeshCloudSession
participant QtMeshCloudClient
User->>CloudCLIPipeline: cloud upload file (--json or --no-scan)
CloudCLIPipeline->>QtMeshCloudSession: create session and uploadPackage(manifest)
QtMeshCloudSession->>QtMeshCloudClient: createProject, requestUploadUrls, upload files
QtMeshCloudClient-->>QtMeshCloudSession: project and upload responses
QtMeshCloudSession-->>CloudCLIPipeline: uploadFinished(ok, projectUrl)
CloudCLIPipeline-->>User: print JSON or text result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: e14d495125
ℹ️ 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".
| QString projectName; | ||
| bool jsonOutput = false; | ||
| bool runScan = true; | ||
| for (int i = 2; i < argc; ++i) { |
There was a problem hiding this comment.
For qtmesh cloud upload model.obj, this loop starts at argv[2], which is the literal upload that CloudCLIPipeline::run just dispatched on. The positional branch then records mainFile = "upload" before it reaches the real path, so CLI uploads fail with file not found: upload or package the wrong local file if one happens to exist.
Useful? React with 👍 / 👎.
| int cmdCloudDelete(int argc, char* argv[]) | ||
| { | ||
| QString projectId; | ||
| for (int i = 2; i < argc; ++i) { |
There was a problem hiding this comment.
For qtmesh cloud delete <project-id>, argv[2] is the literal delete, so this loop sets projectId to delete and ignores the requested id that follows. The delete command therefore cannot delete the intended project and may target the wrong id if such a project exists.
Useful? React with 👍 / 👎.
| return 2; | ||
| } | ||
|
|
||
| const QString sub = QString::fromUtf8(argv[2]); |
There was a problem hiding this comment.
CLIPipeline::run supports editor-binary invocations like QtMeshEditor --cli cloud status and passes the original argv here, but in that form argv[2] is cloud, not status. This makes every cloud command through the editor executable fail as an unknown cloud subcommand; locate the token after cloud/skip --cli instead of hard-coding this index.
Useful? React with 👍 / 👎.
Comment on lines
+2610
to
+2611
| if (!m_cloudSession) | ||
| m_cloudSession = new QtMeshCloudSession(token, this); |
There was a problem hiding this comment.
When m_cloudSession already exists, this reuses the bearer token captured when the session was constructed. If a user opens cloud projects, signs out, then signs in as another account, the next project list still uses the old token (often revoked by sign-out), so project loading/upload targets the previous session until app restart; clear the session on sign-out or replace it when the stored token differs.
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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (5)
src/QtMeshCloudSession.cpp-108-110 (1)
108-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProgress math is off-by-one during file uploads.
uploadProgress(i + 1, total + 1, ...)never reaches full completion during the per-file loop and can leave UI progress short of 100% unless a finalization tick is emitted separately.Either emit
totalas denominator here or add an explicit finaltotal+1/total+1progress update before/aftercompleteUpload.🤖 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 108 - 110, The progress calculation inside the QMetaObject::invokeMethod lambda is off-by-one: change the emitted denominator so the per-file updates can reach 100% by either emitting uploadProgress(i + 1, total, label) instead of uploadProgress(i + 1, total + 1, label) in the lambda, or keep the current per-file emits but add an explicit final emit uploadProgress(total + 1, total + 1, label) (or uploadProgress(total, total, label) if you switch to zero-based counting) immediately before/after calling completeUpload so the UI receives a final completion tick; update the lambda that currently calls emit uploadProgress(...) and/or add the final emit adjacent to completeUpload accordingly.src/QtMeshCloudClient.cpp-661-682 (1)
661-682:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStandardize
deleteProjectbreadcrumbs and include outcome events.This path logs only a start breadcrumb and uses
cloud.project. For a significant operation, align category naming with the repository guideline and add explicit success/failure breadcrumbs.Suggested patch
- SentryReporter::addBreadcrumb(QStringLiteral("cloud.project"), + SentryReporter::addBreadcrumb(QStringLiteral("file.export"), QStringLiteral("QtMesh Cloud deleteProject: start")); @@ out.responseBodySnippet = trimSnippet(responseBody); out.ok = nerr == QNetworkReply::NoError && out.httpStatus >= 200 && out.httpStatus < 300; if (!out.ok) { out.errorString = nerr != QNetworkReply::NoError ? transportErr : QStringLiteral("HTTP %1").arg(out.httpStatus); if (!out.responseBodySnippet.isEmpty()) out.errorString += QStringLiteral(" — ") + out.responseBodySnippet; + SentryReporter::addBreadcrumb(QStringLiteral("file.export"), + QStringLiteral("QtMesh Cloud deleteProject: failure HTTP %1").arg(out.httpStatus), + QStringLiteral("warning")); + } else { + SentryReporter::addBreadcrumb(QStringLiteral("file.export"), + QStringLiteral("QtMesh Cloud deleteProject: ok")); }As per coding guidelines, significant operations should emit Sentry breadcrumbs using the prescribed category taxonomy.
🤖 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.cpp` around lines 661 - 682, Change the single "start" breadcrumb in deleteProject to use the standardized category and emit outcome breadcrumbs: replace the initial SentryReporter::addBreadcrumb call that currently uses "cloud.project" with the repository-standard category (e.g. "cloud.project.operation") and then, after computing out.ok/out.httpStatus/out.errorString, emit a success breadcrumb SentryReporter::addBreadcrumb("cloud.project.operation", "deleteProject: success") when out.ok is true, otherwise emit a failure breadcrumb SentryReporter::addBreadcrumb("cloud.project.operation", QStringLiteral("deleteProject: failure — %1").arg(out.errorString)) so the operation name and error/HTTP status are recorded; keep using the existing symbols deleteProject, out.ok, out.errorString and out.httpStatus to build the messages.Source: Coding guidelines
src/CloudAccountMenuButton.cpp-345-347 (1)
345-347:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
refresh()currently overrides the external upload gating state.Line 346 re-enables upload for any signed-in user, so opening the menu can undo
setUploadEnabled(false)fromMainWindowand expose an action that immediately errors.Suggested change
// src/CloudAccountMenuButton.h + bool m_uploadAllowedByContext = true; // src/CloudAccountMenuButton.cpp void CloudAccountMenuButton::refresh() { ... - m_uploadAction->setEnabled(signedIn); + m_uploadAction->setEnabled(signedIn && m_uploadAllowedByContext); ... } void CloudAccountMenuButton::setUploadEnabled(bool enabled) { + m_uploadAllowedByContext = enabled; if (m_uploadAction) - m_uploadAction->setEnabled(enabled && CloudCredentialStore::hasSession()); + m_uploadAction->setEnabled(m_uploadAllowedByContext && CloudCredentialStore::hasSession());Also applies to: 354-362
🤖 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/CloudAccountMenuButton.cpp` around lines 345 - 347, The refresh() method currently unconditionally re-enables m_uploadAction for signed-in users and thus can undo an external gate like MainWindow::setUploadEnabled(false); modify refresh() so it respects the externally controlled upload state: replace the unconditional m_uploadAction->setEnabled(signedIn) with m_uploadAction->setEnabled(signedIn && m_uploadEnabled) (or call the existing setter that MainWindow uses, e.g., setUploadEnabled(...)), and apply the same guarded logic to the other refresh sites mentioned (the block around lines 354-362) so refresh never overrides an externally stored upload-enabled flag.src/mainwindow.cpp-2374-2375 (1)
2374-2375:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpload action enablement doesn’t match the actual upload precondition.
updateCloudUploadActionState()enables upload when entities exist, butuploadFilesToQtMeshCloud()still requires a non-emptymainAssetPathand immediately rejects otherwise. This creates a predictable enabled-but-unusable action path.Suggested change
- const bool hasAsset = !primaryCloudAssetPath().isEmpty() - || !Manager::getSingleton()->getEntities().isEmpty(); + const bool hasAsset = !primaryCloudAssetPath().isEmpty();Also applies to: 2655-2660
🤖 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 2374 - 2375, The enablement check in updateCloudUploadActionState() (hasAsset = !primaryCloudAssetPath().isEmpty() || !Manager::getSingleton()->getEntities().isEmpty()) doesn't match the stricter precondition in uploadFilesToQtMeshCloud() which rejects when primaryCloudAssetPath()/mainAssetPath is empty; make the two consistent by updating uploadFilesToQtMeshCloud(): change its precondition to allow upload when either primaryCloudAssetPath() is non-empty OR Manager::getSingleton()->getEntities() is non-empty (matching the hasAsset logic), and if primaryCloudAssetPath() is empty but entities exist, branch to build/serialize the upload payload from the entities (or create a temporary asset) instead of immediately rejecting; alternatively, if you prefer to require an asset, update updateCloudUploadActionState() to use only !primaryCloudAssetPath().isEmpty() so the action is disabled when no main asset—pick one approach and apply it consistently in updateCloudUploadActionState() and uploadFilesToQtMeshCloud().src/CloudUploadProgress.cpp-28-34 (1)
28-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset visual/control state in
start()before showing a new upload.If a prior upload finished and the next one starts before
hideProgress()runs, the cancel button can stay disabled and failure styling can carry over.Suggested change
void CloudUploadProgress::start(const QString& label, int total) { + m_cancelButton->setEnabled(true); + m_label->setStyleSheet({}); m_label->setText(label); m_bar->setRange(0, qMax(1, total)); m_bar->setValue(0); show(); }🤖 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/CloudUploadProgress.cpp` around lines 28 - 34, CloudUploadProgress::start currently only sets m_label and m_bar and shows the widget, so leftover state (disabled cancel button or failure styling) can persist from a prior run; update start() to also reset visual and control state before show(): set the cancel button enabled (e.g., m_cancelButton->setEnabled(true)), clear any failure/errored styling or flags used to indicate a failed upload (remove failure CSS class or reset m_failed/m_status state), reset progress text/state (if any), and ensure m_bar value/range are initialized as shown; this mirrors hideProgress() cleanup so new uploads always start with a clean enabled cancel button and no failure styling.
🧹 Nitpick comments (1)
src/AssetScanController.cpp (1)
322-334: ⚡ Quick winAdd breadcrumbs for isolated pre-upload scans.
runIsolatedScanJsonSync()is part of a user-triggered upload flow and performs a significant scan operation, but it currently emits no breadcrumb for start/failure/success.As per coding guidelines, add
SentryReporter::addBreadcrumb()for significant I/O operations withfile.import.🤖 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/AssetScanController.cpp` around lines 322 - 334, Add Sentry breadcrumbs around the isolated pre-upload scan in runIsolatedScanJsonSync: before calling runScanSubprocessSync emit a breadcrumb via SentryReporter::addBreadcrumb(...) with category "file.import" and a message like "isolated scan started" (include rootPath/includePattern in data); on non-ok outcome emit a failure breadcrumb with category "file.import" and message "isolated scan failed" (include outcome.message and any identifying inputs) and then set errorOut as now; on success emit a success breadcrumb with category "file.import" and message "isolated scan succeeded" (include summary info or size of outcome.jsonBytes). Ensure the breadcrumbs do not change control flow and are added immediately before/after the runScanSubprocessSync call and outcome handling in runIsolatedScanJsonSync.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/CloudCLIPipeline.cpp`:
- Around line 178-182: The loops that parse arguments incorrectly start at
argv[2] (the subcommand token), so change both loops that currently read "for
(int i = 2; ..." to start at index 3 (e.g., "for (int i = 3; ...") so parsing of
projectId and filePath skips the subcommand; update the parsing logic that
assigns to projectId and filePath (the loops that check
arg.startsWith(QLatin1Char('-')) and set projectId/filePath) to iterate from i =
3 instead of i = 2 to avoid treating the subcommand itself as an argument.
- Around line 239-317: The upload flow in CloudCLIPipeline (around
ProjectPackager::buildManifest through QtMeshCloudClient::completeUpload) lacks
Sentry breadcrumbs; add SentryReporter::addBreadcrumb() calls with category
"file.export" to instrument start/failure/success: add a breadcrumb before
requesting URLs (e.g., "upload.start" with projectName and fileCount), add one
per file before calling QtMeshCloudClient::uploadFileContent (e.g.,
"file.export.start" with upload filename/fileId), add a breadcrumb on per-file
upload failure/success after uploadFileContent returns (e.g.,
"file.export.error" / "file.export.success" with fileId and errorString if any),
and add a final breadcrumb after completeUpload indicating overall
"upload.complete" or on completeUpload failure ("upload.error") including
projectUrl, project.projectSlug and errorString; use
SentryReporter::addBreadcrumb consistently for these points.
In `@src/CloudProjectsDialog.cpp`:
- Around line 31-40: Wrap the dialog open/close actions so they emit Sentry
breadcrumbs: add a small handler method (e.g.
CloudProjectsDialog::onOpenSelected()) that is invoked from the m_openButton
clicked lambda and from the m_list itemDoubleClicked connection and, inside it,
call SentryReporter::addBreadcrumb("ui.action", "cloud-projects.open") before
retrieving the selected item and calling QDesktopServices::openUrl(QUrl(url));
likewise replace the direct connect for closeButton with a lambda or a new
CloudProjectsDialog::onCloseClicked() that calls
SentryReporter::addBreadcrumb("ui.action", "cloud-projects.close") and then
QDialog::reject(); ensure the double-click connection points at the shared
onOpenSelected handler so the double-click path also emits the same breadcrumb.
- Around line 32-39: The click handler for m_openButton opens project.projectUrl
directly; instead validate the URL before opening by constructing a QUrl,
ensuring url.isValid(), url.isLocalFile() is false, and that
url.scheme().toLower() == "https" (or match an allow-list of safe schemes/hosts)
before calling QDesktopServices::openUrl(QUrl(url)); if validation fails, do not
open. Also add SentryReporter::addBreadcrumb(...) with category "ui.action" and
a message like "Open in Browser" inside that validated-open path, and add a
similar SentryReporter::addBreadcrumb(...) for the dialog Close action (e.g., in
the close/Reject/Close button handler) so user-facing actions (Open in Browser
and Close) are recorded; ensure the double-click handler delegates to the same
validated open logic so it benefits from the same checks and breadcrumb.
In `@src/CloudUploadDialog.cpp`:
- Around line 58-63: Add Sentry breadcrumbs for the user actions wired in this
dialog: call SentryReporter::addBreadcrumb(...) with category "ui.action" when
cancelButton is clicked (in the cancel connect), when m_uploadButton is clicked
(in the upload connect) and when m_projectCombo currentIndexChanged fires
(inside the existing lambda before/after calling hasSelectedProject()). Also add
a breadcrumb for the dependency detection operation (the significant scan/parse
code referenced around lines 105-109) using SentryReporter::addBreadcrumb(...)
with an appropriate category such as "file.import" or "file.export" and a short
message describing the operation; include any pertinent metadata (selected
project id/name) where available. Ensure breadcrumb text is concise and use the
same SentryReporter API everywhere.
In `@src/CloudUploadProgress.cpp`:
- Around line 24-25: Add a Sentry breadcrumb for the cancel button click in
CloudUploadProgress so the user action is recorded before emitting
cancelRequested(): update the slot or lambda connected to m_cancelButton (the
connection that currently calls CloudUploadProgress::cancelRequested) to call
SentryReporter::addBreadcrumb("ui.action", "cancel_upload" or similar short
descriptor) immediately before invoking cancelRequested(), ensuring the
breadcrumb is added even if cancelRequested() triggers early returns or thread
changes.
In `@src/DependencyResolver.cpp`:
- Around line 251-257: The code branch that handles ext == QLatin1String("obj")
currently only checks mainInfo.completeBaseName() + ".mtl" and must instead
parse the OBJ file for "mtllib" directives: open the OBJ file referenced by
mainInfo, scan lines for tokens beginning with "mtllib" (handling multiple
filenames on a line and quoted/unquoted names), for each referenced mtl name
resolve it relative to mainInfo.absolutePath() (and also treat
absolute/absolute-URL forms appropriately), then for each resolved mtlPath if
QFileInfo::exists(mtlPath) call appendEntry(out, seen, mtlPath,
QStringLiteral("material"), mainInfo.fileName()) and
collectMtlDependencies(mtlPath, out, seen); keep the existing basename.mtl probe
only as a fallback if no mtllib directives are found.
In `@src/mainwindow.cpp`:
- Around line 229-233: Add a UI breadcrumb when the user clicks the File →
Upload menu by calling SentryReporter::addBreadcrumb(...) with category
"ui.action" in the menu click handler: insert a call at the start of
MainWindow::uploadFilesToQtMeshCloud (the slot connected from
m_cloudUploadMenuAction) that creates a breadcrumb describing the action (e.g.,
"File → Upload to QtMesh Cloud") before the existing cloud.upload logging; do
the same for the other corresponding menu/toolbar handlers mentioned (the other
menu action handlers referenced in the review) so all menu clicks emit a
ui.action breadcrumb.
- Around line 2610-2611: signOutOfQtMeshCloud() must clear the cached session to
avoid using a stale bearer token: if m_cloudSession is non-null, destroy it and
set m_cloudSession = nullptr (e.g. call m_cloudSession->deleteLater();
m_cloudSession = nullptr; or delete m_cloudSession; m_cloudSession = nullptr;
depending on QObject ownership) so subsequent calls in showCloudProjectsDialog()
and the initial listing in uploadFilesToQtMeshCloud() will recreate a
QtMeshCloudSession with the new token; no other changes required to
QtMeshCloudSession or QtMeshCloudClient::fetchProjects.
- Around line 2616-2621: The lambda connected to
QtMeshCloudSession::projectsListed in MainWindow::showCloudProjectsDialog
captures local variables (loop, projects, error) by reference but the connection
is left active, risking a UAF; change the connect call to use
Qt::SingleShotConnection (add Qt::SingleShotConnection as the connection type
parameter) so the slot is invoked once and auto-disconnected, or alternatively
explicitly disconnect the signal right after loop.exec() returns; locate the
connect line referencing m_cloudSession, projectsListed, and the lambda in
MainWindow::showCloudProjectsDialog and update it to use
Qt::SingleShotConnection (or add a matching disconnect after loop.exec()).
- Around line 2605-2624: The progress dialog Cancel button isn't wired to abort
the nested QEventLoop(s) or the cloud request: connect the
QProgressDialog::canceled signal (the local progress instance) to a lambda that
calls m_cloudSession->cancel() and quits the appropriate QEventLoop (e.g. loop
and the other listLoop) so the UI unblocks immediately; additionally, update
QtMeshCloudSession::listProjects() to respect the m_canceled flag (check
m_canceled before starting work and after any blocking/waitable call and return
early or propagate cancellation to the transport) so calls to
QtMeshCloudSession::cancel() actually stop/avoid calling
QtMeshCloudClient::fetchProjects() (or otherwise abort the in-flight request)
instead of waiting for the transport timeout.
In `@src/MCPServer.cpp`:
- Around line 5151-5159: ProjectPackager::buildManifest is being called with an
empty dependency list and although ScanEngine::run()/scanReportToJsonObject
stores results in manifest.scanSummary, the discovered sidecar files are never
added to manifest.files so only the root asset is uploaded; fix this by
extracting the discovered file paths from the scan report (use ScanEngine::run
return or the JSON from ScanEngine::scanReportToJsonObject) and either (a) pass
that dependency list into ProjectPackager::buildManifest instead of {} or (b)
after building the manifest, append the discovered paths into manifest.files
(and update any metadata entries needed) so textures/materials discovered by
ScanConfig are included in the uploaded package; refer to
ProjectPackager::buildManifest, manifest.files, manifest.scanSummary,
ScanEngine::run and ScanEngine::scanReportToJsonObject when making the change.
- Around line 5070-5145: The code reads
CloudCredentialStore::loadSession().token in toolCloudLogout,
toolCloudDeleteProject, and toolCloudUpload without calling
migrateLegacySettingsIfNeeded(), causing upgraded installs with only legacy
credentials to appear signed-out; fix by calling
CloudCredentialStore::migrateLegacySettingsIfNeeded() before each token read in
MCPServer::toolCloudLogout, MCPServer::toolCloudDeleteProject, and
MCPServer::toolCloudUpload so the session is migrated before checking token
validity.
- Around line 634-638: callTool() currently gates all handlers with
ensureOgreInitialized(), which causes the new cloud handlers (toolCloudStatus,
toolCloudLogin, toolCloudLogout, toolCloudListProjects, toolCloudDeleteProject)
to fail in headless sessions; update the dispatcher so scene-dependent tools
still call ensureOgreInitialized() but the cloud management handlers bypass that
check: either (A) add an explicit whitelist in the tool dispatch logic to skip
ensureOgreInitialized() for the QStringLiteral names "cloud_status",
"cloud_login", "cloud_logout", "cloud_list_projects", "cloud_delete_project", or
(B) move the ensureOgreInitialized() call out of the global callTool() path and
into the scene-dependent handlers (e.g. those that use Ogre), leaving the
cloud_* handler methods free to run without Ogre init; implement one of these
fixes and ensure callTool() invokes the selected cloud handler methods
(toolCloudStatus, toolCloudLogin, toolCloudLogout, toolCloudListProjects,
toolCloudDeleteProject) without calling ensureOgreInitialized().
In `@src/QtMeshCloudSession.cpp`:
- Around line 57-61: When handling the existing-project branch (createNewProject
== false) in QtMeshCloudSession.cpp you must populate project.projectUrl the
same way the new-project path does so uploadFinished emits a valid URL; in the
else branch that currently sets project.ok, project.ownerSlug and
project.projectSlug, also construct and assign project.projectUrl (using the
same URL format/templating logic used elsewhere for new projects) so the
uploaded project's URL is not dropped.
- Around line 22-31: The worker lambdas capture raw this and call
QMetaObject::invokeMethod(this, ...) which can use-after-free if the session is
destroyed; change the lambda(s) that create QThread workers (the QThread::create
block that calls QtMeshCloudClient::fetchProjects and other similar worker
lambdas) to capture a QPointer<QtMeshCloudSession> (e.g.
QPointer<QtMeshCloudSession> self(this)) instead of raw this, and before calling
QMetaObject::invokeMethod or emitting signals check that self is not null (or
call invokeMethod(self.data(), ...)); alternatively implement a small QObject
worker whose lifetime is parented to the session and move the work to that
worker and invoke back using the worker or the guarded QPointer — update all
similar sites (the other worker lambdas mentioned in the review) accordingly so
no worker captures raw this or invokes on a possibly-destroyed pointer.
---
Minor comments:
In `@src/CloudAccountMenuButton.cpp`:
- Around line 345-347: The refresh() method currently unconditionally re-enables
m_uploadAction for signed-in users and thus can undo an external gate like
MainWindow::setUploadEnabled(false); modify refresh() so it respects the
externally controlled upload state: replace the unconditional
m_uploadAction->setEnabled(signedIn) with m_uploadAction->setEnabled(signedIn &&
m_uploadEnabled) (or call the existing setter that MainWindow uses, e.g.,
setUploadEnabled(...)), and apply the same guarded logic to the other refresh
sites mentioned (the block around lines 354-362) so refresh never overrides an
externally stored upload-enabled flag.
In `@src/CloudUploadProgress.cpp`:
- Around line 28-34: CloudUploadProgress::start currently only sets m_label and
m_bar and shows the widget, so leftover state (disabled cancel button or failure
styling) can persist from a prior run; update start() to also reset visual and
control state before show(): set the cancel button enabled (e.g.,
m_cancelButton->setEnabled(true)), clear any failure/errored styling or flags
used to indicate a failed upload (remove failure CSS class or reset
m_failed/m_status state), reset progress text/state (if any), and ensure m_bar
value/range are initialized as shown; this mirrors hideProgress() cleanup so new
uploads always start with a clean enabled cancel button and no failure styling.
In `@src/mainwindow.cpp`:
- Around line 2374-2375: The enablement check in updateCloudUploadActionState()
(hasAsset = !primaryCloudAssetPath().isEmpty() ||
!Manager::getSingleton()->getEntities().isEmpty()) doesn't match the stricter
precondition in uploadFilesToQtMeshCloud() which rejects when
primaryCloudAssetPath()/mainAssetPath is empty; make the two consistent by
updating uploadFilesToQtMeshCloud(): change its precondition to allow upload
when either primaryCloudAssetPath() is non-empty OR
Manager::getSingleton()->getEntities() is non-empty (matching the hasAsset
logic), and if primaryCloudAssetPath() is empty but entities exist, branch to
build/serialize the upload payload from the entities (or create a temporary
asset) instead of immediately rejecting; alternatively, if you prefer to require
an asset, update updateCloudUploadActionState() to use only
!primaryCloudAssetPath().isEmpty() so the action is disabled when no main
asset—pick one approach and apply it consistently in
updateCloudUploadActionState() and uploadFilesToQtMeshCloud().
In `@src/QtMeshCloudClient.cpp`:
- Around line 661-682: Change the single "start" breadcrumb in deleteProject to
use the standardized category and emit outcome breadcrumbs: replace the initial
SentryReporter::addBreadcrumb call that currently uses "cloud.project" with the
repository-standard category (e.g. "cloud.project.operation") and then, after
computing out.ok/out.httpStatus/out.errorString, emit a success breadcrumb
SentryReporter::addBreadcrumb("cloud.project.operation", "deleteProject:
success") when out.ok is true, otherwise emit a failure breadcrumb
SentryReporter::addBreadcrumb("cloud.project.operation",
QStringLiteral("deleteProject: failure — %1").arg(out.errorString)) so the
operation name and error/HTTP status are recorded; keep using the existing
symbols deleteProject, out.ok, out.errorString and out.httpStatus to build the
messages.
In `@src/QtMeshCloudSession.cpp`:
- Around line 108-110: The progress calculation inside the
QMetaObject::invokeMethod lambda is off-by-one: change the emitted denominator
so the per-file updates can reach 100% by either emitting uploadProgress(i + 1,
total, label) instead of uploadProgress(i + 1, total + 1, label) in the lambda,
or keep the current per-file emits but add an explicit final emit
uploadProgress(total + 1, total + 1, label) (or uploadProgress(total, total,
label) if you switch to zero-based counting) immediately before/after calling
completeUpload so the UI receives a final completion tick; update the lambda
that currently calls emit uploadProgress(...) and/or add the final emit adjacent
to completeUpload accordingly.
---
Nitpick comments:
In `@src/AssetScanController.cpp`:
- Around line 322-334: Add Sentry breadcrumbs around the isolated pre-upload
scan in runIsolatedScanJsonSync: before calling runScanSubprocessSync emit a
breadcrumb via SentryReporter::addBreadcrumb(...) with category "file.import"
and a message like "isolated scan started" (include rootPath/includePattern in
data); on non-ok outcome emit a failure breadcrumb with category "file.import"
and message "isolated scan failed" (include outcome.message and any identifying
inputs) and then set errorOut as now; on success emit a success breadcrumb with
category "file.import" and message "isolated scan succeeded" (include summary
info or size of outcome.jsonBytes). Ensure the breadcrumbs do not change control
flow and are added immediately before/after the runScanSubprocessSync call and
outcome handling in runIsolatedScanJsonSync.
🪄 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: c37d3274-c283-4445-8fb3-a69ea558c1f8
📒 Files selected for processing (28)
src/AppLaunchHandler.cppsrc/AssetScanController.cppsrc/AssetScanController.hsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/CloudAccountMenuButton.cppsrc/CloudAccountMenuButton.hsrc/CloudCLIPipeline.cppsrc/CloudCLIPipeline.hsrc/CloudProjectsDialog.cppsrc/CloudProjectsDialog.hsrc/CloudUploadDialog.cppsrc/CloudUploadDialog.hsrc/CloudUploadProgress.cppsrc/CloudUploadProgress.hsrc/DependencyResolver.cppsrc/DependencyResolver.hsrc/MCPServer.cppsrc/MCPServer.hsrc/ProjectPackager.cppsrc/ProjectPackager.hsrc/ProjectPackager_test.cppsrc/QtMeshCloudClient.cppsrc/QtMeshCloudClient.hsrc/QtMeshCloudSession.cppsrc/QtMeshCloudSession.hsrc/mainwindow.cppsrc/mainwindow.h
Comment on lines
+32
to
+39
| connect(m_openButton, &QPushButton::clicked, this, [this]() { | ||
| const QListWidgetItem* item = m_list->currentItem(); | ||
| if (!item) | ||
| return; | ||
| const QString url = item->data(Qt::UserRole + 1).toString(); | ||
| if (!url.isEmpty()) | ||
| QDesktopServices::openUrl(QUrl(url)); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and show relevant sections
ls -la src/CloudProjectsDialog.cpp
sed -n '1,120p' src/CloudProjectsDialog.cpp
sed -n '120,260p' src/CloudProjectsDialog.cpp
# Check for breadcrumb instrumentation in this file
rg -n "SentryReporter::addBreadcrumb|addBreadcrumb\\(" src/CloudProjectsDialog.cpp
# Check where URL data role is set/used
rg -n "Qt::UserRole \\+ 1|UserRole \\+ 1|projectUrl|data\\(Qt::UserRole|setProjects\\(" src/CloudProjectsDialog.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2191
Harden external URL handling and add Sentry breadcrumbs for dialog actions
project.projectUrl is API-fed and is opened directly; validate it with an allow-list (prefer https) before calling QDesktopServices::openUrl(...).
Also add SentryReporter::addBreadcrumb() (category: ui.action) for the dialog’s user-facing actions (at least Close and “Open in Browser”; double-click should be covered by the same open handler).
🔧 Proposed fix
- connect(m_openButton, &QPushButton::clicked, this, [this]() {
+ connect(m_openButton, &QPushButton::clicked, this, [this]() {
const QListWidgetItem* item = m_list->currentItem();
if (!item)
return;
const QString url = item->data(Qt::UserRole + 1).toString();
- if (!url.isEmpty())
- QDesktopServices::openUrl(QUrl(url));
+ const QUrl parsed = QUrl::fromUserInput(url);
+ if (parsed.isValid() && parsed.scheme().compare(QStringLiteral("https"), Qt::CaseInsensitive) == 0)
+ QDesktopServices::openUrl(parsed);
});🤖 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/CloudProjectsDialog.cpp` around lines 32 - 39, The click handler for
m_openButton opens project.projectUrl directly; instead validate the URL before
opening by constructing a QUrl, ensuring url.isValid(), url.isLocalFile() is
false, and that url.scheme().toLower() == "https" (or match an allow-list of
safe schemes/hosts) before calling QDesktopServices::openUrl(QUrl(url)); if
validation fails, do not open. Also add SentryReporter::addBreadcrumb(...) with
category "ui.action" and a message like "Open in Browser" inside that
validated-open path, and add a similar SentryReporter::addBreadcrumb(...) for
the dialog Close action (e.g., in the close/Reject/Close button handler) so
user-facing actions (Open in Browser and Close) are recorded; ensure the
double-click handler delegates to the same validated open logic so it benefits
from the same checks and breadcrumb.
Comment on lines
+58
to
+63
| connect(cancelButton, &QPushButton::clicked, this, &QDialog::reject); | ||
| connect(m_uploadButton, &QPushButton::clicked, this, &QDialog::accept); | ||
| connect(m_projectCombo, &QComboBox::currentIndexChanged, this, [this](int) { | ||
| m_uploadButton->setEnabled(hasSelectedProject()); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Instrument upload dialog actions and dependency scan with breadcrumbs.
This adds a major user-facing workflow, but no ui.action breadcrumbs are emitted for cancel/upload/selection interactions, and no significant-operation breadcrumb is emitted for dependency detection.
🔧 Proposed fix
+#include "SentryReporter.h"
+
- connect(cancelButton, &QPushButton::clicked, this, &QDialog::reject);
- connect(m_uploadButton, &QPushButton::clicked, this, &QDialog::accept);
+ connect(cancelButton, &QPushButton::clicked, this, [this]() {
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("cloud.upload_dialog.cancel"));
+ reject();
+ });
+ connect(m_uploadButton, &QPushButton::clicked, this, [this]() {
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("cloud.upload_dialog.confirm_upload"));
+ accept();
+ });
connect(m_projectCombo, &QComboBox::currentIndexChanged, this, [this](int) {
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("cloud.upload_dialog.project_changed"));
m_uploadButton->setEnabled(hasSelectedProject());
});
@@
void CloudUploadDialog::rebuildDependencyList()
{
m_dependencyList->clear();
+ SentryReporter::addBreadcrumb(QStringLiteral("file.import"),
+ QStringLiteral("cloud.upload_dialog.detect_dependencies"));
m_dependencies = DependencyResolver::detect(m_mainAssetPath);As per coding guidelines: “Add Sentry breadcrumbs for all user-facing actions and significant operations using SentryReporter::addBreadcrumb() with categories: ui.action … file.import/file.export for I/O”.
Also applies to: 105-109
🤖 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 58 - 63, Add Sentry breadcrumbs for
the user actions wired in this dialog: call SentryReporter::addBreadcrumb(...)
with category "ui.action" when cancelButton is clicked (in the cancel connect),
when m_uploadButton is clicked (in the upload connect) and when m_projectCombo
currentIndexChanged fires (inside the existing lambda before/after calling
hasSelectedProject()). Also add a breadcrumb for the dependency detection
operation (the significant scan/parse code referenced around lines 105-109)
using SentryReporter::addBreadcrumb(...) with an appropriate category such as
"file.import" or "file.export" and a short message describing the operation;
include any pertinent metadata (selected project id/name) where available.
Ensure breadcrumb text is concise and use the same SentryReporter API
everywhere.
Source: Coding guidelines
Comment on lines
+5151
to
+5159
| PackageMetadata manifest = ProjectPackager::buildManifest(filePath, {}, projectName); | ||
| const bool runScan = !args.contains(QStringLiteral("scan")) || args.value(QStringLiteral("scan")).toBool(true); | ||
| if (runScan) { | ||
| ScanConfig config; | ||
| config.roots = {QFileInfo(filePath).absolutePath()}; | ||
| config.includePatterns = {QFileInfo(filePath).fileName()}; | ||
| manifest.scanSummary = ScanEngine::scanReportToJsonObject( | ||
| ScanEngine::run(config, config.roots.first())); | ||
| } |
There was a problem hiding this comment.
cloud_upload currently uploads only the root asset.
ProjectPackager::buildManifest(filePath, {}, projectName) hard-codes an empty dependency list, and the optional scan result is only stored in manifest.scanSummary. None of the discovered sidecar files are added back into manifest.files, so textures/materials referenced by the asset never make it into the uploaded package.
🤖 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/MCPServer.cpp` around lines 5151 - 5159, ProjectPackager::buildManifest
is being called with an empty dependency list and although
ScanEngine::run()/scanReportToJsonObject stores results in manifest.scanSummary,
the discovered sidecar files are never added to manifest.files so only the root
asset is uploaded; fix this by extracting the discovered file paths from the
scan report (use ScanEngine::run return or the JSON from
ScanEngine::scanReportToJsonObject) and either (a) pass that dependency list
into ProjectPackager::buildManifest instead of {} or (b) after building the
manifest, append the discovered paths into manifest.files (and update any
metadata entries needed) so textures/materials discovered by ScanConfig are
included in the uploaded package; refer to ProjectPackager::buildManifest,
manifest.files, manifest.scanSummary, ScanEngine::run and
ScanEngine::scanReportToJsonObject when making the change.
Link cloud upload sources into unit tests, fix cloud CLI argument parsing, harden project packaging paths, guard async cloud session callbacks, reuse sessions across auth changes, and run MCP pre-upload scans in a subprocess. 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 `@src/CloudAccountMenuButton.cpp`:
- Line 346: The refresh() method currently force-enables m_uploadAction
(m_uploadAction->setEnabled(true)), which overrides setUploadEnabled(hasAsset)
and MainWindow::updateCloudUploadActionState(); remove that unconditional enable
(or change it to respect the current upload-enabled state) so refresh() does not
re-enable the action—locate the m_uploadAction->setEnabled(true) call in
refresh() and delete it (or replace it with logic that queries the existing
upload-enabled flag or calls setUploadEnabled(hasAsset) instead).
🪄 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: 7ef82fef-f4b7-4fd2-b750-27dbd308bb8b
📒 Files selected for processing (9)
src/CloudAccountMenuButton.cppsrc/CloudCLIPipeline.cppsrc/DependencyResolver.cppsrc/MCPServer.cppsrc/ProjectPackager.cppsrc/QtMeshCloudSession.cppsrc/mainwindow.cppsrc/mainwindow.htests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- src/mainwindow.h
- src/MCPServer.cpp
- src/DependencyResolver.cpp
- src/QtMeshCloudSession.cpp
- src/ProjectPackager.cpp
- src/mainwindow.cpp
- src/CloudCLIPipeline.cpp
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@src/CloudAccountMenuButton.cpp`:
- Line 346: The refresh() method currently force-enables m_uploadAction
(m_uploadAction->setEnabled(true)), which overrides setUploadEnabled(hasAsset)
and MainWindow::updateCloudUploadActionState(); remove that unconditional enable
(or change it to respect the current upload-enabled state) so refresh() does not
re-enable the action—locate the m_uploadAction->setEnabled(true) call in
refresh() and delete it (or replace it with logic that queries the existing
upload-enabled flag or calls setUploadEnabled(hasAsset) instead).
🪄 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: 7ef82fef-f4b7-4fd2-b750-27dbd308bb8b
📒 Files selected for processing (9)
src/CloudAccountMenuButton.cppsrc/CloudCLIPipeline.cppsrc/DependencyResolver.cppsrc/MCPServer.cppsrc/ProjectPackager.cppsrc/QtMeshCloudSession.cppsrc/mainwindow.cppsrc/mainwindow.htests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- src/mainwindow.h
- src/MCPServer.cpp
- src/DependencyResolver.cpp
- src/QtMeshCloudSession.cpp
- src/ProjectPackager.cpp
- src/mainwindow.cpp
- src/CloudCLIPipeline.cpp
🛑 Comments failed to post (1)
src/CloudAccountMenuButton.cpp (1)
346-346:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid overriding upload availability in
refresh()Line 346 force-enables
m_uploadAction, which undoessetUploadEnabled(hasAsset)whenever the menu opens. For signed-in users with no asset, this makes the action clickable again despite the intended disabled state fromMainWindow::updateCloudUploadActionState().Suggested fix
@@ - m_openProjectsAction->setEnabled(signedIn); - m_uploadAction->setEnabled(true); + m_openProjectsAction->setEnabled(signedIn);🤖 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/CloudAccountMenuButton.cpp` at line 346, The refresh() method currently force-enables m_uploadAction (m_uploadAction->setEnabled(true)), which overrides setUploadEnabled(hasAsset) and MainWindow::updateCloudUploadActionState(); remove that unconditional enable (or change it to respect the current upload-enabled state) so refresh() does not re-enable the action—locate the m_uploadAction->setEnabled(true) call in refresh() and delete it (or replace it with logic that queries the existing upload-enabled flag or calls setUploadEnabled(hasAsset) instead).
Skip Ogre scene dependency probing unless a live editor session exists so unit tests do not crash after earlier Manager singleton use, and instrument cloud project/upload dialogs with Sentry ui.action breadcrumbs. Co-authored-by: Cursor <cursoragent@cursor.com>
…tate ProjectPackager no longer walks the Ogre scene during manifest building, which fixes the ProjectPackager unit-test segfault on CI. CloudAccountMenuButton refresh now respects the asset availability set by setUploadEnabled. Co-authored-by: Cursor <cursoragent@cursor.com>
jsonPassesPathSanitisationLint iterated a temporary QJsonObject twice, which segfaulted ProjectPackager.SanitisedManifestJson on Linux CI. Co-authored-by: Cursor <cursoragent@cursor.com>
|
4 tasks
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
qtmesh cloud/ MCP cloud tools.Test plan
/v1/projects.qtmesh cloud upload model.fbxheadlessly against a signed-in session.cloud_list_projects,cloud_upload, andcloud_delete_project.Made with Cursor
Summary by CodeRabbit
cloudsubcommands (login/logout/status/list/delete/upload) and updated command routing/help.