fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: an empty group name is a name; and stop reserving the group rows

Browse Source 
Copilot's seventh pass. One applied, one applied by *withdrawing* the
previous fix rather than patching it again.

**`RenderColGroup::label` is an `Option<String>`.** `""` 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) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-09-19 23:45:08 -07:00
parent fb2122f23d
commit d9a4790d4d
3 changed files with 43 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/improvise-wasm-client/src/render.rs
+13 -7
View File
@@ -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<String>,
pub collapsed: bool,
pub span: usize,
}
@@ -329,7 +335,7 @@ fn col_groups(cols: &AxisFrame) -> Vec<RenderColGroup> {
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<_>>(),
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"
);
tests/grid_frame_agreement.rs
+5 -5
View File
@@ -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.
web/app.js
+25 -28
View File
@@ -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);
}