refactor(services): split tray and mpris untrusted-input parsers into tested parse.rs submodules

[vibechoom]

Why

tray.rs (976 LOC) and mpris.rs (879 LOC) are the two largest single-file services and have zero test modules — yet they contain exactly the pure, hermetically-testable logic that most deserves tests: parse_layout_node/parse_menu_entry recursively parse untrusted DBusMenu structures from arbitrary tray apps (separator roots, invisible nodes, submenu recursion, toggle-type strings), and read_metadata/parse_artist_array/parse_length/parse_track_id parse arbitrary media-player metadata including documented spec violations (i64 lengths, string trackids). bluetooth/ and wifi/ already model the answer: a parse.rs + types.rs split with the parser under unit test. Also de-risks PR #198 (tray diff-by-key), which leans on parser output stability.

Sketch

• Promote tray.rs → tray/{mod.rs,parse.rs,types.rs} and mpris.rs → mpris/{mod.rs,parse.rs}.
• Move parse_layout_node, parse_menu_entry, bool_prop/str_prop/i32_prop, strip_accel into tray/parse.rs; parse_artist_array, parse_length, parse_track_id and the map-extraction half of read_metadata into mpris/parse.rs.
• Both parsers take OwnedValue, so tests construct zvariant Structure/Array fixtures in memory — pure hermetic bucket, runs in the default cargo test and the nix doCheck.
• Adversarial cases to pin: <3-field structures, invisible root, separator-as-root, deep submenu nesting, negative mpris:length, trackid as Variant-wrapped ObjectPath.
• Public API (menu(), Player) unchanged.

Effort: medium · Value: high

---

_{💡 From a Fable codebase idea-sweep — grounded against main @ 610a499. The sketch is a starting point, not a spec.}

[vibechoom]

Triage

Severity: medium

What it asks

Promote the two biggest single-file services — tray.rs (976 LOC) and mpris.rs (879 LOC) — into tray/{mod,parse,types}.rs and mpris/{mod,parse}.rs, moving the untrusted-input parsers into a parse.rs submodule and pinning them with hermetic unit tests, mirroring the existing bluetooth/ and wifi/ layout.

Where it stands in the code

Every function the sketch names exists and is exactly the shape it claims — no test coverage in either file (grep for #[test]/#[cfg(test)] → 0 in both):

• tray (all pure, zvariant-typed): parse_layout_node crates/hytte-services/src/tray.rs:305, parse_menu_entry crates/hytte-services/src/tray.rs:359 (the recursion, the invisible-root and separator-root short-circuits, the children-display == "submenu" recursion all live here), the prop helpers str_prop/bool_prop/i32_prop crates/hytte-services/src/tray.rs:925 (all three at :925/:932/:939), and strip_accel crates/hytte-services/src/tray.rs:952. Types that would move to types.rs: Menu crates/hytte-services/src/tray.rs:93, MenuEntry crates/hytte-services/src/tray.rs:100, ToggleType crates/hytte-services/src/tray.rs:107, MenuItem crates/hytte-services/src/tray.rs:115.
• mpris: the parsers that would move to parse.rs are the three pure field parsers — parse_artist_array crates/hytte-services/src/mpris.rs:823, parse_length crates/hytte-services/src/mpris.rs:846, parse_track_id crates/hytte-services/src/mpris.rs:864 — each taking Option<&OwnedValue> with no side effects. Note read_metadata crates/hytte-services/src/mpris.rs:765 is not in that pure set: it's an async fn that does the get_property(bus_name, …, "Metadata") bus call and then delegates to the three parsers, so it stays as the (bus-touching) orchestrator in mod.rs — only its pure delegates get extracted and tested.

The template the sketch points at is real and in-tree: crates/hytte-services/src/bluetooth/mod.rs:31 declares mod parse; (and mod types; on the next line, :32), and both crates/hytte-services/src/bluetooth/parse.rs:62 and crates/hytte-services/src/wifi/parse.rs:206 carry #[cfg(test)] mod tests. The system-tests feature gate at crates/hytte-services/Cargo.toml:17 is opt-in, so these pure parser tests fall in the default cargo test bucket (and the nix doCheck) — the sketch's hermeticity claim holds.

Feasibility / risk

Sound and low-risk. The pure parsers already take OwnedValue / Option<&OwnedValue>, so fixtures are in-memory zvariant Structure/Array — no bus, no daemon. Two corrections to the sketch's "public API unchanged" line, both trivial but load-bearing:

• The public accessor is tray::items() crates/hytte-services/src/tray.rs:208 (returning Vec<TrayItem> with menus embedded) plus tray::menu_event() crates/hytte-services/src/tray.rs:434 — not the menu() the sketch names. On the mpris side it's players() crates/hytte-services/src/mpris.rs:162 / Player crates/hytte-services/src/mpris.rs:69. Both modules are re-exported via pub mod at crates/hytte-services/src/lib.rs:34 (tray) and crates/hytte-services/src/lib.rs:18 (mpris).
• strip_accel is pub fn and thus part of pub mod tray's surface — moving it into parse.rs needs a re-export from tray/mod.rs (or pub(super) on the parser fns) to keep the surface identical.

Value beyond regression-locking: the tests have real bug-finding potential, not just freezing current behavior. parse_track_id's doc comment promises it handles "a Variant wrapping one of those" crates/hytte-services/src/mpris.rs:861, but the body only tries OwnedObjectPath::try_from crates/hytte-services/src/mpris.rs:871 then String::try_from — it does not peel a nested Value::Value. The sketch's own adversarial case ("trackid as Variant-wrapped ObjectPath") is precisely the one that would confirm or refute that gap. Worth writing that case first.

Dependencies

• tray icon menus weird #8 / PR fix(tray): diff-by-key tray update, stop rebuild-on-every-emit (#8) #198 — real and live. tray icon menus weird #8 ("tray icon menus weird") is open; PR fix(tray): diff-by-key tray update, stop rebuild-on-every-emit (#8) #198 ("diff-by-key tray update", open, not draft) keys tray diffing on parser output. Landing this refactor's tests first gives fix(tray): diff-by-key tray update, stop rebuild-on-every-emit (#8) #198 a stability contract to lean on; the sketch's "de-risks fix(tray): diff-by-key tray update, stop rebuild-on-every-emit (#8) #198" is accurate. Sequence refactor(services): split tray and mpris untrusted-input parsers into tested parse.rs submodules #233 before or alongside fix(tray): diff-by-key tray update, stop rebuild-on-every-emit (#8) #198.
• chore(ci): run clippy and the system-tests bucket (ephemeral dbus + xvfb) in CI #232 (run system-tests + clippy in CI) — light: these tests are hermetic (default bucket) and will run under nix build regardless, but chore(ci): run clippy and the system-tests bucket (ephemeral dbus + xvfb) in CI #232 is what guarantees they execute on every push.

Recommendation

Build-now. @annikahannig — small, safe, high-leverage: it puts the first unit tests on the two parsers that ingest arbitrary third-party tray/media-player payloads, follows a template already twice-proven in the tree, and de-risks the in-flight #198. No design fork here; the only judgment call is sequencing it ahead of #198 so that PR can rely on a tested parser. Suggest starting with the parse_track_id Variant case to settle the doc-vs-code question above.

[vibechoom]

Building the tray half (triage: build-now). tray.rs → tray/{mod,parse,types}.rs + hermetic adversarial parser tests (separator-root, invisible node, deep submenus, garbage props → default), public API unchanged. Deferring the mpris split to a follow-up — it conflicts with my open #250 which edits mpris.rs; will do it once #250 lands. Ready-PR incoming. 🚂