Back to Serial Studio

Common Mistakes — Silent Breakage

doc/claude/common-mistakes.md

4.0.313.5 KB
Original Source

Common Mistakes — Silent Breakage

On-demand lookup table, grouped by subsystem so a targeted read is possible. These are gotchas the linter can't always catch. Rule-driven mistakes are listed once in the style sections (CLAUDE.md "Code Style") — don't restate. Four essay-length entries live in full in doc/claude/architecture/; their rows here are pointers.

Hotpath & Frame Pool

MistakeFix
Mutexes in FrameReader / CircularBufferRecreate via resetFrameReader() / reconfigure(). SPSC is the contract.
Qt::QueuedConnection on the frame hotpath when both ends are on mainQt::DirectConnection. Queued = queue-full drops at 10+ kHz.
QMap::operator[] on m_sourceFrames[sourceId].find() and validate — operator[] silently inserts.
std::make_shared<DataModel::TimestampedFrame>(...) directly on the FrameBuilder hotpathUse acquireFrame(src) / acquireFrame(src, ts). Direct make_shared bypasses the slot pool and brings back per-frame heap allocs.
Calling .toDouble() on any receiver (QString, QStringView, QByteArray, QVariant, QJsonValue)Use the matching SerialStudio::toDouble(...) overload (inline, fast_float-backed). code-verify.py:qt-todouble-direct blocks direct calls outside SerialStudio.h/ThirdParty/. Qt's string parser walks the full locale + double-conversion pipeline even for plainly non-numeric text (measured >2x parse-throughput loss); the QVariant/QJsonValue overloads additionally parse string-typed payloads (JSON "5.5" loads as 5.5 instead of silently 0) and fall back to Qt for non-string types. Frame.h can't include SerialStudio.h (circular) — that's why the number-parsing read()/serialize() family lives out-of-line in Frame.cpp.
Treating CapturedData::data as a smart pointer (*data->data, data->data->size(), if (!data->data))It's a QByteArray now. Use data->data, data->data.size(), data->data.isEmpty(). The shared_ptr indirection was removed because QByteArray is already COW with atomic refcount.
Adding a new input to Dashboard::streamAvailable(), the FrameBuilder any-async-sink poll, or SerialStudio::isAnyPlayerOpen() without wiring its change signal to the matching cache refresh (updateStreamAvailable / refreshAnyAsyncSink / the player lambdas)The hotpath reads cached flags, not the live getters. A missed signal leaves the cache stale: frames silently never reach the dashboard, or exports silently stop. Wire the new signal with Qt::DirectConnection (queued refreshes lag a full event-loop turn behind frames already flowing). Flag mechanics: architecture/dataflow.md "Cached Hotpath Flags".
Polling a singleton getter per frame on the parse/publish path (AppState::operationMode(), consumer enabled(), isAnyPlayerOpen(), per-frame std::map::find)Cache it as a member refreshed by the owning signal; the per-frame cost of guarded-static hops + map walks is what dragged the native parser gate down. See the cached members in FrameBuilder (m_operationMode, m_playerOpen, m_anyAsyncSink) and FrameParser (m_engine0Cache).
Returning QList<QStringList> from a new native template when a single-row view split would doImplement INativeParser::parseSpans (views into the frame bytes, -1 when the contract can't hold: multi-row, latch state, content rewriting like quote-unescaping). The span lane is what lets Native parse without per-token allocations; the QList path is the fallback, not the default.
Writing dataset.value / slot strings via implicit-share assignment on the span laneUse assign_utf8_in_place / assign_string_in_place (Frame.h). A share-assign re-links buffers, so the next in-place write detaches and re-allocates every frame — the zero-alloc steady state silently degrades back to per-frame mallocs.
Caching a raw pointer (or QByteArrayView) into an extracted frame's data->data past the synchronous parseExtracted CapturedData objects are pool slots reused IN PLACE once every CapturedDataPtr drops (FrameReader::claimCapturedSlot). Holding the CapturedDataPtr is safe (keeps the slot pinned); a COW QByteArray copy is safe (the refill swaps buffers instead of reusing a shared one); a raw pointer/view is NOT — it reads the next frame's bytes. Off-main consumers never see pool slots (they get driver chunks or detached copies), keeping the count-1 probe exact.
Publishing a TimestampedFrame to the dashboard without stamping structureGeneration = m_framePoolGenerationThe dashboard skips its per-frame compare_frames() structural revalidation whenever the cached per-source generation matches, so a frame that leaves the field at its default 0 (or a stale value) makes Dashboard::hotpathRxFrame either reconfigure every frame or — worse — never reconfigure after a real layout change. Every acquireFrame / trySpanLane publish site (pool slot and heap fallback) must set it; the generation only advances via invalidateFramePool(), which already fires on every structural change (project sync, mode/connection change, Quick Plot channel-count change).

Timestamps

MistakeFix
QMetaObject::invokeMethod(...) forwarding received data without capturing timeCapture SteadyClock::now() in the callback, pass to publishReceivedData(data, timestamp).
Export/report worker calling steady_clock::now() to stamp a frameUse monotonicFrameNs(frame->timestamp, baseline) on the shared TimestampedFramePtr.

Dashboard & Widgets

MistakeFix
Mixing workspace IDs with group IDsWorkspace IDs are ≥ 1000; Taskbar::deleteWorkspace() branches on that.
Adding a widget that displays Dataset::value (the string) without registering it in Dashboard::buildValuePushesNumeric datasets propagate the string only to observable targets (DataGrid-group copies + m_lastFrame, which dashboard.getData serializes); every other target keeps a stale string on purpose. Add the new widget's dataset copies to the string_targets set or its cells silently show old values.
Per-frame getDatasetWidget / getGroupWidget / widgetCount lookups inside a Dashboard::update*Series helperResolve pointers at configure time into a push table (SeriesPush / GpsPush / Plot3DPush, mirroring LinePush); the per-frame walk must be lookup-free. Rebuild the table in the matching configure*; clearPushTables() drops them on reset.

ProjectModel & Project Files

MistakeFix
buildTreeModel() inside an item-change handlerDefer with QTimer::singleShot(0, ...).
Mutating ProjectModel on every keystrokeUpdate the tree item in-place via m_*Items.
Force-rebuilding buildSourceModel on selectionGuard with m_awaitingContextRebuild.
Stamping current_writer_version() on every live Frame::serializeOnly project saves (ProjectModel::serializeToJson) carry version metadata. Live frames keep schemaVersion = 0.

IO & Drivers

MistakeFix
createDriver() for UI configConnectionManager::instance().uart() etc.
Live driver with empty device listCall refreshSerialDevices() / refreshSerialPorts() in open() if empty.
BLE selectDevice(index) with placeholder compensation in setDriverPropertysetDriverProperty is raw; selectDevice subtracts 1.
Querying the live driver for configurationOk()Check the UI driver — live may not be synced yet.
Stopping a read thread that uses the connect(&thread, &QThread::started, this, &readLoop, Qt::DirectConnection) idiom with only m_running = false + wait()started fires before QThread::run(), so when readLoop returns the thread falls into exec() and never finishes on its own — the wait() times out every close and the terminate() fallback fires (TerminateThread on Windows kills the thread mid-dispatcher and corrupts quit). Call thread.quit() before wait(); quit() is safe even before exec() starts (it sets quitNow, so exec() returns immediately). HID::cleanupDevice() is the reference; the gs_usb CANable backend shipped without it and crashed every quit.

Scripting & Parsers

MistakeFix
Per-dataset transformCode with a leftover placeholderDatasetTransformEditor::onApply discards code that doesn't define transform().
Driving setInterrupted(true) from a QTimer on the thread that runs QJSValue::call()The event loop is blocked during the call, so the timer never fires — the original watchdog no-op. Arm a DataModel::JsWatchdog; only JsWatchdogThread flips the flag (off-thread). code-verify.py:js-interrupt-off-thread blocks setInterrupted(true) outside JsWatchdogThread.cpp.
Touching the QML-embedded code editors (JsCodeEditor & siblings) without preserving the hidden-widget plumbingThree invariants (position sync, ShortcutOverride forwarding, completer-popup rerouting) hold the embedded editor together — read architecture/scripting.md "Embedded Code Editors" in full before editing.
Writing string-based byte access (string.byte, string.unpack, charCodeAt, split) in a Lua/JS parser that runs under the Binary decoder — including the importer-generated Lua templatesBinary frames arrive as a 1-indexed byte table (Lua) / byte Array (JS), not a string — every frame errors out. Index directly, or convert once at the top of parse(). Full semantics + reference patterns in architecture/scripting.md "Binary-decoder scripts". Both importers shipped this bug in 2026-06.

Export & Sessions

MistakeFix
Looking for session DB code under app/src/SQLite/It's app/src/Sessions/namespace Sessions for DatabaseManager, Export, and Player (all three).

Qt & QML UI

MistakeFix
Setter without guard returnif (m_foo == foo) return; before assign + emit.
Running heavy work synchronously inside a QFileDialog::fileSelected slot (open another dialog, parse a file, mutate model)On macOS fileSelected fires from inside QFileDialog::done(), which runs from an NSSavePanel KVO callback (ViewBridge/NSRemoteViewMarshal). Re-entering Qt synchronously can leave the WA_DeleteOnClose dialog deleted under the panel and crash on return. Always wrap the body in QMetaObject::invokeMethod(this, [...] { ... }, Qt::QueuedConnection) so done() unwinds first. Same applies to slots calling dialog->deleteLater() — defer the work, not just the deletion.
Binding the same QML Shortcut sequence twice in one window (e.g. view-level StandardKey.Open gated on activeFocus + an unconditional window-level one)When both are enabled Qt fires activatedAmbiguously() — which nobody handles — so the key does nothing. Bind each sequence once per window, or make the enabled: gates mutually exclusive.
Combining %n with .arg() in a translated string — tr("%n elements").arg(count) / qsTr("%n elements").arg(count)%n is Qt's plural marker consumed by the count overload (tr("%n elements", nullptr, count)), not .arg(). Passing it through .arg() leaves the literal %n in the UI (or mangles it) and breaks every .ts/.qm translation. When building a string with .arg(), always use numbered placeholders: tr("%1 elements").arg(count), same for qsTr(). Use .arg() notation only — never mix in another substitution style. A common breakage when adding new features.

CI & Platform

MistakeFix
Injecting a second -platform after --headless/--benchmark already injected offscreen (Windows adjustArgumentsForFreeType appends -platform windows:fontengine=freetype)Qt takes the last -platform, so the appended one silently defeats offscreen and the "headless" run uses the real windows GUI platform — which blocks on a session-less CI runner (looks like a hang). Skip the freetype override when a -platform is already present. Windows-only: Linux/macOS never call adjustArgumentsForFreeType.
Running --benchmark-hotpath (or any CLI path) of the GUI-subsystem (WIN32_EXECUTABLE TRUE) Windows exe and expecting the shell to wait + capture stdoutA /SUBSYSTEM:WINDOWS binary detaches from the launching console — CI hangs with no output and no exit code. Benchmark a throwaway editbin /SUBSYSTEM:CONSOLE copy of the exe; ship the GUI-subsystem original. Full mechanics: architecture/dataflow.md "Hotpath Benchmark", CI gotcha.
Registering the main thread with MMCSS before the Qt message handler is installed, or treating the QThread::start priority warning it triggers as a real failureThe warning is benign (the thread lands at its exact pre-MMCSS priority); register only via Platform::AppPlatform::registerIngestThreadWithMmcss() AFTER qInstallMessageHandler, and never start a QThread expecting to inherit the boosted band. Full contract: architecture/startup.md "MMCSS coexistence contract".

Process & Trust

MistakeFix
Bundled scope creep — slipping an unrelated bug-fix, "small cleanup", rename, or import-sort into the same diff as the user's actual askName it in chat first ("noticed X — want it in this pass?"). Every unrelated file you touch costs the reviewer an audit pass, and "all the changes were individually correct" doesn't restore the trust the surprise diff cost. The user can always say yes; they can't say no after the fact.