diff --git a/HANDOFF.md b/HANDOFF.md new file mode 100644 index 0000000000..fc4ddb9312 --- /dev/null +++ b/HANDOFF.md @@ -0,0 +1,38 @@ +# Morning Handoff + +## Finished + +- Reviewed #1999 and related issues #1458, #1848 and #1165. Their quoting, threshold and editor PRs do not fix malformed formula entry. +- Reproduced acceptance of invalid expression rules, comparison bounds and data-bar formula thresholds on upstream `23829d8`. +- Added entry validation using the existing parser, preserving workbook contents and undo/redo on rejection. +- Added four engine tests and one dialog test: errors retain the entered text; correcting it succeeds. Valid formulas and literal comparisons save and reopen. + +## Try It + +- Run `cargo +1.95.0 test -p gridcraft-engine --test conditional_formula_validation`. +- Run `cargo +1.95.0 test -p gridcraft-ui-egui --test conditional_formula_validation`. +- In Conditional Formatting → New Rule, enter `=A1>`; OK should show an error and keep the dialog open. Change it to `=A1>5` and confirm. + +## Checks + +- Baseline: three regression tests failed because malformed formulas were accepted; the valid-formula test passed. +- After the fix: all four engine tests and the dialog test passed. Full workspace: 1,665 passed, 0 failed, 1 ignored (`cargo +1.95.0 test --workspace`). Log: `/tmp/gridcraft-1999-workspace-tests.log`. +- Focused engine Clippy (`--all-targets -- -D warnings`), formatting, diff, assets, dependency layers, parity, keytips and WASM checks passed. WASM reports an unchanged dead-code warning in `backstage.rs`. +- `cargo +1.95.0 xtask ci` stopped on unchanged upstream `crates/ui-egui/src/desktop_theme.rs:36`: `clippy::let_unit_value` on macOS. + +## Decisions + +- Validate at rule creation before any workbook edit; retain the existing dialog error handling. +- Existing malformed imported rules are not repaired or discarded. Syntax validation does not establish general Excel compatibility. +- Keep #1822/#1885 integration separate: current main has no rule-edit command or configurable colour-scale formula stops. Their future builders must retain the same validation. + +## Remaining + +- Published [PR #2008](https://github.com/storytold/gridcraft/pull/2008) from `codex/validate-cf-formulas-1999`, based on upstream `23829d8`. GitHub CI and automated review were pending at publication. +- Shared `craftrules` instructions and local-only `plan/` files were unavailable; checked-in GridCraft instructions were followed. + +## Review First + +- `crates/engine/src/cmd/insert.rs`: validation before mutation. +- `crates/engine/tests/conditional_formula_validation.rs` and `crates/ui-egui/tests/conditional_formula_validation.rs`: rejection, preservation and correction. +- `docs/decisions.md`: related-PR boundaries and imported-file limitation. diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 9f044613dc..e9ac4b1dd1 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -1335,6 +1335,11 @@ fn preset_style(name: &str) -> Style { s } +fn cf_formula(value: String) -> Result { + gridcraft_formula::parse(&value).map_err(|e| bad("home.conditionalFormat", format!("invalid conditional-format formula: {e}")))?; + Ok(value) +} + fn add_cf(s: &mut Session, p: &Json) -> Result { let ranges = target_ranges(s, p)?; let sheet = target_sheet(s, p)?; @@ -1351,8 +1356,8 @@ fn add_cf(s: &mut Session, p: &Json) -> Result { _ => preset_style(g("preset").as_deref().unwrap_or("lightRedFillDarkRedText")), }); let kind = g("type").unwrap_or_else(|| "cellIs".into()); - let value_kind = |v: Option<&Json>, default: CfValueKind| -> CfValueKind { - match v { + let value_kind = |v: Option<&Json>, default: CfValueKind| -> Result { + Ok(match v { Some(Json::Object(o)) => { let t = o.get("type").and_then(Json::as_str).unwrap_or("num"); let val = o.get("value").and_then(Json::as_f64).unwrap_or(0.0); @@ -1361,12 +1366,12 @@ fn add_cf(s: &mut Session, p: &Json) -> Result { "max" => CfValueKind::Max, "percent" => CfValueKind::Percent(val), "percentile" => CfValueKind::Percentile(val), - "formula" => CfValueKind::Formula(o.get("value").and_then(Json::as_str).unwrap_or("").into()), + "formula" => CfValueKind::Formula(cf_formula(o.get("value").and_then(Json::as_str).unwrap_or("").into())?), _ => CfValueKind::Number(val), } } _ => default, - } + }) }; let rule = match kind.as_str() { "cellIs" => { @@ -1391,9 +1396,9 @@ fn add_cf(s: &mut Session, p: &Json) -> Result { format!("\"{}\"", v.replace('"', "\"\"")) } }; - CfRule::CellIs { op, a: quote(a), b: b.map(quote), style } + CfRule::CellIs { op, a: cf_formula(quote(a))?, b: b.map(|v| cf_formula(quote(v))).transpose()?, style } } - "expression" => CfRule::Expression { formula: g("formula").unwrap_or_default().trim_start_matches('=').to_string(), style }, + "expression" => CfRule::Expression { formula: cf_formula(g("formula").unwrap_or_default().trim_start_matches('=').to_string())?, style }, "containsText" => CfRule::ContainsText { text: g("text").unwrap_or_default(), style }, "notContainsText" => CfRule::NotContainsText { text: g("text").unwrap_or_default(), style }, "beginsWith" => CfRule::BeginsWith { text: g("text").unwrap_or_default(), style }, @@ -1429,8 +1434,8 @@ fn add_cf(s: &mut Session, p: &Json) -> Result { CfRule::ColorScale { stops } } "dataBar" => CfRule::DataBar { - min: value_kind(rule.get("min"), CfValueKind::Min), - max: value_kind(rule.get("max"), CfValueKind::Max), + min: value_kind(rule.get("min"), CfValueKind::Min)?, + max: value_kind(rule.get("max"), CfValueKind::Max)?, color: super::format::color_param(rule.get("color")).unwrap_or(Color::rgb(0x63, 0x8E, 0xC6)), gradient: rule.get("gradient").and_then(Json::as_bool).unwrap_or(true), show_value: true, diff --git a/crates/engine/tests/conditional_formula_validation.rs b/crates/engine/tests/conditional_formula_validation.rs new file mode 100644 index 0000000000..ff647c078b --- /dev/null +++ b/crates/engine/tests/conditional_formula_validation.rs @@ -0,0 +1,89 @@ +//! Malformed conditional-format formulas must be refused before they reach a saved workbook (#1999). + +use gridcraft_engine::Session; +use serde_json::{Value, json}; + +fn session() -> Session { + let mut s = Session::new(); + s.new_workbook(); + s.execute("home.conditionalFormat", json!({"range": "A1:A3", "rule": {"type": "expression", "formula": "A1>5"}})).unwrap(); + // A rejected rule must preserve both existing rules and a pending redo. + s.execute("cell.set", json!({"cell": "B1", "input": "7"})).unwrap(); + s.execute("edit.undo", json!({})).unwrap(); + s +} + +fn rejects_without_changes(rule: Value) { + let mut s = session(); + let before = serde_json::to_value(&s.doc().unwrap().wb).unwrap(); + let history = (s.doc().unwrap().undo.len(), s.doc().unwrap().redo.len()); + let error = s.execute("home.conditionalFormat", json!({"range": "A1:A3", "rule": rule})).expect_err("malformed formula was accepted"); + assert!(error.to_string().contains("formula"), "{error}"); + assert_eq!(serde_json::to_value(&s.doc().unwrap().wb).unwrap(), before); + assert_eq!((s.doc().unwrap().undo.len(), s.doc().unwrap().redo.len()), history); +} + +#[test] +fn invalid_expression_is_refused_without_changing_rules_or_history() { + for formula in ["=A1>", "=1+", "", " "] { + rejects_without_changes(json!({"type": "expression", "formula": formula})); + } +} + +#[test] +fn invalid_cell_comparison_bounds_are_refused() { + rejects_without_changes(json!({"type": "cellIs", "operator": "greaterThan", "value": "=1+"})); + rejects_without_changes(json!({"type": "cellIs", "operator": "between", "value": 1, "value2": "=A1>"})); +} + +#[test] +fn invalid_data_bar_formula_bounds_are_refused() { + for bound in ["min", "max"] { + let mut rule = json!({"type": "dataBar"}); + rule[bound] = json!({"type": "formula", "value": "1+"}); + rejects_without_changes(rule); + } +} + +#[test] +fn valid_formulas_and_literal_comparisons_still_save_and_reopen() { + let mut s = Session::new(); + s.new_workbook(); + for rule in [ + json!({"type": "expression", "formula": "A1>5"}), + json!({"type": "expression", "formula": "=AND(A1>0,A1<10)"}), + json!({"type": "expression", "formula": "=FutureFunction(A1)"}), + json!({"type": "expression", "formula": "=#REF!"}), + json!({"type": "cellIs", "operator": "between", "value": "=$Z$1", "value2": "=MAX($Z$2,10)"}), + json!({"type": "cellIs", "operator": "equal", "value": "1+"}), + json!({"type": "cellIs", "operator": "equal", "value": 5}), + json!({"type": "dataBar", "min": {"type": "formula", "value": "$Z$1"}, "max": {"type": "formula", "value": "MAX($Z$2,10)"}}), + ] { + s.execute("home.conditionalFormat", json!({"range": "A1:A3", "rule": rule})).unwrap(); + } + let formulas = |s: &Session| { + use gridcraft_engine::model::{CfRule, CfValueKind}; + s.doc() + .unwrap() + .wb + .active() + .unwrap() + .cond_formats + .iter() + .map(|cf| { + let values = match &cf.rule { + CfRule::Expression { formula, .. } => vec![formula.as_str()], + CfRule::CellIs { a, b, .. } => std::iter::once(a.as_str()).chain(b.as_deref()).collect(), + CfRule::DataBar { min: CfValueKind::Formula(a), max: CfValueKind::Formula(b), .. } => vec![a.as_str(), b.as_str()], + _ => panic!("unexpected rule"), + }; + values.into_iter().map(|v| gridcraft_formula::print(&gridcraft_formula::parse(v).unwrap())).collect::>() + }) + .collect::>() + }; + let expected = formulas(&s); + let saved = s.execute("file.saveBytes", json!({"format": "xlsx"})).unwrap(); + let mut reopened = Session::new(); + reopened.execute("file.open", json!({"name": "rules.xlsx", "base64": saved["base64"]})).unwrap(); + assert_eq!(formulas(&reopened), expected); +} diff --git a/crates/ui-egui/tests/conditional_formula_validation.rs b/crates/ui-egui/tests/conditional_formula_validation.rs new file mode 100644 index 0000000000..3a90ad103e --- /dev/null +++ b/crates/ui-egui/tests/conditional_formula_validation.rs @@ -0,0 +1,38 @@ +//! A rejected conditional-format formula stays available for correction (#1999). + +use egui_kittest::kittest::Queryable as _; +use gridcraft_engine::Session; +use gridcraft_ui_egui::SheetApp; +use serde_json::json; + +#[test] +fn invalid_formula_keeps_the_dialog_open_until_corrected() { + let mut session = Session::new(); + session.new_workbook(); + let mut app = SheetApp::new(session, Default::default()); + app.open_dialog("cfQuick", json!({"type": "expression"})); + app.dialog.as_mut().unwrap().values.insert("formula".into(), json!("=A1>")); + let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_ui_state( + |ui, app: &mut SheetApp| { + let ctx = ui.ctx().clone(); + app.logic(&ctx); + app.ui(ui); + }, + app, + ); + h.run_steps(4); + h.get_by_label(" OK ").click(); + h.run_steps(3); + assert!(h.state().session.doc().unwrap().wb.active().unwrap().cond_formats.is_empty()); + assert_eq!(h.state().dialog.as_ref().unwrap().values.get("formula"), Some(&json!("=A1>"))); + assert!(h.state().message.as_ref().unwrap().1.contains("formula")); + + h.state_mut().message = None; + h.state_mut().dialog.as_mut().unwrap().values.insert("formula".into(), json!("=A1>5")); + h.run_steps(3); + h.get_by_label(" OK ").click(); + h.run_steps(3); + assert!(h.state().dialog.is_none()); + assert!(h.state().message.is_none()); + assert_eq!(h.state().session.doc().unwrap().wb.active().unwrap().cond_formats.len(), 1); +} diff --git a/docs/decisions.md b/docs/decisions.md index 7b0a99a384..3643709fbe 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1,5 +1,19 @@ # Decisions +## Validate conditional-format formulas before creating rules (#1999) + +Use the existing formula parser before `home.conditionalFormat` changes the workbook. Reject +malformed expression rules, either comparison bound, and either data-bar formula threshold; +keep literal comparisons, syntactically valid unknown functions and error references valid. +The existing dialog error path keeps the entered formula available for correction. This is +entry validation, not evaluation or a repair pass over imported workbooks. + +The related work remains separate: #1458 fixes sheet-name quoting; #1848/#1885 change threshold +types and rendering; #1165/#1822 add the rule editor. Current main's `home.manageRules` only +lists, deletes and reorders rules. When #1822 adds editing and colour-scale formula stops, its +shared rule builder must retain this validation; #1885's new formula-bearing threshold types +also need the same check. + ## Formula localization uses the existing interface preference PR #98 builds on the persisted `UiState.language` introduced by #31 and extended by #104. @@ -70,4 +84,4 @@ to 115% or 100%. That's a large relative step at low zoom and a small one near 4 relative steps keep each notch feeling the same, and the doublings land on 25, 50, 100, 200 and 400%. The status bar's Zoom In / Zoom Out buttons and Ctrl+Alt+= / Ctrl+Alt+- keep their 10% steps, so the wheel and the buttons now step differently. Whether they should share one ladder -(this one, Excel's, or another) is left to the maintainers. \ No newline at end of file +(this one, Excel's, or another) is left to the maintainers. diff --git a/docs/file-format-parity.md b/docs/file-format-parity.md index 5a75889863..9b2cf51f5a 100644 --- a/docs/file-format-parity.md +++ b/docs/file-format-parity.md @@ -1,6 +1,6 @@ # File-format parity -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-19 · **Change:** minor (.xls dialog sheets are skipped instead of read as worksheets; `.xls` import reads each sheet's display options from WINDOW2: right-to-left, gridlines, headings, zeros, formulas; XLSX text boxes and shapes keep their paragraphs' right-to-left direction on save; CSV/TSV/TXT can open with every field kept as text via `file.open {csvAsText}`; formulas over Excel's 8,192 characters, and `%` or call chains past the nesting limit, are parse errors instead of stack overflows when a file is opened; XLSX PivotTables read and write `rowGrandTotals`/`colGrandTotals` the way Excel does, so a table saved with only one grand total opens with the right one; HTML export keeps right-to-left) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-19 · **Change:** minor (conditional-format entry refuses malformed formulas before saving, #1999; .xls dialog sheets are skipped instead of read as worksheets; `.xls` import reads each sheet's display options from WINDOW2: right-to-left, gridlines, headings, zeros, formulas; XLSX text boxes and shapes keep their paragraphs' right-to-left direction on save; CSV/TSV/TXT can open with every field kept as text via `file.open {csvAsText}`; formulas over Excel's 8,192 characters, and `%` or call chains past the nesting limit, are parse errors instead of stack overflows when a file is opened; XLSX PivotTables read and write `rowGrandTotals`/`colGrandTotals` the way Excel does, so a table saved with only one grand total opens with the right one; HTML export keeps right-to-left) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Every format Excel opens or saves, with GridCraft's read and write support, fidelity and tests. Part of [target-app-parity.md](target-app-parity.md). **File formats: ~50% ready, 80–120 h** @@ -13,6 +13,12 @@ support comes from `crates/engine/src/io.rs` (`FileKind`), `crates/xlsx/src` and ## Workbook formats +Conditional-format entry rejects malformed expression rules, comparison formulas and data-bar +formula thresholds before they can be saved to XLSX (#1999). Rejection preserves existing rules +and undo/redo history; valid formulas and literal comparisons are covered by save/reopen tests. +This does not repair malformed rules already present in imported files or establish general +Excel compatibility for every formula the parser accepts. + Ink created with Draw is written as standard, unfilled DrawingML polylines, without visible point captions or private metadata. Supported numeric polylines reopen as editable ink, retaining points, colour, integer width and anchor. Curves, filled paths, transforms and unsupported line styles are @@ -85,6 +91,7 @@ refresh. Counted under features (Get & Transform, ~10% ready), not here. | Date | Change | Summary | |---|---|---| +| 2026-10-19 | minor | Conditional-format command validates expression, comparison and data-bar formulas before changing the workbook (#1999); imported-rule repair remains out of scope | | 2026-10-19 | minor | .xls import skips dialog sheets (WsBool `fDialog`), which share the worksheet BOF type and were read as empty worksheets | | 2026-10-19 | minor | `.xls` import reads WINDOW2: right-to-left sheets (common in older Arabic and Hebrew files) open right-to-left, and hidden gridlines, headings and zero values and Show Formulas carry over | | 2026-10-19 | trivial | XLSX text boxes and shapes keep right-to-left paragraph direction (`a:pPr rtl`, ECMA-376 Part 1 §21.1.2.2.7): read from the first paragraph and written on every paragraph; output without it is unchanged |