Skip to content

fix(macos): keychain prompts, session persistence, texture apply, mesh-tex generation - #718

Merged
fernandotonon merged 6 commits into
masterfrom
fix/texture-apply-and-keychain-prompts
Jun 13, 2026
Merged

fix(macos): keychain prompts, session persistence, texture apply, mesh-tex generation#718
fernandotonon merged 6 commits into
masterfrom
fix/texture-apply-and-keychain-prompts

Conversation

@fernandotonon

@fernandotonon fernandotonon commented Jun 13, 2026

Copy link
Copy Markdown
Owner

Fixes a cluster of macOS / Material-Editor issues found while testing the AI texture features.

What was broken & fixed

  1. Repeated macOS keychain prompts at startup (4×). CloudCredentialStore stored the cloud session in the OS secret store (Keychain/Credential Manager/libsecret); every macOS keychain read raises a prompt and the account control reads the session several times at launch. → Session now stored in QSettings (per-user prefs, no prompt). Removed the Security.framework/wincred/libsecret code, the CloudCredentialStore_linux.* files, and their CMake/link wiring. Kept an in-process cache.

  2. Cloud session not persisting (logged out each launch). The sign-in handler removed the cloudToken/expiresAt/email QSettings keys right after saveSession() wrote them (leftover keychain-era cleanup). → Removes dropped.

  3. Applying a texture didn't show on the model (only the preview updated):

    • RTShaderHelper::wirePbrSlotsForFFP now wires an unnamed/numeric diffuse TUS (e.g. an Assimp-imported "0" slot) as the diffuse fallback when there's no named albedo/diffuse_map slot — previously only named slots got the FFP colour-op, so the texture loaded but never rendered.
    • MaterialEditorQML::ensureTextureInMaterialGroup loads the chosen texture into the material's resource group before binding, so the RTSS-rendered mesh can resolve it (was resolving against the wrong group → yellow placeholder while the disk-reading preview looked fine).
    • updateMaterialText re-applies via SubEntity::setMaterial(ptr) instead of setMaterialName(sameName) (stale technique pointer).
  4. Plain AI (txt2img) texture not applying: onSDGenerationCompleted now routes the result through setTextureName() (group resolution + RTSS rebind) instead of a raw texUnit->setTextureName().

  5. "Use selected mesh (depth-conditioned)" never started: QML called MaterialEditorQML.hasSelectedMesh(), but it's a Q_PROPERTY → the call threw "not a function" and silently aborted the handler. Now read as a property. Missing-ControlNet emits a non-fatal sdGenerationNotice (was sdGenerationError, which reset the run) so it proceeds in prompt-only mode. The checkbox no longer unchecks itself when generation starts (auto-uncheck keys off selection loss, not the disabled state).

  6. Logged-out account icon blank in deployed builds: the avatar used an SVG QIcon that renders empty without the Qt SVG icon-engine plugin (showed locally, vanished in the installed app). The glyph is now drawn with QPainter for both states — no plugin dependency.

  7. Docs: Homebrew install now includes brew trust fernandotonon/qtmesheditor (Homebrew now requires trusting third-party taps before loading their casks — this is the new expected workflow, not a cask bug).

Testing

  • CloudCredentialStore_test.cpp updated for the QSettings backend (round-trip, cache, no-op migration).
  • Verified interactively on macOS: no keychain prompts, session persists across restarts, manual + plain-AI + mesh-conditioned texture generation all apply to the model, logged-out avatar renders.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added non-fatal status notices for Stable Diffusion generation events.
  • Bug Fixes

    • Fixed mesh-conditioned texture generation and checkbox behavior during generation.
    • Improved material texture resolution and rendering; better handling for materials lacking explicit diffuse slots.
    • Updated account/avatar rendering to show a generic profile glyph when appropriate.
  • Documentation

    • Added macOS Homebrew trust step to installation docs.
  • Chores

    • Bumped project release to 3.5.3; updated packaging and publishing workflow and reduced reliance on OS secret-store integration.
  • Tests

    • Updated tests and added a cache-reset hook for session handling.

fernandotonon and others added 2 commits June 12, 2026 17:23
Two startup/usability fixes:

1. macOS keychain prompt spam. CloudAccountMenuButton::refresh() reads the
   cloud session 3-4 times during startup (migrate + hasSession +
   cloudDisplayName), and on macOS every SecItemCopyMatching can raise a
   "QtMeshEditor wants to use confidential information" dialog. Add an
   in-process cache to CloudCredentialStore so the OS keychain/credential
   store is queried at most once per process. Save primes the cache, clear
   sets it to the empty state, so post-sign-in/out refreshes never re-hit
   the OS. Also set kSecAttrAccessibleAfterFirstUnlock so the item is
   readable without an interactive unlock each session.

2. "Applying texture to the model does nothing" for materials whose diffuse
   TUS has a numeric/empty name (Assimp-imported "0" slot, user-added unit).
   RTShaderHelper::wirePbrSlotsForFFP only wired NAMED slots, so the diffuse
   never got its FFP modulate op and the texture loaded but never rendered.
   Add a fallback: when a pass exposes no recognized albedo/diffuse_map slot,
   the first plain textured slot receives the diffuse modulate op. Fixes the
   manual setTextureName path, plain txt2img, and the MCP set_texture path in
   one place.

Tests: cache-serves-reads and corrupt-file cache-reset coverage.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…h-tex generation

Several related macOS / Material-Editor fixes reported in testing:

1. Repeated macOS keychain prompts at startup. CloudCredentialStore no longer
   uses the OS secret store (Keychain / Credential Manager / libsecret) — every
   macOS keychain read raised a confirmation dialog and the account control reads
   the session several times at launch. The session now lives in QSettings (the
   per-user preference store, no prompt). Removes the Security.framework / wincred
   / libsecret code, the CloudCredentialStore_linux.* files, and their CMake/link
   wiring. Keeps an in-process cache to collapse the startup reads.

2. Cloud session not persisting (user logged out each launch). The device-code
   sign-in handler removed the cloudToken/expiresAt/email QSettings keys right
   after saveSession() wrote them — leftover keychain-era cleanup. Those keys are
   now the storage, so the removes are dropped.

3. Applying a texture didn't show on the model (only the preview updated):
   - RTShaderHelper::wirePbrSlotsForFFP now wires an unnamed/numeric diffuse TUS
     (e.g. an Assimp-imported "0" slot) as the diffuse fallback when no
     albedo/diffuse_map slot exists — previously only named slots got the FFP
     colour-op, so the texture loaded but never rendered.
   - MaterialEditorQML::ensureTextureInMaterialGroup loads the chosen texture
     into the MATERIAL's resource group before binding, so the RTSS-rendered
     mesh can resolve it (it was resolving against the wrong group → yellow
     placeholder while the disk-reading preview looked fine).
   - updateMaterialText re-applies via SubEntity::setMaterial(ptr) instead of
     setMaterialName(sameName), which could keep a stale technique pointer.

4. Plain AI (txt2img) texture not applying: onSDGenerationCompleted now routes
   the generated texture through setTextureName() (group resolution + RTSS
   rebind) instead of a raw texUnit->setTextureName().

5. "Use selected mesh (depth-conditioned)" never started: the QML called
   MaterialEditorQML.hasSelectedMesh() but it is exposed as a Q_PROPERTY, so the
   call threw "not a function" and silently aborted the handler. Read it as a
   property. Also: missing-ControlNet now emits a non-fatal sdGenerationNotice
   (was sdGenerationError, which reset the in-flight run) so generation proceeds
   in prompt-only mode. The "Use selected mesh" checkbox no longer unchecks
   itself when generation starts (auto-uncheck now keys off selection loss, not
   the disabled state).

6. Logged-out account icon blank in deployed builds: the avatar used an SVG
   QIcon that renders empty without the Qt SVG icon-engine plugin (showed
   locally, vanished in the installed app). The avatar glyph is now drawn with
   QPainter for both states — no plugin dependency.

7. Docs: Homebrew install now includes `brew trust fernandotonon/qtmesheditor`
   (Homebrew now requires trusting third-party taps before loading their casks).

Tests updated for the QSettings-backed CloudCredentialStore.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@fernandotonon, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 48 minutes and 29 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 279fd7ee-edee-4bb3-b262-060e1c140d1e

📥 Commits

Reviewing files that changed from the base of the PR and between 175fbfa and 3705dc4.

📒 Files selected for processing (1)
  • src/CMakeLists.txt
📝 Walkthrough

Walkthrough

This PR migrates QtMeshEditor's cloud session storage to per-user QSettings with an in-process cache, drops libsecret wiring, updates tests and CMake, refactors avatar rendering, improves material/RTSS texture resolution and Stable Diffusion flows, refines PBR diffuse fallback logic, and updates macOS packaging/docs and pinned release refs to 3.5.3.

Changes

Cloud Credential Storage Migration & Rendering Improvements

Layer / File(s) Summary
Cloud credential storage: QSettings-based persistence
src/CloudCredentialStore.h, src/CloudCredentialStore.cpp, src/CloudCredentialStore_test.cpp, src/CloudCredentialStore_linux.h, src/CloudCredentialStore_linux.c, src/mainwindow.cpp
CloudCredentialStore now stores token/expiry/email in per-user QSettings with an in-process cache and adds resetCacheForTesting(). Legacy secret-store/read-from-file migration is preserved only when needed; Linux libsecret implementation and headers are removed; tests updated for QSettings behavior and cache semantics.
Build system cleanup: Remove OS secret store dependencies
src/CMakeLists.txt
Remove libsecret detection and libsecret wiring; document QSettings-backed storage; keep one-time legacy read links for main app (macOS Security / Windows advapi32); remove UnitTests legacy secret-store linkage.
Avatar rendering refactoring
src/CloudAccountMenuButton.cpp
Simplify AvatarButton to draw avatars/person glyph in paintEvent, remove SVG logged-out icon member and separate SVG path.
Material texture resolution and SD generation improvements
src/MaterialEditorQML.h, src/MaterialEditorQML.cpp, qml/TexturePropertiesPanel.qml
Add ensureTextureInMaterialGroup() to register/load textures into material resource group; force sub-entity rebind via material pointer; emit non-fatal sdGenerationNotice when depth model missing; force diffuse assignment on SD completion; QML treats hasSelectedMesh as a property, adds onSdGenerationNotice handling, and updates mesh-checkbox clearing logic.
PBR shader slot wiring improvements
src/RTShaderHelper.cpp
Refactor wirePbrSlotsForFFP to detect named diffuse slot and choose first eligible unnamed fallback (excluding known non-diffuse slots), then apply FFP wiring accordingly.
Packaging and docs: Homebrew cask, workflow, pins
packaging/macos/qtmesheditor.rb.in, .github/workflows/deploy.yml, README.md, website/src/DocsApp.jsx, website/src/data/content.js, website/src/hooks/useQtmeshActionRef.js, CMakeLists.txt, DEBIAN-control.in
Add canonical Homebrew cask template and regenerate cask from template in workflow; update README and website to include brew trust for tap and bump action/container pins to 3.5.3; update project version to 3.5.3; remove libsecret-1-0 from Debian control Depends.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐰 I hopped from secrets to settings light,

Cached tokens snug and migrations right,
Avatars sketched with painter's cheer,
Textures found and shaders clear,
Brew trust tapped, the release takes flight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.79% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly addresses the main fixes in the changeset: macOS keychain prompts, session persistence, texture application, and mesh-based texture generation issues.
Description check ✅ Passed The description covers the repository's template structure with a detailed 'What was broken & fixed' section, technical details, and testing verification, though it doesn't use the exact template sections specified.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/texture-apply-and-keychain-prompts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 47aa978d59

ℹ️ 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".

Comment thread src/CloudCredentialStore.cpp Outdated
Comment on lines +103 to +106
// Tokens already live in QSettings under the same keys, so there is nothing
// to migrate. Retained as a no-op so existing callers (the account control's
// refresh) keep compiling, and as the hook point should the storage backend
// change again.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve existing secret-store sessions during migration

For users upgrading from a build that saved the cloud session in Keychain/Credential Manager/libsecret, this no-op leaves the only stored token behind in the old backend while loadSession() now reads only QSettings. In that upgrade path the account control calls this migration hook and then hasSession(), so previously signed-in users are treated as logged out until they authenticate again; the migration should read the old secret/fallback location once and copy it into the new QSettings keys before dropping the platform-store code.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
src/CMakeLists.txt (1)

505-507: ⚡ Quick win

Remove the stale Debian libsecret dependency too.

This block says the secret-store libraries are gone, but DEBIAN-control.in still declares libsecret-1-0. The Linux package will keep pulling the removed dependency unless that metadata is updated alongside this cleanup.

🤖 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/CMakeLists.txt` around lines 505 - 507, The comment in CMakeLists.txt
indicates platform secret stores were removed but packaging metadata still lists
libsecret; update the Debian package metadata by removing the libsecret-1-0
dependency from DEBIAN-control.in (and any other packaging control files
referencing libsecret-1-0) so the produced .deb no longer pulls the stale
library; ensure you also update any packaging-related variables or comments that
mention secret-store linking to avoid future confusion.
🤖 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/CloudCredentialStore.cpp`:
- Around line 40-50: writeToSettings currently calls QSettings::sync() but
ignores failures; update writeToSettings to check QSettings::status() after
settings.sync() and return a bool indicating success (true when status ==
QSettings::NoError). Then change saveSession (the caller that currently returns
true if session.token is non-empty) to call writeToSettings and propagate its
boolean result (return false if writeToSettings reports an error), ensuring both
places mentioned (the token branch and the other branch around lines 63–70) use
the new return value so MainWindow::signInToQtMeshCloud() receives failure when
sync fails.
- Around line 30-37: The readFromSettings() function populates email and
expiresAt even when there is no token, returning a partial CloudSession; change
readFromSettings() to treat a missing/empty
settings.value(AppSettingsKeys::cloudToken()) as an empty session by checking
the token first and returning the default CloudSession immediately if token is
empty, only reading AppSettingsKeys::cloudTokenExpiresAt() and
AppSettingsKeys::cloudUserEmail() when a non-empty token exists so loadSession()
cannot return stale identity data.

In `@src/MaterialEditorQML.cpp`:
- Around line 1008-1076: In ensureTextureInMaterialGroup, the new disk-read path
that loads the image into Ogre should record a Sentry breadcrumb for
file.import; after successfully loading the bytes and calling
tm.loadImage(texStd, matGroup, image) add
SentryReporter::addBreadcrumb("file.import", {{"path", srcPath}, {"status",
"ok"}, {"name", texStd}}) and in the catch/all-failure branch (or just before
each early return when srcPath is missing or file open fails) add a failure
breadcrumb like SentryReporter::addBreadcrumb("file.import", {{"path", srcPath},
{"status", "failed"}, {"reason","not found"/"io error"}}) so texture-resolution
diagnostics consistently log imports from ensureTextureInMaterialGroup.

In `@src/RTShaderHelper.cpp`:
- Around line 503-505: The current diffuse detection uses literal checks
("albedo" || "diffuse_map") which duplicates logic and misses aliases defined
elsewhere; replace those manual string checks with the existing helper
isAlbedoSlotName(n) so hasNamedDiffuse is set when any albedo alias (e.g.
"Diffuse", "BaseColor", "albedo", "diffuse_map") is present; update both
occurrences around the hasNamedDiffuse assignment (the block that sets
hasNamedDiffuse = true and the similar check later) to call isAlbedoSlotName(n)
instead of comparing to hardcoded strings.

---

Nitpick comments:
In `@src/CMakeLists.txt`:
- Around line 505-507: The comment in CMakeLists.txt indicates platform secret
stores were removed but packaging metadata still lists libsecret; update the
Debian package metadata by removing the libsecret-1-0 dependency from
DEBIAN-control.in (and any other packaging control files referencing
libsecret-1-0) so the produced .deb no longer pulls the stale library; ensure
you also update any packaging-related variables or comments that mention
secret-store linking to avoid future confusion.
🪄 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: 2b9b88f2-1917-4bcf-80f8-5c101db7a03f

📥 Commits

Reviewing files that changed from the base of the PR and between 6e3d723 and 47aa978.

📒 Files selected for processing (15)
  • README.md
  • qml/TexturePropertiesPanel.qml
  • src/CMakeLists.txt
  • src/CloudAccountMenuButton.cpp
  • src/CloudCredentialStore.cpp
  • src/CloudCredentialStore.h
  • src/CloudCredentialStore_linux.c
  • src/CloudCredentialStore_linux.h
  • src/CloudCredentialStore_test.cpp
  • src/MaterialEditorQML.cpp
  • src/MaterialEditorQML.h
  • src/RTShaderHelper.cpp
  • src/mainwindow.cpp
  • website/src/DocsApp.jsx
  • website/src/data/content.js
💤 Files with no reviewable changes (2)
  • src/CloudCredentialStore_linux.h
  • src/CloudCredentialStore_linux.c

Comment thread src/CloudCredentialStore.cpp
Comment thread src/CloudCredentialStore.cpp Outdated
Comment thread src/MaterialEditorQML.cpp
Comment thread src/RTShaderHelper.cpp Outdated
fernandotonon and others added 3 commits June 12, 2026 22:12
The update-homebrew-cask release step appended a `postflight do...end` block to
the END of the cask file with `>> "$CASK_FILE"`. The cask already had a
postflight inside its `cask do...end`, so the appended block landed AFTER the
closing `end` — Homebrew parsed it as a top-level `postflight` call and rejected
the cask with "undefined method 'postflight'", breaking every install.

- Remove the broken append; the canonical cask already does lsregister inside
  its single postflight.
- Add packaging/macos/qtmesheditor.rb.in as the canonical template (modern
  double-quote DSL, one postflight: quarantine clear + lsregister, both CLI
  binaries, correct com.none.QtMeshEditor zap path). CI rewrites the whole cask
  from it if an older tap revision is missing lsregister, instead of appending.

The tap repo (fernandotonon/homebrew-qtmesheditor) was fixed directly so the
current 3.5.2 release installs again.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…in CI

- Bump project version 3.5.2 -> 3.5.3 (CMakeLists.txt, single source of truth);
  synced pinned doc refs via scripts/sync-doc-versions-from-cmake.sh.
- update-homebrew-cask CI job now regenerates the ENTIRE cask from
  packaging/macos/qtmesheditor.rb.in on every release instead of sed-patching /
  appending: checks out this repo for the template, substitutes @Version@ /
  @sha256@, and overwrites the tap's Casks/qtmesheditor.rb. This removes both the
  stray-second-postflight bug (#718) and the stale single-quote sed patterns that
  would no longer match the double-quote cask DSL. Verified the generated cask
  passes `ruby -c`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- CloudCredentialStore: migrate pre-QSettings sessions. migrateLegacySettings-
  IfNeeded now does a one-time read of the old OS secret store (macOS Keychain /
  Windows Credential Manager) + the legacy mode-0600 fallback file and copies it
  into QSettings, so users who signed in on an older build aren't logged out.
  Re-added the macOS Security / Windows advapi32 link for that read; libsecret
  stays dropped (no Linux read-back). (Codex P2)
- readFromSettings: return an empty session when no token is stored instead of
  hydrating email/expiry from stray keys. (CodeRabbit)
- saveSession: propagate QSettings::sync() failure (writeToSettings now returns
  status == NoError) so a non-persistent login is reported, not silently
  swallowed. (CodeRabbit major)
- RTShaderHelper::wirePbrSlotsForFFP: use the existing isAlbedoSlotName() helper
  (covers "Diffuse"/"BaseColor" too) instead of literal "albedo"/"diffuse_map"
  checks, so the unnamed-diffuse fallback can't mis-wire a pass that uses an
  alias. (CodeRabbit major)
- ensureTextureInMaterialGroup: add file.import Sentry breadcrumbs on
  success/failure. (CodeRabbit)
- DEBIAN-control.in: drop the now-unused libsecret-1-0 dependency. (CodeRabbit)

Tests: added legacy-fallback-file migration + tokenless-settings coverage.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@fernandotonon

Copy link
Copy Markdown
Owner Author

Addressed all review feedback in 175fbfa:

  • Preserve existing secret-store sessions during migration (Codex P2): migrateLegacySettingsIfNeeded() now does a one-time read of the old macOS Keychain / Windows Credential Manager + the legacy cloud_session.dat fallback and copies it into QSettings, so users upgrading from a pre-QSettings build stay signed in. Re-added the macOS Security / Windows advapi32 link for that read only; libsecret stays dropped.
  • Treat tokenless settings as an empty session (CodeRabbit): readFromSettings() returns {} immediately when no token is present — no more partial sessions / stale email.
  • Propagate QSettings::sync() failures from saveSession() (CodeRabbit): writeToSettings() now returns status() == NoError and saveSession() propagates it (and doesn't prime the cache on failure).
  • Use isAlbedoSlotName() for diffuse detection/wiring (CodeRabbit): both spots in wirePbrSlotsForFFP now use the helper (covers Diffuse/BaseColor), so the unnamed-diffuse fallback can't mis-wire an aliased pass.
  • file.import breadcrumb in ensureTextureInMaterialGroup (CodeRabbit): added on success and failure.
  • Remove stale libsecret-1-0 Debian dependency (CodeRabbit): dropped from DEBIAN-control.in.

Added tests for the legacy-fallback-file migration and the tokenless-settings case. All changed files compile; the only local link failure is the pre-existing qHash(TextureDecoder::MaterialKey) in the PS1 runtime (unrelated, present on master, macOS-only — CI runs tests on Linux).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
.github/workflows/deploy.yml (1)

2078-2081: 💤 Low value

Optional: Add persist-credentials: false for read-only template checkout.

This checkout is read-only (used only to access the canonical cask template), so following security best practices, add persist-credentials: false to prevent the workflow token from persisting in the checked-out repository.

🛡️ Suggested enhancement
     - name: Check out QtMeshEditor (for the canonical cask template)
       uses: actions/checkout@v4
       with:
         path: qtmesheditor-src
+        persist-credentials: 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 @.github/workflows/deploy.yml around lines 2078 - 2081, The checkout step
named "Check out QtMeshEditor (for the canonical cask template)" using
actions/checkout@v4 should be made read-only: add the persist-credentials: false
key to that step so the workflow token is not persisted into the checked-out
qtmesheditor-src repository; update the checkout step configuration to include
persist-credentials: false alongside the existing path: qtmesheditor-src.
src/CloudCredentialStore.cpp (1)

150-163: ⚡ Quick win

Add Sentry breadcrumbs for session persist/migration paths.

saveSession() and migrateLegacySettingsIfNeeded() handle user-visible auth persistence and migration outcomes, but these significant operations are not breadcrumbed. Please add SentryReporter::addBreadcrumb() on success/failure paths (file.export for settings writes, file.import for legacy migration reads/writes).

Suggested patch sketch
 bool CloudCredentialStore::saveSession(const CloudSession& session)
 {
     if (!session.hasToken())
         return false;
     if (!writeToSettings(session)) {
+        SentryReporter::addBreadcrumb(QStringLiteral("Cloud session persist failed"),
+                                      QStringLiteral("file.export"));
         // The write didn't persist (permissions / disk). Don't claim success or
         // prime the cache, so the caller can surface the failure.
         invalidateCache();
         return false;
     }
+    SentryReporter::addBreadcrumb(QStringLiteral("Cloud session persisted"),
+                                  QStringLiteral("file.export"));
     g_sessionCache = session;
     g_cacheValid = true;
     return true;
 }
@@
 void CloudCredentialStore::migrateLegacySettingsIfNeeded()
 {
@@
     const CloudSession legacy = readLegacySecretStore();
-    if (legacy.hasToken() && writeToSettings(legacy)) {
+    if (legacy.hasToken() && writeToSettings(legacy)) {
+        SentryReporter::addBreadcrumb(QStringLiteral("Legacy cloud session migrated"),
+                                      QStringLiteral("file.import"));
         g_sessionCache = legacy;
         g_cacheValid = true;
+    } else if (legacy.hasToken()) {
+        SentryReporter::addBreadcrumb(QStringLiteral("Legacy cloud session migration failed"),
+                                      QStringLiteral("file.import"));
     }
 }

As per coding guidelines, add breadcrumbs for all user-facing/significant operations via SentryReporter::addBreadcrumb() with the prescribed categories.

Also applies to: 193-207

🤖 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/CloudCredentialStore.cpp` around lines 150 - 163, Add Sentry breadcrumbs
to record success/failure of session persistence and legacy migration: inside
saveSession (around writeToSettings success/failure paths) call
SentryReporter::addBreadcrumb() with category "file.export" and include context
(e.g., outcome "write_success" or "write_failure", and any error/permission
info) on both the branch where writeToSettings returns false (before
invalidateCache) and where it succeeds (after g_sessionCache/g_cacheValid are
set); likewise, in migrateLegacySettingsIfNeeded add
SentryReporter::addBreadcrumb() calls with category "file.import" for the
legacy-read attempt and for the migration write outcome (both success and
failure), including minimal context fields to indicate the step and result so
these events are breadcrumbed for later diagnostics.

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/CMakeLists.txt`:
- Around line 527-532: The UnitTests target needs the same platform-specific
legacy secret-store linkage as the main app: update CMakeLists to also call
target_link_libraries for the UnitTests target (in addition to
${CMAKE_PROJECT_NAME}) on Apple and WIN32 so UnitTests links "-framework
Security" on macOS and advapi32 on Windows; this ensures
CloudCredentialStore.cpp (built via TEST_SOURCES) and
CloudCredentialStore_test.cpp which exercises
CloudCredentialStore::migrateLegacySettingsIfNeeded() will link correctly.

---

Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 2078-2081: The checkout step named "Check out QtMeshEditor (for
the canonical cask template)" using actions/checkout@v4 should be made
read-only: add the persist-credentials: false key to that step so the workflow
token is not persisted into the checked-out qtmesheditor-src repository; update
the checkout step configuration to include persist-credentials: false alongside
the existing path: qtmesheditor-src.

In `@src/CloudCredentialStore.cpp`:
- Around line 150-163: Add Sentry breadcrumbs to record success/failure of
session persistence and legacy migration: inside saveSession (around
writeToSettings success/failure paths) call SentryReporter::addBreadcrumb() with
category "file.export" and include context (e.g., outcome "write_success" or
"write_failure", and any error/permission info) on both the branch where
writeToSettings returns false (before invalidateCache) and where it succeeds
(after g_sessionCache/g_cacheValid are set); likewise, in
migrateLegacySettingsIfNeeded add SentryReporter::addBreadcrumb() calls with
category "file.import" for the legacy-read attempt and for the migration write
outcome (both success and failure), including minimal context fields to indicate
the step and result so these events are breadcrumbed for later diagnostics.
🪄 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: ba953e4e-f7d2-4d81-83cc-9a620dc33a25

📥 Commits

Reviewing files that changed from the base of the PR and between 47aa978 and 175fbfa.

📒 Files selected for processing (11)
  • .github/workflows/deploy.yml
  • CMakeLists.txt
  • DEBIAN-control.in
  • README.md
  • packaging/macos/qtmesheditor.rb.in
  • src/CMakeLists.txt
  • src/CloudCredentialStore.cpp
  • src/CloudCredentialStore_test.cpp
  • src/MaterialEditorQML.cpp
  • src/RTShaderHelper.cpp
  • website/src/hooks/useQtmeshActionRef.js
✅ Files skipped from review due to trivial changes (3)
  • website/src/hooks/useQtmeshActionRef.js
  • CMakeLists.txt
  • README.md
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/RTShaderHelper.cpp
  • src/CloudCredentialStore_test.cpp
  • src/MaterialEditorQML.cpp

Comment thread src/CMakeLists.txt
- Mirror the legacy secret-store linkage on the UnitTests target: it compiles
  CloudCredentialStore.cpp (which CloudCredentialStore_test.cpp exercises via
  migrateLegacySettingsIfNeeded -> readLegacySecretStore), so macOS/Windows test
  builds need "-framework Security" / advapi32 or they fail to link. (CodeRabbit)
- Gate PS1-runtime *_test.cpp the same way as their impl: those sources are only
  added to SRC_FILES when ENABLE_PS1_RIP is ON, but the _test.cpp files were
  globbed unconditionally — so a tests build with ENABLE_PS1_RIP=OFF linked e.g.
  TextureDecoder_test.cpp against a missing qHash(TextureDecoder::MaterialKey)
  and failed. Skip src/PS1/runtime/*_test.cpp when ENABLE_PS1_RIP is OFF.

UnitTests now links cleanly locally (macOS, PS1 rip off).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@fernandotonon

Copy link
Copy Markdown
Owner Author

Addressed in 3705dc4: added the -framework Security / advapi32 link to the UnitTests target too (it compiles CloudCredentialStore.cpp and the test exercises migrateLegacySettingsIfNeeded). Also fixed a latent test-build issue: src/PS1/runtime/*_test.cpp were globbed unconditionally while their impl is gated on ENABLE_PS1_RIP, so a tests build with the flag off failed to link (qHash(TextureDecoder::MaterialKey)); those tests are now gated the same way. UnitTests links cleanly locally now.

@sonarqubecloud

Copy link
Copy Markdown

@fernandotonon
fernandotonon merged commit 937d619 into master Jun 13, 2026
20 checks passed
@fernandotonon
fernandotonon deleted the fix/texture-apply-and-keychain-prompts branch June 13, 2026 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant