Skip to content

fix #1458: one Excel sheet-name quoting rule for formulas, pivot sources and ADDRESS - #1529

Merged
aiongg merged 3 commits into
storytold:mainfrom
HyakoV3:fix/1458-sheet-quoting
Oct 22, 2026
Merged

aiongg merged 3 commits into
storytold:mainfrom
HyakoV3:fix/1458-sheet-quoting

Conversation

@HyakoV3

@HyakoV3 HyakoV3 commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1458.

Excel quotes a sheet name when it starts with a digit or ., holds anything but letters/digits/_/., reads as an A1 cell within the sheet or as an R1C1 reference, or is a boolean (ADDRESS leaves booleans bare). GridCraft had several copies of this rule, none matching.

  • gridcraft_core::addr: sheet_needs_quotes, quote_sheet, unquote_sheet, split_sheet (L0, usable by every crate).
  • Formula printer re-exports them (gridcraft_formula::quote_sheet / needs_quotes keep working), so '.x'!A1 is no longer saved as .x!A1.
  • PivotTable sources (and Solver, which shares them), ADDRESS, Go To, chart, xlsx and calc unquoting use the core helpers; the Name Box now reads '' as '.
  • Tests: formula round trip (sheet_names_excel_quotes_stay_quoted), core table of names, ADDRESS cases, engine round trip through a saved XLSX plus pivot source for a sheet named A1, Name Box on a sheet Bob's.

cargo xtask ci passes. Not covered: #829's external-workbook prefix in needs_quotes (its wrapper can call the core function), and #904 (CELL("address")).

Fixes #1458, #2009, #2010, #2011, #2012, #2013

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of worksheet names containing apostrophes, spaces, punctuation, or names resembling cell references and boolean values.
    • Excel-compatible quoting is now preserved in formulas, named ranges, chart links, PivotTable sources, and XLSX files.
    • Corrected display and resolution of named ranges on specially named worksheets.
  • Documentation

    • Updated file-format and function compatibility documentation with worksheet-name quoting behavior.
  • Tests

    • Added coverage for quoting, apostrophes, formula persistence, XLSX round trips, and Name Box display.

HyakoV3 and others added 3 commits October 19, 2026 17:36
…ivot sources and ADDRESS

Sheet names starting with '.' or reading as R1C1 (R, C, RC, R1C1...) were saved
unquoted, so Excel could not open the file; PivotTable sources and ADDRESS left
cell-like names (A1) bare. The quoting rule, unquoting and sheet splitting now
live once in gridcraft_core::addr and replace the copies in the formula printer,
pivot, ADDRESS, chart, xlsx, Solver, Go To and the Name Box (which now reads ''
as ').

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…rytold#1458)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 51b367f5-35fb-480d-b0d3-d0c39be79465
📥 Commits

Reviewing files that changed from the base of the PR and between 23829d8 and 52ab0e6.

📒 Files selected for processing: 18
  • crates/calc/src/eval.rs
  • crates/chart/src/resolve.rs
  • crates/core/src/addr.rs
  • crates/core/src/lib.rs
  • crates/engine/src/cmd/edit.rs
  • crates/engine/src/cmd/mod.rs
  • crates/engine/src/cmd/pivot.rs
  • crates/engine/src/pivot.rs
  • crates/engine/tests/sheet_name_quoting.rs
  • crates/formula/src/printer.rs
  • crates/formula/src/tests.rs
  • crates/functions/src/lookup.rs
  • crates/ui-egui/src/lib.rs
  • crates/ui-egui/tests/interaction.rs
  • crates/xlsx/src/array_formula.rs
  • crates/xlsx/src/pivot.rs
  • docs/file-format-parity.md
  • docs/function-parity.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The change centralizes Excel-compatible sheet-name quoting, unquoting, and reference splitting in gridcraft_core. Formula printing, ADDRESS, engine reference formatting, and sheet-reference resolution use the shared utilities. Tests cover quoting rules, apostrophe escaping, formula serialization, PivotTable references, and Name Box display.

Changes: Sheet reference quoting

Layer / File(s) Summary
Shared sheet-name rules
crates/core/src/addr.rs, crates/core/src/lib.rs, crates/formula/src/printer.rs, crates/formula/src/tests.rs, crates/functions/src/lookup.rs, docs/function-parity.md
Core adds shared quote detection, quoting, unquoting, and reference-splitting utilities. Formula printing and ADDRESS use the shared quoting rules, with tests and documentation covering the behavior.
Engine reference formatting
crates/engine/src/cmd/edit.rs, crates/engine/src/cmd/mod.rs, crates/engine/src/pivot.rs, crates/engine/tests/sheet_name_quoting.rs, docs/file-format-parity.md
Engine table and pivot references use the shared formatting and splitting utilities. Tests cover quoted formula references through XLSX save and reopen, and quoting cell-like sheet names in ADDRESS and PivotTable sources.
Sheet reference resolution
crates/calc/src/eval.rs, crates/chart/src/resolve.rs, crates/engine/src/cmd/pivot.rs, crates/engine/src/pivot.rs, crates/ui-egui/src/lib.rs, crates/ui-egui/tests/interaction.rs, crates/xlsx/src/array_formula.rs, crates/xlsx/src/pivot.rs
Calc, chart, engine, XLSX, and UI code use shared utilities to parse or unquote sheet references. The UI interaction test covers a defined name on a sheet containing an apostrophe.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: echelon, neilcawse

Merge Risk: ⚪ Minimal · up to 52ab0

This change makes Excel sheet-name quoting consistent across formulas, ADDRESS, pivots and the Name Box. No merge-blocking issue was identified.

Security architecture risk: 🔵 Low · up to 52ab0

The change consolidates reference handling without an identified new authority bypass. Examined consumers retain workbook-local lookup and output escaping. Residual risk is concentrated in compatibility across the many consumers of the shared parsing rules.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — For the examined calculation, chart, navigation, and pivot-export paths, changed name decoding affects references within the supplied workbook. These paths do not demonstrate a new cross-workbook or privileged-service capability.

Security Findings and Attack Paths

  • inferred — No introduced authority-bypass or XML-injection path was established in the inspected consumers: decoded names still pass workbook lookup, emitted sheet references double apostrophes, and XLSX output retains attribute escaping. This conclusion does not establish exhaustive security coverage.

Trust Boundaries and Controls

  • observed — Navigation retains workbook-local sheet resolution. Direct go_to ranges are parsed before selection handling, and nonexistent qualified sheets fail before switching sheets. Generated table targets use an existing workbook table’s range.

Resilience and Maintainability Implications

  • observed — The examined XLSX pivot-source resolver retains its recursion-depth bound and fallible range and sheet validation while adopting the shared splitter.
Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 73.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 16 files. (2 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly summarizes the main change: centralized Excel sheet-name quoting for formulas, PivotTable sources, and ADDRESS. It is specific and related to the changeset.
Linked Issues check Passed The PR implements the coding requirements in #1458. crates/core/src/addr.rs provides shared Excel-compatible quoting, unquoting, and reference splitting. Formula printing re-exports the shared helpe…
Out of Scope Changes check Passed The changes stay within #1458. The source changes centralize and apply sheet-reference handling across the listed formula, engine, function, chart, UI, and XLSX paths. The added tests and parity-docum…
Full details: Linked Issues check

Explanation

The PR implements the coding requirements in #1458. crates/core/src/addr.rs provides shared Excel-compatible quoting, unquoting, and reference splitting. Formula printing re-exports the shared helpers. The engine, Solver-related pivot paths, ADDRESS, Go To, chart, XLSX, calc, and Name Box paths use the shared behavior. Tests cover core rules, formula and XLSX round trips, ADDRESS, PivotTable sources, and apostrophes in Name Box sheet names.

Full details: Out of Scope Changes check

Explanation

The changes stay within #1458. The source changes centralize and apply sheet-reference handling across the listed formula, engine, function, chart, UI, and XLSX paths. The added tests and parity-document updates support the same behavior. The PR explicitly excludes external-workbook prefix handling from #829 and CELL("address") from #904.

Full details: Docstring Coverage

Explanation

Docstring coverage is 73.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 16 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@aiongg
aiongg merged commit 69f6ea3 into storytold:main Oct 22, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sheet references: names like .x, R1C1 or A1 are left unquoted; Excel can't open the saved file

2 participants