monolith

Notes for the codebase review

Collected while building M5 to M7 and the follow-ups. Each item names where to look. Ordered roughly by how much it would pay back.

Structure

• State is a god object. It has 70 public fields in state.rs. They cover backend handles, every smithay protocol state, layout, animation, capture, imports, Xwayland, lock, idle, activation and launch tracking. Every module is impl State, so nothing has a boundary and any function can touch anything. The protocol states are mostly inert after construction. They could live in one Protocols struct, with the live subsystems (lock, captures, imports, xwayland, toplevel handles) owning their own data and taking narrow arguments.
• Three copies of “draw an output”. drm::render_surface, nested::render and capture::output_elements each assemble cursor plus lock-or-scene, and each destructures State field by field to satisfy the borrow checker. capture::output_elements is the general version. The backends should call one function and keep only presentation.
• scene::build takes ten arguments and carries #[expect(clippy::too_many_arguments)]. Most of them are one borrowed view of the world. A SceneInput struct, or the split above, removes it.
• drm::render_surface does too much. It handles VRR switching, timing and budget, scene assembly, lock confirmation, capture damage, frame callbacks and the slow-frame log. VRR and stats could be methods on Surface.
• handlers.rs is a grab bag. It holds compositor commit logic, xdg-shell, decorations, seat, selection, DnD, syncobj, hold_until_readable, workspace actions and fullscreen. The commit handler alone runs import, window commit, unmapped mapping, first-buffer, popups, layers and capture damage in sequence. A per-role dispatch would read better.
• layout/src/lib.rs is 1000 lines. Tags, outputs, parking, scratchpads, focus history and arrangement share one impl Layout. Scratchpads and output parking look separable.
• render-vk/src/renderer.rs is 1100 lines. It mixes the smithay trait impls, submission and stall handling, the shm cache, read-back and the new DmabufImporter.

Duplication

• CLEAR is defined identically in drm.rs, nested/mod.rs and capture.rs.
• Closing a window (xdg send_close or X11 close) is written out in both commands.rs and toplevels.rs. It belongs on a window helper next to root() and identity().
• Window lookup is a linear scan: window_id by surface, x11_window_id, the toplevel key lookup in toplevels.rs, and ListHandles::window. That’s fine at desktop window counts, but four spellings of the same search should be one index.
• Smoke-test shell scripts were rewritten for every protocol (/tmp/m4fix/smoke_*.sh). A small xtask or .nu harness that starts nested monolith with a client list and collects logs would make protocol checks repeatable, and could gate CI later.

Code quality

• Per-dispatch work that doesn’t need to run. after_dispatch runs publish_workspaces and publish_toplevels after every batch of events. They rebuild full snapshots and lock every toplevel’s data for titles even when nothing changed. A dirty flag set by commits, layout changes and focus changes would make them nearly free.
• Silent let _ = on X11 configure and set_* calls, on notifier.successful, and on several protocol sends. Most are fine, but the X11 ones hide real failures.
• configure() in state.rs handles both xdg and X11 windows with two inline branches. Once more window kinds exist (none planned), it wants a small trait.
• Xwayland lifecycle. A SIGKILLed monolith leaves its /tmp/.X*-lock and socket behind. claim handles stale locks by pid, but a recycled pid makes it skip that display for good. Low priority.
• Focus::X11(Box<X11Surface>) exists only to satisfy large_enum_variant. That’s fine, but X11Surface is 432 bytes, which is worth knowing.
• Comment density is uneven. Some modules carry a doc line on most items, others none. Check against the comment rules in one sweep.

Tests

• The layout proptest pulls its weight. It caught the hidden root-leaf bug the moment the “hidden tiles have no area” invariant was added. More invariants in check_geometry (focus always visible, tags always viewed somewhere) would be cheap and high-yield.
• Nothing tests the protocol servers. workspace.rs and toplevel.rs diff snapshots by hand, which is exactly the strange, racy logic the test policy wants covered. Candidates are a snapshot-diff test driving two update calls through a client, or a pure function that computes the diff and a test of that.
• render-vk has golden and dmabuf tests. Nothing pins the submit-stall absorption logic, since StallPhase::saw is pure and tiny. It’s a candidate only if it ever changes.
• monolith itself has no tests, so behaviour such as lock confirmation, capture waiting and import blockers is only smoke-tested. Most of it needs a display. The pure parts (the frame budget, floats(), workspace filtering) could be pulled out and tested if they ever break.

Smaller observations

• state.rs free functions root, identity, configure, gaps and xkb_config are window, layout and config helpers living in the state module.
• The layers.rs debug lines added to chase the quickshell report can be dropped or kept at trace.
• design/parked.md and this file overlap a little. Parked is for behaviour and this is for structure.