From d9a4790d4d239288ca4cb8036cc9f26d23acd0d0 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sat, 19 Sep 2026 23:45:08 -0700 Subject: [PATCH] fix: an empty group name is a name; and stop reserving the group rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot's seventh pass. One applied, one applied by *withdrawing* the previous fix rather than patching it again. **`RenderColGroup::label` is an `Option`.** `""` was the ungrouped-run sentinel, but an empty string is a legal group *name* — `Category::add_item_in_group` accepts one and the TUI draws its indicator like any other — so a real group serialized identically to no group at all and the page dropped its indicator on a truthiness test. `None` is now the run in no group, and `app.js` checks for null. **Inline group rows are no longer reserved in the viewport, and the reason is written down.** Copilot was right twice in a row here, about two opposite mistakes. Counting the current group rows is a feedback loop (a smaller window crosses fewer groups and asks for a larger one). Keeping the maximum is a ratchet: one group header per data row stores an oversized cost on the first oversized projection and clamps the grid to a single row *permanently*. Both were on this branch. The third option — invent a density heuristic — is the one this repo warns against, in a file no test here can execute. So the reservation keeps only what depends on the axes and not on the window: the header lines, the group line, the row-header width. The residue is an over-request bounded by the groups in view, and the off-screen-selection gap it leaves is neither new nor specific to groups — nothing in this page calls `scrollIntoView` at all. Filed rather than guessed at. Co-Authored-By: Claude Opus 5 (1M context) --- crates/improvise-wasm-client/src/render.rs | 20 +++++--- tests/grid_frame_agreement.rs | 10 ++-- web/app.js | 53 ++++++++++------------ 3 files changed, 43 insertions(+), 40 deletions(-) diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index 8a8e858d..2fe695ba 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -141,12 +141,18 @@ impl RenderRow { /// A run of columns under one group header. /// -/// `label` is empty for a run of columns in no group, which keeps the runs a -/// partition of the columns: the spans sum to the column count, so a page -/// laying them out as `colspan` cannot leave a hole. +/// `label` is `None` for a run of columns in **no** group, which keeps the +/// runs a partition of the columns: the spans sum to the column count, so a +/// page laying them out as `colspan` cannot leave a hole. +/// +/// It is an `Option` rather than an empty string because an empty string is +/// a legal group *name* — `Category::add_item_in_group` accepts one, and the +/// TUI draws its indicator like any other. A sentinel would serialize that +/// group identically to an ungrouped run and the page would drop its +/// indicator (raised by Copilot on PR #23). #[derive(Debug, Clone, Default, PartialEq, Serialize)] pub struct RenderColGroup { - pub label: String, + pub label: Option, pub collapsed: bool, pub span: usize, } @@ -329,7 +335,7 @@ fn col_groups(cols: &AxisFrame) -> Vec { let mut any = false; for group in cols.groups() { any |= group.is_some(); - let label = group.map(|g| g.name.clone()).unwrap_or_default(); + let label = group.map(|g| g.name.clone()); let collapsed = group.is_some_and(|g| g.collapsed); // Coalesced by NAME alone, which is what `src/ui/grid.rs` does // (`group_name != prev_group`), and the two must agree. Collapse is @@ -426,9 +432,9 @@ mod tests { assert_eq!( col_groups(&frame) .iter() - .map(|g| (g.label.as_str(), g.span, g.collapsed)) + .map(|g| (g.label.as_deref(), g.span, g.collapsed)) .collect::>(), - vec![("Q1", 2, false), ("Q2", 1, false)], + 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" ); diff --git a/tests/grid_frame_agreement.rs b/tests/grid_frame_agreement.rs index 984f998b..0e24cc8c 100644 --- a/tests/grid_frame_agreement.rs +++ b/tests/grid_frame_agreement.rs @@ -315,22 +315,22 @@ fn the_two_front_ends_draw_the_same_column_header() { cache.col_frame.len(), "the spans must cover every column, or a colspan layout leaves a hole" ); - let named: Vec<(&str, usize)> = model + let named: Vec<(Option<&str>, usize)> = model .col_groups .iter() - .map(|g| (g.label.as_str(), g.span)) + .map(|g| (g.label.as_deref(), g.span)) .collect(); assert_eq!( named, - vec![("Q1", 6)], + vec![(Some("Q1"), 6)], "Q1 covers all six columns, INCLUDING the two under the ungrouped \ Sep — `expand_category` emits a header on entering a group and no \ boundary on leaving one, so the nearest-preceding walk keeps \ answering Q1 (see `AxisFrame::group_of`). That is `GridLayout`'s \ rule and the TUI paints the same span; it is reproduced here rather \ than corrected, because correcting it changes what the TUI draws. \ - When it is fixed this line becomes [(\"Q1\", 4), (\"\", 2)] and this \ - is the test that says so." + When it is fixed this line becomes [(Some(\"Q1\"), 4), (None, 2)] \ + and this is the test that says so." ); // And the group line the TUI paints names the same group. diff --git a/web/app.js b/web/app.js index 2e585b85..9ab6391c 100644 --- a/web/app.js +++ b/web/app.js @@ -116,9 +116,9 @@ function setShellStatus(text) { // // It is measured off the painted model rather than predicted, and defaults to // the old one-and-one before the first paint, which is the only moment there -// is nothing to measure. `groups` is kept as its own high-water mark — see -// the tail of `repaint`. -let frameCost = { view: null, rows: 1, cols: 1, groups: 0 }; +// is nothing to measure. What it counts, and the one thing it deliberately +// does not, is at the tail of `repaint`. +let frameCost = { rows: 1, cols: 1 }; /** The extent this window can show, in whole DATA cells. */ function extent() { @@ -262,33 +262,26 @@ function repaint() { // header lines, a group line if there is one, the row header's width in // columns, and every inline group row. // - // The group rows are the awkward one, because they depend on how many - // rows the window asked for: counted directly they are a feedback loop — - // a smaller window crosses fewer groups and asks for a larger one, which - // crosses more. So the reservation is a HIGH-WATER MARK, which cannot - // oscillate because it only ever rises, and it rises in the safe - // direction: over-reserving asks for fewer rows than fit and leaves - // blank space, where under-reserving asks for more rows than the table - // can show and is the off-screen-selection bug itself. It resets when - // the view changes, so a dense document does not make every later view - // pessimistic. - if (model.view_name !== frameCost.view) { - frameCost = { view: model.view_name, rows: 1, cols: 1, groups: 0 }; - } - const groups = Math.max( - frameCost.groups, - model.rows.reduce((n, row) => n + (row.kind === "group" ? 1 : 0), 0), - ); + // Inline group ROWS are NOT reserved, and that is a decision rather than + // an oversight. They take real height, but how many are in view depends on + // how many rows the window asked for, so every way of counting them is + // either a feedback loop (count the current ones: a smaller window crosses + // fewer groups and asks for a larger one) or a ratchet (keep the maximum: + // a group-dense grid stores an oversized cost on its first oversized + // projection and never lets it fall — one group header per row clamps the + // grid to a single row, permanently). Both were tried on this PR. + // + // What is left is an over-request bounded by the groups in view. The cost + // is that a grouped grid asks for a few rows more than the table shows, + // which matters only for a selection near the bottom edge — and that gap + // is not new and not specific to groups: nothing in this page calls + // `scrollIntoView` at all, so a selection the client has not yet scrolled + // to can already sit below the fold. Filed as its own ticket rather than + // answered with a heuristic in a file no test in this repo can execute. const next = { - view: model.view_name, - groups, - rows: model.col_levels.length + (model.col_groups.length > 0 ? 1 : 0) + groups, + rows: model.col_levels.length + (model.col_groups.length > 0 ? 1 : 0), cols: model.row_levels, }; - // The high-water mark is committed unconditionally; only the ANNOUNCEMENT - // is gated on the extent changing. Committing it alongside would discard a - // risen group count whenever a falling header line cancelled it out, and a - // later repaint would then under-reserve — the bug this is here to stop. const changed = next.rows !== frameCost.rows || next.cols !== frameCost.cols; frameCost = next; if (changed) sendViewport(); @@ -316,7 +309,11 @@ function gridTable(model) { for (const group of model.col_groups) { const th = document.createElement("th"); th.colSpan = group.span; - th.textContent = group.label ? indicator(group.collapsed) + " " + group.label : ""; + // `label` is null for a run in no group. An empty STRING is a legal + // group name and still gets its indicator, which is why this is a null + // check and not a truthiness test. + th.textContent = + group.label === null ? "" : indicator(group.collapsed) + " " + group.label; th.classList.add("group"); groups.appendChild(th); }