[codex] Add QtMeshEditor model management tab - #907
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughChangesThe PR adds QtMeshEditor model catalog
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AISettingsDialog
participant AIModelCatalog
participant ModelDownloader
participant ModelFiles
User->>AISettingsDialog: Select model and choose Download
AISettingsDialog->>AIModelCatalog: downloadModel(id)
AIModelCatalog->>ModelDownloader: startDownload(url, path, label)
ModelDownloader->>ModelFiles: Save model file
ModelDownloader-->>AIModelCatalog: downloadCompleted
AIModelCatalog-->>AISettingsDialog: Update status and model list
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/LLMSettingsWidget_test.cpp (1)
32-50: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftExercise the new catalog controls, not only their tab label.
Add tests for selection-dependent actions, busy-state gating, and dispatch of download/delete/bulk operations.
As per coding guidelines, “Add Google Test unit tests for new functionality.”
🤖 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/LLMSettingsWidget_test.cpp` around lines 32 - 50, Extend LLMSettingsWidgetTest beyond HasTabWidget and TabNames to exercise the new catalog controls: verify selection-dependent actions, busy-state gating, and dispatch of download, delete, and bulk operations. Use the widget’s existing control and handler symbols, and add Google Test coverage for the enabled/disabled states and emitted operation dispatches.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 `@qml/AISettingsDialog.qml`:
- Around line 514-517: Add a per-model confirmation step to the
Local.ThemedButton deletion flow before invoking
AIModelCatalog.deleteModel(modelData.id). Reuse the existing confirmation-dialog
pattern used by Remove All or the widget, passing the selected model’s identity
and only deleting after the user confirms; preserve the current enabled
condition.
In `@src/AIModelCatalog.cpp`:
- Around line 366-400: Add SentryReporter::addBreadcrumb calls throughout
AIModelCatalog::downloadModel and the related completion, failure, cancellation,
and deletion handlers. Record each operation’s start and final outcome,
including the model identifier/name and relevant failure or cancellation
details, while preserving existing status and control flow.
- Around line 451-460: Update the model-removal logic around findSpec and the
file-deletion loop to avoid deleting files still required by other installed
ModelSpec entries. Before removing each file and its .part companion, check
whether the path is referenced by another installed model, and only delete
unshared paths while preserving the existing removal behavior for exclusively
owned files.
- Around line 125-157: Update the model download and installation flow
associated with the base URL variables such as pbrBase, rembgBase, and
captionBase to use immutable, pinned revisions instead of mutable resolve/main
URLs. Add cataloged SHA-256 digests for every model artifact, verify each
downloaded file against its expected digest before marking it complete or
installing it, and reject mismatches so unverified ONNX/GGUF data is never
loaded.
- Around line 456-464: Update the deletion logic around removedAny for both
affected paths so it distinguishes missing files from failed removals: check
whether each path exists before attempting QFile::remove, collect existing paths
that could not be removed, and report those remaining files. Only use the
successful “Deleted” status when no existing files remain; preserve the no-files
message when nothing existed.
In `@src/LLMSettingsWidget.cpp`:
- Around line 739-793: Add a SentryReporter::addBreadcrumb call with ui.action
to each catalog action handler: onAIModelCatalogDownloadClicked,
onAIModelCatalogDownloadAllClicked, onAIModelCatalogDeleteClicked,
onAIModelCatalogDeleteAllClicked, onAIModelCatalogRefreshClicked, and
onAIModelCatalogOpenFolderClicked. Record the corresponding action only when the
user action proceeds, after any null-item or confirmation cancellation returns.
In `@src/LLMSettingsWidget.h`:
- Around line 17-18: Restore the ENABLE_STABLE_DIFFUSION compile-time boundary
around all Stable Diffusion dependencies. In src/LLMSettingsWidget.h at lines
17-18, 50-59, and 134-150, guard the SDManager include, slots, and widget
members; in src/LLMSettingsWidget.cpp at lines 29-45, 88-95, and 606-609, guard
initialization/signal wiring, SD tab construction, and post-download scanning;
in src/LLMSettingsWidget_test.cpp at lines 37-50, condition tab expectations on
ENABLE_STABLE_DIFFUSION. Use `#ifdef` ENABLE_STABLE_DIFFUSION consistently around
the affected includes and calls.
---
Outside diff comments:
In `@src/LLMSettingsWidget_test.cpp`:
- Around line 32-50: Extend LLMSettingsWidgetTest beyond HasTabWidget and
TabNames to exercise the new catalog controls: verify selection-dependent
actions, busy-state gating, and dispatch of download, delete, and bulk
operations. Use the widget’s existing control and handler symbols, and add
Google Test coverage for the enabled/disabled states and emitted operation
dispatches.
🪄 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: e8fc88be-6903-4eaa-92c1-1ace3d53eafb
📒 Files selected for processing (12)
qml/AISettingsDialog.qmlsrc/AIModelCatalog.cppsrc/AIModelCatalog.hsrc/CMakeLists.txtsrc/LLMSettingsWidget.cppsrc/LLMSettingsWidget.hsrc/LLMSettingsWidget_test.cppsrc/MaterialEditorQML.cppsrc/MaterialEditorQML_qml_test.cppsrc/main.cppsrc/mainwindow.cpptests/CMakeLists.txt
| const QString pbrBase = resolveBaseUrl( | ||
| "ai/pbrModelBaseUrl", "QTMESH_PBR_MODEL_BASE_URL", | ||
| "https://huggingface.co/fernandotonon/QtMeshEditor-models/resolve/main/"); | ||
| const QString rembgBase = resolveBaseUrl( | ||
| "ai/rembgModelBaseUrl", "QTMESH_REMBG_MODEL_BASE_URL", | ||
| "https://huggingface.co/fernandotonon/QtMeshEditor-models/resolve/main/rembg/"); | ||
| const QString triposrBase = resolveBaseUrl( | ||
| "ai/triposrModelBaseUrl", "QTMESH_TRIPOSR_MODEL_BASE_URL", | ||
| "https://huggingface.co/fernandotonon/QtMeshEditor-models/resolve/main/triposr/"); | ||
| const QString triposgBase = resolveBaseUrl( | ||
| "ai/triposgModelBaseUrl", "QTMESH_TRIPOSG_MODEL_BASE_URL", | ||
| "https://huggingface.co/fernandotonon/QtMeshEditor-models/resolve/main/triposg/"); | ||
| const QString unirigBase = resolveBaseUrl( | ||
| "ai/unirigModelBaseUrl", "QTMESH_UNIRIG_MODEL_BASE_URL", | ||
| "https://huggingface.co/fernandotonon/QtMeshEditor-models/resolve/main/unirig/"); | ||
| const QString skinTokensBase = resolveBaseUrl( | ||
| "ai/skintokensModelBaseUrl", "QTMESH_SKINTOKENS_MODEL_BASE_URL", | ||
| "https://huggingface.co/fernandotonon/QtMeshEditor-models/resolve/main/skintokens/"); | ||
| const QString inbetweenBase = resolveBaseUrl( | ||
| "ai/inbetweenModelBaseUrl", "QTMESH_INBETWEEN_MODEL_BASE_URL", | ||
| "https://huggingface.co/fernandotonon/QtMeshEditor-models/resolve/main/inbetween/"); | ||
| const QString motionBase = resolveBaseUrl( | ||
| "ai/motionLibraryBaseUrl", "QTMESH_MOTION_LIBRARY_BASE_URL", | ||
| "https://huggingface.co/fernandotonon/QtMeshEditor-models/resolve/main/motion/"); | ||
| const QString t2mBase = resolveBaseUrl( | ||
| "ai/t2mModelBaseUrl", "QTMESH_T2M_MODEL_BASE_URL", | ||
| "https://huggingface.co/fernandotonon/QtMeshEditor-models/resolve/main/motion/"); | ||
| const QString segmentBase = resolveBaseUrl( | ||
| "ai/segmentModelBaseUrl", "QTMESH_SEGMENT_MODEL_BASE_URL", | ||
| "https://huggingface.co/fernandotonon/QtMeshEditor-models/resolve/main/segment/"); | ||
| const QString captionBase = resolveBaseUrl( | ||
| "ai/captionModelBaseUrl", "QTMESH_CAPTION_MODEL_BASE_URL", | ||
| "https://huggingface.co/fernandotonon/QtMeshEditor-models/resolve/main/caption/"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Verify downloaded model integrity before installation.
The mutable resolve/main URLs are downloaded without an expected digest. An upstream replacement or compromise would be accepted and later loaded as trusted ONNX/GGUF data. Pin immutable revisions and verify a cataloged SHA-256 before marking each file complete.
Also applies to: 503-515
🤖 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/AIModelCatalog.cpp` around lines 125 - 157, Update the model download and
installation flow associated with the base URL variables such as pbrBase,
rembgBase, and captionBase to use immutable, pinned revisions instead of mutable
resolve/main URLs. Add cataloged SHA-256 digests for every model artifact,
verify each downloaded file against its expected digest before marking it
complete or installing it, and reject mismatches so unverified ONNX/GGUF data is
never loaded.
![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: b0c7911968
ℹ️ 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".
| m_tabWidget->addTab(sdModelsTab, "SD Models"); | ||
| m_tabWidget->addTab(sdSettingsTab, "SD Settings"); |
There was a problem hiding this comment.
In builds using the default ENABLE_STABLE_DIFFUSION=OFF, this dialog now still adds the SD Models/Settings tabs after the feature guard was removed. Those controls can offer SD model downloads and load/settings actions even though SDWorker will only report that Stable Diffusion support is not enabled; the QML settings dialog still hides the same tab via stableDiffusionEnabled, so the top-menu widget should keep the SD tabs guarded or otherwise hide/disable them for non-SD builds.
Useful? React with 👍 / 👎.
45fff63 to
411382d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/LLMSettingsWidget.cpp (1)
600-623: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winReturn before generic LLM/SD completion handling for catalog files.
Although
catalogDownloadis detected, each queued artifact still re-enables generic controls and rescans all LLM/SD models. Bulk catalog downloads repeat this work for every file.Proposed fix
const bool catalogDownload = AIModelCatalog::instance()->busy() && !AIModelCatalog::instance()->activeModelId().isEmpty(); + if (catalogDownload) + return; + m_downloadButton->setEnabled(true);- updateAIModelCatalogList(); - - if (!catalogDownload) { - QMessageBox::information(this, "Download Complete", - QString("Model %1 has been downloaded successfully.").arg(modelName)); - } + QMessageBox::information(this, "Download Complete", + QString("Model %1 has been downloaded successfully.").arg(modelName));🤖 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/LLMSettingsWidget.cpp` around lines 600 - 623, Use the existing catalogDownload condition in the download-completion handler to return before generic LLM/Stable Diffusion completion handling. Catalog artifacts should refresh only the catalog list and avoid re-enabling generic download controls, rescanning LLM/SD models, updating their lists, or showing the generic completion dialog for each file; preserve the current behavior for non-catalog downloads.
♻️ Duplicate comments (1)
src/AIModelCatalog.cpp (1)
472-479: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOnly preserve shared files when another model variant is installed.
Existence of the shared path comes from the selected model itself, so this marks every referenced path as shared even when the other variant is absent. Deleting TripoSR/TripoSG can consequently leave large files and the selected model in “Partial” state.
Proposed fix
for (const ModelSpec& other : std::as_const(owner)) { - if (other.id == spec->id) + if (other.id == spec->id || !isDownloaded(other)) continue; for (const FileSpec& otherFile : other.files) { - if (QFileInfo::exists(otherFile.path)) - sharedPaths.insert(otherFile.path); + sharedPaths.insert(otherFile.path); } }🤖 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/AIModelCatalog.cpp` around lines 472 - 479, Update the shared-path collection around the owner iteration to include a path only when another model variant is actually installed, rather than relying solely on QFileInfo::exists(otherFile.path). Use the available installation/state information for each other ModelSpec, while preserving the self-ID exclusion and existing sharedPaths behavior.
🧹 Nitpick comments (1)
src/AIModelCatalog.cpp (1)
51-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClassify model downloads as
file.importbreadcrumbs.Use
file.importconsistently for download starts and terminal outcomes instead ofai.models.As per coding guidelines, “use
file.import/file.exportfor I/O.”Also applies to: 77-79, 406-407, 449-450, 586-600
🤖 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/AIModelCatalog.cpp` around lines 51 - 64, Update Sentry breadcrumb categories in AIModelCatalog download handling to use file.import instead of ai.models for download starts and all terminal outcomes, including the referenced success, failure, and queued-download paths. Preserve the existing breadcrumb messages and severity while applying the category consistently throughout the identified locations.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/AIModelCatalog.cpp`:
- Around line 35-38: Update AIModelCatalog::qmlInstance to mark the returned
process-wide singleton with QJSEngine::setObjectOwnership using CppOwnership
before returning it, ensuring QML does not delete the instance while preserving
the existing instance() return behavior.
In `@src/LLMSettingsWidget.cpp`:
- Around line 54-62: Remove the activeModelChanged connection and its lambda
from LLMSettingsWidget, leaving statusMessageChanged as the sole catalog
status-label update path. Preserve the existing statusMessageChanged handler so
download start, completion, failure, and cancellation messages remain visible
without being overwritten.
- Around line 806-813: Update
LLMSettingsWidget::onAIModelCatalogOpenFolderClicked to create the ai_models
directory with QDir before opening it, then check the boolean result from
QDesktopServices::openUrl and report failure through the existing
UI/error-reporting mechanism instead of failing silently.
---
Outside diff comments:
In `@src/LLMSettingsWidget.cpp`:
- Around line 600-623: Use the existing catalogDownload condition in the
download-completion handler to return before generic LLM/Stable Diffusion
completion handling. Catalog artifacts should refresh only the catalog list and
avoid re-enabling generic download controls, rescanning LLM/SD models, updating
their lists, or showing the generic completion dialog for each file; preserve
the current behavior for non-catalog downloads.
---
Duplicate comments:
In `@src/AIModelCatalog.cpp`:
- Around line 472-479: Update the shared-path collection around the owner
iteration to include a path only when another model variant is actually
installed, rather than relying solely on QFileInfo::exists(otherFile.path). Use
the available installation/state information for each other ModelSpec, while
preserving the self-ID exclusion and existing sharedPaths behavior.
---
Nitpick comments:
In `@src/AIModelCatalog.cpp`:
- Around line 51-64: Update Sentry breadcrumb categories in AIModelCatalog
download handling to use file.import instead of ai.models for download starts
and all terminal outcomes, including the referenced success, failure, and
queued-download paths. Preserve the existing breadcrumb messages and severity
while applying the category consistently throughout the identified locations.
🪄 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: b984d540-4183-4c19-9ecc-3d443e215938
📒 Files selected for processing (12)
qml/AISettingsDialog.qmlsrc/AIModelCatalog.cppsrc/AIModelCatalog.hsrc/CMakeLists.txtsrc/LLMSettingsWidget.cppsrc/LLMSettingsWidget.hsrc/LLMSettingsWidget_test.cppsrc/MaterialEditorQML.cppsrc/MaterialEditorQML_qml_test.cppsrc/main.cppsrc/mainwindow.cpptests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- src/CMakeLists.txt
- src/MaterialEditorQML.cpp
- src/main.cpp
- src/MaterialEditorQML_qml_test.cpp
- src/LLMSettingsWidget_test.cpp
- src/AIModelCatalog.h
- qml/AISettingsDialog.qml
ae193ad to
4f3bdfc
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/AIModelCatalog.cpp`:
- Around line 53-54: Replace the generic “ai.models” category in the model
file-operation breadcrumbs with “file.import” for all download/import lifecycle
events and “file.export” for all deletion/export lifecycle events, including the
additional referenced locations. Keep the existing breadcrumb messages and
behavior unchanged.
- Around line 459-464: Update AIModelCatalog::deleteModel and the other deletion
method around the referenced second location to block deletion when either
m_busy or the shared singleton downloader is active. Reuse the same
downloader-active condition used by both download entry points, and preserve the
existing status message and early return behavior.
- Around line 474-501: Update the sharedPaths construction in the model-removal
logic to include files only from other models that are actually installed,
rather than every catalog entry in owner. Use the existing installed-state
marker or validation used by the surrounding model-management code, while
preserving the current removal and keptSharedPaths behavior for genuinely
installed shared files.
- Around line 448-454: Update the active operation state around the all-model
download flow, including the symbols that set m_activeModelName and the
completion handling near the all-model download result, so unavailable
build-gated models are not represented as having been downloaded. Preserve a
distinct operation name/status indicating that only available QtMeshEditor
models were downloaded when any models are unavailable.
🪄 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: 8abff1d3-257b-4d25-8995-0dde6b71dfe0
📒 Files selected for processing (12)
qml/AISettingsDialog.qmlsrc/AIModelCatalog.cppsrc/AIModelCatalog.hsrc/CMakeLists.txtsrc/LLMSettingsWidget.cppsrc/LLMSettingsWidget.hsrc/LLMSettingsWidget_test.cppsrc/MaterialEditorQML.cppsrc/MaterialEditorQML_qml_test.cppsrc/main.cppsrc/mainwindow.cpptests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (10)
- src/CMakeLists.txt
- tests/CMakeLists.txt
- src/LLMSettingsWidget.h
- src/MaterialEditorQML_qml_test.cpp
- src/AIModelCatalog.h
- src/mainwindow.cpp
- src/main.cpp
- src/LLMSettingsWidget_test.cpp
- src/MaterialEditorQML.cpp
- src/LLMSettingsWidget.cpp
4f3bdfc to
77e7273
Compare
|
|
Review follow-up on the model integrity thread: I’m leaving SHA-256 verification/pinned immutable revisions out of this PR because the catalog/repo does not currently include authoritative digests for the upstream model artifacts. Adding this safely needs a trusted digest inventory from the model publishing/release process; I did not invent hashes or pin arbitrary revisions in this UI-focused change. All other actionable review feedback has been addressed in the current branch. |
1 participant
Summary
Features
Bugfixes
MaterialEditorQML_testlinking by including Stable Diffusion manager sources in the shared test support target.ENABLE_STABLE_DIFFUSIONguards so SD tabs are hidden in non-SD builds.Technical Details
AIModelCatalogas a QML singleton backed byModelDownloaderand app data storage underai_models.Verification
git diff --checkcmake --build build --target MaterialEditorQML_test -j2QT_QPA_PLATFORM=offscreen ./build/debug/MaterialEditorQML_qml_test --gtest_filter=QMLComponentLoadingTest.AISettingsDialogLoadsWithoutErrorscmake --build build --target QtMeshEditor -j2cmake --build build_onnx_cpu --target QtMeshEditor -j2Summary by CodeRabbit
New Features
Tests