monolith

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 Options 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

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.