CI failed with cargo clippy --workspace --all-targets -- -D warnings,
not a test. The actual cause: I'd been validating locally against
rustc/clippy 1.95.0 (the toolchain this machine had installed before
now), while CI installs plain "stable" (1.99.0 as of this run) — two
lints new since 1.95.0 fired there and nowhere in my local checks:
- clippy::int_plus_one on `src.end.row + 1 <= cap` in grid.rs,
simplified to `src.end.row < cap` (same meaning, no overflow risk
change: row is a u32 and cap is already saturating-clamped below
MAX_ROWS - 1, so src.end.row + 1 can't wrap).
- clippy::disallowed_methods on the new fill_handle_perf example's
three Instant::now() calls — the project's own lint from the time-
panic fix (#125 history): reading the clock traps on wasm. This
example is native-only (a `cargo run --release` benchmark, never
built for wasm), so it gets the same #![allow(...)] perf.rs already
carries, with the same justification.
Updated the local toolchain to stable (rustup update stable) and
re-ran cargo clippy --workspace --all-targets -- -D warnings clean
before this push, to actually match what CI checks instead of
guessing again.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cargo fmt reformatted the new example's while-loop condition onto its
own line; clippy's nonminimal_bool caught a negated is_some_and that
reads more plainly as is_none_or.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follow-up to #210, which the reviewer correctly rejected: the stated
cause (a sparse column with a stray distant value) doesn't hold, since
the scan already stops at the first empty cell. This is a different,
narrower fix that doesn't change behaviour for any input — it never
truncates a long fill, unlike #210's cap — only how fast a genuinely
long, densely-populated run is scanned.
grid.rs's fill-handle double-click called Sheet::value_ref once per
row to find where the neighbouring column's data ends. Each call does
a band lookup + row lookup + column lookup (three BTreeMap probes) via
CellStore::get. For a real dense run (e.g. a filled column near the
1,048,576-row sheet limit) that's a lot of point lookups on the UI
thread in one frame: ~59ms for a 1,000,000-row run in a release build
(measured with the new fill_handle_perf example), likely worse under
wasm where GridCraft actually runs in the browser.
- crates/model/src/store.rs: CellStore::contiguous_value_run walks
each band's row BTreeMap with `range` (one in-order pass per band)
instead of one `get` per row, so a dense run costs roughly one
tree-traversal step per row instead of one binary search per row.
Deliberately narrower than `has`/`get`: a stored cell always decides
the row on its own (even with an empty value), matching what
`value_ref`'s callers actually test for, not `has`'s "formula counts
regardless of its cached value" (used for Ctrl+arrow navigation).
- crates/model/src/sheet.rs: Sheet::last_contiguous_nonempty layers
`spill` back in wherever contiguous_value_run finds no stored cell,
exactly where plain `value_ref` would fall through to it.
- crates/ui-egui/src/grid.rs: the fill-handle double-click now calls
this once instead of looping over value_ref.
Result: 999,999-row dense run, 59.2ms -> 24.7ms (2.40x), same answer
(asserted in the example). Four new tests in sheet.rs, including a
300-trial property test against a naive value_ref-loop reference
(spills, formula-with-empty-value, gaps, blanks) — all pass, plus the
full existing suite (`cargo test -p gridcraft-model -p gridcraft-ui-egui
-p gridcraft-engine`; the only failures are #210-era gridcraft-engine
CSV tests already failing on main, unrelated to this change).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
