Skip to content

feat(editor): open .xlsx and .xlsm workbooks in a sheet viewer - #13532

Open
SrAlvarado wants to merge 24 commits into
stablyai:mainfrom
SrAlvarado:feat/xlsx-spreadsheet-viewer
Open

feat(editor): open .xlsx and .xlsm workbooks in a sheet viewer#13532
SrAlvarado wants to merge 24 commits into
stablyai:mainfrom
SrAlvarado:feat/xlsx-spreadsheet-viewer

Conversation

@SrAlvarado

@SrAlvarado SrAlvarado commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Summary

Opening an .xlsx file in Orca showed "Binary file — cannot display". The read path had no mime type for workbooks, so they fell through to the generic binary placeholder — even though the editor already renders CSV/TSV as a table.

Workbooks now open read-only in the editor: one tab per worksheet, on a sheet surface that follows Excel and Google Sheets, with the layout and formatting the file itself declares. .xlsm is included because it is the same OOXML container (macros are never read or evaluated).

No new dependency. The parser is hand-rolled, the same trade-off csv-parse.ts already documents for CSV instead of papaparse. A workbook is a zip of XML parts, and a viewer only needs a few of them — a small, fully specified slice. Inflating goes through the platform's DecompressionStream('deflate-raw'), so there is no new package and no native code.

What it reads from the file
Values shared, inline and rich text (phonetic <rPh> runs dropped); booleans; cached formula results and error codes; numbers as stored
Number formats currency, grouping, decimals, percent, trailing-comma scaling, the four sections with their colours, [$€-2] symbols, accounting codes, and the numeric built-in ids Excel never writes out
Dates built-in and custom date codes, the 1900 phantom leap day, the 1904 system, elapsed-duration codes left as numbers
Cell styling solid fills, font colour, size, bold, italic, per-edge borders with weights and dash styles
Layout merged ranges, author-set column widths and row heights, horizontal and vertical alignment, indent levels, wrapped text
Conditional formatting cellIs comparisons, the blank tests and the text tests, resolved against <dxfs> and applied in priority order with stopIfTrue
Drawings anchored images, inlined as data URLs over their cell range
Charts column, bar, line, area, scatter, pie and doughnut, drawn as inline SVG from the series the file caches, including the plots a chart overlays in one frame
Sparklines in-cell micro-charts a Sheets export left as formula text (see Notes)

Colours resolve from rgb, theme (against theme1.xml, whose first two index pairs are swapped relative to clrScheme order) and the legacy indexed palette, with tint applied in luminance space.

Both axes are virtualized, so a sheet reporting thousands of columns or hundreds of thousands of rows stays responsive. Zoom follows editorFontZoomLevel, the same store value Monaco and the notebook viewer use, so the existing editor shortcuts work on a sheet.

Columns and rows are both resizable by dragging the grip on a heading's trailing edge, with double-click to size from content again; the grip is focusable and takes arrow keys, so the sizes are reachable without a pointer. A dragged size is stored unzoomed, so it keeps its proportion when the reader then zooms. Rows are bounded tighter than columns — 12 to 400 pixels — because a sheet has far more of them and a runaway row hides every one after it.

A row the file leaves without a customHeight is auto-fitted from the largest font in it, since those are the rows Excel measures itself and one shared default clipped exactly the ones a heading made taller. A row the file does size keeps that size, clipping included — that height is the author's decision, and Excel honours it the same way, which is why the grip exists.

Text is clipped where a spreadsheet clips it, and not before. An overflowing label runs over a neighbour that is merely filled, because that is what Excel does; only content stops it, plus a cell belonging to a merge, which this row reads as empty because the value lives in the anchor. It spills the way its alignment points — right-aligned text leftwards, centred text both ways — and a merge spills past its own edge into the empty columns after it, which is how a wide title fits a merge narrower than itself.

A merge that spans rows is a harder case, and the two obvious fixes are both wrong. Letting the anchor cell take the merge's full height makes it overflow into the rows below; because each virtualized row carries a transform and is therefore its own stacking context, whichever row wins the paint order hides the other's row number, and lifting the anchor row only swaps which numbers vanish. So the value goes to the overlay layer instead, which is where anything taller than one row already belonged — the cells underneath keep painting the band's fill and borders, no row overflows, and nothing needs lifting. The anchor cell carries the value as its aria-label, since the text itself now sits outside the table, and the overlay copy is aria-hidden so it is announced once.

A chart's overlaid plots are all drawn. Excel builds a target line by laying a scatterChart over an areaChart in one plot area, so the mark type belongs to the series rather than to the chart. Three things a scatter series needs that no other does: its values live in c:yVal, its x positions in c:xVal — two points meant to span the plot are otherwise squeezed into the first category step — and c:scatterStyle plus c:symbol decide whether it is a line, bare points, or both. A target line is the case where all three matter at once.

What it deliberately does not read. expression conditional rules need a formula evaluator, and colorScale, dataBar and iconSet render a scale rather than a differential format — all four are dropped so the cell keeps its own style rather than being painted on a guess. Also absent: data validation dropdowns, text rotation, and mixed-format runs within one cell.

Judgement calls worth a reviewer's attention

The sheet canvas is light in both themes. A workbook's fills are authored against a white sheet, so on a dark canvas they read as arbitrary blocks of colour rather than as the highlighting the author meant — which is why Excel and Sheets both keep a white sheet inside dark application chrome. The values live in one token family in main.css, so a dark-sheet variant is a six-value change if you would rather have one.

Cell ink is derived, not copied. The workbook's font colour wins when it clears WCAG AA against its own fill, using the large-text ratio of 3:1 once the text is 18pt or 14pt bold — otherwise a deliberate accent colour gets repainted, which is a real bug I hit on a real file. When the declared colour fails, the ink falls back to whichever of black or white contrasts more. On an unfilled cell the font colour is left alone entirely, because the background is whatever the theme uses and contrast cannot be checked; that loses colour on unfilled text, and I took an unreadable cell to be worse than a monochrome one.

Cells sit at the bottom of their row by default. That is what Excel and Sheets do when the author sets no vertical alignment, and one real stylesheet set vertical on 131 of its cells, so honouring it changes far more of a sheet's look than the count of lines suggests. CsvViewer keeps centring, since a CSV has no such default to be faithful to.

Numeric alignment is inferred from the rendered text. The grid only ever receives strings, so a right-aligned column — the main cue that a column is numeric — has to be recognised from the formatted value. That means the inference has to accept grouped thousands and currency symbols (1.234,50 €), which it previously did not, left-aligning whole currency columns as though they were text. It also means text that merely looks numeric can be right-aligned; the alternative is threading the cell type through, which is the better fix and a larger one.

spreadsheet-cell-contrast.ts holds two hex constants (#000000 / #ffffff). STYLEGUIDE says not to hardcode a hex where a variable covers it — no token covers "maximal-contrast ink over an arbitrary colour from a data file", which is why they are there rather than in main.css.

Structure

Module Responsibility
xlsx-zip-archive.ts OPC/zip container: central directory, lazy per-part inflate with a bounded output
xlsx-xml-elements.ts quote-aware forward-only tag scanner, entity decoding
xlsx-part-paths.ts OPC relationship target resolution
xlsx-color.ts, xlsx-theme-palette.ts colour resolution and the theme palette
xlsx-cell-formats.ts the <cellXfs> table, shared by every style reader
xlsx-number-format.ts, xlsx-serial-date.ts, xlsx-shared-strings.ts value-level concerns
xlsx-cell-styles.ts, xlsx-cell-borders.ts fills, fonts, borders, alignment
xlsx-chart-xml.ts, xlsx-chart-series.ts, xlsx-chart.ts chart part readers, series, and plot assembly
xlsx-differential-formats.ts, xlsx-conditional-formatting.ts, xlsx-conditional-styles.ts <dxf> overrides, rule parsing and evaluation, and painting them over a cell's own style
xlsx-worksheet-grid.ts, xlsx-worksheet-layout.ts, xlsx-drawings.ts sheet contents, layout, images
xlsx-workbook.ts orchestration: sheet order, hidden state, supporting parts
SpreadsheetGrid.tsx, SpreadsheetCell.tsx the virtualized grid and one cell, shared with CsvViewer
SpreadsheetGridOverlay.tsx, spreadsheet-grid-overlay.ts the layer over the grid: charts, images, sparklines, and the text of a merge that spans rows
use-spreadsheet-resize.ts, SpreadsheetResizeHandle.tsx reader-set column widths and row heights, one implementation over both axes
spreadsheet-row-heights.ts auto-fitting a row the file leaves unsized
XlsxViewer.tsx load and error states, sheet tabs, footer

The first commit is a pure extraction of the shared grid out of CsvViewer with no behaviour change; reviewing the commits in order should be easier than the whole diff.

Screenshots

New UI, so this needs a capture — attaching one of a multi-sheet workbook to the conversation rather than committing it, per the contributor guide.

Testing

  • pnpm lint
  • pnpm typecheck
  • pnpm test — 49871 passed (one unrelated load-sensitive flake in right-sidebar, green in isolation)
  • pnpm build
  • Added or updated high-quality tests that would catch regressions

22 of the 44 new modules are their own test file. The tests build real zip bytes with node:zlib through a fixture builder rather than checking in binary blobs, so each case varies one detail of the package and still goes through the same container Excel writes.

Beyond the happy path: stored vs deflated entries, incompressible payloads, UTF-8 part names, an archive comment containing the end-of-central-directory signature, zip64 rejection, a corrupt or truncated central directory, a zeroed local header, deflate-bomb bounds, tag-prefix collisions (<c> vs <cols>), a legal > inside an attribute value, out-of-grid cell references, the 1900/1904/leap-day edges, cellXfs vs cellStyleXfs, a numFmt under <dxfs>, missing rels parts, a relationship id that does not match the sheet ordinal, merge placement against a scrolled column window, and the row and column caps.

Six bugs were found by these tests or by reading real files rather than fixtures, and each is fixed with a regression test:

  1. OPC relationship targets resolved against the _rels folder instead of the owning part, so any producer not using the exact layout we guessed would have opened as an empty workbook.
  2. No bound on inflated part size — a workbook inside the read budget could expand to gigabytes.
  3. Inline strings kept phonetic runs while shared strings dropped them, so the same Japanese value rendered two ways.
  4. Elapsed-duration codes ([h]:mm:ss) were classified as dates, turning 36 hours into a calendar day.
  5. A > inside an attribute value truncated attribute parsing (legal XML — only < and & must be escaped).
  6. The contrast rule repainted an 18pt bold accent heading black by applying the body-text ratio to it.

AI Review Report

Reviewed with Claude Code across several rounds, including a CodeRabbit pass whose findings are answered in the conversation.

  • Cross-platform. No new shortcuts, labels or accelerators. No path use in the new code: OPC part names are always /-separated inside the package regardless of host, and xlsx-part-paths.ts does its own segment handling rather than using node:path, which would corrupt them on Windows.
  • SSH / remote / local. Flagged early that a viewer wired only into fs:readFile would work locally and show a placeholder over SSH. The relay read path uses the same previewable-binary map on both its single-shot and streaming metadata paths. IMAGE_MIME_TYPES is deliberately left alone because terminal artifact previews use it for image-shaped payloads, and the mobile file-explorer path is untouched since mobile cannot render a workbook.
  • Performance. Parsing is off the render pass and cancellable. The XML scanner is forward-only and allocates per element rather than building a DOM (also why it works in the node Vitest environment, which has no DOMParser). Both axes virtualize; off-screen columns collapse into one spacer track per side, which also removed a per-column grid-template-columns string from every row's inline style. Column sizing samples the first 200 rows; number formats and merge lookups are cached per style and bucketed per row. A 13-sheet, 8073-row real workbook parses in about 70ms.
  • UI quality. New surface tokens are declared in main.css for :root, .dark and the @theme inline block, and every utility added was checked against the compiled CSS — twice during this work a Tailwind class compiled to nothing (bg-editor-surface is not a utility this project has, and text-[…] arbitrary values cannot be disambiguated between a size and a colour), so the compiled output is now the check rather than the class name looking plausible. Tabs use role="tablist"/role="tab" with aria-selected; cells carry aria-colindex because virtualization means the DOM no longer reflects real column positions; the virtualized body is a role="rowgroup". No max-lines disable was added.

Security Audit

  • Untrusted input. The parser treats the file as hostile. Zip offsets and sizes are validated against the buffer before any read; the end-of-central-directory scan is bounded to the maximum comment size; zip64 and unknown compression methods are refused with a clear message; payload sizes come from the central directory so a zeroed local header cannot desync the reader; and inflation stops at both the declared size and an absolute 256MB ceiling, since deflate reaches roughly 1000:1 on crafted input.
  • Resource exhaustion. Workbooks get a 20MB read budget rather than the 50MB image/PDF one, because they are parsed into renderer objects instead of streamed. Sheets stop at 200k rows with the cap surfaced in the footer. Cell references are rejected past Excel's own 16384×1048576 grid. Merge indexing is bucketed per covered row, not per covered cell, and capped. Image count and byte size are both bounded.
  • Path handling. Relationship targets are package-relative, normalized with .. collapsed and unable to escape the package root (asserted by test), and only ever index the in-memory zip entry map — no filesystem access. TargetMode="External" relationships are skipped, since their target is a URI. Authorization is unchanged: reads still go through resolveAuthorizedPath.
  • Execution / injection. Nothing is evaluated. Macros in .xlsm are never read; formulas are not executed, only their cached result is shown. No innerHTML — cell text goes through React as text, and images are inlined as data: URLs from bytes already in the archive. External references, DDE links and OLE objects are ignored.
  • IPC / secrets / dependencies. No new IPC surface, no new dependency, no credentials or network access.

Notes

  • Charts are drawn from the values the file caches. Types outside the supported set (radar, bubble, stock, surface) render a frame naming the type rather than an empty box. A chart declaring a second value axis is plotted on one scale rather than two. Validated against fixtures modelled on Excel's chart XML — no workbook containing a real chart was available on the machine this was written on, which is worth weighing when reading that part of the coverage.
  • In-cell sparklines go beyond Excel parity, on purpose. A budget template downloaded from Google Sheets shows two balance columns and four progress bars; none is a chart object. They are SPARKLINE() calls, which Sheets' export wraps in __xludf.DUMMYFUNCTION("…") with an empty cached result — so the cell holds the author's intent and no value, and Excel shows blank cells there too. This renders them, which is a product decision rather than a fix: the chart is fully described in the file and a reader wants to see what its author drew. It is isolated in its own commit so it can be dropped without touching anything else. Evaluating the MAX/MIN bound that sibling sparklines share is the part that matters — without it each bar fills its own cell and the comparison the author built is lost.
  • Not claimed on purpose: legacy .xls and .xlsb. They are a different, non-OOXML binary format this parser cannot read, and adding them to the mime map would swap a truthful "cannot display" for a parse error.
  • Also not read from the file: conditional formatting, vertical alignment, text rotation, and cell-level rich-text runs with mixed formatting inside one cell.
  • Diffing a workbook is unchanged (still "Binary file changed"). A cell-level workbook diff is a much larger feature.
  • Editing is out of scope: the viewer is read-only and the file is never written back.

@SrAlvarado
refactor(editor): extract the shared spreadsheet grid from CsvViewer
4aaafea 
The virtualized sheet grid CsvViewer renders is about to have a second
consumer, so it moves into SpreadsheetGrid with its column sizing split
into a pure, testable module. CsvViewer keeps delimiter sniffing, parsing
and the row/column counts.

No behavior change: the markup, class names and virtualization settings
are the ones CsvViewer already used.
@SrAlvarado
feat(editor): open .xlsx and .xlsm workbooks in a sheet viewer
f626460 
Opening a workbook showed "Binary file — cannot display": the read path
had no mime type for .xlsx, so it fell through to the generic binary
placeholder. Workbooks now render read-only in the editor, one tab per
worksheet, on the same virtualized grid CsvViewer uses.

The parser is hand-rolled, like csv-parse.ts, rather than pulling in
SheetJS: a workbook is a zip of XML parts, and reading the few parts a
viewer needs (workbook, rels, sharedStrings, styles, worksheets) is a
small well-specified slice. Inflating goes through the platform's
DecompressionStream, so there is no new dependency and no native code.

Values are shown as stored, with two interpretations: shared/inline/rich
text is joined into the cell value, and date-formatted numbers are
rendered as dates because a bare serial like 45658 is meaningless to a
reader. The 1900 phantom leap day and the 1904 date system are both
handled. Sparse rows and columns are padded so values stay in place.

Remote parity: the relay read path gets the same mime types, so an .xlsx
in an SSH worktree opens the same way. Workbooks get a tighter 20MB read
budget than the 50MB image/PDF one, because they are parsed into renderer
objects rather than streamed to a viewer, and sheets are capped at 200k
rows with the cap surfaced in the footer.

Legacy .xls and .xlsb are deliberately not claimed — they are a different,
non-OOXML format this parser cannot read, and claiming them would replace
a truthful placeholder with an error.
@coderabbitai

coderabbitai Bot commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The change adds .xlsx and .xlsm binary previews with MIME-specific size limits. It adds ZIP, XML, workbook, worksheet, date, style, and Base64 parsing utilities. The renderer adds XlsxViewer and a reusable virtualized SpreadsheetGrid. CSV rendering now uses the shared grid. Binary preview routing distinguishes spreadsheets, images, and unsupported files. Tests cover file handling, parsing, rendering, limits, errors, and workbook state changes.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 39.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description is detailed and relevant, but it omits required template sections and information, including a linked issue, visual proof, AI disclosure, review, checklist, and author. Add the missing template sections, provide the required issue link and visual proof or an N/A explanation, and complete the testing and checklist items.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely identifies the main change: adding read-only .xlsx and .xlsm workbook viewing.

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.

❤️ Share

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

coderabbitai[bot]
coderabbitai Bot reviewed Aug 10, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (10)
src/renderer/src/components/editor/EditorContent.tsx (1)

46-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Use retryable loading for XlsxViewer.

lazy() caches a rejected chunk import. A transient chunk-load failure can prevent later workbook previews until restart. Use lazyWithRetry for this viewer, and verify its retry defaults match other lazy editor viewers.

src/renderer/src/components/editor/xlsx-workbook-test-fixtures.ts (1)

156-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Escape the sheet name in the name attribute.

buildSharedStringsXml escapes its text at line 175, but buildWorkbookXml interpolates sheet.name raw. If a test uses a sheet name that contains &, <, or ", the fixture emits malformed XML. The resulting test failure would point at the parser instead of the fixture.

escapeXmlText already covers the four characters an attribute value needs.

♻️ Proposed change 
-        `<sheet name="${sheet.name}" sheetId="${index + 1}"${sheet.hidden === true ? ' state="hidden"' : ''} r:id="rId${index + 1}"/>`
+        `<sheet name="${escapeXmlText(sheet.name)}" sheetId="${index + 1}"${sheet.hidden === true ? ' state="hidden"' : ''} r:id="rId${index + 1}"/>`
src/renderer/src/components/editor/xlsx-xml-elements.ts (1)

26-55: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Self-closing detection can misread a quoted attribute value.

Line 36 treats the element as self-closing when the character before > is /. A quoted attribute value that ends with / produces the same character, for example <Relationship Id="rId1" Target="http://host/">…</Relationship>. The scanner then reports an empty inner and resumes inside the element. The same heuristic repeats at Line 164 in findCloseTagStart, so nesting depth can also be wrong for that input.

The current consumers (row, c, t, si, sheet, Relationship) are self-closing or attribute-only in real producer output, so the impact is limited. If you want to close the gap, track quote state while scanning to > instead of matching the last character.

src/renderer/src/components/editor/xlsx-part-paths.ts (1)

8-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider ignoring external relationship targets.

resolveXlsxPartPath treats every target as a package-relative part name. A relationship with TargetMode="External" holds a URI, for example https://host/book.xlsx. The function then produces a meaningless part name such as https:/host/book.xlsx.

readRelationshipTargets in src/renderer/src/components/editor/xlsx-workbook.ts (Lines 110-116) stores every relationship, so an external target can enter the map. A sheet r:id never points at an external part, so no current path is broken. Skipping entries with TargetMode="External" would make the contract explicit.

src/renderer/src/components/editor/xlsx-number-formats.ts (2)

25-52: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

numFmt elements outside <numFmts> also reach this scan.

forEachXlsxXmlElement scans the whole stylesXml for numFmt. <dxfs> differential formats can also contain <numFmt>. If such an element reuses a built-in id with a non-date code, Line 38 removes that built-in id for every cell format. Scoping the scan to the numFmts block, as parseCellFormatNumberFormatIds does for cellXfs, removes the ambiguity.

72-82: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Elapsed-time formats are classified as dates.

[h]:mm:ss and [mm]:ss are elapsed-duration formats. Line 77 strips the bracketed section, and the remaining mm:ss matches DATE_FORMAT_TOKEN_PATTERN. The viewer then renders serial 1.5 as a calendar date instead of 36:00:00. Detecting a leading [h], [m], or [s] section and returning false keeps those cells as their stored number.

src/renderer/src/components/editor/xlsx-shared-strings.ts (1)

3-17: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

A self-closing <rPh/> can delete real text.

PHONETIC_RUN_PATTERN requires a </rPh> close tag. If a producer writes <rPh sb="0" eb="1"/> and a later </rPh> exists in the same <si>, the lazy match spans both and removes the text between them. The schema requires a <t> child, so Excel output is unaffected. Adding the self-closing alternative to the pattern removes the risk.

src/renderer/src/components/editor/xlsx-worksheet-grid.ts (1)

36-62: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Row-cap stop conflicts with the out-of-order row comment.

Line 44 returns false and ends the scan on the first row whose index reaches maxRows. The comment at Lines 51-53 states that rows can arrive out of order. With out-of-order rows, one high r value discards every remaining row and marks the sheet truncated. Excel writes rows in ascending order, so normal files are unaffected. If you want both behaviors, continue scanning and only mark truncated for skipped rows.

src/renderer/src/components/editor/xlsx-worksheet-grid.test.ts (1)

76-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a regression test for an oversized column reference.

The suite covers an unparseable reference but not a syntactically valid reference with a very large column, for example r="AAAAAAAA1". That input drives the padding loop in readRowCells. A test that asserts the parser rejects or clamps the column would lock in the bound requested on src/renderer/src/components/editor/xlsx-worksheet-grid.ts Lines 64-80.

src/renderer/src/components/editor/xlsx-workbook.ts (1)

121-135: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Resolve sharedStrings.xml and styles.xml through the relationship map.

Both helpers assume the conventional file name next to the workbook part. parseXlsxWorkbook already builds relationshipTargets at Line 44. A producer that names the parts differently, but declares the sharedStrings and styles relationship types, then loses all strings and all date formatting. Looking the targets up by relationship type, and keeping the current name as the fallback, matches the approach already used for worksheets.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6d91c42e-a3f6-4285-be67-8ad5c2e34c30

📥 Commits

Reviewing files that changed from the base of the PR and between c5023fa and f626460.

📒 Files selected for processing (39)
  • src/main/ipc/filesystem.test.ts
  • src/main/ipc/filesystem.ts
  • src/relay/fs-handler-file-read.ts
  • src/relay/fs-handler-previewable-binary.test.ts
  • src/relay/fs-handler-utils.ts
  • src/renderer/src/components/editor/CsvViewer.tsx
  • src/renderer/src/components/editor/EditorContent.test.tsx
  • src/renderer/src/components/editor/EditorContent.tsx
  • src/renderer/src/components/editor/SpreadsheetGrid.tsx
  • src/renderer/src/components/editor/XlsxViewer.test.tsx
  • src/renderer/src/components/editor/XlsxViewer.tsx
  • src/renderer/src/components/editor/binary-file-preview-kind.test.ts
  • src/renderer/src/components/editor/binary-file-preview-kind.ts
  • src/renderer/src/components/editor/spreadsheet-grid-columns.test.ts
  • src/renderer/src/components/editor/spreadsheet-grid-columns.ts
  • src/renderer/src/components/editor/xlsx-cell-reference.test.ts
  • src/renderer/src/components/editor/xlsx-cell-reference.ts
  • src/renderer/src/components/editor/xlsx-number-formats.test.ts
  • src/renderer/src/components/editor/xlsx-number-formats.ts
  • src/renderer/src/components/editor/xlsx-part-paths.test.ts
  • src/renderer/src/components/editor/xlsx-part-paths.ts
  • src/renderer/src/components/editor/xlsx-serial-date.test.ts
  • src/renderer/src/components/editor/xlsx-serial-date.ts
  • src/renderer/src/components/editor/xlsx-shared-strings.test.ts
  • src/renderer/src/components/editor/xlsx-shared-strings.ts
  • src/renderer/src/components/editor/xlsx-workbook-test-fixtures.ts
  • src/renderer/src/components/editor/xlsx-workbook.test.ts
  • src/renderer/src/components/editor/xlsx-workbook.ts
  • src/renderer/src/components/editor/xlsx-worksheet-grid.test.ts
  • src/renderer/src/components/editor/xlsx-worksheet-grid.ts
  • src/renderer/src/components/editor/xlsx-xml-elements.test.ts
  • src/renderer/src/components/editor/xlsx-xml-elements.ts
  • src/renderer/src/components/editor/xlsx-zip-archive.test.ts
  • src/renderer/src/components/editor/xlsx-zip-archive.ts
  • src/renderer/src/i18n/locales/en.json
  • src/renderer/src/lib/base64-bytes.test.ts
  • src/renderer/src/lib/base64-bytes.ts
  • src/shared/spreadsheet-file-extensions.test.ts
  • src/shared/spreadsheet-file-extensions.ts

Comment thread src/renderer/src/components/editor/SpreadsheetGrid.tsx Outdated
Comment thread src/renderer/src/components/editor/xlsx-workbook.ts Outdated
Comment thread src/renderer/src/components/editor/xlsx-worksheet-grid.ts Outdated
Comment on lines +64 to +80
function readRowCells(rowXml: string, context: XlsxWorksheetContext): string[] {
const cells: string[] = []
let nextColumnIndex = 0

forEachXlsxXmlElement(rowXml, 'c', (cellElement) => {
const reference = cellElement.attributes.r
const parsed = reference === undefined ? null : parseXlsxCellReference(reference)
const columnIndex = parsed?.columnIndex ?? nextColumnIndex
nextColumnIndex = columnIndex + 1
while (cells.length < columnIndex) {
cells.push('')
}
cells[columnIndex] = readCellText(cellElement.attributes, cellElement.inner, context)
})

return cells
}

@coderabbitai coderabbitai Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

No upper bound on the parsed column index. readRowCells pads a row up to the column index returned by parseXlsxCellReference, so a crafted reference such as r="AAAAAAAA1" can drive an allocation loop of billions of entries. The compressed-workbook limit does not prevent this, because one short reference produces the large index.

  • src/renderer/src/components/editor/xlsx-worksheet-grid.ts#L64-L80: reject or clamp a column index above the SpreadsheetML maximum of 16384 before the padding loop, unless parseXlsxCellReference already does so.
  • src/renderer/src/components/editor/xlsx-worksheet-grid.test.ts#L76-L82: add a test with a valid but oversized column reference that asserts the parser clamps or drops the cell instead of allocating.
📍 Affects 2 files
  • src/renderer/src/components/editor/xlsx-worksheet-grid.ts#L64-L80 (this comment)
  • src/renderer/src/components/editor/xlsx-worksheet-grid.test.ts#L76-L82

Comment thread src/renderer/src/components/editor/xlsx-worksheet-grid.ts
Comment thread src/renderer/src/components/editor/xlsx-zip-archive.ts
Comment thread src/renderer/src/components/editor/XlsxViewer.tsx
@SrAlvarado
fix(editor): harden the workbook reader against review findings
a374379 
Addresses the review on stablyai#13532:

- Bound the inflated size of a zip part. The 20MB read budget only capped
  the compressed file, so a deflate bomb (roughly 1000:1) could expand to
  gigabytes in renderer memory before any error. The declared size now
  lowers the ceiling, an absolute 256MB cap applies regardless of what the
  archive claims, and inflate aborts mid-stream once either is exceeded.
  Real workbooks inflate 4-9x, so this leaves an order of magnitude of room.
- Drop phonetic runs from inline strings, not just shared strings. The same
  Japanese value rendered differently depending on which form the producer
  used; the stripping moved into the shared text-run reader so every form
  gets it.
- Do not classify elapsed-duration formats as dates. `[h]:mm:ss` means
  36 hours for serial 1.5, not a calendar day, so those cells keep their
  stored number instead of being rendered as a date.
- Scope the numFmt scan to <numFmts>. A <numFmt> under <dxfs> belongs to a
  single conditional-format rule and was clearing built-in date ids for
  every cell format.
- Find the end of an open tag with quote state instead of indexOf('>'). XML
  only requires < and & to be escaped in an attribute value, so a legal >
  there truncated attribute parsing.
- Resolve sharedStrings.xml and styles.xml through their relationship types
  with the conventional name as fallback, matching what worksheets already
  did, and skip TargetMode="External" relationships, whose target is a URI
  rather than a part name.
- Keep scanning after a row lands past the row cap, so one out-of-order row
  no longer discards every row after it.
- Add role="rowgroup" to the virtualized grid body, so assistive technology
  keeps the table's owned-row relationship (also fixes CsvViewer).
- Format row and column counts with getIntlLocale() so separators follow the
  app language, not the OS locale.
- Escape the sheet name in the test fixture builder.

Regression tests added for each, including a valid-but-oversized column
reference (`r="AAAAAAAA1"`), which was already bounded at the SpreadsheetML
16384-column limit but had no test pinning it.
@SrAlvarado

SrAlvarado commented Aug 10, 2026

Copy link
Copy Markdown
Author

Thanks — this was a genuinely useful review. Pushed a374379a addressing it. 13 of the 16 items were real and are fixed; 2 don't reproduce and I've explained why below; 1 I'm accepting as a test-only change.

Fixed

Bound the inflated size of a part (🟠 Major, raised twice) — correct, and the most important finding here. The 20MB budget only capped the compressed file, so a deflate bomb could expand to gigabytes before the viewer said anything, and sharedStrings.xml had no row-cap backstop either. entry.uncompressedSize now lowers the ceiling (never raises it, since it is attacker-controlled), an absolute 256MB per-part cap applies regardless of what the archive declares, and inflateRaw aborts mid-stream and cancels the reader once either is exceeded. A declared size of 0 — what streaming archivers write — falls back to the absolute cap rather than rejecting.

On the number: real workbooks inflate 4–9x (measured on a 325KB file here, largest part 1.4MB), so 256MB keeps an order of magnitude of headroom over the 20MB budget while removing the unbounded case. Three tests: inflates-past-declared-size, declared-size-past-ceiling, and declared-size-of-zero still readable.

Inline strings kept phonetic runs (🟡) — real inconsistency, and the right diagnosis. Rather than duplicating the strip at the inline-string call site, the <rPh> handling moved into readXlsxXmlTextRuns, which is the single reader every storage form goes through (shared, inline, rich text). xlsx-shared-strings.ts got simpler as a result. Also took the related nitpick: the pattern now matches the self-closing <rPh/> form first, so a later </rPh> can't make the lazy match swallow real text.

Elapsed-duration formats classified as dates (🔵, but really a correctness bug) — you're right and my test had the wrong expectation pinned: I asserted [h]:mm:ss was a date. Serial 1.5 under that format means 36 hours, not a calendar day. Those codes now return false and the cell keeps its stored number; formatting durations would be a new feature, not a fix. Test updated and extended to [hh]:mm, [mm]:ss, [ss].0.

numFmt outside <numFmts> (🔵) — real. A <numFmt> under <dxfs> belongs to one conditional-format rule, and mine was letting it delete a built-in date id for every cell format. Scan is now scoped to the <numFmts> block, the same way parseCellFormatNumberFormatIds already scoped <cellXfs>. Test added.

Self-closing detection (🔵) — the underlying gap is real, though not via your example. <Relationship Target="http://host/"> ends /">, so the character before > is the closing quote, not /, and it was already read correctly. What does break it is a legal > inside an attribute value — XML only requires < and & to be escaped there — which made indexOf('>') stop early and truncate attribute parsing. Fixed properly: the open tag is now scanned with quote state, in both forEachXlsxXmlElement and findCloseTagStart. Once the scan is outside quotes at >, a preceding / is necessarily outside quotes too, so the self-closing check is sound. Tests for both shapes.

sharedStrings.xml / styles.xml via the relationship map (🔵) — agreed, and it was an inconsistency with what worksheets already did. Both now resolve by relationship type with the conventional name as fallback. Test asserts a workbook naming them strings.xml / formats.xml keeps its strings and its date formatting.

External relationship targets (🔵) — taken. TargetMode="External" relationships are skipped, so a URI never gets resolved as a package part. Test covers a sheet sharing an id with an external link.

Row-cap stop vs. out-of-order rows (🔵) — fair catch on the contradiction with my own comment. The scan now skips the over-cap row and keeps going, so one high r no longer discards every row after it; truncated still reports it. Scan cost is bounded by the new inflated-size cap.

role="rowgroup" (🟡) — taken. Note this markup predates the PR (it came out of CsvViewer unchanged), so the fix lands in the shared grid and improves both viewers.

getIntlLocale() (🟡) — taken in XlsxViewer. Worth flagging that the repo has 35 bare toLocaleString() calls in the renderer today, so this is the correct pattern rather than the prevailing one; I left the pre-existing ones alone to keep the diff scoped.

Escape the sheet name in the fixture builder (🔵) — taken; a fixture that emits malformed XML would point the failure at the parser.

Doesn't reproduce

"No upper bound on the parsed column index" (🔴 Critical) — this one doesn't hold, and your own hedge is the reason: "unless parseXlsxCellReference already does so." It does. columnIndexFromXlsxLetters checks index > MAX_COLUMN_COUNT (16384) inside its accumulation loop and returns null, so r="AAAAAAAA1" bails after 4 letters and never reaches the padding loop — the row index is bounded the same way at 1048576. xlsx-cell-reference.test.ts already asserted XFE1 → null.

The suggested test was still worth adding, though: the bound lived in one module and the padding loop in another, with nothing pinning the relationship. xlsx-worksheet-grid.test.ts now feeds r="AAAAAAAA1" through the grid parser and asserts the cell falls back to the next sequential column instead of allocating.

"Use retryable loading for XlsxViewer" (🔵) — already the case. EditorContent.tsx:3 is import { lazyWithRetry as lazy } from '@/lib/lazy-with-retry', so the lazy() on line 46 is lazyWithRetry, identical to every other viewer in that file. Only RichMarkdownEditor additionally passes a reloadKey, which is specific to its reload path.

Verification

pnpm lint, pnpm typecheck and pnpm build clean. Full suite: 49303 passed. One unrelated flake in github-project/project-view-wrapper-source-context-boundary.test.ts — a 30s timeout on a dynamic import under full-suite load; it passes in isolation in 7.7s, passed in the pre-change run, and touches nothing in this PR.

Also re-validated against a real Excel-authored workbook (13 sheets, 8073 rows, shared strings, date columns) after the changes: identical output, ~72ms.

coderabbitai[bot]
coderabbitai Bot reviewed Aug 10, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/renderer/src/components/editor/xlsx-zip-archive.ts (1)

211-228: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Consider a lower absolute ceiling than 256 MiB.

The cancel-on-exit and pre-push cap checks are correct. The peak memory is still notable in the worst case. The chunk list can reach inflatedSizeLimit, and the final contiguous copy adds the same amount again. With a declared size of 256 MiB, a crafted part can force roughly 512 MiB in the renderer before the read completes.

The comment at Line 23 states that real workbooks inflate 4-9x over a 20 MB budget. A ceiling near 64 MiB would keep an order of magnitude of headroom and halve the worst-case peak. This is a tuning choice, so treat it as optional.

src/renderer/src/components/editor/xlsx-zip-archive.test.ts (1)

5-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Bind the DataView to the view window in the test helper.

new DataView(bytes.buffer) ignores bytes.byteOffset. The helper also derives the end-record position from bytes.length. The two disagree if the argument is a subarray of a larger buffer.

This file adds a test that reads an archive from a non-zero byte offset. That test does not use this helper today, so the current assertions are correct. If a later test passes an offset view here, the helper reads unrelated bytes and the test fails for the wrong reason. The same applies to the new DataView(bytes.buffer) writes at Lines 177, 187, and 196.

♻️ Proposed offset-safe helper 
-function centralDirectoryOffset(bytes: Uint8Array): number {
-  return new DataView(bytes.buffer).getUint32(bytes.length - 22 + 16, true)
-}
+function archiveView(bytes: Uint8Array): DataView {
+  return new DataView(bytes.buffer, bytes.byteOffset, bytes.byteLength)
+}
+
+function centralDirectoryOffset(bytes: Uint8Array): number {
+  return archiveView(bytes).getUint32(bytes.byteLength - 22 + 16, true)
+}
ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a935f58d-ae8b-4f47-b0e6-d716af9c5e07

📥 Commits

Reviewing files that changed from the base of the PR and between f626460 and a374379.

📒 Files selected for processing (14)
  • src/renderer/src/components/editor/SpreadsheetGrid.tsx
  • src/renderer/src/components/editor/XlsxViewer.tsx
  • src/renderer/src/components/editor/xlsx-number-formats.test.ts
  • src/renderer/src/components/editor/xlsx-number-formats.ts
  • src/renderer/src/components/editor/xlsx-shared-strings.ts
  • src/renderer/src/components/editor/xlsx-workbook-test-fixtures.ts
  • src/renderer/src/components/editor/xlsx-workbook.test.ts
  • src/renderer/src/components/editor/xlsx-workbook.ts
  • src/renderer/src/components/editor/xlsx-worksheet-grid.test.ts
  • src/renderer/src/components/editor/xlsx-worksheet-grid.ts
  • src/renderer/src/components/editor/xlsx-xml-elements.test.ts
  • src/renderer/src/components/editor/xlsx-xml-elements.ts
  • src/renderer/src/components/editor/xlsx-zip-archive.test.ts
  • src/renderer/src/components/editor/xlsx-zip-archive.ts
🚧 Files skipped from review as they are similar to previous changes (9)
  • src/renderer/src/components/editor/xlsx-number-formats.ts
  • src/renderer/src/components/editor/xlsx-worksheet-grid.test.ts
  • src/renderer/src/components/editor/xlsx-shared-strings.ts
  • src/renderer/src/components/editor/SpreadsheetGrid.tsx
  • src/renderer/src/components/editor/xlsx-xml-elements.test.ts
  • src/renderer/src/components/editor/xlsx-worksheet-grid.ts
  • src/renderer/src/components/editor/XlsxViewer.tsx
  • src/renderer/src/components/editor/xlsx-workbook-test-fixtures.ts
  • src/renderer/src/components/editor/xlsx-workbook.ts

@SrAlvarado
feat(editor): give the sheet grid spreadsheet cell styling
5ea8f3d 
The grid read as a list of rows rather than a sheet of cells: the cell
hairlines used the border token with an opacity modifier (border/40), and
in dark mode --border is already rgb(255 255 255 / 0.07), so the vertical
gridlines landed near 0.03 alpha and were effectively invisible.

Gridlines now use the border token at full strength on every cell, which
is what STYLEGUIDE.md specifies it for ("all hairlines"), and the row-number
gutter uses the same muted band as the column header — so both read as
headers the way a spreadsheet's do.

Cells also take their alignment from the value: numbers, dates and times
right, booleans and cached error codes centered, text left. That is the cue
that tells a reader which columns are numeric, and it has to be derived from
the rendered text because the grid only receives strings. Workbook column
letters are centered over their column; a CSV keeps its heading row
left-aligned, since that row is the file's own text rather than a label.
coderabbitai[bot]
coderabbitai Bot reviewed Aug 10, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bdbf8a2e-3290-4bba-9d1d-ef4ff11cf69c

📥 Commits

Reviewing files that changed from the base of the PR and between a374379 and 5ea8f3d.

📒 Files selected for processing (4)
  • src/renderer/src/components/editor/SpreadsheetGrid.tsx
  • src/renderer/src/components/editor/XlsxViewer.tsx
  • src/renderer/src/components/editor/spreadsheet-cell-alignment.test.ts
  • src/renderer/src/components/editor/spreadsheet-cell-alignment.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/renderer/src/components/editor/XlsxViewer.tsx

Comment thread src/renderer/src/components/editor/SpreadsheetGrid.tsx Outdated
@SrAlvarado
feat(editor): render workbook cell fills and virtualize columns
1277339 
Two changes that both come out of looking at a real styled workbook.

Cell styling. A workbook's fills, font colours and bold now render, which the
PR description previously listed as out of scope. styles.xml is read for
<fills>, <fonts> and the cellXfs a cell's `s` attribute indexes; colours
resolve from rgb (ARGB), theme (against theme1.xml, whose first two index
pairs are swapped relative to clrScheme order) and the legacy indexed
palette, with tint applied in luminance space the way Excel's palette shades
are. Only solid patterns become a background: a hatch like gray125 is a
texture, and painting it as a solid block would invent a colour.

The ink is derived rather than copied. An author picks a fill against Excel's
white sheet, so on a dark theme the app's foreground is frequently
unreadable on top of it. The workbook's font colour wins when it clears WCAG
AA against its own fill — the white-on-dark-header case — and otherwise the
ink falls back to whichever of black or white contrasts more. On an unfilled
cell the font colour is left alone entirely, because the background is
whatever the theme uses and contrast cannot be checked.

Column virtualization. `columnCount` comes from the widest used row, so a
sheet with one far-right used cell reports thousands of columns; the grid
rendered a cell per column in every visible row, and worse, put a
grid-template-columns string with one track per column into the inline style
of every row. Both axes are virtualized now, with the off-screen columns
collapsed into one spacer track on each side. It stays a CSS grid rather than
absolutely-positioned cells so the sticky row-number gutter keeps its
normal-flow position and the header and body keep sharing one template.
Cells carry aria-colindex, since the DOM no longer reflects real column
positions.

Both apply to CsvViewer through the shared grid; the wide-sheet problem was
there too.
@SrAlvarado

SrAlvarado commented Aug 10, 2026

Copy link
Copy Markdown
Author

Good catch on the column virtualization — that one was real and worse than the comment describes. Fixed, plus this push adds the cell styling the PR previously declared out of scope.

Column virtualization (🟠 Major)

Confirmed. columnCount comes from the sheet's widest used row, so a workbook with one far-right used cell reports thousands of columns, and the grid rendered a cell per column in every visible row — roughly 500k nodes for a sheet reaching XFD. The second half of it was worse and I had missed it: gridTemplateColumns held one track per column, so that ~100KB string went into the inline style of every rendered row.

Rather than bound the displayed columns, the grid now virtualizes both axes. The off-screen columns collapse into one spacer track on each side of the template:

grid-template-columns: 48px <leading spacer>px <rendered widths…> <trailing spacer>px

Keeping it a grid — instead of absolutely positioning cells — is deliberate: the sticky row-number gutter has to stay a normal-flow grid item, and the header and body keep sharing one template, which is what stops them drifting apart. The template string is now a few dozen characters regardless of sheet width, and the rendered cell count follows the viewport.

Also added aria-colindex on header and data cells, since with virtualization the DOM no longer reflects real column positions and assistive technology needs the index explicitly.

Regression tests in SpreadsheetGrid.test.tsx: a 5000-column sheet renders far fewer cells and headers than it declares, and no row carries a per-column template in its inline style.

This applies to CsvViewer too, which shares the grid — a wide CSV had the same problem.

Cell styling (new in this push)

Workbook fills, font colours and bold now render, which the description previously listed as out of scope. Reviewed against a real Excel-authored workbook: the dark blue header bands, the yellow input cells and the light blue bands all come through as declared.

Two things worth a reviewer's attention:

Ink is derived, not just copied. A fill is chosen by its author against Excel's white sheet, so on a dark theme the app's own foreground is frequently unreadable on top of it. The workbook's font colour wins when it clears WCAG AA on that fill (the white-on-dark-header case), otherwise the ink falls back to whichever of black or white contrasts more. spreadsheet-cell-contrast.ts owns that, with a test asserting every fill ends up ≥ 4.5:1.

Font colour is only honoured on filled cells. Without a fill we do not know the background — it is whatever the active theme uses — so we cannot verify contrast, and a workbook's white font on an unfilled cell would vanish on the light theme. Those cells keep the theme foreground. That loses colour on unfilled text; I judged an unreadable cell worse than a monochrome one, but say the word if you would rather be faithful there.

Two hex constants live in spreadsheet-cell-contrast.ts (#000000 / #ffffff). STYLEGUIDE says not to hardcode a hex where a variable covers it — no token covers "maximal-contrast ink over an arbitrary colour from a data file", which is why they are there rather than in main.css. Happy to be told otherwise.

Also in this push: the cell hairlines were using border-border/40, and since dark mode's --border is already rgb(255 255 255 / 0.07), the vertical gridlines landed near 0.03 alpha and were invisible — the grid read as a list of rows, not a sheet. They now use the token at full strength, which is what STYLEGUIDE specifies it for. Numbers, dates and times right-align, booleans and error codes center.

Verification

pnpm lint, pnpm typecheck and pnpm build clean. Full suite: 49365 passed.

Two tests flake under full-suite load on this machine, and I want to be precise rather than wave them off: github-project/project-view-wrapper-source-context-boundary.test.ts and native-chat/transcript-watch-liveness.test.ts. Across four runs the failing set was {both}, {native-chat}, {github-project}, {none} — a different subset each time. Both pass in isolation (native-chat five times in a row), both pass with this branch's changes stashed out, and neither touches code this PR modifies (one is a main-process file watcher, the other the GitHub Projects view). If they are known-flaky on CI too, no action needed here; if they are not, that is worth a look independently of this PR.

@SrAlvarado
feat(editor): give the sheet viewers an Excel-style light canvas
1f1dcba 
The grid inherited the app's dark chrome, which fought the file it was
showing: a workbook's cell fills are authored against a white sheet, so on a
dark canvas they read as arbitrary blocks of colour rather than as the
highlighting the author meant.

Adds a spreadsheet surface token family to main.css — surface, foreground,
header band, header foreground, gridline, strong gridline and row hover — and
uses it in SpreadsheetGrid and the two viewers' footers. The values follow
Excel and Google Sheets: white sheet, light grey header bands for the column
letters and the row-number gutter, a stronger divider under the header row and
beside the gutter, hairline gridlines between cells, and the active sheet tab
lifted onto the sheet surface.

The tokens hold the same values in light and dark on purpose, which is what
both applications do inside a dark chrome, for the same reason the cell ink is
derived from the fill rather than the theme. It is one place to change if a
maintainer wants a dark sheet variant instead.

Cells also move off the monospace stack onto the app's proportional font with
tabular-nums, so text reads like a spreadsheet while digits still line up in a
column.

Worth noting for whoever touches this next: `bg-editor-surface` is not a
utility this project has — the token is not exposed in the `@theme inline`
block, so that class compiles to nothing. The working form elsewhere in the
codebase is `bg-[var(--editor-surface)]`. Every utility added here was checked
against the compiled CSS.
@SrAlvarado
SrAlvarado force-pushed the feat/xlsx-spreadsheet-viewer branch from a401d7a to 1f1dcba Compare August 10, 2026 10:28
coderabbitai[bot]
coderabbitai Bot reviewed Aug 10, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/renderer/src/components/editor/xlsx-worksheet-grid.test.ts (1)

280-286: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Merge styles when repeated rows merge values.

Line 280 adds repeated-row coverage. parseXlsxWorksheetGrid merges rows[rowIndex], but it replaces styles[rowIndex] with the later sparse row. A repeated row with styled cells in different columns loses the earlier cell styles.

Merge the style row with the same positional semantics as the value row. Add an assertion for styles from both repeated row elements.

🧹 Nitpick comments (2)
src/renderer/src/components/editor/SpreadsheetGrid.tsx (1)

40-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reduce implementation-comment length.

Keep only the non-obvious constraint. The identifiers and tests already describe most implementation details.

  • src/renderer/src/components/editor/SpreadsheetGrid.tsx#L40-L53: reduce the component overview to concise virtualization and reset rationale.
  • src/renderer/src/components/editor/SpreadsheetGrid.test.tsx#L34-L37: remove the repeated DOM and style-size explanation.
  • src/renderer/src/components/editor/spreadsheet-grid-columns.ts#L68-L76: retain a short spacer-template contract description.

As per coding guidelines: “Comments must be concise, limited to non-obvious information, and preferably one line.”

Source: Coding guidelines

src/renderer/src/components/editor/xlsx-workbook.test.ts (1)

164-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a font color that differs from the fallback ink.

The expected #ffffff is also the readable fallback for the dark #4472c4 fill. This test passes if declared font colors are ignored.

Use a legible non-white font color on a dark fill, then assert that exact color.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9f6b501b-613b-432b-8b6d-b78f382f45d1

📥 Commits

Reviewing files that changed from the base of the PR and between 5ea8f3d and 1277339.

📒 Files selected for processing (21)
  • src/renderer/src/components/editor/SpreadsheetGrid.test.tsx
  • src/renderer/src/components/editor/SpreadsheetGrid.tsx
  • src/renderer/src/components/editor/XlsxViewer.test.tsx
  • src/renderer/src/components/editor/XlsxViewer.tsx
  • src/renderer/src/components/editor/spreadsheet-cell-contrast.test.ts
  • src/renderer/src/components/editor/spreadsheet-cell-contrast.ts
  • src/renderer/src/components/editor/spreadsheet-grid-columns.test.ts
  • src/renderer/src/components/editor/spreadsheet-grid-columns.ts
  • src/renderer/src/components/editor/xlsx-cell-formats.test.ts
  • src/renderer/src/components/editor/xlsx-cell-formats.ts
  • src/renderer/src/components/editor/xlsx-cell-styles.test.ts
  • src/renderer/src/components/editor/xlsx-cell-styles.ts
  • src/renderer/src/components/editor/xlsx-color.test.ts
  • src/renderer/src/components/editor/xlsx-color.ts
  • src/renderer/src/components/editor/xlsx-number-formats.ts
  • src/renderer/src/components/editor/xlsx-theme-palette.ts
  • src/renderer/src/components/editor/xlsx-workbook-test-fixtures.ts
  • src/renderer/src/components/editor/xlsx-workbook.test.ts
  • src/renderer/src/components/editor/xlsx-workbook.ts
  • src/renderer/src/components/editor/xlsx-worksheet-grid.test.ts
  • src/renderer/src/components/editor/xlsx-worksheet-grid.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/renderer/src/components/editor/spreadsheet-grid-columns.test.ts
  • src/renderer/src/components/editor/xlsx-number-formats.ts
  • src/renderer/src/components/editor/xlsx-workbook-test-fixtures.ts

Comment thread src/renderer/src/components/editor/xlsx-cell-formats.ts
Comment on lines +39 to +41
function readIdAttribute(value: string | undefined, fallback: number): number {
const parsed = Number.parseInt(value ?? '', 10)
return Number.isInteger(parsed) && parsed >= 0 ? parsed : fallback

@coderabbitai coderabbitai Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject partially numeric XML attributes.

Number.parseInt and Number.parseFloat accept valid prefixes. Values such as numFmtId="14x", theme="4x", indexed="2.5", and tint="0.5x" therefore produce invented formatting or colors instead of using the safe fallback.

  • src/renderer/src/components/editor/xlsx-cell-formats.ts#L39-L41: require the complete ID string to be a non-negative integer before converting it.
  • src/renderer/src/components/editor/xlsx-color.ts#L97-L119: require complete numeric values for tint, theme, and indexed before resolving them.
  • src/renderer/src/components/editor/xlsx-cell-formats.test.ts#L16-L27: add partial-number ID cases and assert fallback IDs.
  • src/renderer/src/components/editor/xlsx-color.test.ts#L47-L50: add partial-number theme, indexed, and tint cases and assert null or the untinted base color.
📍 Affects 4 files
  • src/renderer/src/components/editor/xlsx-cell-formats.ts#L39-L41 (this comment)
  • src/renderer/src/components/editor/xlsx-color.ts#L97-L119
  • src/renderer/src/components/editor/xlsx-cell-formats.test.ts#L16-L27
  • src/renderer/src/components/editor/xlsx-color.test.ts#L47-L50

Comment thread src/renderer/src/components/editor/xlsx-cell-styles.ts
Comment on lines +54 to +57
if (backgroundColor !== undefined) {
style.backgroundColor = backgroundColor
style.textColor = pickReadableCellTextColor(backgroundColor, font?.color)
}

@coderabbitai coderabbitai Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Preserve declared font colors when a cell has no fill.

The current implementation drops an explicit font color unless the cell also has a solid fill. This makes common no-fill styles, such as red values or white title text, render with the application foreground instead of the workbook font color.

  • src/renderer/src/components/editor/xlsx-cell-styles.ts#L54-L57: assign font.color when no background exists. Use pickReadableCellTextColor only when a resolved fill provides a contrast background.
  • src/renderer/src/components/editor/xlsx-cell-styles.test.ts#L50-L57: expect style index 3 to include textColor: '#ffffff', and add a no-fill colored-font case.
Proposed implementation 
     if (backgroundColor !== undefined) {
       style.backgroundColor = backgroundColor
       style.textColor = pickReadableCellTextColor(backgroundColor, font?.color)
+    } else if (font?.color !== undefined) {
+      style.textColor = font.color
     }
📝 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
if (backgroundColor !== undefined) {
style.backgroundColor = backgroundColor
style.textColor = pickReadableCellTextColor(backgroundColor, font?.color)
}
if (backgroundColor !== undefined) {
style.backgroundColor = backgroundColor
style.textColor = pickReadableCellTextColor(backgroundColor, font?.color)
} else if (font?.color !== undefined) {
style.textColor = font.color
}
📍 Affects 2 files
  • src/renderer/src/components/editor/xlsx-cell-styles.ts#L54-L57 (this comment)
  • src/renderer/src/components/editor/xlsx-cell-styles.test.ts#L50-L57

Comment thread src/renderer/src/components/editor/xlsx-worksheet-grid.ts
@SrAlvarado
feat(editor): honour sheet layout and zoom in the workbook viewer
efe6014 
Opening a styled workbook showed three gaps against how the same file reads in
Excel or Google Sheets, all visible in a downloaded budget template.

Merged cells. `<mergeCells>` was ignored, so a merged banner rendered as
separate cells with its text clipped into the first one. Rows are virtualized
independently, so a real row span is not available; instead a merge renders as
a band — every row it covers gets one cell spanning the merge's columns, and
only the anchor carries the value. The span is clamped to the columns actually
rendered, because with columns virtualized a merge can start or end outside the
window and a longer span would eat the spacer track and knock the row out of
alignment with the header. The index is bucketed per covered row rather than
per covered cell, since a merge may legally span a whole column.

Column widths. `<cols customWidth>` was ignored and every column was sized from
its content, so the layout did not match the file. Declared widths now win over
content sizing, converted from Excel's character units. A `<col>` without
`customWidth` is skipped: Excel writes those for a style span or an outline
level, and adopting the default width it repeats there would override content
sizing with a value the author never chose.

Zoom. The sheet had none. It now follows `editorFontZoomLevel`, the same store
value Monaco and the notebook viewer use, so the existing editor zoom shortcuts
work on a sheet and the level is shared across surfaces. Font size, row height,
the row-number gutter and the column widths all scale together.

Still not read from the file, and visible in that same template: number formats
beyond dates (a currency cell shows 1000 where Excel shows 1.000 €), row
heights, wrapped text, and charts or images.
@SrAlvarado
feat(editor): apply workbook number formats to cell values
bc79fa8 
A currency cell showed 1000 where the same file reads 1.000 € in Excel and
Google Sheets. Only date formats were interpreted; every other numeric format
fell through to the stored value, which is the most visible way a viewer can
misrepresent a sheet.

Format codes are now parsed into the parts a value needs — literals, digit
placeholders, grouping, percent and thousands scaling, section colours — and
rendered through Intl.NumberFormat. Intl rather than assembling digits by hand
because the separators a code implies belong to the viewer, not the file: the
same `#,##0.00` reads 1,000.00 in English and 1.000,00 in Spanish. The active
app locale is passed into the parse, so a sheet reformats if the language
changes.

Covered: the four section forms (positive, negative, zero and their colours),
`[$€-2]` currency symbols, escaped and quoted literals, accounting codes with
their fill and reserved-width placeholders, percent, trailing-comma scaling,
and the numeric built-in ids Excel never writes into `<numFmts>`.

Verified against a downloaded budget template: every currency cell now renders
as its source application does.
@SrAlvarado
feat(editor): honour cell alignment, wrapped text and row heights
d4f23ab 
Three more things the file said that the viewer ignored.

An author-set horizontal alignment now wins over inferring one from the value,
which is only the fallback for a cell whose format leaves it general. `justify`
and `distributed` are skipped: neither has a counterpart in a read-only cell,
and pretending otherwise would move text the author did not move.

`wrapText` cells now wrap instead of clipping to an ellipsis, which is what made
a merged instruction paragraph unreadable.

Row heights come from `<row ht customHeight>`, converted from points at 96 over
72, and feed the row virtualizer per index so a sheet keeps its own rhythm. As
with column widths, a row that only records the default height is ignored —
Excel writes `ht` on every row of some files.
@SrAlvarado
feat(editor): read cell borders, font sizes and anchored images
3a1d376 
Opening a downloaded budget template made clear what was still missing, and it
was not what it looked like. The bar chart and the large percentage block in
that file are not drawings at all — its two drawing parts are empty
`<xdr:wsDr/>` shells. The visuals come from cell-level features the viewer
ignored: the file declares 46 fonts and 27 borders.

Borders. `<borders>` is read per edge, with weights and dash styles mapped to
their CSS equivalents and the edge colour resolved through the same theme
palette as fills. A declared edge replaces the default gridline on that side
only, so a cell with one underline keeps the grid intact everywhere else. An
empty `<border/>` and `style="none"` both draw nothing, which matters because
Excel writes the empty element on every cell that sets any other edge.

Font size and italic. Sizes are stored relative to the workbook's own default
font, so a sheet keeps its typographic hierarchy while still following the app's
base size and the zoom level. The scale is clamped so one absurd size cannot
push a row out of the viewport, and a difference too small to see is dropped.
Italic reads its toggle the same way bold does, honouring an explicit
`val="0"` override.

Images. Worksheet drawings are followed through their relationships to the media
part and inlined as data URLs, positioned over the cell range their anchor
names. Both `twoCellAnchor` and `oneCellAnchor` are read; an absolute anchor is
skipped since it has no cell to hang on. They render in a separate
pointer-events-none layer rather than as cells, so a drawing spanning a range
cannot disturb the grid tracks the header and rows share. Count and byte size
are both bounded, since each image is inlined into renderer memory.

Charts are still not rendered. A DrawingML chart renderer is a separate surface
of its own — chart types, series, axes, legends and styling — and no part of it
is shared with what this commit adds.
@SrAlvarado
fix(editor): keep an author accent colour on large cell text
58f04ec 
The contrast rule applied the WCAG body-text ratio of 4.5:1 to every filled
cell, which discarded exactly the colours an author picks deliberately. A brand
orange heading on white is 3.1:1: below the body threshold, above the large-text
one, and legible — it is what both Excel and Google Sheets show. The viewer was
repainting it black.

The threshold now follows WCAG's own distinction: 3:1 once the text is large,
meaning 18pt or 14pt bold, measured from the font's declared point size rather
than the rendered pixel size so it does not shift with the zoom level. Body text
keeps the 4.5:1 rule, and a colour that fails even the large-text ratio is still
replaced.

Found by reading a downloaded budget template rather than a fixture: its 18pt
bold title came through black instead of its own orange.
@SrAlvarado
feat(editor): draw workbook charts as inline SVG
1ee982c 
Charts anchored in a worksheet are now drawn instead of skipped, so opening a
workbook shows what the file shows. Column, horizontal bar, line, area, scatter,
pie and doughnut are plotted; a type outside that set renders a frame naming it
rather than an empty box the reader cannot explain.

Charts reach the sheet through a graphic frame rather than a picture, so the
anchor reader now follows `<c:chart r:id>` as well as a picture's blip, and the
sheet exposes one list of drawings — charts and images together — since both
occupy a cell range and share the same overlay layer.

Series, categories and titles come from the cached values the file stores, read
by point index rather than document order: a sparse cache lists only the points
that have values, so reading them in sequence shifts every later value onto the
wrong category. A series colour is taken from its own `spPr`, or from the
workbook theme's accents in order — which is what Excel does when a series
declares none, so reproducing it is faithful rather than a palette choice. One
theme parse now serves cell fills, font colours and chart series alike.

On the rendering, the split of authority is deliberate. The form and the series
colours belong to the file; recolouring an author's series to satisfy a palette
would misreport the document, the same way reformatting their numbers would. So
the project's visualization guidance is applied to everything the file does *not*
specify: hairline recessive gridlines, axis ticks on round numbers, a value axis
anchored at zero for the forms that grow from a baseline, 2px lines with round
caps, markers of at least 8px carrying a surface ring, a 2px surface gap between
touching marks, an area drawn as a wash rather than a saturated block, a legend
whenever there are two or more series and never for one, no value printed on
every point, and text in text tokens rather than the series colour. Every mark
carries a `<title>` and the chart an `aria-label`, so identity never rests on
colour alone.

A chart declaring a second value axis is plotted on one scale and says so on the
parsed model rather than drawing two, and a frame too small to label legibly says
that instead of drawing an illegible thumbnail.

Validated against fixtures modelled on Excel's own chart XML — no workbook with a
real chart was available on this machine, which is worth knowing when reading the
test coverage.
@AmethystLiang AmethystLiang added the P2 Normal priority: nice-to-have or lower urgency label Aug 10, 2026
@SrAlvarado
feat(editor): draw in-cell sparklines a Sheets export left as formula…
bdbac71 
… text

A budget template downloaded from Google Sheets shows two balance columns and
four progress bars. None of them is a chart object: the file's drawing parts are
empty shells and it declares no chart at all. The bars are `SPARKLINE()` calls,
Sheets' in-cell micro-charts, and the export wraps each one in
`__xludf.DUMMYFUNCTION("…")` — a marker that preserves the formula text — with an
empty cached result. So the cell carries the author's intent and no value, and
opening that file in Excel shows blank cells too.

This draws them. Deliberately more than Excel parity, and worth calling out as a
product decision rather than a fix: the chart is fully described in the file, and
a reader opening the workbook wants to see what its author drew. It sits in its
own commit so it can be dropped without touching the rest.

The formula is parsed for its chart type, its data reference and its options,
undoing the quote doubling the export introduced. A bound may be a literal or a
`MAX`/`MIN` over a range, which is how sibling sparklines share a scale — the two
balance columns are pinned to `MAX(D17:E17)`, so without evaluating it each would
fill its own cell and the comparison the author built would be lost. Anything else
falls back to the data's own bounds rather than guessing. Colours come from the
formula, including `firstcolor` for a single-value column.

The numbers behind the plot do not survive as rendered text, so the sheet parser
also collects raw numeric values and sparkline formulas by cell — gated on a cheap
text probe, so a sheet without sparklines pays nothing.

Column, bar, line and win/loss are drawn, each filling its cell, with the mark
titled and the plot labelled so it is not colour-alone.

Verified against the real template: six sparklines resolve to exactly what Sheets
renders — a navy column at 1000 and an orange one at 1500 sharing a 1500 ceiling,
and grey/navy pairs for planned against actual.
@SrAlvarado
fix(editor): draw a merged sparkline once, and let labels overflow em…
1fd92f2 
…pty columns

Two defects visible on the first render of a real workbook, side by side with how
the same file reads in its source application.

A sparkline in a merged cell was drawn four times. The merge band is painted one
row at a time — rows are virtualized independently, so a real row span is not
available — and the sparkline was drawn inside each of those rows, turning one
column into a stack of four blocks. Sparklines now live in the same overlay layer
as charts and images, positioned over the whole merge, which is the block its
author sized for it. The overlay's geometry and its layer moved into their own
module and component, since the grid was over the line limit.

Labels were clipped where a spreadsheet overflows them. A sheet does not truncate
a long label to its column: the text runs across the neighbours while they are
empty and stops at the first one holding something. "Presupuesto mensual" came
out as "Presupuesto mens…" with four empty columns beside it. A left-aligned
label now reaches across following empty, unfilled columns, bounded so one label
in a sparse sheet cannot widen the grid. A neighbour that carries a fill stops it,
because the text would otherwise run over a coloured block.

The grid's own tests were passing against an empty document: happy-dom reports
every element as zero-sized, so the virtualizers concluded nothing was on screen
and rendered no cells, which made "fewer cells than columns" true of nothing. The
viewport is now stubbed, so those assertions run against a grid that actually
rendered.
@SrAlvarado
fix(editor): render dates by their format code and area gradients
4dfce93 
Opening a real Excel-authored workbook beside the same file in Excel online
showed four differences. Three were the viewer's, and one was a regression from
the previous commit.

Dates ignored the author's format code. Every date rendered ISO, so a cell
formatted `dd/mm/yyyy` read 2025-01-01, and a chart axis cached as serials with
`d\-m` beside them read 46168 where the file says 26-5. Format codes are now
rendered token by token — padded and unpadded day, month and year, month and
weekday names through the viewer's locale, a twelve-hour clock when the code asks
for a meridiem, quoted and escaped literals kept verbatim. The one genuine
ambiguity is handled: `m` is a month on its own and a minute beside an hour or a
second. The ISO form stays as the fallback for a code with no date token, and for
the phantom 1900 leap day, which has no real date behind it.

Area and line charts no longer force zero onto the value axis. Excel starts a
column or bar at zero but auto-scales these, and a weight series between 172 and
178 drawn from zero is a flat line — which is how that chart came out.

Area series gradients are read and painted. Excel writes `a:gradFill` for an area
far more often than a flat colour, and it is the plot's dominant visual: dropping
it left a chart that reads as unfilled. Stops resolve through the theme like every
other colour, and a gradient with fewer than two stops is ignored rather than
treated as one.

The regression: letting a label overflow into empty columns also let it bleed
vertically, so a 24pt title ran over the band beneath it. The span now clips its
own height while still being allowed to be wider than its cell.

Verified against the file that surfaced all of this: the axis reads 26-5, 2-6,
9-6 as Excel shows it, and the series carries its teal-to-amber gradient.

Still not drawn from that chart: its second plot. The file overlays a scatter on
the area to draw a target line, and only the first plot is rendered.
@SrAlvarado
feat(editor): place cell text where the workbook says, vertically and…
43d413d 
… indented

A stylesheet sets `vertical` on almost every cell — 131 times in one real
budget — and the viewer ignored all of them, centering every value instead.
Read `vertical` and `indent` from `<alignment>` and default to bottom, which
is where Excel and Sheets put a value when the author sets nothing.

Numbers were also being left-aligned once a number format grouped their
thousands or added a currency symbol, so a whole budget column read as text.
The rendered string is all the grid gets for a CSV, so widen the inference to
the forms a number format actually writes.

Extracts the cell into SpreadsheetCell.tsx: the grid was at its line limit
and the vertical-alignment and indent branches had nowhere left to go.
@SrAlvarado
feat(editor): paint the conditional formatting a workbook declares
36c7948 
A sheet's highlights live in `<conditionalFormatting>` rather than in a cell's
own style, so a budget that marks every filled category and reddens every
negative total rendered flat. Read the rules, resolve the `<dxf>` each one
paints, and apply them in priority order with `stopIfTrue`.

Scoped to the rules a viewer can decide on its own: cellIs, the blank tests and
the text tests. `expression` rules need a formula evaluator, and colorScale,
dataBar and iconSet render a scale rather than a differential format — all are
dropped rather than guessed at, so a cell keeps its own style.

A `<dxf>` inverts the pattern colours of a normal `<fill>`: the visible fill is
in bgColor and fgColor is left as the system default, so this needs a reader of
its own rather than the one `<fills>` uses.

Also stops `numericValues` collecting from non-numeric cells. It held the `<v>`
of any cell, which for a shared string is its index into the string table — now
that conditional rules compare against those numbers, a `< 5` rule would have
highlighted text cells by their position in the table.
@SrAlvarado
feat(editor): let the reader size a sheet's columns
465ed6e 
A viewer that sizes columns from content alone has no answer for a column whose
values are wider than any sample, or for one the reader simply wants narrower.
Adds a grip on each heading's right edge: drag to size, double-click to size
from content again.

The grip is focusable and takes arrow keys, so column widths are reachable
without a pointer, and it uses pointer capture rather than window listeners
because the pointer leaves a 6px target constantly mid-drag.

A reader's width is stored unzoomed and wins over both the file's declared width
and sizing from content, so a column keeps its proportion across zoom levels.
@SrAlvarado
fix(editor): stop clipping a merged title and a label beside a filled…
55e5ca5 
… range

Two ways text was being cut that a spreadsheet does not cut.

A merge is drawn as a band, because rows are virtualized independently and a
real row span is not available — but the cell owning the value was still bound
to the height of the merge's first row. A 24pt title merged down two rows had
24px to render 33px of text, so its bottom half disappeared. The anchor now
takes the height of every row the merge covers.

And a neighbour's fill was treated as occupying it, which clipped any heading
that sat beside a banded but empty range. A spreadsheet paints an overflowing
label straight over the colour; only content stops it. A cell belonging to a
merge does count as occupied, though — this row reads it as empty because the
value lives in the anchor — so the test is now for a merge, not for a fill.
@SrAlvarado
feat(editor): draw the plots a chart overlays, not just the first
e7d9e8c 
Excel builds a target line by laying a scatterChart over an areaChart in one
plot area. The parser read only the first plot, so a weight chart lost the
orange target line its author added — the series was there in the file with a
name and a colour and never reached the screen.

Every plot is now read, in document order, and each series carries the kind of
the plot it came from so the renderer can draw an area fill and a line in the
same frame. A chart's own kind stays the first plot's, which is what sets the
axis orientation.

Three things a scatter series needs that the others do not: its values live in
`c:yVal` rather than `c:val`, its x positions in `c:xVal` — without which two
points meant to span the plot were squeezed into the first category step — and
`c:scatterStyle` decides whether it is a line or bare points, with `c:symbol`
deciding whether the points show at all. A target line is exactly the case
where all three matter: two points, a line, no markers.

Splits the chart module in three on the way past its line limit: the low-level
part readers, the series reader, and the chart assembler.
@SrAlvarado
feat(editor): let the reader size a sheet's rows, and auto-fit the un…
3495a4c 
…sized ones

Row heights had no answer for content taller than the row: the grip only existed
on column headings, so a reader who could see text being clipped could do nothing
about it. The row heading now carries the same grip on its bottom edge, dragged
with the pointer or sized with Up and Down from the keyboard.

Generalizes the resize hook and the grip over both axes rather than copying them,
with rows bounded tighter than columns — 12 to 400 pixels — because a sheet has
far more of them and a runaway row hides every one after it.

Also auto-fits a row the file leaves without a customHeight, from the largest
font in it. Excel measures those rows itself, which is why the author never set a
height, and using one default for all of them clipped exactly the rows a heading
made taller. A row the file does size explicitly keeps that size, clipping
included: that height is the author's decision, and Excel honours it too.
@SrAlvarado
fix(editor): lift a merged row so the row below stops painting over i…
54def7a 
…ts text

The merged title was still being clipped after being given the height of every
row its merge covers. The cause is CSS, not arithmetic: each virtualized row
carries a `transform` to position itself, and a transform makes the element its
own stacking context. That trapped the cell's `z-index` inside its own row, so
the next row — a later sibling — painted straight over the half of the text
that reached into it.

The lift has to happen on the row, where the elements really are siblings, so a
row owning a merge that reaches downwards is raised as a whole.

Also stops the row virtualizer re-measuring the sheet on every render: the
effect depended on the identity of a callback that changes with cellStyles, so
it fired continuously rather than when a height actually changed.
@SrAlvarado
fix(editor): draw a row-spanning merge's text over the grid, not insi…
f0bfe76 
…de a row

My last two attempts at the clipped merged title were both wrong, and the second
made things worse: giving the anchor cell the merged height made it overflow into
the rows below, and because each virtualized row carries a `transform` and is
therefore its own stacking context, whichever row won the paint order hid the
other's row number. Lifting the anchor row only swapped which numbers vanished —
the anchors' before, the covered rows' after.

The overlay layer already existed for exactly this, and says so: anything taller
than a row has to be positioned over the grid rather than inside it. A merge that
spans rows now hands its value to that layer, which knows the true rectangle, so
no row overflows and no row needs lifting. The cells underneath keep painting the
band's fill and borders.

The anchor cell carries the value as its aria-label, since the text itself now
lives outside the table, and the overlay copy is aria-hidden so it is announced
once.

Removes the two abandoned mechanisms rather than leaving them to be found later.
@SrAlvarado
fix(editor): spill a label the way its alignment points, and past a m…
c88582b 
…erge

Overflow only ever ran rightwards, so every right-aligned heading was clipped
even with an empty column beside it — "SALDO INICIAL" became "SAL…" while the two
columns to its left sat empty. Excel spills a label the way its alignment points:
right-aligned text leftwards, centred text both ways.

A merge did not spill at all, so a title merged across four columns was clipped
at the merge's own edge rather than running into the empty columns after it. That
is the same rule, and it is why a wide title fits a merge narrower than itself;
the merge's own columns are excluded from the test that stops the spill, or it
would stop itself immediately.

Verified against a real budget: the title's box grows from 344px to 663px, and
"SALDO INICIAL" gains 307px of reach where it previously had none.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@brennanb2025 brennanb2025 Awaiting requested review from brennanb2025 brennanb2025 is a code owner

Assignees

@brennanb2025 brennanb2025

Labels

P2 Normal priority: nice-to-have or lower urgency

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@SrAlvarado @AmethystLiang @brennanb2025