Fix template calcChain regeneration and $= formula cells (MiniExcel.OpenXml) - #993
Open
martin-vitous wants to merge 1 commit into
Open
Conversation
…cel.OpenXml) Porting the v1.x template-render fixes to the v2 OpenXml line, where the engine moved to src/MiniExcel.OpenXml/ and a LINQ-to-XML rewrite. Two defects made Excel reject an otherwise-valid rendered package: - $= formula cells kept their source t="inlineStr" attribute after the inline string was replaced with an <f> child — a cell typed inlineStr with no <is> is schema-invalid. The attribute is now removed. - The regenerated calcChain entry took the cell address from the running child-index, which is wrong for sparse rows (cells with no content are not emitted, so position != column). The address is now read from the cell's own r attribute. - The template's own calcChain was reused/copied even though row insertion shifts every formula cell, leaving stale entries; and an empty calcChain (no rendered formulas) was written, which is schema-invalid. The chain is now regenerated only when formulas were rendered and dropped entirely otherwise — Excel rebuilds it on open. The <f> namespace fix from the v1 change is not needed here: v2's XElement-based serialization already emits it in the spreadsheetml namespace.
📝 WalkthroughWalkthroughInline-string formula conversion now produces valid formula cells and derives references from cell addresses. Template saving no longer copies stale ChangesCalcChain Formula Updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/MiniExcel.OpenXml/Templates/OpenXmlTemplate.Impl.cs`:
- Around line 943-949: Update the cell reference assignment near cellRef so
calcChain entries use the complete c@r value directly whenever it is present,
rather than reconstructing it with rowIndex or the running index. Preserve the
existing coordinate fallback only when c@r is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4ade420-4e26-4481-b2b7-bd2137761a9f
📒 Files selected for processing (2)
src/MiniExcel.OpenXml/Templates/OpenXmlTemplate.Impl.cssrc/MiniExcel.OpenXml/Templates/OpenXmlTemplate.cs
Comment on lines
+943
to
+949
| // take the column from the cell's own reference — the running index is wrong for | ||
| // sparse rows (cells without content are not emitted, so position != column) | ||
| var cellRef = cell.Attribute("r")?.Value; | ||
| var columnLetters = cellRef is null ? "" : new string(cellRef.TakeWhile(char.IsLetter).ToArray()); | ||
| var celRef = string.IsNullOrEmpty(columnLetters) | ||
| ? CellReferenceConverter.GetCellFromCoordinates(index, rowIndex) | ||
| : $"{columnLetters}{rowIndex}"; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the complete cell address for calcChain entries.
rowIndex can differ from the rendered cell’s row in enumerable/merged rows, so this produces a chain entry for the wrong cell. Use c@r directly when present.
Proposed fix
var cellRef = cell.Attribute("r")?.Value;
-var columnLetters = cellRef is null ? "" : new string(cellRef.TakeWhile(char.IsLetter).ToArray());
-var celRef = string.IsNullOrEmpty(columnLetters)
+var celRef = string.IsNullOrEmpty(cellRef)
? CellReferenceConverter.GetCellFromCoordinates(index, rowIndex)
- : $"{columnLetters}{rowIndex}";
+ : cellRef;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // take the column from the cell's own reference — the running index is wrong for | |
| // sparse rows (cells without content are not emitted, so position != column) | |
| var cellRef = cell.Attribute("r")?.Value; | |
| var columnLetters = cellRef is null ? "" : new string(cellRef.TakeWhile(char.IsLetter).ToArray()); | |
| var celRef = string.IsNullOrEmpty(columnLetters) | |
| ? CellReferenceConverter.GetCellFromCoordinates(index, rowIndex) | |
| : $"{columnLetters}{rowIndex}"; | |
| // take the column from the cell's own reference — the running index is wrong for | |
| // sparse rows (cells without content are not emitted, so position != column) | |
| var cellRef = cell.Attribute("r")?.Value; | |
| var celRef = string.IsNullOrEmpty(cellRef) | |
| ? CellReferenceConverter.GetCellFromCoordinates(index, rowIndex) | |
| : cellRef; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/MiniExcel.OpenXml/Templates/OpenXmlTemplate.Impl.cs` around lines 943 -
949, Update the cell reference assignment near cellRef so calcChain entries use
the complete c@r value directly whenever it is present, rather than
reconstructing it with rowIndex or the running index. Preserve the existing
coordinate fallback only when c@r is missing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ports the v1.x template-render fixes to the v2
MiniExcel.OpenXmlline (the engine moved tosrc/MiniExcel.OpenXml/with a LINQ-to-XML rewrite).Two defects made Excel reject an otherwise-valid rendered package:
$=formula cells kept their sourcet="inlineStr"attribute after the inline string was replaced with an<f>child. A cell typedinlineStrwith no<is>is schema-invalid. The attribute is now removed.rattribute.The
<f>namespace fix from the v1 change is not needed here: v2'sXElement-based serialization already emits it in the spreadsheetml namespace.Existing
MiniExcelTemplateTests/MiniExcelTemplateAsyncTestsstay green on net8/9/10.Summary by CodeRabbit