# Maintainability review — 2026-08-01

> **Status (2026-08-01): all nine themes implemented and committed** — see
> commits 35a151b (1), 4a68dbf (3), b752aa9 (4), 82dc899 (2), f93be26 (5),
> 21b946e (6), fb6a823 (9), 48d5a24 (8), and the rot sweep (7) following.

Six specialist reviewers (app shell, tools, script/tutorial engine, canvas/theme,
widget/data, cross-cutting) swept the tree for code rot, complexity, duplication,
and API-shape problems; a skeptical synthesis pass re-verified doubtful claims
against the code, merged 36 raw findings into 8 themes, and dropped 5 as weak
(listed at the bottom, with reasons). Ranked by maintenance pain relieved per
unit effort. Breaking changes were in scope (project undeployed).

Sequencing note: themes 1 and 3 unlock theme 4 (decompose `App::ui` once, not
twice); theme 1's `resolve_at_pos` seeds theme 5's hit-test module. Theme 9
(added after review discussion) obsoletes the mod.rs:23-27 doc fix inside
theme 7's sweep — doing 9 makes that promise true instead of softening it.


## 1. One canonical hit-target resolver on Drawing; delete resize_block's hand-rolled dispatch  
*(effort M, breaking)*

**Findings (verified evidence):**

- Hit-test priority order is hand-re-derived at 5+ call sites instead of resolved once — src/tools/select_tool.rs lines 49-53, editor_at_pos (107-141), click_to_select (171-195), drag_to_move (204-260); src/tools/select_pin.rs:66-122, src/tools/resize_block.rs:365-368/489-516, src/tools/multi_pin_select.rs:50-53; src/widget/drawing.rs exposes 15 separate `*_at_pos` methods
- resize_block.rs reimplements select_tool's drag dispatch instead of calling it, and silently drops two branches — select_tool.rs:203-262 `drag_to_move()` called by select_pin.rs:99, multi_select.rs:134, multi_pin_select.rs:60, edit_route.rs:125; resize_block.rs:462-537 hand-rolls a subset: title_at_pos (489), route_at_pos (499), `selection_rect.contains(pos)` (508), shape_at_pos (516), marquee fallback (529). VERIFIED and worse than claimed: the hand-rolled chain omits type_at_pos, port_at_pos, pin_text_at_pos, AND route_label_at_pos
- ResizeBlock::Selected's widget() match arm is a ~205-line god-arm owning six unrelated jobs — src/tools/resize_block.rs:334-538; tests (701-792) only exercise the pure geometry helpers
- Four near-identical ~20-line pin label hit-test bodies; move_block/move_port collision loop copy-pasted — src/widget/drawing.rs:1550-1683 (pin_text_at_pos/pin_type_at_pos/pin_stub_at_pos/pin_tag_at_pos, verified: identical loop shape differing only in the rect getter); move_block (drawing.rs:2223-2246) and move_port (2249-2269)

**Recommendation:** Step 1 (bug-adjacent, do first): replace resize_block.rs:462-537's non-resize-corner branches with a call to `select_tool::drag_to_move(data, pos, painter)`, keeping only the resize-corner check — this is what select_pin/multi_select/multi_pin_select/edit_route already do and it deletes ~60 lines plus the god-arm's largest section. Step 2: give Drawing a single `fn resolve_at_pos(pos, painter) -> Option<HitTarget>` (enum over title/type/pin-name/pin-type/pin-tag/pin-stub/route-label/route/shape/port) encoding the canonical z-order once; rewrite drag_to_move, click_to_select, editor_at_pos, and the hover-cursor chain as matches over it. Step 3: while extracting, factor the pin quadruplet through one `pin_label_at_pos(rect_of: impl Fn(...) -> Option<Rect>)` and the move_block/move_port collision loop through one shared checker.

**Payoff:** The most-used tool stops misclassifying drags on type labels and pin names today (a live behavioral divergence, not a hypothetical); every future hit-testable element is added in exactly one ordered list instead of 5+ hand-copied chains, and hover feedback can no longer drift from click/drag behavior (it already has: the hover chain omits pin_tag/pin_stub).

## 2. Unify the three synthetic-input drivers (Session, Replay, advance_replay) into one sim-frame engine  
*(effort L, breaking)*

**Findings (verified evidence):**

- Three drivers hand-copy the same pointer/press/dragging tracking, and app.rs admits it 'mirrors' Session — src/script/session.rs:34-44 fields, track_pointer (L95-114), idle_frame (L82-92); src/tutorial/replay.rs:44-54 identical fields, track_pointer (L155-173, verified byte-for-byte identical match arms), idle_event (L132-137); third copy inlined in src/app.rs:476-550 with the confessing comment at L472-474
- The easing curve is copy-pasted between the two Step interpreters — src/script/step.rs:287-290 and src/script/lowering.rs:72-75, verified verbatim-identical
- Player and Replay each reimplement the sim-frame catch-up budget with a different algorithm and a duplicated constant — src/tutorial/player.rs:29 `MAX_FRAMES_PER_SHOW: u32 = 8` consumed by a while-loop at L203-249; src/tutorial/replay.rs:26 same constant, divide-and-cap at L85-94, doc comments already drifted in wording
- advance_replay rebuilds the whole document→schema projection up to 8x per real frame plus once more for cue drawing — src/app.rs:483-484 `let projected = crate::schema::model::Document::from(&self.state.document)` inside the catch-up loop (verified), second full conversion at src/app.rs:1534 in the same frame
- CueTarget::Relative is special-cased behind wildcard matches — step.rs:104, step.rs:230-235, step.rs:505-517, lowering.rs:196-204; three of four sites use a wildcard `_` fallback

**Recommendation:** Extract one `SimDriver` (or free-function set) owning: the pending_enter/pointer/press/dragging tracker, the 'apply a SimFrame to document+tool+painter' step, and a `SimClock` with `begin_show(dt) -> u32` holding MAX_FRAMES_PER_SHOW once. Session::step, Replay, and App::advance_replay all call it against their own document. While rewriting advance_replay, hoist the schema projection out of the per-sim-frame loop (or give Lowering a document_ng-native resolver) and reuse it for the cue draw at app.rs:1534. Move `ease` next to `Progress` as the single definition, and route all four CueTarget-Relative sites through one exhaustive `CueTarget::resolve_as_drag_dest`-style method so the wildcard fallbacks disappear.

**Payoff:** Pointer/drag bookkeeping fixes and new SimFrame kinds land in one place instead of three (the app.rs comment already concedes the copies exist to drift); the visual cursor and the actual synthetic gesture can no longer desync on easing; replay/--author catch-up stops paying up to 9 O(document) string-cloning projections per real frame in the exact authoring workflow the project is actively building (per recent commits b4fc979/2917f24/99fd387).

## 3. Make App's mode a type: one Mode enum for Editing/Tutorial/Replay/Authoring  
*(effort M, breaking)*

**Findings (verified evidence):**

- App mixes four mutually-exclusive modes as independent optional fields — src/app.rs:59-152: `tutorial: Option<Tutorial>` (131), `tutorial_stash: Option<EditorState>` (134), `pending_replay: Option<Level>` (140), `replay: Option<Replay>` (143), `author_footer: bool` (136), `author_last_hover` (146), `author_drag` (149), `author_edit` (152); checked ad hoc at 388, 1266/1519, 1425/1503 (field declarations verified at app.rs:128-152)
- The --author recorder's session state is three raw Option fields on App, unlike Tutorial/Replay's proper encapsulation — src/app.rs:144-152; recorder logic spread across record_author_commands (src/app.rs:720-779), show_author_footer (605-642), pointer_target/world_script_object/world_script_target (647-715)
- load_tutorial_level and load_replay_level are near-identical, must be kept in lockstep — src/app.rs:399-435 and src/app.rs:440-468 (verified: identical parse/finalize_load/error-arm/state-rebuild/undoer-reset/after_document_swap/fit_to_rect_instant bodies; only the level source and `start_level` differ)
- pointer_target and world_script_object are two parallel "what's at this world position" resolvers — src/app.rs:647-685 and src/app.rs:690-704, both re-derive corner_spelling → anchor_at_pos → shape_at_pos (verified; note the port gap in world_script_object is documented as intentional — "None over … shapes scripts can't name (ports)" — so this is duplication, not a live bug)

**Recommendation:** Introduce `enum Mode { Editing, Tutorial(Tutorial, Box<EditorState>), Replay(Replay), Authoring(AuthorRecorder) }` where AuthorRecorder owns last_hover/drag/edit and a `record(capture, time)` method mirroring Replay's shape (the pattern Tutorial and Replay already prove out). Fold the two load-level functions into one `fn load_level(&mut self, level: &Level)` plus the tutorial-only `start_level` call at its call site. Have one function compute a typed `PointerObject { Corner, Pin, Block }` and derive both the footer label and the script spelling from it, keeping the intentional port filter as an explicit match arm in the script-spelling projection.

**Payoff:** Tutorial-and-replay-both-Some becomes unrepresentable (the project's own stated invariant style), App loses eight mode fields and ~175 lines of recorder plumbing from its main impl, and level-loading policy changes land once instead of twice — in the --author/--replay area that is under the heaviest active development right now.

## 4. Decompose App::ui into named phases and factor the action-dispatch boilerplate  
*(effort L)*

**Findings (verified evidence):**

- App::ui is a ~843-line function fusing seven unrelated phases — src/app.rs:1061 `fn ui(&mut self, ui: &mut egui::Ui, _frame: &mut eframe::Frame) {` through the closing brace at src/app.rs:1903 (span verified: 1061→~1903); phases at 1073, 1089-1154, 1159-1188, 1191-1206, 1211, 1219-1242, 1245-1570, 1579-1618, 1622-1868, 1875-1879, 1882-1895, 1900-1902
- The 234-line, 30-arm action-dispatch match is a god switch with per-arm boilerplate — src/app.rs:1630 `match pending_action {` (verified; 42 `Some(Action::` occurrences in app.rs); 29 occurrences of `Drawing::new(&mut self.state.document, &self.state.path)` verified by grep, e.g. lines 1632, 1637, 1655, 1663, 1673, 1684, 1691, 1699, 1708, 1724, 1748
- role_picker and pin_type_picker are two Option fields whose mutual exclusion is convention, not enforced — src/app.rs:96 and src/app.rs:101 (verified); manual reconstruction into OpenPicker at 1078-1084; neither Action::OpenRolePicker (1798-1804) nor Action::OpenPinTypePicker (1805-1811) clears the other; blanket clear only at 1464-1467

**Recommendation:** Extract each phase into a `&mut self` method (`show_popups(&mut self, ctx) -> Option<Action>`, `show_canvas(...)`, `handle_keyboard(...)`, `dispatch_action(...)`) so ui() reads as the ordered phase list, making the load-bearing ordering structural instead of comment-documented. Inside dispatch, add `fn mutate_and_reroute(&mut self, f: impl FnOnce(&mut Drawing))` to collapse the 15+ copies of the Drawing::new + update_routes + tool-reset triple, and split the match into a few themed dispatch functions. Replace role_picker/pin_type_picker with one `popup: Option<Popup>` where `enum Popup { Role(RoleTarget), PinType(Vec<LineAnchor>) }`, mapping 1:1 onto the existing OpenPicker so opening one popup structurally closes the other. Do this AFTER theme 3 (the Mode enum shrinks ui()'s mode-conditional branches first, so you decompose the smaller function once, not twice).

**Payoff:** New popups, shortcuts, and Action variants become local edits to one named phase instead of edits inside an 843-line borrow of self; the mutate/reroute/reselect triple can no longer be half-forgotten in a new action arm; the both-popups-open state becomes unrepresentable.

## 5. Split the Drawing god-facade along its concerns and make spatial invalidation structural  
*(effort L, breaking)*

**Findings (verified evidence):**

- `Drawing<'a>` is a god object bundling CRUD, clipboard, hit-testing, and routing — src/widget/drawing.rs:141-147 struct; single `impl<'a> Drawing<'a>` at line 172 to ~2374; file is 5338 lines total (verified), lines 2648-5338 tests; next largest file is app.rs at 1923 (verified)
- Drawing is a 108-method god-facade mixing hit-testing, mutation, routing, and clipboard — clipboard (lines 409-874), routing (1758-2192), 15 hit-test methods (1387-1750), mutation (1112-2657); ~82 `ShapeId::Rect(...)` match arms (verified: grep count 82) across ~10 parallel matches (parent_and_id, delete_shape, copy_selection, shape(), shape_mut(), shape_tag_hidden, flip_shape_pins, move_shape, move_shapes, moved_set_and_delta)
- Spatial index staleness is prevented by a frame-level heuristic, not by Drawing's own mutations — src/app.rs:1870-1878 `let may_have_mutated = action_fired || ctx.input(|i| i.pointer.any_down() || i.pointer.any_released())` (verified, comment included); the heuristic already needed the replay special case at src/app.rs:551-554

**Recommendation:** Split Drawing into cohesion-based facades it composes: a hit-testing module (which theme 1's resolve_at_pos naturally seeds), a Clipboard type, a routing-update surface, and the mutation API. Move ShapeId dispatch into methods on ShapeId/ShapeRef so a new shape variant is enumerated once, not at ~10 match sites. While restructuring the mutation surface, thread a dirty flag (or `&mut CachedIndex`) through it so every mutating method invalidates the spatial index itself, deleting both the app.rs:1870-1878 heuristic and the replay special case at 551-554.

**Payoff:** Review and merge-conflict pressure stops concentrating in one 2200-line impl; a new ShapeId variant becomes a compiler-guided change instead of a 10-site hunt; the staleness bug class the replay path already patched around once becomes impossible to reintroduce from any future mutation path.

## 6. Tighten the Renderer/Painter boundary: typed units in, forwarding boilerplate out  
*(effort L, breaking)*

**Findings (verified evidence):**

- WorldPx/Zoom typed units exist but the Painter/Renderer/Style drawing API still takes raw f32 — Painter::rect/circle/PaletteStroke.width take plain f32 (painter.rs:236-252, :454-468; palette.rs:82-85) while units.rs defines the newtypes; call sites unwrap with `.get()` (verified count: 46 across the cited files, not the claimed 59 — magnitude holds)
- Renderer is hand-forwarded by three separate implementors (plus Style again) — trait at src/canvas/mod.rs:38-164; `impl Renderer for Painter` (painter.rs:547-649, verified), `impl Renderer for SvgRenderer` (svg.rs:296-459, verified), `impl Renderer for OpacityWrapper` (opacity.rs:55-177, verified at opacity.rs:55), `Style<R>` forwards again (theme/style.rs:39-187)
- hit_test.rs is pinned to the concrete Painter for a reason its own code no longer needs — src/render/hit_test.rs:1-4 doc claims 'a real galley' but the file only calls `painter.text_size(...)` and `painter.theme()` (verified: lines 35, 54, 165 and signatures at 27, 46, 75, 87, 108, 164 all `Style<'_, Painter>`); forces tools/route_start.rs's test helper (233-253) to build a full egui Painter
- Painter::new's 7-argument constructor forces 5 call sites to hand-duplicate throwaway registry boilerplate — painter.rs:104-126; call sites verified at view.rs:357, app.rs:508, widget/render_bench.rs:137, script/session.rs:136, tools/route_start.rs:241

**Recommendation:** One coordinated pass over the drawing boundary: (1) change Renderer/Painter/PaletteStroke signatures to accept WorldPx, converting to screen f32 only inside scale_stroke/w2s — deletes the 46 `.get()` unwraps and restores the invariant at the boundary that matters; (2) fold Painter's inherent methods into its trait impl (nothing calls them un-generically) and macro-generate OpacityWrapper's fade-and-forward bodies; (3) relax hit_test.rs signatures to `&Style<'_, impl Renderer>` (matching widget/display.rs) and fix the stale module doc; (4) add `Painter::headless(egui_painter, palette)` for the non-view.rs call sites.

**Payoff:** Passing a screen-space or negative magnitude to a draw call becomes a type error again (the owner's explicit preference, currently defeated at every call site); adding a draw primitive drops from five hand-synced bodies to two; hit-test estimators become testable without spinning up an egui Context.

## 7. Rot sweep: dead scene_rect, stale comments, document_ng rename, pub narrowing, one logging channel, Tool/ToolName drift guard  
*(effort M, breaking)*

**Findings (verified evidence):**

- scene_rect is dead machinery: restored from storage but never saved or read back — src/app.rs:65-66, src/main.rs:111-116, src/app.rs:352 (verified: only `storage.set_string("preferences", ...)` at src/app.rs:1032 exists; nothing writes scene_rect back and nothing reads it after set)
- Stale comment justifies the &'static-str leak scheme by citing the debugger, which has since been removed — src/script/parse.rs:256-257 vs commit 8bc4c02 (verified in recent commit log: 'Author levels with --replay and an --author footer; cull the debugger')
- `document_ng` is the only document model but still carries its migration-era name — src/lib.rs:4 `pub(crate) mod document_ng;` (verified: no sibling `document` module; 70 files reference document_ng); src/schema/mod.rs:1-2 bakes the name into doc comments
- `pub mod font` and `pub mod script` in lib.rs have no external consumer — src/lib.rs:7 and :18 (verified: every other module is pub(crate); main.rs references only blockworx::app and blockworx::tutorial)
- Failure reporting is split between eprintln!/println! and tracing::error!/warn! with no rule — src/app.rs:311-320 eprintln! vs src/app.rs:417 and :455 tracing::error! for the same 'document failed to parse, fall back' event class (verified; 9 tracing error/warn sites crate-wide vs dozens of println/eprintln)
- Tool and ToolName are two 26-variant enums kept in sync by hand — src/tools/tool.rs:64-91 and src/tools/names.rs:2-29 (verified: 26 variants each), Tool::from_name (tool.rs:100-129), displayed_tool (names.rs:80-110)

**Recommendation:** One mechanical cleanup pass: delete App::scene_rect + its init (app.rs:352) + the restore block (main.rs:111-116); fix the parse.rs:256-257 comment to the current one-parse-per-process invariant; rename document_ng → document (nothing collides); narrow `pub mod font`/`pub mod script` to pub(crate); route all fallback/failure reporting through tracing, reserving println! for the --author recorder's intentional stdout script output (app.rs:727-771, 1414); fix mod.rs:23-27 to stop promising pixel-identical text layout across backends (svg.rs:17-20 already documents the divergence). For Tool/ToolName, skip the derive-macro machinery (contra the findings' primary recommendation — it's complexity the problem doesn't need) and add the cheap drift guard instead: a test asserting `Tool::from_name(n).name() == n` for every ToolName variant, which catches a copy-pasted wrong name() at test time.

**Payoff:** Startup wiring stops sending readers on a dead scene_rect chase; every future reader of 70 files stops wondering where non-ng `document` went; failures become uniformly filterable/assertable through tracing subscribers; Tool/ToolName drift fails a test instead of mis-highlighting the toolbar at runtime.

## 8. View::show: split concerns and replace the four-bool editor-feedback stash with one struct  
*(effort M)*

**Findings (verified evidence):**

- View::show is a 250-line god function that also threads a 4-field bool stash across frames — src/canvas/view.rs:223-473; four bools at view.rs:41-44 (`text_edit_lost_focus`, `text_edit_enter_pressed`, `text_edit_tab_pressed`, `text_edit_escape_pressed`), set at 459-468, consumed/cleared at 347-354 of the next call (file verified at 500 lines, consistent with the span)

**Recommendation:** Split show into `handle_pan_zoom`, `draw_grid`, and `render_pending_edit` helpers; replace the four bools + last_edit_id with one `EditorFeedback` struct produced by render_pending_edit and merged into Interaction in one place, so a new editor signal is one field in one struct instead of a field + a clear + an OR-in site kept in sync by hand.

**Payoff:** Grid, zoom/pan, and in-place-editor changes stop risking each other; the deferred-frame editor state machine gains a single point of extension (relevant soon: replay/session code synthesizes exactly these editor signals, so a fifth signal is plausible).


## 9. SVG export: lay text out with epaint, keep ttf-parser outlines  
*(effort S–M; corrected theme — supersedes the dropped "SvgRenderer reimplements
text layout" finding below, whose proposed fix was wrong but whose problem is
real and cheaply solvable)*

**Findings (verified evidence):**

- Glyph *shapes* are already exact and viewer-independent: `SvgRenderer` traces the same embedded font bytes to `<path>` outlines via ttf-parser (svg.rs:11-14). Only *layout* is reimplemented: advance-sum measurement (svg.rs:215-233), a greedy word-wrapper (`wrap_text`, svg.rs:245+), and line spacing from raw face metrics.
- The on-screen layouter shapes text through harfrust (in the dependency tree via epaint), so screen advances include kerning/shaping; the SVG's naive advance-sum does not. The drift is small for short Latin labels (the module doc admits "a pixel or two", svg.rs:17-20) but can flip a wrap decision when a line sits near `max_width` — a label that fits on screen wraps in the export.

**Recommendation:** Reuse epaint's layout *output*, not its algorithm: construct a standalone `epaint::text::Fonts` from the same `FontDefinitions` that `crate::font::build_fonts` already produces (no `egui::Context` required; use `pixels_per_point = 1.0` so egui's physical-pixel rounding doesn't quantize the resolution-independent SVG), call `fonts.layout_job(…)`, and have `SvgRenderer` emit its existing ttf-parser outline for each galley glyph at the galley-given position. `text_size`/`text_size_wrapped` become `galley.size()`; delete `measure`/`wrap_text` (~80 lines). Plumbing: `SvgRenderer::new` grows a `Fonts`/`FontDefinitions` parameter at its three call sites (widget/display.rs:32 plus two test files). Known bounded caveat: epaint's `Glyph` exposes chars, not shaped glyph ids, so a ligature renders as its constituent chars at shaped positions — irrelevant for this app's labels.

**Payoff:** The export becomes pixel-faithful to the screen — kerning and wrap decisions included — by deleting the parallel layout implementation rather than abstracting it; the wrap-flip divergence class disappears; `render_path_tests` can stop tolerating text divergence between backends. (Theme 7's doc fix for mod.rs:23-27 then becomes unnecessary — the promise becomes true instead.)


## Dropped findings (and why)

- **Action is a 29-variant enum that mixes fine-grained tool events with whole-app commands** — The census is accurate (verified: Action::Paste/Action::Nudge have zero constructors under src/tools/), but the supporting argument is partly wrong — the size_of test at tool.rs:271-276 measures Tool, not Action, so app-level Action variants do not grow 'the type every Tool variant is measured against'. Substantively, the single Action channel is the simpler design the owner prefers: one uniform command path for toolbar, keyboard, and tools. Splitting it is a breaking L-effort change whose only payoff is shortening the enum a tool author skims; the real review pain (the god match) is fixed by theme 4's themed dispatch helpers without a type split.
- **`elapsed_secs` round-trips through Duration with no effect, and its comment describes behavior the code doesn't have** — Half the claim is a misreading: '.max(0.1)' IS the 'floored to a demo-visible length' the comment describes (a 0.1s lower bound, not truncation), verified at src/app.rs:201-204 — the comment is accurate. The Duration wrap/unwrap is a genuine but one-line wart; not schedulable work, just fold into any passing edit of the function.
- **SvgRenderer reimplements text layout from scratch instead of sharing logic with the egui-backed Painter** — *Superseded by theme 9.* The synthesis correctly rejected the reviewer's proposed fix (a glyph-advance-parameterized wrap abstraction — speculative machinery), but the problem admits a simpler solution the review didn't evaluate: consume epaint's layout output directly. See theme 9.
- **toolbar.rs's selection_overlay is a 287-line function fusing UI layout, action dispatch, and placement math** — Verified long, but it is linear, cohesive immediate-mode UI in the standard egui idiom: button rows producing Option<Action>, plus placement. Interleaving layout and action production is how every egui overlay is written; the placement solver already lives in separate items in the same file. Splitting relieves little recurring pain relative to the higher-ranked themes — revisit only if the overlay keeps accreting actions.
- **step.rs mixes the data model, doc resolution, the interpreter, and the test-only builder in one 696-line file** — 696 lines with clearly delimited sections and colocated tests is within normal bounds, and the one concrete hazard cited — a CueTarget variant added 300 lines from the sampling match — is eliminated by theme 2's exhaustive single resolve method, not by relocating code between files. A file split here is churn without a behavioral guard.