Add Sentry usage analytics and increase test coverage - #172
Conversation
Add reusable in-memory mesh creation helpers (triangle, skeleton, animated entity) to TestHelpers.h and expand test coverage across 7 test files: - AnimationControlWidget: 23 new tests (was 3) for animation tree, keyframes, playback - MCPServer: 15 new tests for tool success paths with in-memory entities - MeshImporterExporter: 8 new tests for in-memory export (OBJ, STL, glTF, Ogre) - TransformOperator: 7 new tests for translate/rotate/scale with entities - PrimitivesWidget: 10 new tests verifying entity creation for all primitives - Manager: 6 new tests for in-memory entity lifecycle and scene hierarchy - Fix about_test/main_test creating local QApplication that destroyed the global one Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Improve visibility into how the app is used by adding session context tags (OS, arch, Qt version, renderer, launch mode), feature usage breadcrumbs for key user actions, and performance transactions for import/export and app startup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Sentry telemetry and CI symbol upload: new SentryReporter::setTag API plus breadcrumbs and transactions added across UI, core, startup, import/export flows; test extended for export formats; CI updated to build with debug symbols and upload them to Sentry. Changes
Sequence Diagram(s)(Skipped — changes are telemetry instrumentation and CI symbol upload, not a new multi-component control flow requiring sequence visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: a2441ed153
ℹ️ 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".
|
|
||
| void TransformOperator::translateSelected(const Ogre::Vector3& translation) | ||
| { | ||
| SentryReporter::addBreadcrumb("ui.transform", "Translate selected"); |
There was a problem hiding this comment.
translateSelected now emits a Sentry breadcrumb on every call, but this function is invoked continuously from mouseMoveEvent while the user drags the gizmo, so a single drag can generate dozens/hundreds of breadcrumbs; the same pattern was added to scale/rotate. In sessions with Sentry enabled this adds avoidable per-frame overhead during interactive transforms and quickly evicts higher-value breadcrumbs from the limited buffer, making crash context less useful. Record this once per transform gesture (e.g., on drag start/end) instead of per incremental step.
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: 2
🧹 Nitpick comments (2)
src/main.cpp (2)
269-272: Use RAII to guarantee transaction close.Line 272 currently closes the startup transaction manually. Wrapping this with
qScopeGuardwould make it safer against future early returns between start and finish.🛡️ Suggested adjustment
auto startupTxn = SentryReporter::startTransaction("app.startup", "app.load"); + auto startupTxnClose = qScopeGuard([&] { SentryReporter::finishTransaction(startupTxn); }); MainWindow w; w.show(); - SentryReporter::finishTransaction(startupTxn);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.cpp` around lines 269 - 272, Replace the manual start/finish pattern for the startup transaction with an RAII guard: call SentryReporter::startTransaction(...) to get startupTxn, then create a qScopeGuard (or similar scope guard) that calls SentryReporter::finishTransaction(startupTxn) when it goes out of scope; remove the explicit SentryReporter::finishTransaction(startupTxn) call so the guard always closes the transaction even if early returns or exceptions occur.
187-190: Deduplicate session tag assignment across launch branches.The same four tags are set in two places (Line 187–Line 190 and Line 243–Line 246). A small helper would reduce drift risk.
♻️ Proposed refactor
+ auto setSessionTags = [](const QString& launchMode) { + SentryReporter::setTag("os", QSysInfo::prettyProductName()); + SentryReporter::setTag("arch", QSysInfo::currentCpuArchitecture()); + SentryReporter::setTag("qt_version", qVersion()); + SentryReporter::setTag("launch_mode", launchMode); + }; ... - SentryReporter::setTag("os", QSysInfo::prettyProductName()); - SentryReporter::setTag("arch", QSysInfo::currentCpuArchitecture()); - SentryReporter::setTag("qt_version", qVersion()); - SentryReporter::setTag("launch_mode", "mcp"); + setSessionTags("mcp"); ... - SentryReporter::setTag("os", QSysInfo::prettyProductName()); - SentryReporter::setTag("arch", QSysInfo::currentCpuArchitecture()); - SentryReporter::setTag("qt_version", qVersion()); - SentryReporter::setTag("launch_mode", mcpWithGuiMode ? "gui+mcp" : "gui"); + setSessionTags(mcpWithGuiMode ? "gui+mcp" : "gui");Also applies to: 243-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.cpp` around lines 187 - 190, The four identical SentryReporter::setTag calls ("os", "arch", "qt_version", "launch_mode") are duplicated across launch branches; extract them into a small helper function (e.g., setSentrySessionTags or initSentryTags) that calls SentryReporter::setTag for each tag and their values (QSysInfo::prettyProductName(), QSysInfo::currentCpuArchitecture(), qVersion(), "mcp"), then replace both repeated blocks with a single call to that helper to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mainwindow.cpp`:
- Around line 516-518: The transaction started with
SentryReporter::startTransaction("ui.import", "file.import") may be skipped on
exception because MeshImporterExporter::importer(...) and exporter(...) can
throw; wrap each call to MeshImporterExporter::importer(...) and
MeshImporterExporter::exporter(...) in a try/catch that ensures
SentryReporter::finishTransaction(txn) is always called (catch std::exception
and catch(...) to cover non-std exceptions), call finishTransaction(txn) inside
the catch blocks (or in a finally-like scope) and then rethrow the exception so
upstream handling remains unchanged; reference the txn local variable and the
MeshImporterExporter::importer and MeshImporterExporter::exporter calls when
making the change (apply same pattern for the other occurrence noted around
lines 523–548).
In `@src/TransformOperator.cpp`:
- Around line 568-569: Remove the per-frame breadcrumb calls from the transform
mutation paths by deleting or relocating SentryReporter::addBreadcrumb
invocations inside translateSelected() and rotateSelected() (they are currently
called from mouseMoveEvent during drags), and instead emit a single breadcrumb
when a drag gesture begins: add one
SentryReporter::addBreadcrumb("ui.transform", "Translate selected" or "Rotate
selected") in mousePressEvent() at the point you detect the start of a transform
drag; ensure no breadcrumb calls remain inside translateSelected() or
rotateSelected() so telemetry only records gesture boundaries.
---
Nitpick comments:
In `@src/main.cpp`:
- Around line 269-272: Replace the manual start/finish pattern for the startup
transaction with an RAII guard: call SentryReporter::startTransaction(...) to
get startupTxn, then create a qScopeGuard (or similar scope guard) that calls
SentryReporter::finishTransaction(startupTxn) when it goes out of scope; remove
the explicit SentryReporter::finishTransaction(startupTxn) call so the guard
always closes the transaction even if early returns or exceptions occur.
- Around line 187-190: The four identical SentryReporter::setTag calls ("os",
"arch", "qt_version", "launch_mode") are duplicated across launch branches;
extract them into a small helper function (e.g., setSentrySessionTags or
initSentryTags) that calls SentryReporter::setTag for each tag and their values
(QSysInfo::prettyProductName(), QSysInfo::currentCpuArchitecture(), qVersion(),
"mcp"), then replace both repeated blocks with a single call to that helper to
avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73d3939c-88b2-468a-8469-efc45d5ef58e
📒 Files selected for processing (10)
src/AnimationWidget.cppsrc/Manager.cppsrc/MaterialEditorQML.cppsrc/MeshImporterExporter_test.cppsrc/PrimitivesWidget.cppsrc/SentryReporter.cppsrc/SentryReporter.hsrc/TransformOperator.cppsrc/main.cppsrc/mainwindow.cpp
| SentryReporter::addBreadcrumb("ui.transform", "Translate selected"); | ||
| if(SelectionSet::getSingleton()->hasNodes()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the breadcrumb locations
cat -n src/TransformOperator.cpp | sed -n '560,580p'Repository: fernandotonon/QtMeshEditor
Length of output: 1031
🏁 Script executed:
# Check lines 604-605 and 646-647
cat -n src/TransformOperator.cpp | sed -n '600,610p'Repository: fernandotonon/QtMeshEditor
Length of output: 509
🏁 Script executed:
cat -n src/TransformOperator.cpp | sed -n '640,650p'Repository: fernandotonon/QtMeshEditor
Length of output: 523
🏁 Script executed:
# Search for where these transform methods are called, especially from event handlers
rg -n 'mouseMoveEvent|translateSelected|scaleSelected|rotateSelected' src/ -A2 -B2 | head -100Repository: fernandotonon/QtMeshEditor
Length of output: 6689
🏁 Script executed:
# Look for gesture boundary methods like mousePress/Release
rg -n 'mousePressEvent|mouseReleaseEvent|mouseMoveEvent' src/ | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 3956
🏁 Script executed:
# Examine the mouseMoveEvent implementation in TransformOperator.cpp
cat -n src/TransformOperator.cpp | sed -n '435,530p'Repository: fernandotonon/QtMeshEditor
Length of output: 4759
🏁 Script executed:
# Also check mousePressEvent and mouseReleaseEvent to understand the gesture boundaries
cat -n src/TransformOperator.cpp | sed -n '411,440p'Repository: fernandotonon/QtMeshEditor
Length of output: 1276
🏁 Script executed:
cat -n src/TransformOperator.cpp | sed -n '529,565p'Repository: fernandotonon/QtMeshEditor
Length of output: 1649
🏁 Script executed:
# Search for scale-related state handling in mouseMoveEvent and elsewhere
rg -n 'TS_SCALE|setSelectedScale|scaleSelected' src/TransformOperator.cpp | head -30Repository: fernandotonon/QtMeshEditor
Length of output: 362
🏁 Script executed:
# Look for all places where transform methods are called to understand the complete flow
rg -n 'translateSelected\(|rotateSelected\(|scaleSelected\(' src/TransformOperator.cppRepository: fernandotonon/QtMeshEditor
Length of output: 722
Move breadcrumbs out of per-frame transform mutation paths.
translateSelected() (line 568) and rotateSelected() (line 646) are called directly from mouseMoveEvent() during drag operations, causing the breadcrumbs to fire on every mouse movement. This floods telemetry and adds unnecessary overhead. Emit a single breadcrumb at gesture boundaries instead (e.g., mousePressEvent for drag start).
Suggested fix
void TransformOperator::translateSelected(const Ogre::Vector3& translation)
{
- SentryReporter::addBreadcrumb("ui.transform", "Translate selected");
if(SelectionSet::getSingleton()->hasNodes())
{
...
}
}
void TransformOperator::scaleSelected(const Ogre::Vector3& scaleFactor)
{
- SentryReporter::addBreadcrumb("ui.transform", "Scale selected");
if(SelectionSet::getSingleton()->hasNodes())
{
...
}
}
void TransformOperator::rotateSelected(const Ogre::Quaternion& rotation)
{
- SentryReporter::addBreadcrumb("ui.transform", "Rotate selected");
if(SelectionSet::getSingleton()->hasNodes())
{
...
}
}Then add a single breadcrumb in mousePressEvent() when a transform drag begins.
Also applies to: 604-605, 646-647
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TransformOperator.cpp` around lines 568 - 569, Remove the per-frame
breadcrumb calls from the transform mutation paths by deleting or relocating
SentryReporter::addBreadcrumb invocations inside translateSelected() and
rotateSelected() (they are currently called from mouseMoveEvent during drags),
and instead emit a single breadcrumb when a drag gesture begins: add one
SentryReporter::addBreadcrumb("ui.transform", "Translate selected" or "Rotate
selected") in mousePressEvent() at the point you detect the start of a transform
drag; ensure no breadcrumb calls remain inside translateSelected() or
rotateSelected() so telemetry only records gesture boundaries.
Build with -g debug info on all platforms and upload symbols to Sentry during releases so stack traces show file names and line numbers instead of raw memory addresses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/deploy.yml (1)
1199-1211: Correct implementation, minor cleanup inconsistency.The macOS debug symbol handling is correct:
dsymutilruns beforestrip(required since dsymutil needs debug info)- dSYM bundle is properly uploaded to Sentry
Unlike Windows and Linux which clean up their debug files, the dSYM bundle at
${{github.workspace}}/bin/QtMeshEditor.dSYMis not removed. While this won't affect the DMG (sincecreate-dmgonly packages the.appbundle), consider adding cleanup for consistency:💡 Optional: Add cleanup for consistency
curl -sL https://sentry.io/get-cli/ | bash sentry-cli debug-files upload --include-sources "${{github.workspace}}/bin/QtMeshEditor.dSYM" + rm -rf "${{github.workspace}}/bin/QtMeshEditor.dSYM"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 1199 - 1211, Add cleanup of the generated dSYM after upload in the "Upload debug symbols to Sentry" step: after running sentry-cli debug-files upload --include-sources for "${{github.workspace}}/bin/QtMeshEditor.dSYM", remove the dSYM bundle (e.g., run rm -rf "${{github.workspace}}/bin/QtMeshEditor.dSYM") to match Windows/Linux cleanup behavior; locate the code using BIN_PATH, the dsymutil and strip commands, and the sentry-cli upload invocation to insert the cleanup command immediately after the upload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 1199-1211: Add cleanup of the generated dSYM after upload in the
"Upload debug symbols to Sentry" step: after running sentry-cli debug-files
upload --include-sources for "${{github.workspace}}/bin/QtMeshEditor.dSYM",
remove the dSYM bundle (e.g., run rm -rf
"${{github.workspace}}/bin/QtMeshEditor.dSYM") to match Windows/Linux cleanup
behavior; locate the code using BIN_PATH, the dsymutil and strip commands, and
the sentry-cli upload invocation to insert the cleanup command immediately after
the upload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b9bc748-4638-4426-bf30-7a53352002d4
📒 Files selected for processing (1)
.github/workflows/deploy.yml
- Move transform breadcrumbs from per-frame methods to mousePressEvent to avoid flooding telemetry during drag operations - Wrap import/export transactions in try/catch to guarantee finishTransaction is called on exception paths - Use qScopeGuard for startup transaction RAII cleanup - Extract duplicated session tag calls into setSentrySessionTags helper Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.cpp`:
- Around line 271-272: The startup transaction created by
SentryReporter::startTransaction (stored in startupTxn) is being finished by a
qScopeGuard that captures the entire main scope, so it only closes after
a.exec() and measures the whole app lifetime; change this so the transaction is
finished at the end of the startup phase instead: either move the qScopeGuard
into a smaller scope that ends before calling a.exec() or explicitly call
SentryReporter::finishTransaction(startupTxn) at the point where startup
completes (before entering the event loop), ensuring startupTxn only covers
initialization work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4437a54-0981-4ff7-a098-bc23fdfc0eab
📒 Files selected for processing (3)
src/TransformOperator.cppsrc/main.cppsrc/mainwindow.cpp
| auto startupTxn = SentryReporter::startTransaction("app.startup", "app.load"); | ||
| auto startupTxnClose = qScopeGuard([&] { SentryReporter::finishTransaction(startupTxn); }); |
There was a problem hiding this comment.
Startup transaction currently measures full app lifetime, not startup.
At Line 271, the transaction starts, but with the current scope guard it closes only when main() exits (after a.exec()), so app.startup spans the entire session.
💡 Suggested fix
auto startupTxn = SentryReporter::startTransaction("app.startup", "app.load");
auto startupTxnClose = qScopeGuard([&] { SentryReporter::finishTransaction(startupTxn); });
MainWindow w;
w.show();
@@
if (mcpWithGuiMode) {
@@
}
+
+// Close startup trace after initial UI bootstrap enters the event loop.
+QTimer::singleShot(0, &a, [&startupTxn]() {
+ SentryReporter::finishTransaction(startupTxn);
+ startupTxn = 0; // keep scope-guard idempotent
+});
int result = a.exec();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.cpp` around lines 271 - 272, The startup transaction created by
SentryReporter::startTransaction (stored in startupTxn) is being finished by a
qScopeGuard that captures the entire main scope, so it only closes after
a.exec() and measures the whole app lifetime; change this so the transaction is
finished at the end of the startup phase instead: either move the qScopeGuard
into a smaller scope that ends before calling a.exec() or explicitly call
SentryReporter::finishTransaction(startupTxn) at the point where startup
completes (before entering the event loop), ensuring startupTxn only covers
initialization work.
|
1 participant
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests