# Codebase review

A read of every crate on 2026-10-17, after M5 to M7 and the M6 follow-ups. Findings are grouped by theme and ordered by payback within each group. The proposed order of work is at the end.

## 1. The monolith crate has no internal boundaries

- **`State` (`state.rs:55`) has 70 public fields**, and all 22 modules are `impl State`. Nothing stops any function touching anything, and every new protocol adds a field and a `new()` line. The fields fall into a few groups:
  - Inert protocol globals that only their own handler touches. These are the 20 or so `*_state` fields and `_dmabuf_global`.
  - The backend: `host`, `drm`, `output_id`, `render_node`, `syncobj`.
  - The window model, spread over seven maps (see below).
  - Subsystems with their own state: `lock`, `captures`, `imports`, `xwayland`, `launches`, `ipc`, plus `inhibitors` and `inhibited`.
  - Input: `suppressed`, `drag`, `pointer_pos`, `move_inverted`, `layer_focus`, `cursor_status`.
  - Redraw flags: `needs_redraw`, `cursor_moved`.
- **The backend is two `Option`s plus side fields.** `host: Option<Host>` and `drm: Option<Drm>` both exist. `output_id` only means something nested and `render_node` only on bare metal. `after_dispatch` calls both `nested::render` and `drm::render` every time, and each returns early when its option is `None`. A `Backend` enum, or a small trait for render, present and outputs, would make the split explicit.
- **One window's state lives in seven places:**
  - `windows`
  - `last_kind`
  - `awaiting`
  - `placed_by_rule`
  - `scene::Borders`
  - `toplevels::ListHandles`
  - the animator's tracks, glides and spawns

  `remove_window` (`state.rs:404`) cleans only some of them. `awaiting` and `last_kind` keep stale ids, and `ListHandles` is cleaned lazily in `publish_toplevels`. A per-window record (`Managed { window, kind, awaiting, by_rule, list_handle }`) would make removal one operation.
- **`apply_layout` (`state.rs:469`, about 110 lines)** does six jobs: it collects the shown windows, detects kind switches for glides, retargets animations, configures clients, maps the space, then syncs focus and idle inhibit. It runs after every command, including `Spawn`, `Quit`, `Reload` and `Close`, through `commands.rs:166`.
- **The commit handler (`handlers.rs:63`)** runs eight steps in sequence:
  - the buffer handler
  - early import
  - the window's own commit
  - capture damage
  - first-commit mapping
  - first-buffer detection
  - popups
  - layers
  
  It looks up the window by root surface twice, scanning `windows` and `unmapped` linearly each time. Routing on the surface's role first (toplevel, popup, layer, lock, cursor, X11) would make each path short.

## 2. Drawing an output exists three times

`drm::render_surface` (`drm.rs:585`, 140 lines), `nested::render` (`nested/mod.rs:169`, 135 lines) and `capture::output_elements` (`capture.rs:265`) each assemble the cursor plus lock-or-scene. Each destructures `State` field by field to get past the borrow checker. On top of that, `render_surface` handles VRR switching, budget accounting, slow-frame logging, lock confirmation, capture damage and frame callbacks. Frame callbacks are written twice, once in each backend, with the lock and layer variants on top.

- One `frame_elements(world, output) -> Vec<Draw>` used by all three.
- One `send_frame_callbacks(world, output, elapsed)`.
- The backends keep pacing and presentation only.
- `scene::build` takes ten arguments (`scene.rs:45`, `#[expect(clippy::too_many_arguments)]`). That disappears once it takes a borrowed view struct.

**Correctness smell.** `anim.prune(now)` runs inside each output's render with that output's own `now` (`drm.rs:658`). With two outputs at different rates, the later one can prune state the earlier one is still drawing. It's also why `animating` only re-arms the output that noticed it. Pruning belongs once per dispatch, after every output has drawn.

## 3. Snapshots for clients are rebuilt three ways, every dispatch

`ipc::broadcast`, `publish_workspaces` and `publish_toplevels` each derive a view of tags, windows and outputs from `Layout` plus `windows`. They run after every batch of events, and they lock every toplevel's role data for titles even when nothing changed.

- A single derived model, computed only when a dirty flag is set by commits, layout and focus changes, would feed all three.
- `slotmap::Key::data(&id).as_ffi()` appears in both `ipc.rs:177` and `toplevels.rs:26`.
- Tag names are looked up by id with a linear `tags().find` twice in `ipc.rs`. `Layout` lacks a `tag(id) -> &Tag` accessor.

## 4. Duplication

- `CLEAR` is defined identically in `drm.rs:40`, `nested/mod.rs:32` and `capture.rs`.
- Closing a window (xdg `send_close` or X11 `close`) is written in both `commands.rs:63` and `toplevels.rs:57`.
- Window lookup is a linear scan in four places: `State::window_id`, `x11_window_id`, the key search in `toplevels.rs:42` and `ListHandles::window`.
- Point-in-rect is written out inline in `drm.rs:787`, `drm.rs:645`, `capture.rs:270` and `state.rs` `surface_under`. The planner's `Rect` has no `contains`.
- Rect helpers are scattered: `inset`, `centred` and `translate` in `layout/lib.rs:979`, `pixels` in `scene.rs:279`, `span` and `with_span` in `tree.rs`.
- `Dir`, `Axis` and `ExpandScope` are defined in both `monolith-ipc` and `monolith-layout`, with hand-written mapping functions in `commands.rs:42`. `Modifier` and `Mods` from config are matched against smithay's `ModifiersState` by hand in two places (`commands.rs:29`, `input.rs:182`).
- `protocols/src/workspace.rs` and `toplevel.rs` repeat the same skeleton: a manager list, a snapshot, a per-manager sync, bind-then-sync, and stop/destroyed pruning. If a third such protocol arrives, factor out the skeleton. With two, they're tolerable.
- **Config defaults live in two places:** the `Default` impls in `config/src/lib.rs` and `default.toml`, 111 lines. The only test (`config/lib.rs:415`) checks that the file parses, not that it agrees with `Default`. `Raw` (`lib.rs:264`) mirrors `Config` field by field as well.

## 5. Module placement

- `state.rs` free functions belong elsewhere. `root`, `identity` and `configure` are window helpers. `gaps` and `xkb_config` are config adapters. `now_ns` is a clock.
- `handlers.rs` mixes core compositor, xdg-shell, seat, selection, DnD, syncobj, decorations and workspace actions. The newer protocols each got their own module (`layers`, `lock`, `idle`, `capture`, `toplevels`). The older ones should follow so the file stops being a junk drawer.
- `render-vk/src/renderer.rs` (1113 lines) holds:
  - the smithay trait impls
  - submission and the NVIDIA stall logic
  - the 160-line `submit` that records all ops
  - upload and read-back
  - the shm cache
  - `DmabufImporter`
  - the image barrier helpers (`barrier`, `acquire`, `release` at `renderer.rs:690`)
  
  The barriers belong next to `sync.rs`. Stall handling and the shm cache would be self-contained modules.
- `layout/src/lib.rs` (1008 lines) covers tags, outputs and parking, windows, focus history, scratchpads and arrangement in one `impl`. Scratchpads (`765–853`) and output parking (`225–339`) are the clean cuts.
- `anim.rs` keeps `VulkanTexture` inside `Ghost`, which ties the animation model to the GPU. The ghost's texture could live in the scene, keyed by window, with the animator holding only geometry and timing.
- The DRM frame pacing (`Budget`, `Surface::next_deadline`, `drm.rs:86–163`) and nested's `Host::predict` are pure timing logic stuck inside backend files.

## 6. Code quality

- **render-vk has 84 `unsafe` sites and no `// SAFETY:` comments.** The monolith crate does comment its few. `submit` keeps one `unsafe` block across about 120 lines of recording. Narrower blocks with an invariant stated per helper would make the Vulkan code reviewable.
- **Silent `let _ =`** sits on X11 `configure` and `set_*` (`state.rs`, `xwayland.rs`), `notifier.successful`, `popup.send_configure`, `display.flush_clients` and protocol sends. The X11 ones can hide real failures, so log those at debug.
- `commands::spawn` starts one OS thread per child just to `wait()` on it. A SIGCHLD source on the event loop reaps them all without threads.
- `drm::on_input` maps absolute pointer motion onto the first output only (`drm.rs:751`). That's wrong for tablets and multi-monitor.
- `Budget::ns` allocates and sorts 32 samples on every call, which is several times per frame. It's cheap, but a fixed array or a running quantile would do.
- A SIGKILLed monolith leaves `/tmp/.X*-lock` and its socket behind (`xwayland.rs:31` cleans up only on `Drop`). `claim` recovers stale locks by pid, but a recycled pid wastes a display number.
- Comment density is uneven across modules. Do one pass against the commenting rules.

## 7. Tests: what earns its keep and what's missing

What exists:
- **planner:** 5 named strategic cases plus 3 proptests (random transitions, rewinding, interrupts). High value, low count. The custom `Rng` inside proptest is unusual but fine.
- **layout:** a 2000-case proptest of random commands with `check_geometry`, plus two named tree cases. One invariant added this week found a real bug at once.
- **motion:** five small curve and velocity tests.
- **render-vk:** golden image tests, dmabuf round-trips and a transform test.
- **config:** one parse test.
- **monolith:** none.

Gaps, by value:
1. **Layout invariants are cheap and catch real bugs.** Add these to `check_geometry`:
   - the focused window is visible
   - every visible window's tag is viewed on its output, or the window is sticky or a shown scratchpad
   - no window appears in two trees
   - parked outputs restore their windows
2. **Frame pacing.** `Budget` and `next_deadline` are pure, and they have been the source of most of the visible bugs: speed changes, periodic misses, slack decay. A small test that drives a timing sequence through them (a spike, then recovery, then miss and hold) would pin the behaviour that took several bare-metal rounds to find.
3. **Protocol snapshot diffing** in `workspace.rs` and `toplevel.rs` is fiddly and has a history of hidden-state bugs. Pull the diff into a pure function from (sent, snapshot) to events and test it with the cases that matter: removal, output enter and leave, and a property change with no structural change.
4. **Config:** assert that `default.toml` parses to the same values as `Default`, which closes the drift in section 4.
5. **Nested smoke harness.** Every protocol this week was verified with a throwaway shell script that starts nested monolith with clients and greps logs. One `.nu` harness (or an `xtask`) that runs scenarios (layer-shell plus a client, lock, capture, toplevel list, Xwayland) would make those checks repeatable. It could run in `flake check` against lavapipe later.

## Proposed order

Done as of 2026-10-17: 1 (State split, see state-split.md), 2 (frame path in frame.rs, pruning once per dispatch), 3 (snapshots gated on Dirty::snapshot, identity cached per window). CLEAR and close-window from item 4 went in along the way. 4 (dedup) and 5 (file moves) are done too. From 5, `now_ns`, `gaps` and `xkb_config` stay in state.rs. The ghost texture moved to `Wm::last_frames`.


1. **Split `State`** into protocol globals, the backend enum, the window model with per-window records, subsystems and input. This unlocks most of the rest.
2. **One output frame path:** element assembly and frame callbacks shared by both backends and capture, with pruning once per dispatch.
3. **One derived snapshot with a dirty flag**, feeding ipc, ext-workspace and wlr-toplevel.
4. **Mechanical dedup:** `CLEAR`, close, window index, `Rect::contains`, the shared enums between ipc and layout, the modifier mapping, and single-source config defaults.
5. **File moves:** `handlers.rs` per protocol, the render-vk barriers, stall and shm cache modules, layout scratchpads and parking, frame pacing out of `drm.rs`.
6. **Tests:** layout invariants, pacing, snapshot diffs, config defaults, then the smoke harness.
7. **Hygiene sweep:** SAFETY comments, `let _ =` triage, the child reaper, absolute pointer mapping, and comments.