diff --git a/Cargo.lock b/Cargo.lock index e2c08ec2..63b9340d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3374,6 +3374,7 @@ dependencies = [ "punktfunk-core", "serde", "serde_json", + "test_reactor", "tracing", "tracing-subscriber", "windows 0.62.2 (git+https://github.com/microsoft/windows-rs?rev=acb5a1a7441033d9312b16842af02eb0c2b403dc)", @@ -4576,6 +4577,18 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "test_reactor" +version = "0.0.0" +source = "git+https://github.com/microsoft/windows-rs?rev=acb5a1a7441033d9312b16842af02eb0c2b403dc#acb5a1a7441033d9312b16842af02eb0c2b403dc" +dependencies = [ + "rustc-hash", + "windows-core 0.62.2 (git+https://github.com/microsoft/windows-rs?rev=acb5a1a7441033d9312b16842af02eb0c2b403dc)", + "windows-reactor", + "windows-reactor-setup", + "windows-reference", +] + [[package]] name = "thiserror" version = "1.0.69" @@ -5430,6 +5443,16 @@ dependencies = [ "windows-time", ] +[[package]] +name = "windows-canvas" +version = "0.0.0" +source = "git+https://github.com/microsoft/windows-rs?rev=acb5a1a7441033d9312b16842af02eb0c2b403dc#acb5a1a7441033d9312b16842af02eb0c2b403dc" +dependencies = [ + "windows-core 0.62.2 (git+https://github.com/microsoft/windows-rs?rev=acb5a1a7441033d9312b16842af02eb0c2b403dc)", + "windows-numerics 0.3.1 (git+https://github.com/microsoft/windows-rs?rev=acb5a1a7441033d9312b16842af02eb0c2b403dc)", + "windows-window", +] + [[package]] name = "windows-collections" version = "0.3.2" @@ -5582,6 +5605,7 @@ version = "0.0.0" source = "git+https://github.com/microsoft/windows-rs?rev=acb5a1a7441033d9312b16842af02eb0c2b403dc#acb5a1a7441033d9312b16842af02eb0c2b403dc" dependencies = [ "rustc-hash", + "windows-canvas", "windows-collections 0.3.2 (git+https://github.com/microsoft/windows-rs?rev=acb5a1a7441033d9312b16842af02eb0c2b403dc)", "windows-composition", "windows-core 0.62.2 (git+https://github.com/microsoft/windows-rs?rev=acb5a1a7441033d9312b16842af02eb0c2b403dc)", @@ -5770,6 +5794,14 @@ dependencies = [ "windows-link 0.2.1 (git+https://github.com/microsoft/windows-rs?rev=acb5a1a7441033d9312b16842af02eb0c2b403dc)", ] +[[package]] +name = "windows-window" +version = "0.0.0" +source = "git+https://github.com/microsoft/windows-rs?rev=acb5a1a7441033d9312b16842af02eb0c2b403dc#acb5a1a7441033d9312b16842af02eb0c2b403dc" +dependencies = [ + "windows-core 0.62.2 (git+https://github.com/microsoft/windows-rs?rev=acb5a1a7441033d9312b16842af02eb0c2b403dc)", +] + [[package]] name = "windows_aarch64_gnullvm" version = "0.42.2" diff --git a/clients/windows/Cargo.toml b/clients/windows/Cargo.toml index b867bb0f..fbafe1d3 100644 --- a/clients/windows/Cargo.toml +++ b/clients/windows/Cargo.toml @@ -94,6 +94,15 @@ serde_json = "1" tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter"] } +# The reactor's own headless test harness, at the SAME pinned rev: the `test` feature gates +# `RenderCx::for_test`/`ChannelDispatcher`; `test_reactor` is the upstream repo's +# RecordingBackend crate (cargo resolves a git dep's workspace member by package name). +# These power tests/reactor_semantics.rs — the characterization suite for the re-render +# semantics this client's architecture depends on. Re-run it on every future SHA bump. +[target.'cfg(windows)'.dev-dependencies] +windows-reactor = { git = "https://github.com/microsoft/windows-rs", rev = "acb5a1a7441033d9312b16842af02eb0c2b403dc", features = ["test"] } +test_reactor = { git = "https://github.com/microsoft/windows-rs", rev = "acb5a1a7441033d9312b16842af02eb0c2b403dc" } + # Embeds the app icon as an exe resource (build.rs) — Windows hosts only (rc.exe from the SDK). # windows-reactor-setup stages the Windows App SDK runtime bootstrap (framework-dependent) next # to the exe — the new-model replacement for the old windows-reactor build.rs; same pinned rev. diff --git a/clients/windows/src/app/mod.rs b/clients/windows/src/app/mod.rs index 417c33da..82bcc6f5 100644 --- a/clients/windows/src/app/mod.rs +++ b/clients/windows/src/app/mod.rs @@ -14,11 +14,22 @@ //! * [`style`] — the shared look (cards, pills, monograms), following the windows-reactor //! gallery: Mica backdrop, a centred max-width column, theme brushes (`ThemeRef`) //! -//! **Re-render discipline** (reactor's rules): each hook-using screen is mounted as its own -//! `component(...)` so its hooks live in an isolated slot list. A child's *sync* `use_state` -//! marks it dirty and re-renders it; an `AsyncSetState` written from a background thread does -//! NOT (the child is pruned when its props are unchanged) — so everything thread-driven -//! (discovery, HUD stats, speed-test results) is held as *root* state and passed down as props. +//! **Re-render discipline** — MEASURED rules, not folklore: each is pinned by a +//! characterization test in `tests/reactor_semantics.rs` against the exact windows-reactor +//! rev in Cargo.toml. Re-run that suite on every bump; a red test means this note is stale. +//! +//! * A child's *sync* `use_state` re-renders it — including under element-equal +//! non-component wrappers (`border`, `scroll_viewer`), and including writes from +//! backend-wired event handlers (`sync_state_child_under_element_equal_border_rerenders`, +//! `sync_state_from_backend_fired_event_rerenders`). Per-screen UI state driven by user +//! events may therefore live in the screen's own component. +//! * An `AsyncSetState` written from a background thread does NOT re-render its owning +//! component: the value lands and the dirty flag is set, but the rerender request is +//! keyed by the component's own HostId, which is never registered — only the root's is — +//! so it is silently dropped and surfaces on the next unrelated pass +//! (`async_state_child_under_element_equal_border_rerenders` asserts the drop). So +//! everything THREAD-driven (discovery, HUD stats, speed-test results, spawn events) +//! is held as *root* state and passed down as props. mod connect; mod help; @@ -272,6 +283,11 @@ fn root(cx: &mut RenderCx, ctx: &Arc) -> Element { // Whether the Edit-profile modal is up. Root state for the reactor-backend-handler reason // above; guarded in the page so it only renders while a profile is actually in scope. let (settings_edit, set_settings_edit) = cx.use_async_state(false); + // Window size, read at ROOT: the settings screen places its scope switcher by the same + // width threshold WinUI's NavigationView uses to collapse its pane (reactor exposes no + // pane-opened/closed event). Registering the size here re-renders the tree on resize, + // which the per-screen `use_inner_size` readers (hosts, library) already caused anyway. + let window = cx.use_inner_size(); // Bumped when a settings edit changes what the page should SHOW without changing any state // it already reads — ANY edit through `settings::commit` (creating an override must surface // its marker as immediately as resetting one clears it), a reset, a profile colour change. @@ -624,6 +640,7 @@ fn root(cx: &mut RenderCx, ctx: &Arc) -> Element { settings_rev, &set_settings_rev, nav_progress, + window.width, ), Screen::Licenses => licenses::licenses_page(&set_screen), Screen::Help => help::help_page(&set_screen), diff --git a/clients/windows/src/app/settings.rs b/clients/windows/src/app/settings.rs index f58576d0..244c6ff9 100644 --- a/clients/windows/src/app/settings.rs +++ b/clients/windows/src/app/settings.rs @@ -580,6 +580,7 @@ pub(crate) fn settings_page( rev: u64, set_rev: &AsyncSetState, progress: f64, + window_width: f64, ) -> Element { // The layer being edited. A scope pointing at a deleted profile degrades to the defaults, // the same rule a dangling host binding follows. @@ -1279,6 +1280,13 @@ pub(crate) fn settings_page( // the navigation rather than as one more settings row — a bar of profile controls above // the whole surface read as clutter. Editing the profile itself (name, colour, delete) // is a modal behind the "Edit profile…" button, not inline chrome. + // + // But ONLY while the pane is expanded: in Auto display mode WinUI collapses the pane + // below 1008 epx (compact rail, then minimal), and pane-footer content is CLIPPED to the + // rail, not adapted. Reactor exposes no pane-opened/closed event, so the switcher follows + // the same width threshold WinUI uses: expanded pane → footer; collapsed → a slim row at + // the top of the content column instead. + let pane_expanded = window_width >= 1008.0; let catalog = ProfilesFile::load(); let mut scope_names = vec!["Default settings".to_string()]; let mut scope_ids: Vec = vec![String::new()]; @@ -1320,25 +1328,53 @@ pub(crate) fn settings_page( set_scope.call(id.clone()); } }); - let mut footer_rows: Vec = vec![switcher.into()]; - if profile_mode { + let edit_button: Option = profile_mode.then(|| { let set_edit = set_edit.clone(); - footer_rows.push( - button("Edit profile\u{2026}") - .on_click(move || set_edit.call(true)) - .into(), - ); - } + button("Edit profile\u{2026}") + .on_click(move || set_edit.call(true)) + .into() + }); // Keyed by scope + the name list: a rename/create/delete changes the ComboBox's items, // and an in-place diff re-sets items (clearing WinUI's selection) while skipping // `selected_index` when it compares equal — the combo then renders blank. A remount // applies every prop. The keyed child sits inside a plain vstack because only a panel's // child list takes the keyed path (same rule as the content column below). - let footer = vstack(vec![vstack(footer_rows) - .spacing(8.0) - .with_key(format!("{scope}\u{1}{}", scope_names.join("\u{1}"))) - .into()]) - .margin(edges(16.0, 0.0, 16.0, 12.0)); + let switcher_key = format!("{scope}\u{1}{}", scope_names.join("\u{1}")); + // Expanded pane: combo over button, in the footer. Collapsed: one horizontal row that + // leads the content column (bottom-aligned so the combo's "Editing" header doesn't push + // the button off its baseline). + let (footer, inline_switcher): (Option, Option) = if pane_expanded { + let rows: Vec = std::iter::once(Element::from(switcher)) + .chain(edit_button) + .collect(); + ( + Some( + vstack(vec![vstack(rows) + .spacing(8.0) + .with_key(switcher_key) + .into()]) + .margin(edges(16.0, 0.0, 16.0, 12.0)) + .into(), + ), + None, + ) + } else { + let row: Vec = std::iter::once(Element::from(switcher)) + .chain(edit_button) + .collect(); + ( + None, + Some( + vstack(vec![hstack(row) + .spacing(8.0) + .vertical_alignment(VerticalAlignment::Bottom) + .with_key(switcher_key) + .into()]) + .margin(edges(24.0, 12.0, 28.0, 0.0)) + .into(), + ), + ) + }; let titled: Vec = std::iter::once( text_block(title) @@ -1358,7 +1394,7 @@ pub(crate) fn settings_page( // but skips `selected_index` whenever the two sections' values compare equal, so the // combos render blank until touched. A panel (vstack) takes the keyed path, so the key // remounts the whole column and every prop is applied fresh. - let content = scroll_view( + let scrolled = scroll_view( // ⚠️ Keyed on (scope, section), not section alone: switching SCOPE re-renders the same // section's controls with different values, and an in-place diff re-sets each reused // ComboBox's items (clearing WinUI's selection) while skipping `selected_index` @@ -1372,6 +1408,16 @@ pub(crate) fn settings_page( ) .opacity(progress) .margin(edges(0.0, (1.0 - progress) * 22.0, 0.0, 0.0)); + // With the pane collapsed the switcher leads the content column, above the scrolling + // region so it stays put — it is still chrome, not a settings row. An Auto/Star grid, + // not a vstack: a StackPanel would hand the scroll_view its DESIRED height and the + // viewport (and scrolling) would be gone. + let content: Element = match inline_switcher { + Some(row) => grid(vec![row.grid_row(0), Element::from(scrolled).grid_row(1)]) + .rows([GridLength::Auto, GridLength::STAR]) + .into(), + None => scrolled.into(), + }; // The delete confirmation, when armed. Declarative, like every other dialog in this shell: // it is an element in the tree with `is_open`, not a call. let confirm: Option = delete_pending.as_ref().and_then(|id| { @@ -1434,9 +1480,8 @@ pub(crate) fn settings_page( .into(), ) }); - let nav = NavigationView::new(items, content) + let mut nav = NavigationView::new(items, content) .pane_title("Settings") - .pane_footer(footer) .selected_tag(section) .on_selection_changed({ let ss = set_section.clone(); @@ -1448,6 +1493,9 @@ pub(crate) fn settings_page( let ss = set_screen.clone(); move || ss.call(Screen::Hosts) }); + if let Some(f) = footer { + nav = nav.pane_footer(f); + } // One grid, so every layer FILLS the window (a vstack would hand the NavigationView its // desired height — clipped when the window is short, floating when it is tall): the nav, // the Edit-profile scrim + card over it, and the delete confirmation (a ContentDialog, diff --git a/clients/windows/tests/reactor_semantics.rs b/clients/windows/tests/reactor_semantics.rs new file mode 100644 index 00000000..7dfedcae --- /dev/null +++ b/clients/windows/tests/reactor_semantics.rs @@ -0,0 +1,327 @@ +#![cfg(windows)] +//! Characterization tests for the windows-reactor re-render semantics this client's +//! architecture depends on. These are not tests of the client — they encode the CONTRACT +//! the client's state placement assumes (see the re-render discipline note in `app/mod.rs`), +//! against the same pinned rev, using reactor's own headless harness (`RecordingBackend`). +//! Run them after every SHA bump: a red test here means the discipline note is stale again, +//! in one direction or the other. +//! +//! History that motivates each case: +//! * The June 2026 rev dropped a background `AsyncSetState` write on the floor unless the +//! owning component's props changed — that bug forced ALL thread-driven state into the +//! root component. Fixed July 2026 (`dirty: Arc` in the engine). +//! * The July 2026 pre-bump rev still pruned a dirty component sitting under an +//! element-equal non-component wrapper (`border`, `scroll_viewer`) — the reconciler's +//! dirty lookup was one level deep. Fixed by the 2026-07-29 pin (upstream regression +//! test: `nested_state_rerender.rs`). +//! * Event handlers wired in the WinUI backend (`add_Click` etc.) were believed to bypass +//! the render flush, so a sync `use_state` write from one never repainted. The harness +//! fires events through the same backend attach path; the third test pins the engine +//! side of that claim. + +use std::cell::{Cell, RefCell}; +use std::rc::Rc; +use std::thread; + +use test_reactor::{Op, RecordingBackend}; +use windows_reactor::{ + border, component, vstack, AsyncSetState, ChannelDispatcher, Component, ControlKind, + Dispatcher, DispatcherQueuePriority, Element, Event, Prop, PropValue, RenderCx, RenderHost, + SetState, TextBlock, ToggleSwitch, +}; + +type Job = Box; + +/// The upstream tests' single-threaded "UI thread": jobs queue until drained. +#[derive(Clone, Default)] +struct TestDispatcher { + queue: Rc>>, +} + +impl TestDispatcher { + fn drain(&self) { + loop { + let item = { + let mut q = self.queue.borrow_mut(); + if q.is_empty() { + None + } else { + Some(q.remove(0)) + } + }; + match item { + Some(f) => f(), + None => break, + } + } + } +} + +impl Dispatcher for TestDispatcher { + fn enqueue(&self, _p: DispatcherQueuePriority, f: Box) -> bool { + self.queue.borrow_mut().push(f); + true + } +} + +fn last_text(ops: &[Op]) -> Option { + ops.iter().rev().find_map(|op| match op { + Op::SetProp { + prop: Prop::Text, + value: PropValue::Str(s), + .. + } => Some(s.clone()), + _ => None, + }) +} + +// --- Case 1: sync `use_state` in a child under an element-equal `border` wrapper --------- +// The client wraps every screen in an animated `border` that compares equal once the +// entrance tween settles; `pair.rs` documents a live workaround for this exact topology. + +struct SyncInner { + renders: Rc>, + setter_out: Rc>>>, +} + +impl Component for SyncInner { + fn render(&self, _props: &(), cx: &mut RenderCx) -> Element { + let (n, set) = cx.use_state(0_u64); + *self.setter_out.borrow_mut() = Some(set); + self.renders.set(self.renders.get() + 1); + Element::TextBlock(TextBlock::new(format!("sync-{n}"))) + } +} + +struct SyncRoot { + renders: Rc>, + setter_out: Rc>>>, +} + +impl Component for SyncRoot { + fn render(&self, _props: &(), _cx: &mut RenderCx) -> Element { + border(component( + SyncInner { + renders: Rc::clone(&self.renders), + setter_out: Rc::clone(&self.setter_out), + }, + (), + )) + .into() + } +} + +#[test] +fn sync_state_child_under_element_equal_border_rerenders() { + let dispatcher = TestDispatcher::default(); + let setter_out = Rc::new(RefCell::new(None)); + let renders = Rc::new(Cell::new(0)); + let host = RenderHost::new( + RecordingBackend::new(), + Box::new(SyncRoot { + renders: Rc::clone(&renders), + setter_out: Rc::clone(&setter_out), + }) as Box, + dispatcher.clone(), + ); + host.kick(); + dispatcher.drain(); + assert_eq!(renders.get(), 1, "inner mounts once"); + + setter_out.borrow().as_ref().unwrap().call(1); + dispatcher.drain(); + + assert_eq!( + renders.get(), + 2, + "a dirty child under an element-equal border must re-render \ + (the pre-2026-07-29 reconciler pruned at the wrapper)" + ); + assert_eq!( + host.with_reconciler(|r| last_text(&r.backend.ops)), + Some("sync-1".to_string()) + ); +} + +// --- Case 2: background `AsyncSetState` write into a child with unchanged props ---------- +// THE case the root-hoisting discipline was built around. MEASURED BROKEN at the 2026-07-29 +// pin, for a new reason: every component's `RenderCx` draws a fresh `HostId` +// (`RenderCx::new` → `HostId::next()`), but rerender callbacks are only registered for the +// ROOT's id (`RenderHost::set_marshaller` / the app bootstrap). A child-owned async write +// stores the value and sets the dirty flag, then calls +// `request_ui_rerender_on_ui_thread(child_host_id)` — a registry miss, silently dropped. +// The new value appears whenever anything ELSE triggers a pass. (Upstream's +// `async_setter_marks_owning_component_dirty_for_rerender` installs the guard for the very +// cx it tests, so it never sees this.) The test asserts the BROKEN behaviour on purpose: +// when a future bump fixes it upstream, this fails loudly and the root-hoisting rule for +// thread-driven state in `app/mod.rs` can finally be relaxed. + +struct AsyncInner { + renders: Rc>, + setter_out: Rc>>>, +} + +impl Component for AsyncInner { + fn render(&self, _props: &(), cx: &mut RenderCx) -> Element { + let (n, set) = cx.use_async_state(0_u64); + *self.setter_out.borrow_mut() = Some(set); + self.renders.set(self.renders.get() + 1); + Element::TextBlock(TextBlock::new(format!("async-{n}"))) + } +} + +struct AsyncRoot { + renders: Rc>, + root_renders: Rc>, + setter_out: Rc>>>, +} + +impl Component for AsyncRoot { + fn render(&self, _props: &(), _cx: &mut RenderCx) -> Element { + self.root_renders.set(self.root_renders.get() + 1); + border(component( + AsyncInner { + renders: Rc::clone(&self.renders), + setter_out: Rc::clone(&self.setter_out), + }, + (), + )) + .into() + } +} + +#[test] +fn async_state_child_under_element_equal_border_rerenders() { + let dispatcher = TestDispatcher::default(); + let channel = ChannelDispatcher::new(); + let setter_out: Rc>>> = Rc::new(RefCell::new(None)); + let renders = Rc::new(Cell::new(0)); + let root_renders = Rc::new(Cell::new(0)); + let host = RenderHost::new( + RecordingBackend::new(), + Box::new(AsyncRoot { + renders: Rc::clone(&renders), + root_renders: Rc::clone(&root_renders), + setter_out: Rc::clone(&setter_out), + }) as Box, + dispatcher.clone(), + ); + host.set_marshaller(Some(channel.marshaller())); + host.kick(); + dispatcher.drain(); + assert_eq!(renders.get(), 1, "inner mounts once"); + + // The write happens OFF the UI thread, exactly like discovery/HUD/speed-test threads. + let set = setter_out.borrow().as_ref().unwrap().clone(); + thread::spawn(move || set.call(1)).join().unwrap(); + + // Marshal back onto the "UI thread", then run the render it requested. + channel.drain(); + dispatcher.drain(); + + assert_eq!( + root_renders.get(), + 1, + "CONTRACT CHANGED: a child-owned async write now triggers a render pass — the \ + host-id registry miss was fixed upstream. Thread-driven state may move out of \ + root(); update the discipline note in app/mod.rs and this test" + ); + assert_eq!( + renders.get(), + 1, + "CONTRACT CHANGED: the child re-rendered from its own async write — see above" + ); + + // The write itself was NOT lost: it lands on the next pass someone else triggers. + // (This is exactly the 'appears after unrelated navigation' failure mode the client + // keeps paying for, and why thread-driven state lives in root().) + host.kick(); + dispatcher.drain(); + assert_eq!(renders.get(), 2); + assert_eq!( + host.with_reconciler(|r| last_text(&r.backend.ops)), + Some("async-1".to_string()), + "the buffered write must surface on the next externally-triggered pass" + ); +} + +// --- Case 3: sync `use_state` written from a backend-fired event handler ----------------- +// The client hoists `forget`/`rename`/`hover` to root because WinUI-wired handlers +// (MenuFlyout `add_Click`, pointer enter/exit) were believed to mark dirty without ever +// pumping the reconciler. The harness fires through the same backend attach path. + +struct ToggleInner { + renders: Rc>, +} + +impl Component for ToggleInner { + fn render(&self, _props: &(), cx: &mut RenderCx) -> Element { + let (n, set) = cx.use_state(0_u64); + self.renders.set(self.renders.get() + 1); + vstack(( + TextBlock::new(format!("evt-{n}")), + ToggleSwitch::new(false).on_toggled(move |_| set.call(n + 1)), + )) + .into() + } +} + +struct ToggleRoot { + renders: Rc>, +} + +impl Component for ToggleRoot { + fn render(&self, _props: &(), _cx: &mut RenderCx) -> Element { + border(component( + ToggleInner { + renders: Rc::clone(&self.renders), + }, + (), + )) + .into() + } +} + +#[test] +fn sync_state_from_backend_fired_event_rerenders() { + let dispatcher = TestDispatcher::default(); + let renders = Rc::new(Cell::new(0)); + let host = RenderHost::new( + RecordingBackend::new(), + Box::new(ToggleRoot { + renders: Rc::clone(&renders), + }) as Box, + dispatcher.clone(), + ); + host.kick(); + dispatcher.drain(); + assert_eq!(renders.get(), 1); + + let toggle_id = host.with_reconciler(|r| { + r.backend + .ops + .iter() + .find_map(|op| match op { + Op::Create { + id, + kind: ControlKind::ToggleSwitch, + } => Some(*id), + _ => None, + }) + .expect("the toggle was created") + }); + host.with_reconciler(|r| r.backend.fire_bool(toggle_id, Event::Toggled, true)); + dispatcher.drain(); + + assert_eq!( + renders.get(), + 2, + "a sync use_state write from a backend-fired handler must schedule and run a \ + re-render — if this fails, the engine really does drop handler-driven dirt, and \ + the root-hoisting of forget/rename/hover stays justified at the engine level" + ); + assert_eq!( + host.with_reconciler(|r| last_text(&r.backend.ops)), + Some("evt-1".to_string()) + ); +}