DarkRoom/docs/code-health.md

DarkRoom — Code health and the cost of a contribution

Status: Audit · 2026-08-27 Companion to: architecture.md, technical-debt.md, view-composition.md

What it costs to add something to this codebase, measured rather than estimated, and the work that would lower the price.

technical-debt.md records compromises that were chosen — each one has a reason that outlived the person who took it. This document records the opposite: friction nobody chose, which accumulated because no single commit was responsible for it. The distinction matters when deciding what to touch. A TD entry is load-bearing until its "done when" is met; an entry here is not defending anything.

It is also not a bug list. Everything below compiles, passes 2,007 tests and ships.

---

1. What was measured

Every figure in this document is reproducible from a clean checkout. Worktrees under .claude/ and build output under target/ are excluded from all counts — including them roughly triples the line totals and was the first thing to get wrong.

# Lines of Rust per crate
for d in core/* ui/* platform/* apps/* tools/*; do
  [ -d "$d/src" ] && echo "$(find $d/src -name '*.rs' -exec cat {} + | wc -l) $d"
done | sort -rn

# unwrap() in production code only — split each file at its #[cfg(test)] marker
# (a naive grep counts ~1,500 and tells you nothing)

# Distinct window properties written per UI module
for f in ui/dr-ui/src/*.rs; do
  echo "$(grep -oP '\b(w|window|win|ui)\.\Kset_[a-z0-9_]+(?=\()' "$f" | sort -u | wc -l) $f"
done | sort -rn

Measure	Value
Rust across 19 crates	~112,000 lines; ~78,000 after comments and blanks
Comment density	28% overall, 20–34% per crate
Test functions	2,007, plus 17 integration test files
.unwrap() in production code	3 — one in dr-gpu, two in dr-ingest
.unwrap() in test code	~1,500, which is where it belongs
unsafe blocks	6
TRACES tags / orphan tags	732 / 0
Resolved dependencies	826
Largest function	dr-ui::run — 1,813 lines
AppWindow members	282 properties + 190 callbacks

CI gates on cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace, a release build, and an Android cross-check. The traceability matrix is regenerated and compared, with a pre-commit hook that keeps it in step.

---

2. The seams, graded

Six things a contributor might plausibly want to add. Each grade was checked against the tree.

Feature	What you touch	Cost
A develop operation split toning, channel mixer	One file in core/dr-pipeline/ops/. Nothing else.	Trivial
A RAW format	A Format variant and magic-byte recognition. dr-decode is a generic TIFF walker carrying only 3 format-specific branches.	Easy
A neighbourhood operation dehaze, a sharpener	Rust in dr-pipeline/src/ops/ implementing Operation + DetailStage, plus a stub YAML declaring rust: and order:.	Moderate
A parameter widget a colour wheel	A WidgetKind variant, develop::supported(), the panel model, a Slint component.	Moderate
A second sync backend S3, WebDAV, a local folder	RemoteBackend is the easy half. Seven UI files construct NextcloudBackend directly and ten signatures take it concretely — see CH-2.	Hard
Anything with its own UI	app.slint's root component, a ~1,000-line wire(), and run() at 1,813 lines — see CH-1.	Hard

The top half of that table is the good half, and it is very good. The bottom half is one problem wearing two hats: dr-ui has no seams, so every UI feature lands in the same three files.

---

3. What is load-bearing, and must not be "tidied"

Listed before the findings deliberately. A remediation document that only enumerates problems invites someone to fix something that was right.

The operation declaration format. ops/*.yaml + build.rs is a working plugin system that happens to resolve at build time — see display-and-extension.md §6. Its deliberate smallness is the point: the expression grammar is restricted so a declaration cannot become a second, worse place to write code. Do not "improve" it by letting a node name arbitrary Rust.

No operation is named in ui/. Verified: all fifteen built-in op ids grepped across every .rs and .slint file in ui/ yield exactly one hit, a localisation test in labels.rs:353. This is what makes "a new operation is one file" true rather than aspirational, and it is the single property most likely to be destroyed by a well-meaning special case in the panel. See CH-5.

Unimplemented widgets degrade rather than break. develop::supported() lists every WidgetKind explicitly instead of using a wildcard, so a new kind added to the core surfaces as a compile error rather than as silence, and a widget nothing draws falls back to sliders with the edit still working (ARCH §4.3a).

The mpsc-plus-timer worker shape. Slint's event loop must never block (NFR-P9). Threading is not what any finding below proposes changing.

sync_rows mutating rows in place. Replacing the model breaks slider dragging.

---

CH-1 — dr-ui has no view layer, and the cost is compounding

Where: ui/dr-ui/src/lib.rs:836, library_ui.rs:4374, collections_ui.rs:1457, ui/dr-ui/ui/app.slint

What it is

Every UI feature lands in the same three places: the Slint root component, one of the wire() functions, and run().

Function	Lines
lib.rs::run	1,813
library_ui::wire	998
collections_ui::wire	964
identity_ui::wire	478
masks_ui::wire	357
settings_ui::wire	317

Above them sits one AppWindow carrying 282 properties and 190 callbacks, with exactly one Slint global in the whole ui/ directory — and it is not exported. Cross-view state therefore has nowhere to live except the root component, and every interaction is a callback registered inside a wire.

The core crates are healthy by the same measure: their largest functions are compose_full at 437 lines and build.rs::emit_node at 558, both of which earn their length. This is specific to dr-ui.

Why it matters more than it did

view-composition.md already diagnosed this and specified the fix, in three independently landable stages, on 2026-08-09. None of the three has landed. In the eighteen days since, the numbers that document itself used have moved:

Its measure	2026-08-09	2026-08-27
run()	500 lines	1,813
library_ui distinct window properties	24	54
lib.rs distinct window properties	23	52
collections_ui	9	11
launch_ui	16	16
set_show_* call sites	7	9
View-state booleans on AppWindow	2	5

Four modules it did not list now write window properties too: settings_ui (39), import_ui (24), identity_ui (18), masks_ui (16).

The prediction it made has also come true literally. It described app.slint compensating for two mutually exclusive booleans with if !root.show-launch && root.show-library chains. There are now five such booleans — show-launch, show-library, show-identity, show-settings, show-import — and the chains at app.slint:1405, :1445 and :1654 are five-term conjunctions. Each new view multiplies the conjunctions rather than adding to them.

None of this is difficult work. It is simply work that every feature must now do, in files every other feature is also editing — which is why two contributors working in parallel conflict by construction, and why a newcomer must read a 1,813-line startup sequence with real ordering constraints before safely inserting a line into it.

What to do

Execute view-composition.md as written. Its analysis holds and its staging is right; stage 1 alone removes the nullable-callback knot and the duplicated post-load sequence, and is worth landing whether or not stages 2 and 3 follow.

One thing to add to it, because it was not in scope there: the wire() functions. They are already sectioned internally by comment, so lifting each section into fn wire_ratings(window, ctl), fn wire_keywords(...) and so on is mechanical, checked entirely by the compiler, and can go one section per commit. It gives a feature a function to own rather than a region of one, which is what removes the conflict, and it does not wait on stage 1.

Do the extraction behind new features rather than as a big-bang refactor. The pile grows either way; the question is only whether each new feature adds to it or subtracts.

Done when: no function in dr-ui exceeds 300 lines, a new view registers itself instead of adding a boolean to AppWindow, and active-view has replaced the boolean set so both-true is unrepresentable.

---

CH-2 — RemoteBackend is an abstraction nothing above dr-sync uses

Where: core/dr-sync/src/lib.rs:46; seven files in ui/dr-ui/src

What it is

The trait is carefully built. Capability negotiation decides the sync strategy; range reads are documented as a hint rather than a guarantee so correctness holds either way; chunked upload is deliberately kept internal so one server's protocol cannot leak into the interface. Every choice is explained where it is made.

And then trash.rs, import.rs, export.rs, derived_sync.rs, launch_ui.rs, library.rs and settings_ui.rs each construct NextcloudBackend directly — 34 references — and ten functions take &NextcloudBackend rather than &dyn RemoteBackend. Exactly two sites in the tree take the trait object, both inside dr-sync itself.

Why it matters

The abstraction currently buys nothing it was designed for. Worse, it reads as though it does: a contributor who wants a WebDAV or local-folder backend will find a well-documented trait, implement it correctly, and only then discover that nothing above dr-sync can be handed the result.

There is no defence of this in the tree, which is what makes it an entry here rather than in technical-debt.md. It is what a single-backend application looks like when the second backend has not yet been attempted.

What to do

Change the ten signatures to &dyn RemoteBackend and construct the backend once, behind something the UI does not name — the same discipline develop.rs already applies to operations. The trait is already correct, so this is a mechanical change, and it is much cheaper now than during a second backend when it would be entangled with that backend's own problems.

Worth doing even if no second backend is ever written: it makes the sync layer testable against a fake, which today it is not.

Done when: NextcloudBackend is named in at most one file in ui/, and a stub backend can be substituted in a test without touching the UI.

---

CH-3 — There is no path in for a contributor who is not already here

Where: repository root

What it is

No CONTRIBUTING.md. No rust-toolchain.toml, though CI pins 1.92.0 exactly. No issue or PR templates.

The documentation that exists is excellent — 7,990 lines across 14 files, including 177 numbered requirements — and all of it is written for someone who has already decided to work on this. Nothing tells a newcomer which document to read first, that core/dr-pipeline/ops/README.md is the door with the lowest bar, or that a clone without git-lfs needs one command before the build succeeds.

That last point is handled well in the code: dr-segment's build script detects an LFS pointer file and fails with an instruction rather than embedding 130 bytes and dying at inference time. It is simply not written anywhere a first-time cloner would look.

Why it matters

It is the cheapest item in this document and it gates every other contribution. A person who cannot get a first build is not going to reach the parts that are good.

What to do

Write CONTRIBUTING.md and point the first door at the operation format. "Add a develop operation" is a genuinely one-file contribution with declared tests that run under cargo test — the best first experience this codebase can offer, and it happens to teach the architecture's central idea on the way through.

Then state the three things that are currently folklore: git lfs is a prerequisite, the first build resolves 826 crates and takes a while (saying so stops it reading as a hang), and the toolchain is 1.92.0. Add rust-toolchain.toml so that last one is enforced rather than documented — a contributor on an older stable currently gets confusing type errors instead of a version message.

Done when: someone who has never seen the repository can clone it, build it, and land a new ops/*.yaml node without asking a question.

---

CH-4 — Coverage is counted by tagging, not by behaviour

Where: docs/traceability.md, tools/traceability

What it is

Not a new finding — display-and-extension.md §7 states it plainly, and traceability.md itself says coverage is the intersection of tagged and defined IDs. The tooling is genuinely good: 732 tags, zero orphans, denominators parsed from requirements.md at run time rather than hardcoded, regenerated in CI and guarded by a pre-commit hook.

What it cannot do is check that the code under a tag does the thing. FR-DEV-8 is currently tagged against instance-buffer plumbing a future spot-removal operation would use; FR-DEV-7 against a history row for a frontend that does not exist. Both read as covered.

Why it matters

The 55.4% figure is an overstatement of unknown size, and the risk is that it is used as a planning input. It is recorded here so that the number keeps its asterisk when read outside the document that already qualified it.

What to do

Nothing structural — the honest framing already exists in two places. Adopt display-and-extension.md's rule going forward: close a requirement with a test that would fail if the behaviour were removed, and let the percentage move slowly and mean something.

Done when: the rule is stated in CONTRIBUTING.md alongside the tag syntax, so it reaches someone adding their first tag.

---

CH-5 — The best invariant in the codebase is unprotected

Where: ui/dr-ui/src, ui/dr-ui/ui

What it is

"No code in ui/ names an operation" (FR-DEV-3a) is what makes the whole declarative pipeline pay off, and it is currently maintained by discipline alone. Nothing fails if someone special-cases exposure in the panel to fix a layout problem at five in the evening.

Why it matters

It is one grep, it would take an hour, and it protects the property this audit rates highest. The failure mode is silent and cumulative: the first special case is defensible, and by the fifth the panel names half the chain and "a new operation is one file" has quietly stopped being true.

What to do

A test that greps ui/dr-ui/src and ui/dr-ui/ui for every id in ops/*.yaml and fails on a hit, with the current labels.rs localisation test as its one allowed exception. It belongs in CI beside the traceability check, which is the existing precedent for a structural gate.

Done when: adding window.set_exposure_slider(...) to the panel fails CI with a message naming FR-DEV-3a.

---

4. Order of work

Ordered by value per hour rather than by size.

	Item	Effort	Why first
1	CH-3 — CONTRIBUTING.md + rust-toolchain.toml	Half a day	Gates everything else; unblocks the trivial seam that already works
2	CH-5 — CI gate on operation names in ui/	An hour	Protects the property everything else in the pipeline rests on
3	CH-2 — make RemoteBackend load-bearing	1–2 days	Mechanical now, entangled later; also makes sync testable
4	CH-1 — split the wire functions	Incremental	No behaviour change, compiler-checked, one section per commit
5	CH-1 — view-composition.md stages 1–3	Sustained	Largest and most invasive; every deferred month adds to the pile

Items 1, 2 and 3 are independent of each other and of the rest. Item 4 does not wait on item 5.

---

5. What this document does not claim

It measures structure, discipline and coupling. It does not assess runtime correctness, GPU shader behaviour, security posture, or whether any tagged requirement is actually implemented — CH-4 is precisely the observation that the last of those is unmeasured.

Line counts and function sizes are proxies. run() being 1,813 lines is a real problem because every feature must edit it, not because 1,813 is a bad number; build.rs::emit_node at 558 lines is not a problem at all. Where a figure appears above, the sentence around it says which of the two it is.

The audit was performed against the working tree at branch android-bundled-face-models on 2026-08-27, which had five modified files in ui/dr-ui/ uncommitted at the time. Those changes were included in the counts.