dtourolle
84fade99ec
docs/ had 26 developer documents flat beside the manual, and the two audiences are very differently sized: most readers want the manual and the gesture reference, a few want the register, the designs and the measurements. The manual and gestures.md stay at the top; everything for someone changing the code moves to docs/dev/, and the two documents that name their own successors — the v0.1 milestone and the UI-refinement plan — go to docs/dev/archive/ rather than being deleted, since both are still cited. docs/README.md is the index, users first. Every reference follows: code comments, Cargo manifests, the workflows, the pre-commit hook, the bench and traceability tools (which locate the repo root by docs/dev/requirements.md now), packaging, the Docker READMEs, CLAUDE.md, CONTRIBUTING.md and the README. The matrix links one level deeper and is regenerated. Links out of the moved documents into the tree gain a level; a link checker over every Markdown file finds none broken.
367 lines
18 KiB
Markdown
367 lines
18 KiB
Markdown
# 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,042 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,042, plus 21 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 | 9 — three of them the face scan's SIMD kernels (faces.md §9) |
|
||
| `TRACES` tags / orphan tags | 793 / 0 |
|
||
| Resolved dependencies | 826 |
|
||
| Largest function | `dr-ui::run` — 1,855 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<br>*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<br>*dehaze, a sharpener* | Rust in `dr-pipeline/src/ops/` implementing `Operation` + `DetailStage`, plus a stub YAML declaring `rust:` and `order:`. | **Moderate** |
|
||
| A parameter widget<br>*a colour wheel* | A `WidgetKind` variant, `develop::supported()`, the panel model, a Slint component. | **Moderate** |
|
||
| A second sync backend<br>*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,855 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.
|
||
|
||
---
|
||
|
||
## <a id="ch-1"></a>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,855 |
|
||
| `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,855** |
|
||
| `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,855-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.
|
||
|
||
---
|
||
|
||
## <a id="ch-2"></a>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.
|
||
|
||
**Done**, with one half deliberately left. `ui/dr-ui/src/remote.rs` is now the only file in the
|
||
interface that names a connector; the ten worker functions take `&dyn RemoteBackend` and will accept
|
||
a stub. Every method the UI ever called on a backend — `get`, `put`, `list`, `delete`, `create_dir`,
|
||
`move_to` — was already on the trait, so nothing had to be added to it.
|
||
|
||
What remains is **credentials**. `AppCredentials` is an app password obtained through Login Flow v2,
|
||
which is a Nextcloud protocol rather than a general notion of how one authenticates to a remote, and
|
||
seven files still name it. Abstracting it needs a decision about what an account *is* across
|
||
backends — an OAuth token, a bucket key pair and an app password have no useful common shape — and
|
||
making that decision before a second backend exists would produce a confident wrong answer. It is a
|
||
design problem rather than a mechanical one, and it should wait for the backend that forces it.
|
||
|
||
---
|
||
|
||
## <a id="ch-3"></a>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.
|
||
|
||
---
|
||
|
||
## <a id="ch-4"></a>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.
|
||
|
||
---
|
||
|
||
## <a id="ch-5"></a>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,855 lines is a real problem because
|
||
every feature must edit it, not because 1,855 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 first measured against `origin/master` at `d4a34ef`, then re-measured after
|
||
`android-bundled-face-models` merged in: `run()` moved from 1,810 lines to 1,855, and the whole-
|
||
library face sweep added its fetching path to `library.rs`. The seam grades are unchanged by that
|
||
merge — the work went into the seams that already existed rather than cutting new ones, which is
|
||
itself the pressure CH-1 describes.
|