diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index dde5fa120..2c3f397da 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -237,10 +237,12 @@ pub(crate) fn apply_style(s: &mut Session, p: &Json, f: impl Fn(&mut Style)) -> // Existing cells in those lines. let cells: Vec<(CellRef, StyleId)> = cx.wb.sheet(sheet).map(|sh| sh.cells.iter_range(*r).map(|(c, x)| (c, x.style)).collect()).unwrap_or_default(); - for (c, st) in cells { - let new = *cache.entry(st).or_insert_with(|| cx.wb.styles.derive(st, &f)); - cx.sheet_mut(sheet)?.set_style(c, new); + for (c, st) in &cells { + let new = *cache.entry(*st).or_insert_with(|| cx.wb.styles.derive(*st, &f)); + cx.sheet_mut(sheet)?.set_style(*c, new); } + // Formatting changed these cells' fonts: re-fit their rows (Excel auto-fits). + cx.fit_rows.extend(cells.iter().map(|(c, _)| (sheet, c.row))); continue; } if r.count() > 4_000_000 { diff --git a/crates/engine/tests/row_autofit.rs b/crates/engine/tests/row_autofit.rs new file mode 100644 index 000000000..8b9af4dd6 --- /dev/null +++ b/crates/engine/tests/row_autofit.rs @@ -0,0 +1,47 @@ +//! Whole-row and whole-column formatting must re-fit row heights (issue #833). + +use serde_json::json; + +fn sheet_mut(s: &mut gridcraft_engine::Session) -> &mut gridcraft_engine::model::Sheet { + std::sync::Arc::make_mut(&mut s.active_mut().unwrap().wb).sheet_mut(0).unwrap() +} + +#[test] +fn whole_row_font_format_grows_row_height_and_undoes() { + let mut s = gridcraft_engine::Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A5", "values": [["plain big bold"]]})).unwrap(); + let before = s.doc().unwrap().wb.active().unwrap().row_height(4); + s.execute("home.fontSize", json!({"range": "5:5", "size": 20})).unwrap(); + let after = s.doc().unwrap().wb.active().unwrap().row_height(4); + assert!(after > before + 10.0, "20pt whole-row format should grow the row: before={before}, after={after}"); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().row_height(4), before, "undo must restore the height"); +} + +#[test] +fn whole_column_font_format_grows_only_rows_with_cells() { + let mut s = gridcraft_engine::Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [["a"]]})).unwrap(); + s.execute("range.setValues", json!({"range": "A50", "values": [["b"]]})).unwrap(); + s.execute("home.fontSize", json!({"range": "A:A", "size": 20})).unwrap(); + let sh = s.doc().unwrap().wb.active().unwrap(); + assert!(sh.row_height(0) > 30.0 && sh.row_height(49) > 30.0, "rows with cells grow"); + assert!(sh.rows.len() <= 3, "full-column format must fit only rows that have cells, rows map has {} entries", sh.rows.len()); +} + +#[test] +fn whole_row_format_does_not_resize_custom_height_rows() { + let mut s = gridcraft_engine::Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A5", "values": [["plain big bold"]]})).unwrap(); + { + let sh = sheet_mut(&mut s); + let e = sh.rows.entry(4).or_default(); + e.custom = true; + e.size = Some(25.0); + } + s.execute("home.fontSize", json!({"range": "5:5", "size": 20})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().row_height(4), 25.0, "custom-height rows keep their height"); +} diff --git a/docs/superpowers/plans/2026-10-12-row-autofit-oversized-fonts.md b/docs/superpowers/plans/2026-10-12-row-autofit-oversized-fonts.md new file mode 100644 index 000000000..af25cfcc7 --- /dev/null +++ b/docs/superpowers/plans/2026-10-12-row-autofit-oversized-fonts.md @@ -0,0 +1,238 @@ +# Row Auto-Fit for Oversized Fonts (#833) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Rows without a user-set height grow to fit their tallest whole-cell font when a file opens and when whole-row/column formatting changes fonts, matching Excel's auto-fit; `customHeight` rows keep their height and clip as Excel does. + +**Architecture:** Two one-site fixes in existing flows. (1) `apply_style`'s whole-row/column branch already collects the touched cells — extend `Ctx::fit_rows` from them so the existing `commit()` → `auto_row_height()` pass re-fits the rows. (2) All file opens funnel through `open_bytes_with_password` in `io.rs` — after parsing, grow non-custom rows via the existing `auto_row_height`, clamped grow-only (a file's taller non-custom row is never shrunk; our estimate can differ from Excel's). No render change: once rows fit, the degenerate top-pin clamp in `cell_text_y` only fires for `customHeight` rows, where clipping matches Excel. + +**Tech Stack:** Rust; crates `engine` (L5) using `model`/`xlsx` (L2) APIs already in scope. No new dependencies. + +**Spec:** Upstream issue https://github.com/storytold/gridcraft/issues/833 and the bounded design approved in chat 2026-10-12 (sequencing decision: plain-cell paths on main now; per-run font measurement is a follow-up once rich-text-runs PR #837 merges — explicitly out of scope here). Triage facts: `auto_row_height` (`crates/engine/src/cmd/mod.rs:458`) has one call site (`commit`), and `format.rs`'s line-target branch `continue`s before the `fit_rows.extend` that the cell-by-cell branch has. + +## Global Constraints + +- No `unwrap`/`expect`/`panic!`/`unsafe` in non-test code (crate lints deny them); errors via `Result` and `?`. Tests may `unwrap`. +- Input-derived numbers are hostile: never iterate `1..MAX_ROWS`; fit only rows that have cells (sparse iteration). Existing caps stay (100,000-row fit guard, 546.0 height cap in `auto_row_height`). +- Excel-parity rules fixed by the spec: non-`custom` rows grow to fit their tallest whole-cell font on open and after whole-row/column formatting; `custom` rows never change; hidden rows stay hidden; open is grow-only (never shrinks a file's height). +- Layering: changes stay in `engine` (L5); `model`/`xlsx`/`core` are consumed, not modified. +- Never break wasm: new code is pure computation, no `std::env`, no threads. +- `cargo xtask ci` green before every commit (fmt, clippy -D warnings, tests, assets, layers, wasm). +- One task id per commit: `#833: `. +- Tests build workbooks in code; no binary fixtures. +- Per-run (rich text) font sizes are out of scope; do not touch `grid.rs` rendering. + +## Review Focus + +1. **`customHeight` rows must never be resized** by open-time fitting or whole-row formatting (user-set heights are sacred). → Task 1 test 3, Task 2 test B. +2. **Open must never shrink** a file's taller non-custom row (our line-height estimate differs from Excel's; shrinking would visibly change files that already look right). → Task 2 test C (grow-only clamp). +3. **Whole-column formatting spans 1,048,576 logical rows** — fitting must mark only rows that have cells, not the million-row span (hostile-input rule). → Task 1 test 2 asserts the sheet's `rows` map stays sparse. +4. **Default-font workbooks open as a no-op**: the plain-row fast path in `auto_row_height` must keep default heights and not pollute the `rows` map (big-file open cost stays O(content), sample files unchanged). → Task 2 test E. +5. **Hidden rows stay hidden** through open-time fitting and formatting. → Task 2 test D. + +(Undo integrity — formatting's height change must vanish on undo — is pinned inside Task 1 test 1.) + +--- + +### Task 1: Whole-row/column formatting re-fits row heights + +**Files:** +- Modify: `crates/engine/src/cmd/format.rs:237-244` (the line-target branch of `apply_style`) +- Test: `crates/engine/tests/row_autofit.rs` (new file) + +**Interfaces:** +- Consumes: `Ctx::fit_rows: Vec<(usize, u32)>` (sheet index, row) — already deduped by `commit()`; `cells: Vec<(CellRef, StyleId)>` already collected at format.rs:238-239. +- Produces: nothing new — behavior only. + +- [ ] **Step 1: Write the failing tests** in `crates/engine/tests/row_autofit.rs`, following the harness in `crates/engine/tests/autofit_filter_buttons.rs` (`Session::new()`, `s.new_workbook()`, `s.execute(...)`, read state via `s.doc().unwrap().wb.active().unwrap()`; copy its `sheet_mut` helper for setting row info): + +```rust +use serde_json::json; + +#[test] +fn whole_row_font_format_grows_row_height_and_undoes() { + let mut s = gridcraft_engine::Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A5", "values": [["plain big bold"]]})).unwrap(); + let before = s.doc().unwrap().wb.active().unwrap().row_height(4); + s.execute("home.fontSize", json!({"range": "5:5", "size": 20})).unwrap(); + let after = s.doc().unwrap().wb.active().unwrap().row_height(4); + assert!(after > before + 10.0, "20pt whole-row format should grow the row: before={before}, after={after}"); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().row_height(4), before, "undo must restore the height"); +} + +#[test] +fn whole_column_font_format_grows_only_rows_with_cells() { + let mut s = gridcraft_engine::Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [["a"]]})).unwrap(); + s.execute("range.setValues", json!({"range": "A50", "values": [["b"]]})).unwrap(); + s.execute("home.fontSize", json!({"range": "A:A", "size": 20})).unwrap(); + let sh = s.doc().unwrap().wb.active().unwrap(); + assert!(sh.row_height(0) > 30.0 && sh.row_height(49) > 30.0, "rows with cells grow"); + assert!(sh.rows.len() <= 3, "full-column format must fit only rows that have cells, rows map has {} entries", sh.rows.len()); +} + +#[test] +fn whole_row_format_does_not_resize_custom_height_rows() { + let mut s = gridcraft_engine::Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A5", "values": [["plain big bold"]]})).unwrap(); + { let sh = sheet_mut(&mut s); let e = sh.rows.entry(4).or_default(); e.custom = true; e.size = Some(25.0); } + s.execute("home.fontSize", json!({"range": "5:5", "size": 20})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().row_height(4), 25.0, "custom-height rows keep their height"); +} +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cargo test -p gridcraft-engine --test row_autofit` +Expected: tests 1 and 2 FAIL (height unchanged — the line branch never marks `fit_rows`); test 3 may PASS already (pins existing behavior). + +- [ ] **Step 3: Implement the marking** in `crates/engine/src/cmd/format.rs`, in the `if full_rows || full_cols` branch, after the `for (c, st) in cells` loop and before its `continue` (line 244): + +```rust +// Formatting changed these cells' fonts: re-fit their rows (Excel auto-fits). +cx.fit_rows.extend(cells.iter().map(|(c, _)| (sheet, c.row))); +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cargo test -p gridcraft-engine --test row_autofit` +Expected: 3 passed. Also run `cargo test -p gridcraft-engine` (no regressions in style/undo behavior). + +- [ ] **Step 5: Commit** + +```bash +git add crates/engine/src/cmd/format.rs crates/engine/tests/row_autofit.rs +git commit -m "#833: whole-row/column formatting re-fits row heights" +``` + +### Task 2: Opened files grow non-custom rows to their tallest font + +**Files:** +- Modify: `crates/engine/src/io.rs:57-74` (`open_bytes_with_password`) and add `fn fit_imported_rows` next to it +- Test: `crates/engine/src/tests.rs` (extend, near the existing `file.open` tests around line 1004) + +**Interfaces:** +- Consumes: `crate::cmd::auto_row_height(wb: &mut Workbook, si: usize, row: u32)` (pub, cmd/mod.rs:458); `Workbook::sheets: Vec>` (pub field); `Sheet::cells.iter() -> impl Iterator` (model store.rs:89); `Sheet::rows: map row -> LineInfo { size: Option, custom: bool, hidden: bool, .. }`; model builders `Workbook::new()`, `wb.styles.derive(StyleId::default(), |s: &mut Style| s.font.size = 20.0) -> StyleId`, `sh.set_value(CellRef, Value)`, `sh.set_style(CellRef, StyleId)`; `gridcraft_xlsx::write_xlsx(&wb) -> Result>`; `crate::io::base64_encode(&[u8])`. +- Produces: `fn fit_imported_rows(wb: &mut Workbook)` — private to `io.rs`, called at both `open_plain` call sites in `open_bytes_with_password`. + +- [ ] **Step 1: Write the failing tests and fixture builder** in `crates/engine/src/tests.rs`. Fixture workbooks are built with the model API directly (never through commands, which would auto-fit them) and exported with `write_xlsx`, which omits `ht` for rows without a size — exactly the bug's input: + +```rust +fn big_font_workbook(row_info: Option<(f32, bool, bool)>) -> Vec { + // row_info = (size, custom, hidden) for row 5 (index 4), None = no row entry at all. + // Cell A5 = "plain big bold", whole-cell font 20pt. 11pt cell in A1 for contrast rows. + // Returns write_xlsx bytes. Uses Workbook::new, styles.derive, set_value, set_style. +} + +#[test] // A — the issue's repro +fn open_grows_default_height_rows_to_their_font() { + let mut s = Session::new(); + s.execute("file.open", json!({"name": "bigfont.xlsx", "base64": crate::io::base64_encode(&big_font_workbook(None))})).unwrap(); + let h = s.doc().unwrap().wb.active().unwrap().row_height(4); + assert!(h >= 30.0, "20pt cell must grow its default-height row on open, got {h}"); + assert_eq!(s.doc().unwrap().wb.active().unwrap().row_height(0), /* default height captured before open */, "11pt row stays default"); +} + +#[test] // B — Review Focus 1 +fn open_keeps_custom_height_rows() { + // fixture with row_info (25.0, true, false): open → row_height(4) == 25.0 +} + +#[test] // C — Review Focus 2 +fn open_never_shrinks_taller_auto_rows() { + // fixture with row_info (60.0, false, false) and an 11pt cell: open → row_height(4) == 60.0 +} + +#[test] // D — Review Focus 5 +fn open_fitting_keeps_hidden_rows_hidden() { + // fixture with row_info (20.0, false, true) and the 20pt cell: open → height grows or keeps 20.0, + // and rows.get(&4).hidden stays true +} + +#[test] // E — Review Focus 4 +fn open_leaves_plain_workbooks_untouched() { + // fixture with only 11pt cells, no row entries: open → sh.rows.get(&0) is None or has size == None +} +``` + +Comments carry each test's assertions where code is elided; fill them in fully when writing. + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cargo test -p gridcraft-engine open_` +Expected: A FAILS (row stays default — nothing fits on open); B and C may PASS (pin current behavior); D/E PASS or FAIL depending on current import behavior — record actual results in the commit-free state; only A's failure is required before implementing. + +- [ ] **Step 3: Implement `fit_imported_rows` in `crates/engine/src/io.rs` and call it from both `open_plain` sites in `open_bytes_with_password`** (the plain early-return at ~line 58-59 and the decrypted path at ~line 71; keep warnings untouched): + +```rust +/// Excel auto-fits rows to their tallest font when opening a file: rows without a user-set +/// height grow to fit their content. Rows the file made taller keep their height — our +/// line-height estimate can differ from Excel's, and shrinking files that already look +/// right would be a visible regression. Custom-height rows are left alone entirely. +fn fit_imported_rows(wb: &mut Workbook) { + for si in 0..wb.sheets.len() { + let Some(sh) = wb.sheet(si) else { continue }; + let mut rows: Vec = sh.cells.iter().map(|(c, _)| c.row).collect(); + rows.sort_unstable(); + rows.dedup(); + for row in rows { + let before = wb.sheet(si).and_then(|sh| sh.rows.get(&row)).and_then(|i| i.size); + crate::cmd::auto_row_height(wb, si, row); + let Some(before) = before else { continue }; + let after = wb.sheet(si).and_then(|sh| sh.rows.get(&row)).and_then(|i| i.size); + if after.is_none_or(|a| before > a + 0.5) { + if let Some(sh) = wb.sheet_mut(si) { + sh.rows.entry(row).or_default().size = Some(before); + } + } + } + } +} +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cargo test -p gridcraft-engine open_` then `cargo test -p gridcraft-engine` (all), plus a sample sanity check: `cargo test -p gridcraft-engine --test command_sequence_fuzz` (undo/round-trip/recalc invariants over opens). + +- [ ] **Step 5: Commit** + +```bash +git add crates/engine/src/io.rs crates/engine/src/tests.rs +git commit -m "#833: opened files grow non-custom rows to their tallest font" +``` + +### Task 3: Docs and tracking + +**Files:** +- Modify: `docs/target-app-parity.md:165` (the "Rows and columns … AutoFit" row) +- Modify: `docs/gaps.md` (revision history; check §24 for a stale row-height mention) +- Modify: `ROADMAP.md` (progress log + revision history + "Last updated" line) + +**Interfaces:** +- Consumes: merged state of Tasks 1-2. +- Produces: nothing (docs). + +- [ ] **Step 1: Update the three docs** + - `docs/target-app-parity.md:165`: append to the evidence column: `; rows auto-fit oversized fonts on open and row formatting (#833)`. + - `docs/gaps.md`: add revision-history row `| 2026-10-12 | minor | rows auto-fit oversized fonts on open and whole-row/column formatting (#833) |`. + - `ROADMAP.md`: progress-log row `| 2026-10-12 | Rows auto-fit oversized fonts: opening a file and whole-row/column formatting grow non-custom rows to their tallest font (#833) |`; matching revision-history row; bump the `**Last updated:**` date. + +- [ ] **Step 2: Verify docs build/lint state unchanged** + +Run: `cargo xtask ci` +Expected: green (fmt, clippy, tests, assets, layers, wasm). + +- [ ] **Step 3: Commit** + +```bash +git add docs/target-app-parity.md docs/gaps.md ROADMAP.md +git commit -m "#833: docs — track row auto-fit for oversized fonts" +``` + +--- + +**Post-PR (not a plan task):** comment on issue #833 linking the PR and noting the per-run font-size follow-up that lands after rich-text-runs PR #837 merges (per-run sizes are not measured by `auto_row_height` yet).