diff --git a/docs/code-health.md b/docs/code-health.md new file mode 100644 index 0000000..139e734 --- /dev/null +++ b/docs/code-health.md @@ -0,0 +1,353 @@ +# DarkRoom — Code health and the cost of a contribution + +**Status:** Audit · 2026-08-27 +**Companion to:** [architecture.md](architecture.md), [technical-debt.md](technical-debt.md), +[view-composition.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](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. + +```bash +# 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,810 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](#ch-2). | **Hard** | +| Anything with its own UI | `app.slint`'s root component, a ~1,000-line `wire()`, and `run()` at 1,810 lines — see [CH-1](#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](display-and-extension.md). 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](#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,810 | +| `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](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,810** | +| `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,810-line startup sequence with real ordering +constraints before safely inserting a line into it. + +### What to do + +Execute [view-composition.md](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](display-and-extension.md) 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](#ch-3) — `CONTRIBUTING.md` + `rust-toolchain.toml` | Half a day | Gates everything else; unblocks the trivial seam that already works | +| 2 | [CH-5](#ch-5) — CI gate on operation names in `ui/` | An hour | Protects the property everything else in the pipeline rests on | +| 3 | [CH-2](#ch-2) — make `RemoteBackend` load-bearing | 1–2 days | Mechanical now, entangled later; also makes sync testable | +| 4 | [CH-1](#ch-1) — split the `wire` functions | Incremental | No behaviour change, compiler-checked, one section per commit | +| 5 | [CH-1](#ch-1) — [view-composition.md](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](#ch-4) is precisely the observation that the last of those is unmeasured. + +Line counts and function sizes are proxies. `run()` being 1,810 lines is a real problem because +every feature must edit it, not because 1,810 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 measured against `origin/master` at `d4a34ef`. It was first taken against the +in-flight branch `android-bundled-face-models`, where `run()` is 1,813 lines rather than 1,810 and +`library.rs` carries 374 further lines; no other figure differs, and the seam grades are identical +on both. Where the two disagree the master figure is the one quoted.