code-review

name	code-review
description	Dusk Studio review checklist - the repo-specific defects that have actually shipped here. Use when reviewing a diff, branch, or PR in this repo, on top of the normal review pass.

Dusk Studio code review

Run the normal review first. These are the checks that have caught real defects in this repo; each one exists because the class of bug reached a commit.

Cross-platform native hosting (src/engine/{clap,lv2,vst3}/)

The editors are per-platform translation units behind one shared header: *Editor.cpp on Linux, *Editor_Mac.mm on Apple. Review the pair, never one alone.

• Every ungated method in the header has a definition in every platform TU. A method whose declaration sits under #if defined(__linux__) (the wrappers' verifyGeometry) is defined only there; one declared unconditionally needs a body everywhere, even if that body is return false; - that is how getActualGeometry exists on macOS. A missing .mm breaks configure ("Cannot find source file"), not just the build, so CMake pre-routing of a file that does not exist yet fails the whole macOS job. Verify the file list in CMakeLists.txt against ls.
• Header comments must not state one platform's behaviour as universal. embed() maps the X11 window immediately but leaves the Cocoa container hidden until reveal(); a comment claiming either as the rule is wrong half the time.
• Native handles cross the boundary as std::uintptr_t, never X11's unsigned long - Windows LLP64 truncates HWND. Check the JUCE wrapper's accessor too, not just the editor signature.
• Coordinate space is per-platform. X11 wants physical pixels (embedscale::toPhysical); Cocoa wants logical points. The house pattern is editorBoundsInPeer() / componentExtentFromEditor() with #if defined(__APPLE__) inside - see ClapPluginEditorComponent. A wrapper calling toPhysical unconditionally is correct only while UI zoom is 1.0.
• X11-only diagnostics stay Linux-gated. getActualGeometry returns false on macOS by design, so an ungated verifyGeometry() prints "host window lost (XGetGeometry failed)" on every macOS editor. Gate declaration, definition, and call site.
• Linux behaviour is an invariant. Any change to a shared header or the Linux TU that cannot be validated on this machine is a flag, not an edit.

Lifetime of deliberately-leaked resources

setLeakOnClose(true) exists because foreign-toolkit UIs hang in teardown. When a resource is leaked on purpose, anything it can still dereference has to outlive it too. A leaked suil instance keeps its controller and ui:resize handle; a leaked CLAP GUI keeps its ClapHost::Callbacks. If the owning object is destroyed while the leaked resource can still tick a timer, that is a use-after-free at shutdown.

Walk the reachable pointer graph from the leaked object and leak exactly that set. Objects the leaked resource cannot reach still get their normal cleanup - leaking more than the graph requires is its own bug. Check both directions: what the leaked resource points at, and what still points at it.

Feature gates and defines

• A constant or helper used under gate A but defined under gate B breaks the first platform where the gates disagree. kScanTimeoutMs was defined under DUSKSTUDIO_HAS_OOP_PLUGINS and used by the native-bundle scan sandbox; macOS without OOP support stopped compiling. Grep every use of a gated symbol for the gate it sits under.
• A default-ON option must derive its default from its dependency, and FATAL_ERROR only when the user asked for it explicitly. Otherwise a machine missing the dependency cannot configure at all. Keep the "requested but absent is fatal" half - a silently-OFF format is not verification.

Platform API contracts

Verify the contract, do not assume the common case:

• CoreFoundation URLs from CFBundleCopyExecutableURL are relative to the bundle, so CFURLCopyFileSystemPath gives back a relative path - observed here as the bare TestClap, which dlopen then treated as a name to search the dyld paths for. Resolve against the base with CFURLGetFileSystemRepresentation(url, /*resolveAgainstBase*/ true, buf, len), check its boolean return (it fails on a buffer too small), and CFRelease the URL on every path.
• Cocoa subview placement is governed by the superview's isFlipped, not the child's. JUCE's peer view is flipped, so top-left coordinates already land correctly - a claim that placement is inverted needs that checked first.

Session load (SessionSerializer::load)

load() mutates the live Session rather than constructing a fresh one, and almost every setter is if (json::has (v, key)). Two consequences that have each shipped as a bug:

• An empty object does not blank a slot. Driving a surplus track/bus/aux slot through restore with {} clears the things written unconditionally (regions, MIDI, automation, plugin state) and leaves everything conditional - name, colour, fader, pan, sends, hardware routing - holding the previous session's values. Blank a slot from the serialized default object, not {}. A partially-populated slot object has the same hole for its absent keys.
• A newly persisted field must reset when absent, or the previous session's value survives into a file that predates the key. Write store (has (k) ? v : modelDefault), and pin it with a test that loads a cut-down {"version":N,"transport":{}} after setting the live value.

Anything a corrupt file can reach that feeds DSP needs a finite guard and a range clamp - std::clamp passes NaN straight through, so clamping alone is not sanitising. Clamp to the range the corresponding control enforces, not an invented one.

Deferred callbacks on the engine

AudioEngine posts to the message thread with dusk::callAsync from paths that can outlive it (device error from the I/O thread, hot-unplug from a change broadcast). The house pattern is a std::shared_ptr<std::atomic<bool>> latch captured by value - midiHotplugAlive, changeListenersAlive, deviceCallbacksAlive - cleared first thing in ~AudioEngine, with the lambda checking it before touching this. A new deferral needs one of these; adding a fresh mechanism alongside them is the review finding, not the fix.

Tests

• A read issued straight after preparePlayback() races the prefetch. BufferedFileReader::prefetch only wakes a background worker, and readRt returns silence on a miss rather than blocking, so an immediate assertion is a flake that looks like a DSP bug. The loop-start pre-cache is filled synchronously in primeLoopCaches, so a test that needs deterministic samples enables a loop over the region and reads through the cache.
• Temp directories must be created atomically, not merely named uniquely: mkdtemp, or create_directory whose false return means someone else won the name, and retry with a fresh candidate. ctest runs cases as parallel processes, so a pid / random_device / clock-tick suffix narrows the window but does not close it.
• Paths compared against scanner output must be normalised - lexically_normal() resolves . but keeps a trailing separator, which then shows up as an empty final filename.
• An error-message assertion that accepts a broad prefix ("dlopen failed:") hides the bug underneath it. Assert on the part that proves the code under test did its job.

Documentation sync

• MANUAL.md platform claims go stale the moment a format gains a platform. Grep it for the format name and for "on Linux" whenever hosting changes.
• A behaviour switch documented for "plugins" must name which rows it reaches. DUSKSTUDIO_USE_OOP_PLUGINS is read only in PluginSlot.cpp, so it never touches native CLAP/LV2/VST3 rows.
• Spec status lines and handoff prompts are load-bearing - a stale "next increment" line makes the following session redo finished work. When a phase lands, update the status line, the resume phrase, and any prompt that names the phase.