Gamify: editor engagement, feature discovery & account persistence (epic #796) - #820
Merged
Merged
Conversation
Editor half of the gamification epic, built against the shipped qtmesh-cloud API (qtmesh-cloud#79): - E-P1 (#797): GamificationManager + persistent cross-process event queue with client idempotency ids; QtMeshCloudClient calls for POST /v1/events/editor, POST /v1/events/operations, GET /v1/me/stats, gamification prefs and data deletion; debounced/backoff flush loop, blocking flush for one-shot CLI processes, typed stats snapshot with an offline disk cache. - E-P2 (#798): noteFeature() one-liners across the 24 feature clusters (GUI controllers, CLI subcommand map, MCP tool map), deduped per session; events carry only {id, feature, at, surface}. - E-P3 (#799): noteOperation() with content-free numeric before/after metrics (retopo, decimate/LOD, optimize, uv_unwrap, auto_rig, skin_weights, fix, texture_atlas, isometric_sprites, vat_bake, vertex_color_bake, morph, motion_inbetween, pbr_synth); attaches the active cloud project context when one is open. - E-P4 (#800): level/XP/streak/nearest-unlockable status block in the cloud account menu, one coalescing unlock toast, achievements deep link to qtmesh.dev/u/<slug>. - E-P5 (#801): dismissible "try this next" welcome-screen card for unused clusters (personalized when stats exist, generic logged out). - E-P6 (#802): default-off consent (one-time non-blocking prompt), Preferences "Progress Sync" section with per-stream toggles, example payload, public-profile toggle and delete-my-data; zero network when logged out or opted out; CLI honours --no-telemetry. Closes #797, closes #798, closes #799, closes #800, closes #801, closes #802. Part of #796. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a gamification and progress-sync subsystem: persistent event storage, a manager for consent/gating/flush/stats/suggestions, cloud API support, new UI surfaces, and feature/operation instrumentation across editor, CLI, and MCP entry points. ChangesGamification / Progress Sync
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant GamificationManager
participant GamificationEventQueue
participant QtMeshCloudClient
Controller->>GamificationManager: noteFeature/noteOperation(key, metrics)
GamificationManager->>GamificationEventQueue: append(entry)
GamificationManager->>GamificationManager: scheduleFlushSoon()
GamificationManager->>QtMeshCloudClient: postEditorEvents/postOperationEvents(batch)
QtMeshCloudClient-->>GamificationManager: accepted ids, newAchievements
GamificationManager->>GamificationEventQueue: acknowledge(ids)
GamificationManager-->>Controller: achievementsUnlocked / suggestionChanged
sequenceDiagram
participant GamificationManager
participant MainWindow
participant User
participant GamificationToast
GamificationManager->>MainWindow: consentPromptRequested()
MainWindow->>User: show consent dialog
User->>MainWindow: accept / decline
MainWindow->>GamificationManager: acceptConsent() / declineConsent()
GamificationManager->>MainWindow: achievementsUnlocked(achievements)
MainWindow->>GamificationToast: showAchievements(achievements)
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: cddf728c06
ℹ️ 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".
| } | ||
|
|
||
| cliWrite(report); | ||
| GamificationManager::noteOperation( |
There was a problem hiding this comment.
For opted-in users, running a successful operation subcommand with --no-telemetry still reaches this unconditional noteOperation, which appends the event to the persistent gamification queue before run() checks s_noTelemetry; because the final block only skips flushing for this process, the queued operation can be sent on a later run/sign-in. This makes --no-telemetry fail for operation history (fix/decimate/retopo/etc.), so the CLI operation hooks need the same guard or the manager needs a per-process suppression flag.
Useful? React with 👍 / 👎.
| return false; | ||
| if (!signedIn()) | ||
| return false; | ||
| settings.setValue(AppSettingsKeys::gamificationConsentPrompted(), true); |
There was a problem hiding this comment.
If the first would-be gamification event for a signed-in user comes from the CLI, there is no MainWindow connected to consentPromptRequested, but this still persists Gamification/consentPrompted=true; subsequent GUI events then return at the prompted check and never show the consent dialog, leaving progress sync off until the user manually finds Preferences. Since this commit adds CLI calls into noteFeature/noteOperation, the manager should avoid consuming the one-time prompt in headless contexts or set this flag from the UI after creating the prompt.
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CLIPipeline.cpp (1)
6686-6699: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate
decimate_lodstats onreport.applied(src/CLIPipeline.cpp:6715-6720)
cmdDecimaterecordsdecimate_lodunconditionally, so areduction <= 0.0no-op still credits a decimation event with unchanged counts. MirrorcmdOptimizeand only callnoteOperation(...)whenreport.appliedis true.🤖 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/CLIPipeline.cpp` around lines 6686 - 6699, The decimation stats are being recorded even when no decimation actually happened, so update cmdDecimate in CLIPipeline.cpp to gate the noteOperation call on report.applied, matching the pattern used by cmdOptimize. Locate the decimateEntity call and ensure the decimate_lod operation is only credited when MeshDecimator::decimateEntity returns a report with applied true, so no-op reductions do not increment stats.
🧹 Nitpick comments (6)
src/GamificationToast.cpp (1)
18-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a Sentry breadcrumb when a toast is shown.
Achievement unlocks are a significant, user-facing milestone. Based on learnings, breadcrumbs should be added for user-facing actions and significant operations using
SentryReporter::addBreadcrumb(...), and this codebase applies that pattern extensively elsewhere (e.g.CloudAccountMenuButton.cpp's "View My Achievements…" handler). Consider adding one here too, e.g. inappendAchievements()when the toast is (re)shown.Also applies to: 47-67
🤖 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/GamificationToast.cpp` around lines 18 - 25, Add a Sentry breadcrumb for the achievement toast display path so user-facing unlock events are captured consistently. Update the toast flow in GamificationToast, ideally in appendAchievements() (or immediately before it is called from showAchievements()), to call SentryReporter::addBreadcrumb(...) with a message describing that achievements were shown/re-shown. Keep the existing parentWindow/empty-check behavior intact and use the same breadcrumb pattern used elsewhere in the UI, such as the CloudAccountMenuButton achievement handler, so the toast display is observable in Sentry.Source: Learnings
src/mainwindow.cpp (2)
2824-2859: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider Sentry breadcrumbs for achievement toast + consent decisions.
Based on learnings, breadcrumbs should be added for user-facing actions and significant operations. Nearly every other user action in this file (menu clicks, dialog outcomes, etc.) is wrapped with
SentryReporter::addBreadcrumb(...), but showing the achievement toast and the accept/decline consent outcome are not. Given consent state materially changes network behavior, a breadcrumb here would help diagnose sync-related support issues.🤖 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 2824 - 2859, Add Sentry breadcrumbs around the gamification UI actions in the GamificationManager signal handlers inside the main window setup: when `GamificationToast::showAchievements(...)` is triggered, record a breadcrumb before showing the toast, and in the `QMessageBox::finished` lambda for the consent prompt, add breadcrumbs for both the accept and decline paths before calling `acceptConsent()` or `declineConsent()`. Use `SentryReporter::addBreadcrumb(...)` consistently with the existing breadcrumb patterns in `src/mainwindow.cpp` so the achievement toast and consent decision are visible in diagnostics.Source: Learnings
802-809: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRegister
GamificationManageroncesrc/mainwindow.cpp:802-809—WelcomeScreen.qmlalready importsPropertiesPanel 1.0, so the extraWelcomeScreenalias is redundant.🤖 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 802 - 809, `GamificationManager` is being registered twice with different module names, but `WelcomeScreen` is redundant because `WelcomeScreen.qml` already imports `PropertiesPanel 1.0`. Remove the extra `qmlRegisterSingletonType` for `WelcomeScreen` in `mainwindow.cpp` and keep the single `PropertiesPanel` registration, using `GamificationManager::qmlInstance` as the shared singleton source.src/GamificationToast.h (1)
14-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNew UI surface built with Qt Widgets instead of QML.
This is an entirely new UI component (toast) implemented as a
QWidgetsubclass. Based on learnings, new UI should be built with QML (Qt Quick), not Qt Widgets — and the rest of this PR's gamification UI (preferences panel, welcome screen nudge) correctly follows that pattern via a QML-exposedGamificationManagersingleton. Consider hosting this toast as a small QML overlay (similar to the existingWelcomeScreenQQuickWidgetoverlay pattern inmainwindow.cpp) instead of a standaloneQWidget.🤖 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/GamificationToast.h` around lines 14 - 36, The new GamificationToast UI is implemented as a standalone QWidget subclass, but it should follow the Qt Quick/QML approach used elsewhere in this PR. Refactor GamificationToast (including showAchievements, appendAchievements, reposition, and updateText) into a QML-backed overlay instead of a QWidget, and host it through the existing QQuickWidget-style overlay pattern used for the WelcomeScreen in mainwindow.cpp. Keep the gamification data flow through GamificationManager, but move the toast presentation into QML so the new surface matches the rest of the UI.Source: Learnings
src/AutoRigController.cpp (1)
310-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
noteOperation("auto_rig", ...)block into a helper.The same 5-line gamification block is duplicated verbatim across
autoRigSelected,emitRigResult, andcommitMarkerRig. Consolidating reduces the risk of the three call sites diverging if the metrics or gating condition change later.♻️ Proposed refactor
+void AutoRigController::noteAutoRigOutcome(const AutoRig::Report& report, bool skinned) +{ + if (!report.applied) + return; + GamificationManager::noteOperation( + QStringLiteral("auto_rig"), + {{QStringLiteral("bones_created"), report.boneCount}, + {QStringLiteral("meshes_skinned"), skinned ? 1 : 0}}); +}Then at each call site:
- if (report.applied) - GamificationManager::noteOperation( - QStringLiteral("auto_rig"), - {{QStringLiteral("bones_created"), report.boneCount}, - {QStringLiteral("meshes_skinned"), skinned ? 1 : 0}}); + noteAutoRigOutcome(report, skinned);Also applies to: 348-353, 638-643
🤖 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/AutoRigController.cpp` around lines 310 - 315, Extract the duplicated GamificationManager::noteOperation("auto_rig", ...) block into a single helper in AutoRigController and reuse it from autoRigSelected, emitRigResult, and commitMarkerRig. Keep the existing gating on report.applied and preserve the bones_created and meshes_skinned metrics exactly as-is, then replace each repeated inline block with the new helper call so the three paths stay in sync.src/GamificationEventQueue.cpp (1)
117-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a Sentry breadcrumb for capacity eviction and data deletion.
Both FIFO eviction (silent data loss beyond a warning log) and
clear()(user-initiated data deletion) are significant, user-impacting operations. Based on learnings, addSentryReporter::addBreadcrumb(...)for user-facing actions and significant operations, with the established categories, so these are visible in crash/session diagnostics alongside the existingqWarning()logging.Also applies to: 185-192
🤖 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/GamificationEventQueue.cpp` around lines 117 - 125, Add Sentry breadcrumbs for the user-impacting data removal paths in GamificationEventQueue so they appear in crash/session diagnostics alongside the existing qWarning logs. Update GamificationEventQueue::enforceCapacity to call SentryReporter::addBreadcrumb(...) when FIFO eviction happens, and update clear() to add a breadcrumb for user-initiated data deletion using the established breadcrumb categories already used elsewhere. Keep the existing warning logging, but ensure both eviction and clear operations are recorded with consistent Sentry context.Source: Learnings
🤖 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/PreferencesDialog.qml`:
- Around line 326-332: `PreferencesDialog.qml` currently calls
`GamificationManager.refreshCloudPrefs()` unconditionally from
`Component.onCompleted`, which still triggers a cloud fetch for signed-in users
who have opted out of gamification. Update the `Row`/`Component.onCompleted`
logic so the refresh only runs when both `GamificationManager.signedIn` and the
privacy/opt-out state allow gamification network traffic, using the existing
`GamificationManager` state checks to gate the call.
In `@src/GamificationEventQueue.cpp`:
- Around line 100-115: mergeFromDisk currently unions the in-memory snapshot
with disk state in GamificationEventQueue, which can resurrect events that were
already removed by acknowledge() or clear() in another process. Update
GamificationEventQueue::mergeFromDisk (and the related append/acknowledge/clear
flow) to track locally removed ids as tombstones or an equivalent exclusion set,
and filter those ids out when rebuilding m_entries so stale writers cannot
reintroduce processed events.
- Around line 185-192: `GamificationEventQueue::clear()` currently clears
`m_entries` even when the persisted wipe in `withFileLock()`/`saveLocked()`
fails, so the in-memory state can diverge from disk. Update
`GamificationEventQueue::clear` to only clear `m_entries` after the file-backed
clear and `saveLocked()` complete successfully, and propagate or handle any
failure from the lock/save path before mutating in-memory state.
In `@src/GamificationEventQueue.h`:
- Around line 49-50: The GamificationEventQueue::clear() API cannot surface
lock/persistence failures to its callers, so update the method contract to
return a success/failure signal instead of void and propagate the result through
the corresponding implementation in the .cpp. Adjust the clear() declaration and
definition, and make GamificationManager’s delete-my-data flow check the
returned status so it can detect and retry a failed wipe when the lock cannot be
acquired.
In `@src/GamificationManager.cpp`:
- Around line 257-267: Queued gamification batches are still being sent after a
stream is disabled, so update the queue handling in GamificationManager to
respect the current usage/ops toggles. In flushNow(), and any related batch
selection logic, filter out or suppress queued feature/operation entries when
setUsageEnabled(false) or setOpsEnabled(false) has been applied, and make sure
the existing stream-specific paths in GamificationManager::setUsageEnabled and
GamificationManager::setOpsEnabled cannot allow already-queued items through.
- Around line 720-756: The delete flow in GamificationManager::deleteCloudData
does not serialize against pending flushes, so a queued or in-flight upload can
repopulate server data after deletion; update this path to pause/defer flushing
until any active flush completes or is invalidated. Also clear the local queued
events and session-related state immediately as part of the local half of the
request, rather than waiting for the server DELETE success path. Keep the
success callback focused on removing remaining local cache/snapshot state and
emitting the completion signal, using the existing deleteCloudData, m_queue,
m_sessionNotedFeatures, and statsChanged/deleteCloudDataFinished logic.
- Around line 612-623: The account-change handling in handleSessionChanged()
keeps queued work across sign-out, which lets one user’s queued metadata be
flushed under the next signed-in account. Update the sign-out path to clear or
partition all account-scoped state, including m_sessionNotedFeatures and any
active project/context used by the queue, or else persist and flush queued items
only for the same account. Keep the existing snapshot reset and stats file
removal, but make sure queued bodies cannot survive an account boundary unless
they are tied to the original user.
---
Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Around line 6686-6699: The decimation stats are being recorded even when no
decimation actually happened, so update cmdDecimate in CLIPipeline.cpp to gate
the noteOperation call on report.applied, matching the pattern used by
cmdOptimize. Locate the decimateEntity call and ensure the decimate_lod
operation is only credited when MeshDecimator::decimateEntity returns a report
with applied true, so no-op reductions do not increment stats.
---
Nitpick comments:
In `@src/AutoRigController.cpp`:
- Around line 310-315: Extract the duplicated
GamificationManager::noteOperation("auto_rig", ...) block into a single helper
in AutoRigController and reuse it from autoRigSelected, emitRigResult, and
commitMarkerRig. Keep the existing gating on report.applied and preserve the
bones_created and meshes_skinned metrics exactly as-is, then replace each
repeated inline block with the new helper call so the three paths stay in sync.
In `@src/GamificationEventQueue.cpp`:
- Around line 117-125: Add Sentry breadcrumbs for the user-impacting data
removal paths in GamificationEventQueue so they appear in crash/session
diagnostics alongside the existing qWarning logs. Update
GamificationEventQueue::enforceCapacity to call
SentryReporter::addBreadcrumb(...) when FIFO eviction happens, and update
clear() to add a breadcrumb for user-initiated data deletion using the
established breadcrumb categories already used elsewhere. Keep the existing
warning logging, but ensure both eviction and clear operations are recorded with
consistent Sentry context.
In `@src/GamificationToast.cpp`:
- Around line 18-25: Add a Sentry breadcrumb for the achievement toast display
path so user-facing unlock events are captured consistently. Update the toast
flow in GamificationToast, ideally in appendAchievements() (or immediately
before it is called from showAchievements()), to call
SentryReporter::addBreadcrumb(...) with a message describing that achievements
were shown/re-shown. Keep the existing parentWindow/empty-check behavior intact
and use the same breadcrumb pattern used elsewhere in the UI, such as the
CloudAccountMenuButton achievement handler, so the toast display is observable
in Sentry.
In `@src/GamificationToast.h`:
- Around line 14-36: The new GamificationToast UI is implemented as a standalone
QWidget subclass, but it should follow the Qt Quick/QML approach used elsewhere
in this PR. Refactor GamificationToast (including showAchievements,
appendAchievements, reposition, and updateText) into a QML-backed overlay
instead of a QWidget, and host it through the existing QQuickWidget-style
overlay pattern used for the WelcomeScreen in mainwindow.cpp. Keep the
gamification data flow through GamificationManager, but move the toast
presentation into QML so the new surface matches the rest of the UI.
In `@src/mainwindow.cpp`:
- Around line 2824-2859: Add Sentry breadcrumbs around the gamification UI
actions in the GamificationManager signal handlers inside the main window setup:
when `GamificationToast::showAchievements(...)` is triggered, record a
breadcrumb before showing the toast, and in the `QMessageBox::finished` lambda
for the consent prompt, add breadcrumbs for both the accept and decline paths
before calling `acceptConsent()` or `declineConsent()`. Use
`SentryReporter::addBreadcrumb(...)` consistently with the existing breadcrumb
patterns in `src/mainwindow.cpp` so the achievement toast and consent decision
are visible in diagnostics.
- Around line 802-809: `GamificationManager` is being registered twice with
different module names, but `WelcomeScreen` is redundant because
`WelcomeScreen.qml` already imports `PropertiesPanel 1.0`. Remove the extra
`qmlRegisterSingletonType` for `WelcomeScreen` in `mainwindow.cpp` and keep the
single `PropertiesPanel` registration, using `GamificationManager::qmlInstance`
as the shared singleton source.
🪄 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: 97ff7967-00e6-4fda-84b4-2fc98f55bd8a
📒 Files selected for processing (45)
CLAUDE.mdqml/PreferencesDialog.qmlqml/WelcomeScreen.qmlsrc/AIAssistManager.cppsrc/AnimationBlender.cppsrc/AnimationControlController.cppsrc/AppSettingsKeys.hsrc/AutoRigController.cppsrc/BatchExporter.cppsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/CloudAccountMenuButton.cppsrc/CloudAccountMenuButton.hsrc/CloudProjectsController.cppsrc/GamificationEventQueue.cppsrc/GamificationEventQueue.hsrc/GamificationEventQueue_test.cppsrc/GamificationManager.cppsrc/GamificationManager.hsrc/GamificationManager_test.cppsrc/GamificationToast.cppsrc/GamificationToast.hsrc/GamificationTypes.cppsrc/GamificationTypes.hsrc/GamificationTypes_test.cppsrc/ImageTo3D/MeshGenController.cppsrc/IsometricSpritesController.cppsrc/LLMManager.cppsrc/MCPServer.cppsrc/MaterialEditorQML.cppsrc/MaterialPresetLibrary.cppsrc/MeshDecimatorController.cppsrc/MeshLodController.cppsrc/MorphAnimationManager.cppsrc/PoseLibrary.cppsrc/QtMeshCloudClient.cppsrc/QtMeshCloudClient.hsrc/QuadRetopoController.cppsrc/SDManager.cppsrc/SkinWeightsController.cppsrc/TexturePaintController.cppsrc/UVEditorController.cppsrc/UvUnwrapController.cppsrc/VATBakerController.cppsrc/mainwindow.cpp
Comment on lines
+185
to
+192
| void GamificationEventQueue::clear() | ||
| { | ||
| withFileLock([this]() { | ||
| m_entries.clear(); | ||
| saveLocked(); | ||
| }); | ||
| m_entries.clear(); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first
ast-grep outline src/GamificationEventQueue.cpp --view expanded || true
# Read targeted sections around the relevant functions
nl -ba src/GamificationEventQueue.cpp | sed -n '1,260p'
# Find call sites/usages of clear()
rg -n "\.clear\(\)|GamificationEventQueue::clear|clear\(" src --glob '!**/build/**' || trueRepository: fernandotonon/QtMeshEditor
Length of output: 249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the file in smaller chunks if needed
nl -ba src/GamificationEventQueue.cpp | sed -n '1,220p'Repository: fernandotonon/QtMeshEditor
Length of output: 204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and print its contents in safe slices if present.
git ls-files | rg -n 'GamificationEventQueue\.cpp$|GamificationEventQueue\.h$' || true
file=$(git ls-files | rg 'GamificationEventQueue\.cpp$' | head -n 1 || true)
if [ -n "${file:-}" ]; then
echo "== $file =="
sed -n '1,260p' "$file"
fi
# Also show nearby references to clear()/withFileLock() if the file exists.
if [ -n "${file:-}" ]; then
rg -n 'withFileLock|clear\(|acknowledge\(|mergeFromDisk|saveLocked|append\(' "$file" || true
fiRepository: fernandotonon/QtMeshEditor
Length of output: 7116
clear() should only drop in-memory state after the persisted wipe succeeds.
withFileLock() can fail, and saveLocked() is ignored; in both cases the final m_entries.clear() still runs, so a failed delete leaves the file intact and the next append()/mergeFromDisk() can bring the entries back. src/GamificationEventQueue.cpp:185-192
🤖 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/GamificationEventQueue.cpp` around lines 185 - 192,
`GamificationEventQueue::clear()` currently clears `m_entries` even when the
persisted wipe in `withFileLock()`/`saveLocked()` fails, so the in-memory state
can diverge from disk. Update `GamificationEventQueue::clear` to only clear
`m_entries` after the file-backed clear and `saveLocked()` complete
successfully, and propagate or handle any failure from the lock/save path before
mutating in-memory state.
Owner
Author
|
Addressed the review feedback (Codex + CodeRabbit) in the follow-up commit:
🤖 Generated with Claude Code |
Review fixes (Codex P1/P2 + CodeRabbit): - --no-telemetry now suspends ALL gamification emission process-wide (GamificationManager::setEmissionSuspended), so CLI operation notes inside subcommands can no longer queue events that a later run would flush. - The one-time consent prompt is only consumed when a GUI listener is connected to consentPromptRequested — headless CLI/MCP runs no longer burn the user's only chance to see the dialog. - Queue removals (acknowledge/clear/removeKind) are tombstoned so a concurrent writer's stale in-memory snapshot can never resurrect removed events; clear() is now fallible and keeps state for retry when the persisted wipe fails. - Disabling a stream (feature usage / operations) also drops that stream's already-queued events, and flush gates each batch on the live toggle. - Queue entries are stamped with the owning user slug; flush drops (never posts) entries recorded for a different account. - Delete-my-data clears local queue/cache immediately, blocks new flushes while pending, and defers the server DELETE until an in-flight flush settles. - refreshCloudPrefs()/setProfilePublic() early-return unless sync is enabled; the Preferences public-profile row only shows (and fetches) when signed in and sync is on. - CLI decimate operation note gated on report.applied. Also adds the gamification sources/headers to tests/CMakeLists.txt's qtmesh_test_common, fixing the unit-tests-linux link failure (undefined GamificationManager symbols in MaterialEditorQML_test). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/GamificationEventQueue.cpp (1)
206-224: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
clear()mutatesm_entriesbefore knowing whether the save succeeded — the "kept in-memory for retry" contract is not actually honored.
m_entries.clear()(line 214) runs unconditionally inside the lock callback, and only afterward issaved = saveLocked()evaluated. IfsaveLocked()fails, the function returnsfalse, but the in-memory queue is already empty — the events are gone from both disk (unwritten) and memory, with no tombstone recorded (tombstone only runs on success at line 222). This contradicts the header's documented contract that "Returns false when the persisted wipe failed (lock or write) — in-memory entries are kept in that case so the caller can retry." This same defect was raised on a prior version of this function; the addedboolreturn signals failure but doesn't actually preserve state.🐛 Proposed fix
bool GamificationEventQueue::clear() { QSet<QString> ids; bool saved = false; - const bool locked = withFileLock([this, &ids, &saved]() { + QList<Entry> snapshot; + const bool locked = withFileLock([this, &ids, &saved, &snapshot]() { mergeFromDisk(); + snapshot = m_entries; for (const Entry& e : std::as_const(m_entries)) ids.insert(e.id); m_entries.clear(); saved = saveLocked(); + if (!saved) + m_entries = snapshot; }); if (!locked || !saved) { // Persisted wipe failed: keep state so the caller can retry, and // report the failure instead of pretending the data is gone. return false; } tombstone(ids); return true; }🤖 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/GamificationEventQueue.cpp` around lines 206 - 224, `GamificationEventQueue::clear()` clears `m_entries` before confirming that `saveLocked()` succeeded, so the in-memory retry contract is broken. Update the `clear()` flow to preserve `m_entries` unless the persisted wipe is successfully saved, using the existing `withFileLock`, `saveLocked`, and `tombstone` logic so that state is only removed from memory after a successful write.
🤖 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/GamificationEventQueue.cpp`:
- Around line 226-246: The removeKind() flow in GamificationEventQueue mutates
m_entries before confirming saveLocked() succeeded, so a failed save leaves
in-memory state inconsistent. Update removeKind() to follow the same safe
pattern as a confirmed write: collect matching ids inside the withFileLock
callback, only apply removals to m_entries after saveLocked() returns true, and
keep tombstone(ids) strictly on the success path. Use removeKind(),
saveLocked(), tombstone(), and withFileLock() to locate and refactor the logic.
In `@tests/CMakeLists.txt`:
- Around line 83-86: The gamification test sources are listed in the shared test
library but never attached to a runnable test target, so the suites will not be
built or discovered by CTest. Update tests/CMakeLists.txt by registering the
gamification test executables for GamificationTypes_test,
GamificationEventQueue_test, and GamificationManager_test, using the existing
qtmesh_test_common setup and the corresponding test source files so they are
compiled and added as CTest entries.
---
Duplicate comments:
In `@src/GamificationEventQueue.cpp`:
- Around line 206-224: `GamificationEventQueue::clear()` clears `m_entries`
before confirming that `saveLocked()` succeeded, so the in-memory retry contract
is broken. Update the `clear()` flow to preserve `m_entries` unless the
persisted wipe is successfully saved, using the existing `withFileLock`,
`saveLocked`, and `tombstone` logic so that state is only removed from memory
after a successful write.
🪄 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: e8b876f8-f1a5-47a7-9743-6164a1bc24fd
📒 Files selected for processing (9)
qml/PreferencesDialog.qmlsrc/CLIPipeline.cppsrc/GamificationEventQueue.cppsrc/GamificationEventQueue.hsrc/GamificationEventQueue_test.cppsrc/GamificationManager.cppsrc/GamificationManager.hsrc/GamificationManager_test.cpptests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- src/GamificationEventQueue.h
- src/GamificationManager.h
- qml/PreferencesDialog.qml
- src/GamificationManager.cpp
QCoreApplication::instance() can be null when a flush/stats/prefs worker finishes during application shutdown — check it before invokeMethod in all five gamification worker lambdas (Sonar new_reliability_rating). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Physically insert/remove the gamification menu entries instead of setVisible(false): macOS QMenu keeps painting hidden QWidgetActions (black progress-bar strips) and mis-tracks hover geometry, which offset the highlight on the first menu items. - Show the section in every signed-in state: "Enable Progress Sync…" action when sync is off (one-click consent + stats fetch), a "Syncing progress…" placeholder while the first stats fetch is in flight, and the level/XP/streak block once stats land. - Fetch stats immediately when sync is enabled (setSyncEnabled / acceptConsent) instead of waiting for the next menu open. - Make CloudAccountMenuButton tests hermetic: pre-set Cloud/legacyMigrationDone so refresh() never probes the OS keychain from tests (on a dev machine with a legacy session it imported the real token into the test scope and broke signed-out expectations). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pairs with qtmesh-cloud#96 (DISCOVERY_FEATURES += lighting →
`first_lighting` "Illuminator" achievement, Cartographer threshold 25):
- featureCatalog() gains the `lighting` entry (nudge card copy:
"Scene Lighting").
- noteFeature("lighting") on LightsController::addLight (covers all
toolbar/viewport add-light paths) and on a successful
HdrEnvironmentController::loadEnvironment (Inspector HDR picker,
choice list and Browse all funnel through it).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- clear()/removeKind(): re-hydrate the in-memory queue from disk when saveLocked() fails, so a failed wipe never leaves memory claiming the entries are gone while the file still has them (CodeRabbit). - setProfilePublic callback: hoist QPointer::data() into a checked raw pointer and drop the self-assigning ternary — Sonar's analyzer flagged the double self-> read as a possible null dereference (cpp:S2259). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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
Editor half of the gamification epic #796 — implements all six slices (E-P1 #797 … E-P6 #802) against the shipped qtmesh-cloud API (qtmesh-cloud#79 / PR #86).
E-P1 — Cloud plumbing + offline event queue (#797)
GamificationEventQueue: persistent, cross-process-safe (QLockFile) JSON queue in<AppData>/gamification/queue.json. Client-generated UUID idempotency ids (the server dedup key), capacity 500 with logged FIFO eviction — replays never double-award, nothing is dropped silently.QtMeshCloudClientgainspostEditorEvents(POST /v1/events/editor),postOperationEvents(POST /v1/events/operations),fetchGamificationStats(GET /v1/me/stats),fetch/setGamificationPrefs,deleteGamificationData,profileUrl()— same blocking-call +QTMESH_API_BASEpattern as the rest of the client.GamificationManager(singleton, QML-exposed): debounced flush + 90 s heartbeat + graceful-shutdown flush on worker threads, exponential backoff (max 30 min);flushBlocking()for one-shot CLI processes. Newly-earned achievements come back from the flush response. Stats parse into a typedGamification::StatsSnapshot(handles the wire's mixed snake/camel casing) and cache to disk for offline rendering.E-P2 — Feature-usage instrumentation (#798)
GamificationManager::noteFeature("<key>", surface)one-liners at the entry points of every mapped cluster (GUI controllers,CLIPipeline::runsubcommand map,MCPServer::callTooltool map). Deduped per session; events carry only{id, feature, at, surface}.Gamification::featureCatalog()and match qtmesh-cloud'sDISCOVERY_FEATURESexactly.E-P3 — Operation outcomes (#799)
noteOperation("<op>", {numeric metrics})with before/after metrics at: retopo (GUI+CLI), decimate/LOD (GUI+CLI), optimize (CLI), uv_unwrap (GUI), auto_rig (GUI template/async/marker + CLI), skin_weights (GUI+CLI), fix (CLI), texture_atlas (GUI), isometric_sprites (GUI), vat_bake (GUI), vertex_color_bake (GUI), morph (GUI), motion_inbetween (GUI), pbr_synth (shared AIAssistManager core).numericMetricsOnly) so asset content/paths can never leak. Ops attachownerSlug/projectSlugwhen a cloud project is being browsed (CloudProjectsControllersets/clears the context).E-P4 — In-app status surface (#800)
CloudAccountMenuButtonmenu gains a status block: level + XP progress bar, streak, nearest-unlockable achievement with progress (computed client-side frommilestoneCatalog()+ counters), and "View My Achievements…" →https://qtmesh.dev/u/<slug>. Stats refresh opportunistically on menu open; last-known snapshot renders offline.GamificationToast: ONE restrained, coalescing, click-to-dismiss unlock toast (no animation).E-P5 — Discovery nudges (#801)
GamificationManager.suggestion— personalized to unused clusters when stats exist, generic rotation when logged out. Rotation cursor + per-card dismissals + a master nudge toggle persist in QSettings.E-P6 — Privacy, consent & offline correctness (#802)
/v1/me/gamification/prefs), and a two-click "Delete my gamification data" (server purge + local queue/cache clear).--no-telemetry.Tests
GamificationTypes_test: catalog/contract keys, wire-payload parsing, cache round-trip, nearest-unlockable ranking.GamificationEventQueue_test: persistence, dedup, ack, FIFO eviction, cross-instance merge.GamificationManager_test: consent gating, session dedup, numeric-metric filtering, stream opt-outs, logged-out no-network flush, full flush against a local HTTP mock (batch shape + achievement toast signal), delete-data, suggestion rotation.Closes #797, closes #798, closes #799, closes #800, closes #801, closes #802. Part of #796.
🤖 Generated with Claude Code
Follow-up commits (review + testing feedback)
c6527ce):--no-telemetrysuspends all emission process-wide; consent prompt only consumed with a GUI listener; queue tombstones prevent stale-snapshot resurrection; stream opt-outs drop queued events; owner-stamped entries never post across accounts; delete-my-data serialized with flushes; no prefs traffic when opted out. Plus theunit-tests-linuxlink fix (gamification sources added toqtmesh_test_common).7941977,28eab02): worker-thread callbacks guardQCoreApplication::instance()against app teardown;setProfilePubliccallback restructured for cpp:S2259.07ab6a2): gamification menu entries are physically inserted/removed (macOSQMenupaints hiddenQWidgetActions — black strips + hover offset); "Enable Progress Sync…" action when signed-in-but-disabled; "Syncing progress…" placeholder; stats fetch fires immediately on enable.CloudAccountMenuButtontests made hermetic (no OS keychain probe).lightingdiscovery cluster (c427012): pairs with qtmesh-cloud#96 (deployed) —noteFeature("lighting")on light creation and HDR environment load; catalog now 25 clusters.28eab02):clear()/removeKind()re-hydrate from disk when the persisted removal fails.Verified end-to-end against production: feature events from the running editor landed on the live account and the stats surface renders them.