doc/claude/common-mistakes.md
Extracted from CLAUDE.md. On-demand lookup table.
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.
| Mistake | Fix |
|---|---|
QMetaObject::invokeMethod(...) forwarding received data without capturing time | Capture SteadyClock::now() in the callback, pass to publishReceivedData(data, timestamp). |
Export/report worker calling steady_clock::now() to stamp a frame | Use monotonicFrameNs(frame->timestamp, baseline) on the shared TimestampedFramePtr. |
buildTreeModel() inside an item-change handler | Defer with QTimer::singleShot(0, ...). |
| Mutexes in FrameReader / CircularBuffer | Recreate via resetFrameReader() / reconfigure(). SPSC is the contract. |
Qt::QueuedConnection on the frame hotpath when both ends are on main | Qt::DirectConnection. Queued = queue-full drops at 10+ kHz. |
QMap::operator[] on m_sourceFrames[sourceId] | .find() and validate — operator[] silently inserts. |
Mutating ProjectModel on every keystroke | Update the tree item in-place via m_*Items. |
createDriver() for UI config | ConnectionManager::instance().uart() etc. |
Force-rebuilding buildSourceModel on selection | Guard with m_awaitingContextRebuild. |
| Live driver with empty device list | Call refreshSerialDevices() / refreshSerialPorts() in open() if empty. |
BLE selectDevice(index) with placeholder compensation in setDriverProperty | setDriverProperty is raw; selectDevice subtracts 1. |
Querying the live driver for configurationOk() | Check the UI driver — live may not be synced yet. |
| Setter without guard return | if (m_foo == foo) return; before assign + emit. |
Looking for session DB code under app/src/SQLite/ | It's app/src/Sessions/ — namespace Sessions for DatabaseManager, namespace SQLite for Export/Player. |
| Mixing workspace IDs with group IDs | Workspace IDs are ≥ 1000; Taskbar::deleteWorkspace() branches on that. |
Stamping current_writer_version() on every live Frame::serialize | Only project saves (ProjectModel::serializeToJson) carry version metadata. Live frames keep schemaVersion = 0. |
Per-dataset transformCode with a leftover placeholder | DatasetTransformEditor::onApply discards code that doesn't define transform(). |
std::make_shared<DataModel::TimestampedFrame>(...) directly on the FrameBuilder hotpath | Use 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. |
Adding a widget that displays Dataset::value (the string) without registering it in Dashboard::buildValuePushes | Numeric 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 helper | Resolve 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. |
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. |
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. |
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. |
| Bundled scope creep — slipping an unrelated bug-fix, "small cleanup", rename, or import-sort into the same diff as the user's actual ask | Name 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. |
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 the --benchmark-hotpath (or any CLI) path of the GUI-subsystem (WIN32_EXECUTABLE TRUE) Windows exe and expecting the shell to wait + capture stdout | A /SUBSYSTEM:WINDOWS binary detaches from the launching console: cmd/PowerShell don't wait for it, its stdout is unwired, and the AttachConsole+CONOUT$ fallback writes to the console screen buffer that GitHub's pipe-based log capture never reads → CI hangs with no output and no exit code (Start-Process -Wait and plain bash both fail differently). For CI, benchmark a throwaway editbin /SUBSYSTEM:CONSOLE copy of the exe — a console-subsystem image stays attached, so the shell waits, stdout pipes through, and the exit code propagates. Leave the shipped exe /SUBSYSTEM:WINDOWS so it never flashes a console for end users. Background: https://www.devever.net/~hl/win32con |
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). |
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 do | Implement 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 lane | Use 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 parse | Extracted 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. |
Registering the main thread with MMCSS (AvSetMmThreadCharacteristics) before the Qt message handler is installed, or assuming the QThread::start priority warning it triggers is a real failure | Qt's default QThread::InheritPriority reads the creator's raw priority (MMCSS-managed ~25, not a THREAD_PRIORITY_* constant) and feeds it back to SetThreadPriority, which rejects it — the thread still starts and lands at NORMAL, its exact pre-MMCSS inherited value, so the failure is benign; explicit priorities (named constants, e.g. Audio.cpp setPriority(HighestPriority)) are unaffected. The coexistence contract: register only via Platform::AppPlatform::registerIngestThreadWithMmcss(), called AFTER qInstallMessageHandler (ModuleManager) so the targeted filter eats the warning, and never start a QThread expecting it to inherit the boosted band. |
Publishing a TimestampedFrame to the dashboard without stamping structureGeneration = m_framePoolGeneration | The 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). |
Touching the QML-embedded code editors (JsCodeEditor & siblings) without preserving the hidden-widget plumbing | The editor is an offscreen QCodeEditor grabbed into a QQuickPaintedItem, so three invariants hold it together: renderWidget() first calls syncWidgetPosition() (moves the hidden top-level to the item's screen position — completer popups and drag auto-scroll resolve global coordinates through it); event() forwards ShortcutOverride to the widget (editing keys handled natively — without it QML Shortcut soup decides, and duplicated sequences across the same window go ambiguous and silently dead); keyPressEvent reroutes to completer()->popup() while it's visible (the popup never has real focus in the embedded setup). Input handlers call renderWidget() directly — dropping that re-introduces one-timer-tick lag on every keystroke/drag. |
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. |
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. |
Writing string-based byte access (string.byte, string.unpack, charCodeAt, split) in a Lua/JS parser that runs under the Binary decoder — including the Lua templates the importers generate (DBCImporter, ModbusMapImporter) | With SerialStudio::Binary, LuaScriptEngine::parseBinary pushes the frame as a 1-indexed table of byte integers and JsScriptEngine::parseBinary passes an Array of bytes — not a string. Every frame dies with bad argument #1 to 'byte' (string expected, got table) (Lua) or frame.split is not a function (JS). Index the table/Array directly (frame[1], sawtooth/LSB walks like the DBC template); when string.unpack is genuinely needed (multi-byte/float fields, Modbus), convert once at the top of parse(): if type(frame) == "table" then frame = string.char(table.unpack(frame)) end. The shipped parser templates (app/rcc/scripts/parser/) and ProtoImporter's bytesToString are the reference patterns; both importers shipped this bug in 2026-06. |