diff --git a/context/code/protocol.md b/context/code/protocol.md index 58717d30..ca92b913 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -862,17 +862,41 @@ frame acquiring the claim by omission. **`group_runs` / `group_run_heads` are the coalescing rule's one home.** The TUI and `render.rs::col_groups` each coalesced adjacent columns sharing a group, -kept in step by a comment (`improvise-8qlr`). Runs coalesce by group **name**, -so two adjacent runs under two different headers of the same name read as one — -that is what both ends did before and is preserved deliberately, because -changing the rule in the commit that moves it would leave the agreement test -unable to tell "both ends agree" from "both ends changed together". +kept in step by a comment (`improvise-8qlr`). The TUI's one divergence is that +it labels its first *visible* column even mid-run, so a scrolled grid does not +draw an unlabelled group line. -`FrameGroup` carries `cat` and could fix it, and **a single home reads as -decided** — which is the new risk and why it is filed as improvise-h0iv rather -than left as a note. The TUI's one -divergence is that it labels its first *visible* column even mid-run, so a -scrolled grid does not draw an unlabelled group line. +**A run is opened by a HEADER, not by a name** (improvise-h0iv). Both prior +copies coalesced by name alone, and `improvise-8qlr` preserved that *because* it +was a consolidation: changing the rule in the commit that moved it would have +left the agreement test unable to tell "both ends agree" from "both ends changed +together". With one home that argument is spent, and the rule can be changed +once for both ends — which is the shape to notice, since a consolidation's +frozen behaviour reads as decided afterwards and nothing says it was only frozen +for the duration of the move. + +**The ticket's own proposed fix does not work, and that is worth more than the +fix.** h0iv suggested comparing the `(cat, name)` pair, `FrameGroup` carrying +`cat` precisely so a client need not work out which category a label came from. +But its own reachability case is *one* category: `Month` grouped into `Q1`, +nested under `Region`, giving `North/Q1` and `South/Q1` adjacent — the two +headers are `(Month, Q1)` and `(Month, Q1)`, **equal as values**, so the pair +separates nothing. `the_two_headers_are_equal_as_values_so_comparing_cat_and_name_cannot_split_them` +pins that, so the next reader does not re-propose it. What separates them is +that they are two different `FrameEntry::Group` entries, and `groups()` already +borrows each from `self.entries` — so the comparison is `std::ptr::eq` on the +header, which is the literal statement of "the same header opened both". + +**The case that settled the product question was not the span but the +indicator.** Two adjacent same-named runs might well be one span a user wants +spanned, which is why h0iv was filed rather than fixed. What is not arguable is +`column_groups_coalesce_by_name_as_the_tui_does`, whose fixture merged +`Month/Q1` (expanded) with `Fiscal/Q1` (**collapsed**) and whose own message +said the merged span is "drawn with the FIRST one's indicator" — a collapsed +group rendered as expanded, with no affordance saying otherwise. A span +boundary is a matter of taste; a wrong collapse glyph is a wrong fact. So: two +runs. Reversing it is one comparison, and the test is renamed +`column_groups_coalesce_per_header_as_the_tui_does` rather than deleted. ## 4. `frame.rs` — one envelope pair, two deployments diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 33e6033a..e9260206 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -368,15 +368,24 @@ is viewport-indexed, the cursor is not. The offsets in the fixture are **nonzero and unequal**, which is what separates "reconciles" from "ignores the offset" *and* from "swaps the two offsets". -### column_groups_coalesce_by_name_as_the_tui_does +### column_groups_coalesce_per_header_as_the_tui_does -(`render.rs`.) The run boundary must be the **name alone**, because that is what -`src/ui/grid.rs` uses (`group_name != prev_group`) and the two ends must not -disagree about where a span begins. Collapse is category-scoped, so two adjacent -groups from different categories may legally share a name and differ in it. -Keying the run on the pair would start a second run — a second indicator the TUI -suppresses — which is a front-end divergence of exactly the kind improvise-anr9 -exists to remove (raised by Copilot on PR #23). +(`render.rs`.) Was `column_groups_coalesce_by_name_as_the_tui_does`, and its +justification was consumed by the consolidation it predates. It read: the run +boundary must be the name alone, "because that is what `src/ui/grid.rs` uses +(`group_name != prev_group`) and the two ends must not disagree". Since +improvise-8qlr `src/ui/grid.rs` uses `AxisFrame::group_run_heads`, so there is +no second copy to agree with — the rule can change once and both ends follow. + +**Its fixture is what settled improvise-h0iv.** It merges `Month/Q1` +(expanded) with `Fiscal/Q1` (**collapsed**), and its own message said the merged +span is "drawn with the FIRST one's indicator". That is a collapsed group +rendered as expanded, with nothing on screen saying otherwise. Whether two +adjacent same-named runs are one span is a question of taste and is why h0iv was +filed rather than fixed; a wrong collapse glyph is not. The expectation is now +three runs, and the collapse flag of each is asserted — which is what makes this +test see the fault at all, since the labels and spans alone cannot distinguish a +merge that keeps the first indicator from a split. ### a_keyboard_edit_in_progress_is_part_of_the_render_model diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index 8dc54cba..3911d1f6 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -528,3 +528,33 @@ Run against the pre-fix tree 2026-09-21: it fails at the `expect_err`, printing `AxisFrame { .., first: 0, .. }` — the forged claim itself. The fix is the removal of one attribute; the whole workspace stays green (1464), which is also the evidence that nothing was relying on the permissive parse. + + +### render::tests::two_runs_under_two_headers_of_one_name_are_two_runs + +### render::tests::the_two_headers_are_equal_as_values_so_comparing_cat_and_name_cannot_split_them + +improvise-h0iv. `group_runs` coalesced adjacent positions by group **name**, so +two runs under two different headers sharing a name read as one and both front +ends drew one span across the boundary. + +The first test is the demonstration, at the level the rule lives: a frame with +`[Group(Month/Q1), North/Jul, North/Aug, Group(Month/Q1), South/Jul, South/Aug]` +and nothing ungrouped between the runs. Before the fix it returns +`[(Q1, 4)]`; after, `[(Q1, 2), (Q1, 2)]`. It asserts the frame really holds +**two** `FrameEntry::Group` entries first, so a fixture that lost one would fail +at the precondition rather than pass with a single run. + +The second test exists because the ticket proposed the wrong fix and a reader +will propose it again. h0iv says `FrameGroup` carries `cat` and "comparing the +pair would fix it" — but its own reachability case is a single category nested +under an outer one, so both headers are `(Month, Q1)`, equal as values. The test +asserts that equality directly. What actually separates them is that they are +two distinct entries in `self.entries`, which `std::ptr::eq` on the borrowed +header states exactly. + +Fault injected 2026-09-21: restoring `prev.map(|p| p.name.as_str()) == ...` +fails the first test with `left: [(Some("Q1"), 4)]`, and fails +`two_abutting_runs_of_one_group_name_are_two_spans_at_both_ends` end to end. The +second test passes under both rules, deliberately — it is a statement about the +data, not about the comparison. diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index 69547db0..a7e19714 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -545,3 +545,28 @@ One thing the shared rule does **not** flatten: the TUI labels its first *visible* column even mid-run, because a scrolled grid would otherwise draw an unlabelled group line. That is a scrolling concession, kept explicitly, and it is why the comparison is run at `col_offset == 0`. + + +### two_abutting_runs_of_one_group_name_are_two_spans_at_both_ends + +improvise-h0iv, end to end. The unit demonstration is in +`context/test-evidence/protocol.md`; this is the same rule driven through a real +`GridLayout`, a real `project::grid_of` and both renderers, because the +reachability of the shape was itself in question. + +`two_level_rows` has exactly h0iv's shape — `Q1` grouping two of three months, +cross-producted under two measures — and is **saved by the ungrouped `Sep` +between the runs**, which is also what makes improvise-gd83's spans come out as +`[(Q1,2), (None,1), (Q1,2), (None,1)]`. `abutting_column_groups` removes `Sep` +and nothing else, so the two `Q1` runs abut. The test asserts that no run is +unlabelled before comparing anything: a fixture that grew an ungrouped column +back would fail there rather than passing on the easy case. + +It then compares the TUI's painted group line against the browser's runs +exactly, which is the part that makes it an agreement test rather than a second +assertion about `group_runs`. + +Fault injected 2026-09-21: restoring the name-only comparison in +`AxisFrame::group_runs` fails this with `left: [(Some("Q1"), 4)]` while the five +existing agreement tests all pass — the ungrouped `Sep` in their fixture is +precisely why they cannot see it. diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index 02f7c0c9..3f4f7489 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -322,14 +322,13 @@ impl AxisFrame { .flatten() } - /// The runs of adjacent positions sharing a group, coalesced by NAME — - /// the coalescing rule's **one home**, both front ends (improvise-8qlr). + /// The runs of adjacent positions sharing a group — one home, both front + /// ends (improvise-8qlr). Opened by a HEADER, not a name: §3a, improvise-h0iv. pub fn group_runs(&self) -> Vec<(Option<&FrameGroup>, usize)> { let mut runs: Vec<(Option<&FrameGroup>, usize)> = Vec::new(); for group in self.groups() { - let name = group.map(|g| g.name.as_str()); match runs.last_mut() { - Some((prev, span)) if prev.map(|p| p.name.as_str()) == name => *span += 1, + Some((prev, span)) if opened_by(*prev, group) => *span += 1, _ => runs.push((group, 1)), } } @@ -377,6 +376,16 @@ impl AxisFrame { } } +/// Same `FrameEntry::Group`, by identity — two equal headers under different +/// outer items are two runs. Both borrow `self.entries`, so this is total. +fn opened_by(one: Option<&FrameGroup>, other: Option<&FrameGroup>) -> bool { + match (one, other) { + (None, None) => true, + (Some(one), Some(other)) => std::ptr::eq(one, other), + _ => false, + } +} + /// The row-header width rule's **one home** (improvise-1125). It borrows /// rather than taking an [`AxisFrame`], for the keypress path — see §3. pub fn header_level_widths<'a>( @@ -1012,6 +1021,75 @@ mod tests { } } + /// Two runs of one group name under two different outer items, with nothing + /// ungrouped between them — improvise-h0iv's reachable shape. + fn repeated_group_frame() -> AxisFrame { + let q1 = || FrameGroup { + cat: "Month".into(), + name: "Q1".into(), + collapsed: false, + }; + AxisFrame { + first: 0, + cats: vec!["Region".into(), "Month".into()], + entries: vec![ + FrameEntry::Group(q1()), + FrameEntry::Item(vec!["North".into(), "Jul".into()]), + FrameEntry::Item(vec!["North".into(), "Aug".into()]), + FrameEntry::Group(q1()), + FrameEntry::Item(vec!["South".into(), "Jul".into()]), + FrameEntry::Item(vec!["South".into(), "Aug".into()]), + ], + } + } + + /// improvise-h0iv: a run is opened by a header, not by a name. See + /// `context/test-evidence/protocol.md`. + #[test] + fn two_runs_under_two_headers_of_one_name_are_two_runs() { + let frame = repeated_group_frame(); + let spans: Vec<(Option<&str>, usize)> = frame + .group_runs() + .into_iter() + .map(|(group, span)| (group.map(|g| g.name.as_str()), span)) + .collect(); + assert_eq!( + frame + .entries + .iter() + .filter(|e| matches!(e, FrameEntry::Group(_))) + .count(), + 2, + "precondition: two separate headers, or there is nothing to split" + ); + assert_eq!( + spans, + vec![(Some("Q1"), 2), (Some("Q1"), 2)], + "one span of 4 draws a single colspan across the North/South boundary" + ); + } + + /// The pair `FrameGroup` carries cannot separate them — h0iv's own proposed + /// fix. See `context/test-evidence/protocol.md`. + #[test] + fn the_two_headers_are_equal_as_values_so_comparing_cat_and_name_cannot_split_them() { + let frame = repeated_group_frame(); + let headers: Vec<&FrameGroup> = frame + .entries + .iter() + .filter_map(|e| match e { + FrameEntry::Group(g) => Some(g), + _ => None, + }) + .collect(); + assert_eq!( + (&headers[0].cat, &headers[0].name), + (&headers[1].cat, &headers[1].name), + "both headers name the same category AND the same group, so the \ + ticket's (cat, name) comparison would still merge them" + ); + } + /// improvise-1wl0: a dropped `first` must not deserialize as the claim that /// this frame LEADS the axis. See `context/test-evidence/protocol.md`. #[test] diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index 24f7202b..2cbdc618 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -513,10 +513,10 @@ mod tests { FrameEntry::Item(vec![label.into()]) } - /// The run boundary must be the **name alone**, or the two front ends - /// disagree about where a span begins. See the evidence file. + /// A run is opened by a HEADER, not by a name — improvise-h0iv, and both + /// ends now ask the one rule. See the evidence file. #[test] - fn column_groups_coalesce_by_name_as_the_tui_does() { + fn column_groups_coalesce_per_header_as_the_tui_does() { let frame = AxisFrame { first: 0, cats: vec!["Month".into()], @@ -534,9 +534,13 @@ mod tests { .iter() .map(|g| (g.label.as_deref(), g.span, g.collapsed)) .collect::>(), - vec![(Some("Q1"), 2, false), (Some("Q2"), 1, false)], - "the two Q1 headers are one span, drawn with the FIRST one's \ - indicator; keying on (name, collapsed) would split them" + vec![ + (Some("Q1"), 1, false), + (Some("Q1"), 1, true), + (Some("Q2"), 1, false) + ], + "merging the two Q1 headers drew the SECOND one with the first's \ + collapse indicator, so a collapsed group looked expanded" ); } diff --git a/tests/grid_frame_agreement.rs b/tests/grid_frame_agreement.rs index 28334701..c7d66655 100644 --- a/tests/grid_frame_agreement.rs +++ b/tests/grid_frame_agreement.rs @@ -315,6 +315,60 @@ fn the_two_front_ends_draw_the_same_column_header() { ); } +/// `two_level_rows` without `Sep`, so the two `Q1` runs ABUT. The existing +/// fixture is saved by the ungrouped month between them — improvise-h0iv. +fn abutting_column_groups() -> Workbook { + let mut m = two_level_rows(); + m.remove_item("Month", "Sep"); + m +} + +/// improvise-h0iv, end to end: two `Q1` runs under two measures are two spans, +/// and both ends draw them. See `context/test-evidence/tui.md`. +#[test] +fn two_abutting_runs_of_one_group_name_are_two_spans_at_both_ends() { + let mut wb = abutting_column_groups(); + let (_app, cache) = whole_grid(&mut wb); + let model = browser_model(&cache); + let lines = tui_lines(&wb, &cache); + let rule = header_rule(&lines); + let levels = model.col_levels.len(); + + let named: Vec<(Option<&str>, usize)> = model + .col_groups + .iter() + .map(|g| (g.label.as_deref(), g.span)) + .collect(); + assert!( + named.iter().all(|(label, _)| label.is_some()), + "precondition: NOTHING ungrouped may separate the runs, which is the \ + whole difference from the fixture above: {named:?}" + ); + assert_eq!( + named, + vec![(Some("Q1"), 2), (Some("Q1"), 2)], + "one span of four draws a single header across the measure boundary" + ); + + let group_line = &lines[rule - levels - 1]; + let expected: Vec = model + .col_groups + .iter() + .flat_map(|g| { + let head = match &g.label { + Some(name) => format!("{} {name}", indicator(g.collapsed)), + None => String::new(), + }; + std::iter::once(head).chain(std::iter::repeat_n(String::new(), g.span - 1)) + }) + .collect(); + assert_eq!( + &tui_header_cells(group_line, &cache), + &expected, + "the TUI draws one group line and the browser decided another" + ); +} + /// improvise-0o6x: a group FIRST on its axis. Separate from `two_level_rows`, /// which orders Carol first to dodge this case on purpose. fn leading_group_rows() -> Workbook {