From 7d3e27fb6db867e418130ff38a9e0e054ad34370 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Sat, 3 Oct 2026 17:21:41 -0400 Subject: [PATCH] fix(preview): index rows and cells by their present keys, cap theme size ExcelJS keeps a row's cells at `_cells[col - 1]`, so a row whose only cell sits in XFD is a dictionary-mode array that `eachCell` and `hasValues` (behind `eachRow`) walk to index 16,384. The worker walked each row four times at load and once per tile, so a small file of far-column rows took seconds to load and to tile. `worksheetMetadata` now builds each sheet's row and cell index from `Object.keys(sheet._rows)` and `Object.keys(row._cells)`, sorted numerically, skipping falsy and Null-type cells exactly as `eachCell({ includeEmpty: false })` does and keeping a row only when it holds one such cell (`hasValues`). Styles, extent and row heights come from that one pass and merges from `sheet._merges`; `sendTile` reads each row's cells from the index. `parseThemePalette` returns the default palette for a theme above 64 * 1024 characters, since its patterns are quadratic on unclosed tags. --- CLAUDE.md | 2 +- docs/architecture-invariants.md | 2 +- src/web/public/spreadsheet-preview-worker.js | 75 +++++++++---- src/web/public/spreadsheet-preview.js | 2 +- src/web/public/spreadsheet-xlsx-core.js | 8 ++ test/spreadsheet-preview-worker.test.ts | 110 +++++++++++++++++++ test/spreadsheet-xlsx-core.test.ts | 17 +++ 7 files changed, 193 insertions(+), 23 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e74878e3..6feba5bd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -289,7 +289,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Attachments** (live external document references; all wiring in `file-routes.ts`): a **registry** maps a stable `attachmentId` to a realpath-resolved, extension-allowlisted absolute path, so browser requests never carry arbitrary absolute paths. ⚠️ The **magic-link scanner** (`codeman://attach?...` in terminal output) is **prompt-injectable**, so its scan path is force-confined to the session workspace; a hostile prompt could otherwise exfiltrate arbitrary host files over SSE. The security gate is an extension **allowlist**, not a blocklist. `document-conversion-limiter.ts` caps converter spawns globally: without it, N large docs detected at once fork N multi-minute processes, which is a resource-exhaustion vector. → [architecture-invariants#attachments](docs/architecture-invariants.md#attachments) -**File-path links (terminal + chat)**: a path an agent prints is clickable on BOTH surfaces and opens the file-preview overlay. ⚠️ ONE pattern (`FILE_PATH_LINK_PATTERN` / `absoluteFilePathPattern()` in constants.js) feeds the xterm link provider AND `_linkifyFilePaths()`, a fresh instance per call (`lastIndex`). The chat linkifier walks TEXT NODES with DOM APIs, never rebuilds sanitized markup as a string. ⚠️ An out-of-workspace path goes through the ATTACHMENT routes (`POST /api/sessions/:id/attachments` with `notify: false`), never by widening `file-content`/`file-raw` or `file-stream-manager`'s `tail -f` allowlist. ⚠️ `TEXT_ATTACHMENT_EXTENSIONS` IS `EDITABLE_EXTENSIONS` (never a second list), and widening READ must never widen RUN: `html`/`htm`/`svg` stay download-only, other text is inert `text/plain`+`nosniff`. Media extensions are single-sourced in `attachment-registry.ts`. ⚠️ **XLSX previews parse in the BROWSER**, never on the server: `spreadsheet-preview.js` fetches the raw route with `?preview=true` (413 above `MAX_XLSX_BROWSER_PREVIEW_BYTES`, 10 MB) and hands the bytes to `spreadsheet-preview-worker.js`, the only place the pinned `exceljs`/`fflate` vendor bundles load (never on page load). `admitXlsx()` caps the ZIP before ExcelJS runs, counting what ExcelJS will EXPAND as well as what it reads (a merge costs its area, a `` past 16384 is refused, validations are never parsed via `ignoreNodes`, and defined names are never expanded: the worker stubs `_definedNames.model`). ⚠️ The INDEX a row or sheet claims is bounded too, since ExcelJS allocates and walks up to it: a `` outside 1-1048576 and an `xl/workbook.xml` `` above `LIMITS.maxSheetId` (65535) are refused, and `sendTile` reuses the merges read at load (`mergesById`), never `sheet.model`, which rebuilds the whole sheet. ⚠️ Admission keys every entry on the name ExcelJS will SEE (`excelJsEntryName()`: JSZip's `.`/`..`/empty-segment resolution, then one leading `/` stripped), refuses two entries that land on one name, and treats anything matching ExcelJS's UNANCHORED `xl/worksheets/sheet.xml` as a worksheet, so `/xl/...` or `xl/./...` cannot skip a counter, and ExcelJS parses only a STORE-only archive rebuilt from the entries admission inflated (`buildAdmittedArchive()`), never the fetched bytes; cell text goes through `textContent`, formulas are never evaluated. Bumping either package or editing the worker/core changes `SPREADSHEET_ASSET_VERSION`, which `npm run check:public-assets` pins. xls/ods stay download-only. → [architecture-invariants#file-path-links-terminal--response-viewer](docs/architecture-invariants.md#file-path-links-terminal--response-viewer) +**File-path links (terminal + chat)**: a path an agent prints is clickable on BOTH surfaces and opens the file-preview overlay. ⚠️ ONE pattern (`FILE_PATH_LINK_PATTERN` / `absoluteFilePathPattern()` in constants.js) feeds the xterm link provider AND `_linkifyFilePaths()`, a fresh instance per call (`lastIndex`). The chat linkifier walks TEXT NODES with DOM APIs, never rebuilds sanitized markup as a string. ⚠️ An out-of-workspace path goes through the ATTACHMENT routes (`POST /api/sessions/:id/attachments` with `notify: false`), never by widening `file-content`/`file-raw` or `file-stream-manager`'s `tail -f` allowlist. ⚠️ `TEXT_ATTACHMENT_EXTENSIONS` IS `EDITABLE_EXTENSIONS` (never a second list), and widening READ must never widen RUN: `html`/`htm`/`svg` stay download-only, other text is inert `text/plain`+`nosniff`. Media extensions are single-sourced in `attachment-registry.ts`. ⚠️ **XLSX previews parse in the BROWSER**, never on the server: `spreadsheet-preview.js` fetches the raw route with `?preview=true` (413 above `MAX_XLSX_BROWSER_PREVIEW_BYTES`, 10 MB) and hands the bytes to `spreadsheet-preview-worker.js`, the only place the pinned `exceljs`/`fflate` vendor bundles load (never on page load). `admitXlsx()` caps the ZIP before ExcelJS runs, counting what ExcelJS will EXPAND as well as what it reads (a merge costs its area, a `` past 16384 is refused, validations are never parsed via `ignoreNodes`, and defined names are never expanded: the worker stubs `_definedNames.model`). ⚠️ The INDEX a row or sheet claims is bounded too, since ExcelJS allocates and walks up to it: a `` outside 1-1048576 and an `xl/workbook.xml` `` above `LIMITS.maxSheetId` (65535) are refused, and `sendTile` reuses the merges read at load (`mergesById`), never `sheet.model`, which rebuilds the whole sheet. ⚠️ The COLUMN index costs the same way (a row's cells live at `_cells[col - 1]`, so one `XFD` cell makes ExcelJS's `eachRow`/`eachCell`/`hasValues` visit 16,384 slots per row): `worksheetMetadata` builds each sheet's row and cell index from the keys that exist (`Object.keys` of `_rows` and `_cells`, skipping falsy and `Null`-type cells as `eachCell({ includeEmpty: false })` does), reads row heights in that pass and merges from `sheet._merges`, and `sendTile` reads cells from that index (`populatedRowsById`); never call ExcelJS's dense `eachRow`/`eachCell` there. `parseThemePalette` returns the default palette for a theme above 64 KB (64 * 1024 characters), since its patterns are quadratic on unclosed tags. ⚠️ Admission keys every entry on the name ExcelJS will SEE (`excelJsEntryName()`: JSZip's `.`/`..`/empty-segment resolution, then one leading `/` stripped), refuses two entries that land on one name, and treats anything matching ExcelJS's UNANCHORED `xl/worksheets/sheet.xml` as a worksheet, so `/xl/...` or `xl/./...` cannot skip a counter, and ExcelJS parses only a STORE-only archive rebuilt from the entries admission inflated (`buildAdmittedArchive()`), never the fetched bytes; cell text goes through `textContent`, formulas are never evaluated. Bumping either package or editing the worker/core changes `SPREADSHEET_ASSET_VERSION`, which `npm run check:public-assets` pins. xls/ods stay download-only. → [architecture-invariants#file-path-links-terminal--response-viewer](docs/architecture-invariants.md#file-path-links-terminal--response-viewer) **Filesystem path picker** (Link Existing "Browse" + the mobile keyboard's `📁 Path` key): lazy one-directory browsing via `GET /api/filesystem/browse`, with `GET /api/filesystem/preview` for the tapped file. Inserts the path **without** Enter, so the prompt is never submitted; the sibling `⌫ All` key clears only the unsent prompt and must never send the agent's `/clear`. ⚠️ This is a **second file-serving surface and inherits neither the attachment confinement nor its ownership scoping** — it allowlists Home, `CASES_DIR`, `/mnt/d` and `CODEMAN_FILE_PICKER_ROOTS`, blocks sensitive trees, and rejects symlink escapes **after** `realpath`. ⚠️ The optional `sessionId` is an ownership boundary that must be `canAccessOwned`-checked by hand (it does not go through `findSessionOrFail`), and in multi-user mode a non-admin gets only their own `userSpacePath` as a root: per-user spaces live INSIDE `homedir()`, so a `Home` root exposes every other user's workspace. Previews go through the same global conversion limiter, and Markdown/TXT/JSON are served as inert `text/plain`. → [architecture-invariants#filesystem-path-picker](docs/architecture-invariants.md#filesystem-path-picker) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index fdd9eb02..cbac82ab 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -362,7 +362,7 @@ A file path an agent prints is a link on both surfaces it can appear on, and cli ⚠️ **The preview overlay must outrank the panel that launched it.** `.file-preview-overlay` sits at `z-index: 5100`, above the response viewer (5000) and its backdrop (4999); at its historical 2000 a path clicked in the chat opened the overlay *behind* the chat, which reads as a dead link. It stays below the toast/picker band (10000+) so a "Saved" toast still lands on top. -**XLSX previews parse in the browser worker, and ExcelJS only ever sees what admission checked.** An `.xlsx` joins the raw routes like any other file (`?preview=true` caps it at 10 MB with a 413); the server never parses it. `spreadsheet-preview.js` hands the bytes to `spreadsheet-preview-worker.js`, the ONLY place the pinned `exceljs`/`fflate` vendor bundles load: fflate and `spreadsheet-xlsx-core.js` at worker start, ExcelJS only after `admitXlsx()` passes, and the page itself never loads either. `admitXlsx()` streams every entry through fflate and enforces the entry count, per-entry and total inflated bytes (64 MB), compression ratio, worksheet, cell, merge and style caps on the actual inflated output, refusing (never truncating) a workbook that trips one. ⚠️ Admission walks LOCAL headers while ExcelJS (JSZip) reads the CENTRAL directory, so overlapping entries (a stored entry hiding a whole `sheet1.xml`, a one-cell decoy later in the stream) once let a file be admitted as 1 cell and parsed as 300k. The worker therefore never hands ExcelJS the fetched bytes: it gets `buildAdmittedArchive()`, a STORE-only `fflate.zipSync(entries, { level: 0 })` of exactly the entries admission inflated, and a name streamed twice is refused. That costs one transient copy of the inflated entries (bounded by the same 64 MB cap) and is pinned by the overlapping-entry fixture in `test/spreadsheet-preview-worker.test.ts`. ⚠️ Admission also has to bound what ExcelJS EXPANDS, not just what it reads: ExcelJS 4.4.0 builds one object per covered cell of a ``, per address of a `` and per index up to ``, so a 6.5 KB file admitted as one cell once cost a gigabyte. The counter therefore charges each merge its full AREA against the cell caps (an unparseable `ref` is refused), refuses a `` whose `min`/`max` is past 16384, and the worker loads with `ignoreNodes: ['dataValidations']` (the preview never shows validations). Defined names expand the same way and live in `xl/workbook.xml`, where admission reads only the `` ids: ExcelJS's `DefinedNames` model setter builds one object per cell of every range (a whole-sheet name exhausted a 4 GB heap), so right after `new ExcelJS.Workbook()` the worker replaces `_definedNames.model` with an `Object.defineProperty` stub whose getter returns `[]` and whose setter drops the value. Print areas and titles are split off in the workbook xform's reconcile, before that setter runs, so they are unaffected, and `defineProperty` throws if a future ExcelJS renames `_definedNames` rather than silently expanding again. ⚠️ Every name admission uses is the one ExcelJS will SEE, not the stored one: JSZip resolves each entry name on load (`utils.resolve`: `.` and empty middle segments dropped, `..` pops a segment) and ExcelJS then strips one leading `/` and matches worksheets with an UNANCHORED `xl/worksheets/sheet.xml`. Checking the stored name once let `/xl/worksheets/sheet1.xml`, `xl/./...`, `xl//...`, `xl/xl/worksheets/sheet1.xml` and `xl/worksheets/sheet1.xml.x` skip every counter. `excelJsEntryName()` mirrors both steps; admission refuses two entries that resolve to one name, keys `inflatedEntries` (so the rebuilt archive) on the resolved name, picks the worksheet and styles counters from it, and counts any name matching ExcelJS's unanchored worksheet pattern as a worksheet. The local-versus-central consistency checks still compare the stored names. The counter reads ``/`` attributes IN ORDER from the tag name with a sticky regex that consumes each quoted value whole (a raw `>` or the other quote character is legal inside a value, so a first-match search could be fed a fake `ref`/`max`), and refuses a tag whose attributes do not parse up to `>` or repeat a name. It counts every `` (ExcelJS keeps one Row object per element, cells or not) against per-sheet and total row caps, and reads each ``'s attributes the same way. ⚠️ The INDEX a row or sheet claims is a cost too, not just how many there are: ExcelJS stores a row at `_rows[r - 1]` and walks `_rows` up to the largest index on every `eachRow` and `sheet.model` (five rows plus one `` took 5 s to load and 1.7 s per tile), and stores a sheet at `_worksheets[sheetId]`, which its `worksheets` getter slices and sorts (`sheetId="30000000"` on a one-cell workbook took 1.6 s and 557 MB). So a `` that is not plain digits in 1 to 1,048,576 is refused (an absent `r` is fine), and a counter for the resolved `xl/workbook.xml` reads every `` tag in order and refuses one that does not parse or whose `sheetId` is not plain digits up to `LIMITS.maxSheetId` (65535; real ids are small, and the `(?=[\s/>])` lookahead keeps the `` container out). The counter also counts every `` in styles.xml with no list-tracking state (a `` inside a comment used to desync it). It carries everything from the last `<` into the next inflate chunk, since `<` can never appear inside an attribute value, and a central-directory `compressedSize` that runs past the file is refused, since the ratio cap divides by it. Fixtures for each are in `test/spreadsheet-preview-worker.test.ts` and `test/spreadsheet-xlsx-core.test.ts`. ⚠️ `sendTile` must never read `sheet.model`, which rebuilds every row and cell model (10-19 ms per tile on a 100k-cell sheet, once per animation frame while scrolling): each sheet's merges are read once in `worksheetMetadata` and kept in `mergesById` next to `populatedRowsById` (replaced on load, cleared on dispose), and a worker test makes the `model` getter throw before requesting a tile. Number formats cap decimals at 30, as Excel does, since `toLocaleString` throws above 100. The worker is a stable URL cache-busted by `SPREADSHEET_ASSET_VERSION` in `spreadsheet-preview.js`, the content hash of the worker, the core and both vendor bundles; `npm run check:public-assets` fails when it drifts, so editing any of those (or bumping either package) means updating the token, or a deploy pairs a new worker with a year-cached old core. +**XLSX previews parse in the browser worker, and ExcelJS only ever sees what admission checked.** An `.xlsx` joins the raw routes like any other file (`?preview=true` caps it at 10 MB with a 413); the server never parses it. `spreadsheet-preview.js` hands the bytes to `spreadsheet-preview-worker.js`, the ONLY place the pinned `exceljs`/`fflate` vendor bundles load: fflate and `spreadsheet-xlsx-core.js` at worker start, ExcelJS only after `admitXlsx()` passes, and the page itself never loads either. `admitXlsx()` streams every entry through fflate and enforces the entry count, per-entry and total inflated bytes (64 MB), compression ratio, worksheet, cell, merge and style caps on the actual inflated output, refusing (never truncating) a workbook that trips one. ⚠️ Admission walks LOCAL headers while ExcelJS (JSZip) reads the CENTRAL directory, so overlapping entries (a stored entry hiding a whole `sheet1.xml`, a one-cell decoy later in the stream) once let a file be admitted as 1 cell and parsed as 300k. The worker therefore never hands ExcelJS the fetched bytes: it gets `buildAdmittedArchive()`, a STORE-only `fflate.zipSync(entries, { level: 0 })` of exactly the entries admission inflated, and a name streamed twice is refused. That costs one transient copy of the inflated entries (bounded by the same 64 MB cap) and is pinned by the overlapping-entry fixture in `test/spreadsheet-preview-worker.test.ts`. ⚠️ Admission also has to bound what ExcelJS EXPANDS, not just what it reads: ExcelJS 4.4.0 builds one object per covered cell of a ``, per address of a `` and per index up to ``, so a 6.5 KB file admitted as one cell once cost a gigabyte. The counter therefore charges each merge its full AREA against the cell caps (an unparseable `ref` is refused), refuses a `` whose `min`/`max` is past 16384, and the worker loads with `ignoreNodes: ['dataValidations']` (the preview never shows validations). Defined names expand the same way and live in `xl/workbook.xml`, where admission reads only the `` ids: ExcelJS's `DefinedNames` model setter builds one object per cell of every range (a whole-sheet name exhausted a 4 GB heap), so right after `new ExcelJS.Workbook()` the worker replaces `_definedNames.model` with an `Object.defineProperty` stub whose getter returns `[]` and whose setter drops the value. Print areas and titles are split off in the workbook xform's reconcile, before that setter runs, so they are unaffected, and `defineProperty` throws if a future ExcelJS renames `_definedNames` rather than silently expanding again. ⚠️ Every name admission uses is the one ExcelJS will SEE, not the stored one: JSZip resolves each entry name on load (`utils.resolve`: `.` and empty middle segments dropped, `..` pops a segment) and ExcelJS then strips one leading `/` and matches worksheets with an UNANCHORED `xl/worksheets/sheet.xml`. Checking the stored name once let `/xl/worksheets/sheet1.xml`, `xl/./...`, `xl//...`, `xl/xl/worksheets/sheet1.xml` and `xl/worksheets/sheet1.xml.x` skip every counter. `excelJsEntryName()` mirrors both steps; admission refuses two entries that resolve to one name, keys `inflatedEntries` (so the rebuilt archive) on the resolved name, picks the worksheet and styles counters from it, and counts any name matching ExcelJS's unanchored worksheet pattern as a worksheet. The local-versus-central consistency checks still compare the stored names. The counter reads ``/`` attributes IN ORDER from the tag name with a sticky regex that consumes each quoted value whole (a raw `>` or the other quote character is legal inside a value, so a first-match search could be fed a fake `ref`/`max`), and refuses a tag whose attributes do not parse up to `>` or repeat a name. It counts every `` (ExcelJS keeps one Row object per element, cells or not) against per-sheet and total row caps, and reads each ``'s attributes the same way. ⚠️ The INDEX a row or sheet claims is a cost too, not just how many there are: ExcelJS stores a row at `_rows[r - 1]` and walks `_rows` up to the largest index on every `eachRow` and `sheet.model` (five rows plus one `` took 5 s to load and 1.7 s per tile), and stores a sheet at `_worksheets[sheetId]`, which its `worksheets` getter slices and sorts (`sheetId="30000000"` on a one-cell workbook took 1.6 s and 557 MB). So a `` that is not plain digits in 1 to 1,048,576 is refused (an absent `r` is fine), and a counter for the resolved `xl/workbook.xml` reads every `` tag in order and refuses one that does not parse or whose `sheetId` is not plain digits up to `LIMITS.maxSheetId` (65535; real ids are small, and the `(?=[\s/>])` lookahead keeps the `` container out). The counter also counts every `` in styles.xml with no list-tracking state (a `` inside a comment used to desync it). It carries everything from the last `<` into the next inflate chunk, since `<` can never appear inside an attribute value, and a central-directory `compressedSize` that runs past the file is refused, since the ratio cap divides by it. Fixtures for each are in `test/spreadsheet-preview-worker.test.ts` and `test/spreadsheet-xlsx-core.test.ts`. ⚠️ `sendTile` must never read `sheet.model`, which rebuilds every row and cell model (10-19 ms per tile on a 100k-cell sheet, once per animation frame while scrolling): each sheet's merges are read once in `worksheetMetadata` and kept in `mergesById` next to `populatedRowsById` (replaced on load, cleared on dispose), and a worker test makes the `model` getter throw before requesting a tile. ⚠️ The COLUMN index a cell claims costs the same way: ExcelJS keeps a row's cells at `_cells[col - 1]`, so a row whose only cell sits in `XFD` is a dictionary-mode array, and ExcelJS's `row.eachCell` (`_cells.forEach`) and `row.hasValues` (which `sheet.eachRow` calls) visit every index up to 16,384, about 0.5 ms per row (10,000 such rows took 13 s to load in Chromium, and each tile paid it again; `XFD` is a legal column, so admission cannot refuse it). `worksheetMetadata` therefore never calls ExcelJS's dense `eachRow`/`eachCell`: `populatedRowIndex()` walks `Object.keys(sheet._rows)` and, per row, `Object.keys(row._cells)`, sorted numerically, skipping falsy and `ValueType.Null` cells exactly as `eachCell({ includeEmpty: false })` does and keeping a row only when it holds one such cell (ExcelJS's `hasValues`). Styles, the extent and row heights come from that one pass, merges from `sheet._merges` (the map the `model` getter itself lists), and the per-row cell lists are what `populatedRowsById` keeps, so `sendTile` reads a row's cells from the index instead of `row.eachCell`. Worker tests make `eachRow`, `eachCell`, `hasValues` and the `model` getter throw across load and a tile, and load and tile 3,000 one-`XFD`-cell rows. `parseThemePalette` returns the default palette for a theme above 64 KB (64 * 1024 characters of the decoded XML; real themes are under 10 KB): its `clrScheme` and slot patterns rescan to the end of the text for every unclosed opening tag, and a 1 MB theme of repeated `` took 21.8 s in Chromium. Number formats cap decimals at 30, as Excel does, since `toLocaleString` throws above 100. The worker is a stable URL cache-busted by `SPREADSHEET_ASSET_VERSION` in `spreadsheet-preview.js`, the content hash of the worker, the core and both vendor bundles; `npm run check:public-assets` fails when it drifts, so editing any of those (or bumping either package) means updating the token, or a deploy pairs a new worker with a year-cached old core. ### Filesystem path picker diff --git a/src/web/public/spreadsheet-preview-worker.js b/src/web/public/spreadsheet-preview-worker.js index 680e2656..16f1ebde 100644 --- a/src/web/public/spreadsheet-preview-worker.js +++ b/src/web/public/spreadsheet-preview-worker.js @@ -24,6 +24,8 @@ importScripts(`vendor/fflate.min.js${spreadsheetAssetQuery}`, `spreadsheet-xlsx- const core = self.CodemanSpreadsheetXlsxCore; let workbook = null; let sheetsById = new Map(); +// Per sheet: its populated rows in order, each with its populated cells in +// column order, built once at load from the keys that exist (`populatedRowIndex`). let populatedRowsById = new Map(); // Merges read once at load: `sheet.model` rebuilds every row and cell model, // which is far too much to pay on every tile. @@ -78,23 +80,56 @@ function normalizeStyle(cell) { return id; } +// Ascending numeric own keys of a sparse array. ExcelJS keeps rows at +// `_rows[r - 1]` and a row's cells at `_cells[col - 1]`, and one far index puts +// the array in dictionary mode, where its own `eachRow`, `eachCell` and +// `hasValues` (forEach/some) visit every index up to the largest: a single XFD +// cell per row costs 16,384 steps a row. Walking the keys that exist does not. +function presentIndices(sparse) { + const indices = []; + for (const key of Object.keys(sparse || [])) { + const index = Number(key); + if (Number.isInteger(index) && index >= 0) indices.push(index); + } + return indices.sort((a, b) => a - b); +} + +// The rows and cells `sheet.eachRow({ includeEmpty: false })` and +// `row.eachCell({ includeEmpty: false })` would visit, in the same order: a +// cell counts when it exists and its type is not `ValueType.Null`, and a row +// counts when it holds at least one such cell (ExcelJS's `row.hasValues`). +function populatedRowIndex(sheet) { + const nullType = self.ExcelJS.ValueType.Null; + const rows = []; + for (const rowIndex of presentIndices(sheet._rows)) { + const row = sheet._rows[rowIndex]; + if (!row) continue; + const cells = []; + for (const cellIndex of presentIndices(row._cells)) { + const cell = row._cells[cellIndex]; + if (cell && cell.type !== nullType) cells.push(cell); + } + if (cells.length > 0) rows.push({ number: row.number, row, cells }); + } + return rows; +} + function worksheetMetadata(sheet) { const cellRefs = []; - const populatedRows = []; - sheet.eachRow({ includeEmpty: false }, (row) => { - populatedRows.push(row.number); - row.eachCell({ includeEmpty: false }, (cell) => { + const rowOverrides = []; + const populatedRows = populatedRowIndex(sheet); + for (const { number, row, cells } of populatedRows) { + for (const cell of cells) { cellRefs.push(cell.address); normalizeStyle(cell); - }); - }); - const merges = Array.from(sheet.model?.merges || []); + } + if (row.hidden) rowOverrides.push([number, 0]); + else if (row.height) rowOverrides.push([number, Math.min(546, Math.max(0, row.height * (4 / 3)))]); + } + // `sheet.model` rebuilds every row and cell model, so merges come straight + // from ExcelJS's own merge map, in the order the model getter would list them. + const merges = Object.values(sheet._merges || {}).map((merge) => merge.range); const extent = core.deriveExtent(cellRefs, merges); - const rowOverrides = []; - sheet.eachRow({ includeEmpty: false }, (row) => { - if (row.hidden) rowOverrides.push([row.number, 0]); - else if (row.height) rowOverrides.push([row.number, Math.min(546, Math.max(0, row.height * (4 / 3)))]); - }); const columnOverrides = []; for (let col = 1; col <= extent.cols; col += 1) { const column = sheet.getColumn(col); @@ -207,16 +242,16 @@ function sendTile(message) { }); }; const populatedRows = populatedRowsById.get(String(message.sheetId)) || []; - for (const rowNumber of populatedRows) { + for (const populated of populatedRows) { if (truncated) break; - if (rowNumber < range.r1) continue; - if (rowNumber > range.r2) break; - const row = sheet.getRow(rowNumber); - if (row.hidden) continue; - row.eachCell({ includeEmpty: false }, (cell) => { - if (cell.col < range.c1 || cell.col > range.c2) return; + if (populated.number < range.r1) continue; + if (populated.number > range.r2) break; + if (populated.row.hidden) continue; + for (const cell of populated.cells) { + if (cell.col < range.c1) continue; + if (cell.col > range.c2) break; addCell(cell); - }); + } } const merges = core.intersectingMerges(mergesById.get(String(message.sheetId)) || [], range); for (const merge of merges) { diff --git a/src/web/public/spreadsheet-preview.js b/src/web/public/spreadsheet-preview.js index afad0882..ff630d83 100644 --- a/src/web/public/spreadsheet-preview.js +++ b/src/web/public/spreadsheet-preview.js @@ -20,7 +20,7 @@ (function initSpreadsheetPreview(global) { 'use strict'; - const SPREADSHEET_ASSET_VERSION = '5bd72f3f823f'; + const SPREADSHEET_ASSET_VERSION = '6e843e269018'; const MAX_PREVIEW_BYTES = 10 * 1024 * 1024; const DEFAULT_TIMEOUT_MS = 20000; const MAX_SCROLL_PX = 8000000; diff --git a/src/web/public/spreadsheet-xlsx-core.js b/src/web/public/spreadsheet-xlsx-core.js index 7daf0a12..fb71f1da 100644 --- a/src/web/public/spreadsheet-xlsx-core.js +++ b/src/web/public/spreadsheet-xlsx-core.js @@ -744,9 +744,17 @@ return undefined; } + // Real theme1.xml files are under 10 KB. The scheme and slot patterns below + // rescan to the end of the text for every opening tag that has no close, so a + // padded theme is quadratic (1 MB of `` took 21.8 s); above this + // many characters (UTF-16 code units of the decoded XML) the default palette + // is used instead. + const MAX_THEME_XML_CHARS = 64 * 1024; + function parseThemePalette(xml) { const palette = DEFAULT_THEME_PALETTE.slice(); const text = typeof xml === 'string' ? xml : ''; + if (text.length > MAX_THEME_XML_CHARS) return palette; const scheme = /<(?:[A-Za-z0-9_]+:)?clrScheme\b[^>]*>([\s\S]*?)<\/(?:[A-Za-z0-9_]+:)?clrScheme\s*>/.exec(text); if (!scheme) return palette; // Fresh pattern per call: a shared /g regex would carry lastIndex across calls. diff --git a/test/spreadsheet-preview-worker.test.ts b/test/spreadsheet-preview-worker.test.ts index da75b871..1c89f1ea 100644 --- a/test/spreadsheet-preview-worker.test.ts +++ b/test/spreadsheet-preview-worker.test.ts @@ -746,3 +746,113 @@ describe('spreadsheet preview worker: tiles reuse the merges read at load', () = expect(tile.cells).toContainEqual(expect.objectContaining({ row: 4, col: 1, text: 'Merged' })); }); }); + +/** Row and Worksheet prototypes of the ExcelJS build the harness hands the worker. */ +function excelJsPrototypes(): { row: Record; sheet: Record } { + const probe = new ExcelJS.Workbook().addWorksheet('probe'); + return { row: Object.getPrototypeOf(probe.getRow(1)), sheet: Object.getPrototypeOf(probe) }; +} + +/** Make ExcelJS's dense row, cell and model walks throw until the returned restore runs. */ +function forbidDenseWalks(): () => void { + const { row, sheet } = excelJsPrototypes(); + const saved = [ + [sheet, 'eachRow', Object.getOwnPropertyDescriptor(sheet, 'eachRow')], + [row, 'eachCell', Object.getOwnPropertyDescriptor(row, 'eachCell')], + [row, 'hasValues', Object.getOwnPropertyDescriptor(row, 'hasValues')], + ] as const; + for (const [target, name] of saved) { + Object.defineProperty(target, name, { + configurable: true, + get() { + throw new Error(`${name} touched`); + }, + }); + } + // `sheet.model` rebuilds every row and cell model the same dense way; load + // still needs its setter, so only reading it throws. + const model = Object.getOwnPropertyDescriptor(sheet, 'model')!; + Object.defineProperty(sheet, 'model', { + configurable: true, + get() { + throw new Error('model touched'); + }, + set: model.set, + }); + return () => { + for (const [target, name, descriptor] of saved) Object.defineProperty(target, name, descriptor!); + Object.defineProperty(sheet, 'model', model); + }; +} + +describe('spreadsheet preview worker: rows and cells are indexed by their present keys', () => { + // ExcelJS keeps a row's cells at `_cells[col - 1]`; one far-column cell makes + // eachCell and hasValues (behind eachRow) visit every index up to 16,384. + it('loads and serves a tile without ExcelJS eachRow, eachCell, hasValues or sheet.model', async () => { + const harness = createHarness(); + const bytes = await fixture(); + const restore = forbidDenseWalks(); + try { + const metadata = await loadMetadata(harness, bytes); + expect(metadata.sheets[0]).toMatchObject({ rows: 4, cols: 3 }); + expect(metadata.sheets[0].rowOverrides).toContainEqual([2, 40]); + expect(metadata.sheets[0].merges).toEqual(['A4:C4']); + const tile = await requestTile(harness, metadata.sheets[0].id, { r1: 1, c1: 1, r2: 4, c2: 3 }); + expect(tile.type, JSON.stringify(tile)).toBe('tile'); + expect( + (tile.cells as Array<{ row: number; col: number; text: string }>).map(({ row, col, text }) => [row, col, text]) + ).toEqual([ + [1, 1, 'Revenue'], + [2, 2, '$1,234.50'], + [3, 3, '$1,234.50'], + [4, 1, 'Merged'], + ]); + expect(tile.merges).toEqual(['A4:C4']); + } finally { + restore(); + } + }); + + // eachCell and eachRow skip a cell whose value is Null (a styled empty ``), + // and a row holding only such cells; the key walk must skip them the same way. + it('skips value-less cells and the rows that hold only them, as ExcelJS eachRow/eachCell do', async () => { + const bytes = await emptyRowsWorkbook( + '7' + ); + const harness = createHarness(); + const metadata = await loadMetadata(harness, bytes); + // The styled empty cells really exist in ExcelJS, as Null-type cells. + expect(harness.peek('sheetsById.values().next().value.findCell(3, 5)?.type')).toBe(0); + expect(harness.peek('sheetsById.values().next().value.findCell(4, 2)?.type')).toBe(0); + expect(metadata.sheets[0]).toMatchObject({ rows: 4, cols: 3 }); + expect(metadata.sheets[0].rowOverrides).toEqual([]); + const tile = await requestTile(harness, metadata.sheets[0].id, { r1: 1, c1: 1, r2: 4, c2: 5 }); + expect((tile.cells as Array<{ row: number; col: number }>).map(({ row, col }) => [row, col])).toEqual([ + [1, 1], + [4, 3], + ]); + }); + + it('loads and tiles 3,000 rows that each hold one XFD cell', async () => { + const rows = Array.from( + { length: 3_000 }, + (_, i) => `${i + 2}` + ).join(''); + const bytes = await emptyRowsWorkbook(rows); + const harness = createHarness(); + const restore = forbidDenseWalks(); + try { + const started = performance.now(); + const metadata = await loadMetadata(harness, bytes); + expect(metadata.sheets[0]).toMatchObject({ rows: 3_001, cols: 16_384 }); + expect(metadata.sheets[0].rowOverrides).toHaveLength(3_000); + const tile = await requestTile(harness, metadata.sheets[0].id, { r1: 1, c1: 16_380, r2: 3_001, c2: 16_384 }); + expect(tile.type, JSON.stringify(tile).slice(0, 200)).toBe('tile'); + expect(tile.cells).toHaveLength(2_500); + expect(tile.cells[0]).toMatchObject({ row: 2, col: 16_384, text: '2' }); + expect(performance.now() - started).toBeLessThan(5_000); + } finally { + restore(); + } + }, 60_000); +}); diff --git a/test/spreadsheet-xlsx-core.test.ts b/test/spreadsheet-xlsx-core.test.ts index 0c16bf46..c33af908 100644 --- a/test/spreadsheet-xlsx-core.test.ts +++ b/test/spreadsheet-xlsx-core.test.ts @@ -412,6 +412,23 @@ const themeXml = ` `; describe('spreadsheet XLSX colour resolution', () => { + // The clrScheme and slot patterns rescan to the end of the text for every + // unclosed opening tag, so a padded theme is quadratic. + it('uses the default palette for a theme above 64 KB, even a 1 MB run of unclosed clrScheme tags', () => { + const padded = ''.repeat(Math.ceil((1024 * 1024) / 13)); + expect(padded.length).toBeGreaterThanOrEqual(1024 * 1024); + const started = performance.now(); + expect(core.parseThemePalette(padded)).toEqual(core.DEFAULT_THEME_PALETTE); + expect(performance.now() - started).toBeLessThan(1_000); + + // The cap is on characters: a real theme padded to exactly 64 KB still parses, one more does not. + const fill = (length: number) => + themeXml.replace('', ``); + expect(fill(64 * 1024)).toHaveLength(64 * 1024); + expect(core.parseThemePalette(fill(64 * 1024))[4]).toBe('#ff0000'); + expect(core.parseThemePalette(fill(64 * 1024 + 1))).toEqual(core.DEFAULT_THEME_PALETTE); + }); + it('parses a theme palette into styles.xml index order, swapping lt/dk against clrScheme order', () => { const palette = core.parseThemePalette(themeXml); expect(palette).toHaveLength(12);