fix(protocol): a group run is opened by a header, not by a name (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 a single
span across the boundary.

THE TICKET'S PROPOSED FIX DOES NOT WORK, and that is worth more than the fix.
h0iv suggested comparing the (cat, name) pair, FrameGroup carrying cat for
exactly that reason. But its own reachability case is ONE category -- Month
grouped into Q1 nested under Region, giving North/Q1 and South/Q1 adjacent --
so both headers are (Month, Q1), equal as values, and the pair separates
nothing. A test pins that so the next reader does not re-propose it. What does
separate them is that they are two distinct FrameEntry::Group entries, which
groups() already borrows from self.entries: std::ptr::eq on the header is the
literal statement of "the same header opened both".

THE PRODUCT QUESTION, decided here and reversible in one comparison. Whether
two adjacent same-named runs are one span a user wants spanned is a matter of
taste, which is why #77 filed rather than fixed. What settled it is not the
span but the indicator: the existing wasm-client test merged Month/Q1
(expanded) with Fiscal/Q1 (COLLAPSED) and its 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 taste; a
wrong collapse glyph is a wrong fact.

That test's justification was also spent: it said the rule must be name-only
"because that is what src/ui/grid.rs uses", and since improvise-8qlr grid.rs
calls group_run_heads. There is no second copy left to agree with, which is the
general point -- a consolidation's frozen behaviour reads as decided afterwards,
and nothing says it was frozen only for the duration of the move.

The agreement fixture gains the case it could not see: two_level_rows minus the
ungrouped Sep, so the two Q1 runs abut, end to end through a real GridLayout and
both renderers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
2026-09-21 03:57:07 -07:00
parent 9b81324d6a
commit c4bf74608f
7 changed files with 252 additions and 28 deletions
+34 -10
View File
@@ -862,17 +862,41 @@ frame acquiring the claim by omission.
**`group_runs` / `group_run_heads` are the coalescing rule's one home.** The TUI **`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, 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**, kept in step by a comment (`improvise-8qlr`). The TUI's one divergence is that
so two adjacent runs under two different headers of the same name read as one — it labels its first *visible* column even mid-run, so a scrolled grid does not
that is what both ends did before and is preserved deliberately, because draw an unlabelled group line.
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".
`FrameGroup` carries `cat` and could fix it, and **a single home reads as **A run is opened by a HEADER, not by a name** (improvise-h0iv). Both prior
decided** — which is the new risk and why it is filed as improvise-h0iv rather copies coalesced by name alone, and `improvise-8qlr` preserved that *because* it
than left as a note. The TUI's one was a consolidation: changing the rule in the commit that moved it would have
divergence is that it labels its first *visible* column even mid-run, so a left the agreement test unable to tell "both ends agree" from "both ends changed
scrolled grid does not draw an unlabelled group line. 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 ## 4. `frame.rs` — one envelope pair, two deployments
+17 -8
View File
@@ -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 unequal**, which is what separates "reconciles" from "ignores the offset"
*and* from "swaps the two offsets". *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 (`render.rs`.) Was `column_groups_coalesce_by_name_as_the_tui_does`, and its
`src/ui/grid.rs` uses (`group_name != prev_group`) and the two ends must not justification was consumed by the consolidation it predates. It read: the run
disagree about where a span begins. Collapse is category-scoped, so two adjacent boundary must be the name alone, "because that is what `src/ui/grid.rs` uses
groups from different categories may legally share a name and differ in it. (`group_name != prev_group`) and the two ends must not disagree". Since
Keying the run on the pair would start a second run — a second indicator the TUI improvise-8qlr `src/ui/grid.rs` uses `AxisFrame::group_run_heads`, so there is
suppresses — which is a front-end divergence of exactly the kind improvise-anr9 no second copy to agree with — the rule can change once and both ends follow.
exists to remove (raised by Copilot on PR #23).
**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 ### a_keyboard_edit_in_progress_is_part_of_the_render_model
+30
View File
@@ -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 `AxisFrame { .., first: 0, .. }` — the forged claim itself. The fix is the
removal of one attribute; the whole workspace stays green (1464), which is also removal of one attribute; the whole workspace stays green (1464), which is also
the evidence that nothing was relying on the permissive parse. 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.
+25
View File
@@ -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 *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 unlabelled group line. That is a scrolling concession, kept explicitly, and it
is why the comparison is run at `col_offset == 0`. 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.
+82 -4
View File
@@ -322,14 +322,13 @@ impl AxisFrame {
.flatten() .flatten()
} }
/// The runs of adjacent positions sharing a group, coalesced by NAME — /// The runs of adjacent positions sharing a group — one home, both front
/// the coalescing rule's **one home**, both front ends (improvise-8qlr). /// ends (improvise-8qlr). Opened by a HEADER, not a name: §3a, improvise-h0iv.
pub fn group_runs(&self) -> Vec<(Option<&FrameGroup>, usize)> { pub fn group_runs(&self) -> Vec<(Option<&FrameGroup>, usize)> {
let mut runs: Vec<(Option<&FrameGroup>, usize)> = Vec::new(); let mut runs: Vec<(Option<&FrameGroup>, usize)> = Vec::new();
for group in self.groups() { for group in self.groups() {
let name = group.map(|g| g.name.as_str());
match runs.last_mut() { 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)), _ => 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 /// The row-header width rule's **one home** (improvise-1125). It borrows
/// rather than taking an [`AxisFrame`], for the keypress path — see §3. /// rather than taking an [`AxisFrame`], for the keypress path — see §3.
pub fn header_level_widths<'a>( 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 /// improvise-1wl0: a dropped `first` must not deserialize as the claim that
/// this frame LEADS the axis. See `context/test-evidence/protocol.md`. /// this frame LEADS the axis. See `context/test-evidence/protocol.md`.
#[test] #[test]
+10 -6
View File
@@ -513,10 +513,10 @@ mod tests {
FrameEntry::Item(vec![label.into()]) FrameEntry::Item(vec![label.into()])
} }
/// The run boundary must be the **name alone**, or the two front ends /// A run is opened by a HEADER, not by a name — improvise-h0iv, and both
/// disagree about where a span begins. See the evidence file. /// ends now ask the one rule. See the evidence file.
#[test] #[test]
fn column_groups_coalesce_by_name_as_the_tui_does() { fn column_groups_coalesce_per_header_as_the_tui_does() {
let frame = AxisFrame { let frame = AxisFrame {
first: 0, first: 0,
cats: vec!["Month".into()], cats: vec!["Month".into()],
@@ -534,9 +534,13 @@ mod tests {
.iter() .iter()
.map(|g| (g.label.as_deref(), g.span, g.collapsed)) .map(|g| (g.label.as_deref(), g.span, g.collapsed))
.collect::<Vec<_>>(), .collect::<Vec<_>>(),
vec![(Some("Q1"), 2, false), (Some("Q2"), 1, false)], vec![
"the two Q1 headers are one span, drawn with the FIRST one's \ (Some("Q1"), 1, false),
indicator; keying on (name, collapsed) would split them" (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"
); );
} }
+54
View File
@@ -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<String> = 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`, /// improvise-0o6x: a group FIRST on its axis. Separate from `two_level_rows`,
/// which orders Carol first to dodge this case on purpose. /// which orders Carol first to dodge this case on purpose.
fn leading_group_rows() -> Workbook { fn leading_group_rows() -> Workbook {