From 51b6be3e7aec3c37880e556438dfd49b6af61886 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Mon, 5 Oct 2026 09:51:04 -0400 Subject: [PATCH] fix(preview): bound rich-text run walks, fold format notices, match Excel number display - richTextPrefix visits at most maxCellTextChars + 1 runs. An empty run adds no text, so a length check alone walked every run of a shared string for every cell referencing it, on every tile. - Two or more unsupported number format warnings in a tile fold into one "N unsupported number formats" entry, and the notice bar has a max-height and scrolls. - General-format and unsupported-format numbers render at 15 significant digits, as Excel does (0.1+0.2 shows 0.3). - TIME_FORMAT accepts a trailing AM/PM, so h:mm AM/PM renders as 2:30 PM instead of falling back to a date. - SPREADSHEET_ASSET_VERSION refreshed. --- CLAUDE.md | 2 +- docs/architecture-invariants.md | 2 +- src/web/public/spreadsheet-preview-worker.js | 3 +- src/web/public/spreadsheet-preview.js | 2 +- src/web/public/spreadsheet-xlsx-core.js | 57 +++++++++++++++++--- src/web/public/styles.css | 4 ++ test/spreadsheet-preview-worker.test.ts | 22 ++++++++ test/spreadsheet-preview.test.ts | 9 ++++ test/spreadsheet-xlsx-core.test.ts | 52 ++++++++++++++++++ 9 files changed, 142 insertions(+), 11 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 280f003f..9fa3160f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -292,7 +292,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. ⚠️ 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. ⚠️ Merges are capped per sheet (`LIMITS.maxMergesPerSheet`, 2,000) AND workbook-wide (`LIMITS.maxMerges`, 10,000), refused as `merge-limit` before ExcelJS loads, since ExcelJS's `_mergeCellsInternal` checks each merge against every earlier one on its sheet (quadratic). Every `` in `xl/styles.xml` is read with `readTagAttributes()` and refused (`number-format`) when its decoded `formatCode` is over 255 characters or has a `[` after its last `]`: ExcelJS's `isDateFmt` rescans to the end of the code per unclosed `[` once per numeric cell, and the code is echoed into the notice bar. ⚠️ `formatCellValue` caps every cell's display text at `LIMITS.maxCellTextChars` (1,000, ellipsis, never splitting a surrogate pair) for every value shape (rich text, hyperlink text, formula source, errors), since structured clone copies each tile cell's whole string to the page and the load timeout no longer covers tiles. ⚠️ 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. ⚠️ Merges are capped per sheet (`LIMITS.maxMergesPerSheet`, 2,000) AND workbook-wide (`LIMITS.maxMerges`, 10,000), refused as `merge-limit` before ExcelJS loads, since ExcelJS's `_mergeCellsInternal` checks each merge against every earlier one on its sheet (quadratic). Every `` in `xl/styles.xml` is read with `readTagAttributes()` and refused (`number-format`) when its decoded `formatCode` is over 255 characters or has a `[` after its last `]`: ExcelJS's `isDateFmt` rescans to the end of the code per unclosed `[` once per numeric cell, and the code is echoed into the notice bar. ⚠️ `formatCellValue` caps every cell's display text at `LIMITS.maxCellTextChars` (1,000, ellipsis, never splitting a surrogate pair) for every value shape (rich text, hyperlink text, formula source, errors), since structured clone copies each tile cell's whole string to the page and the load timeout no longer covers tiles. `richTextPrefix` also visits at most `maxCellTextChars + 1` runs, not only keeps that much text: an empty `` adds nothing, so a shared string of a million empty runs was walked whole per cell, per tile (56 s a tile). ⚠️ 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 6fd2d06f..e9cbe8c2 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -371,7 +371,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. ⚠️ 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. ⚠️ Merges are also capped per sheet (`LIMITS.maxMergesPerSheet`, 2,000) and across the workbook (`LIMITS.maxMerges`, 10,000), both refused as `merge-limit` before ExcelJS loads: on top of the area each merge costs, ExcelJS's `Worksheet._mergeCellsInternal` checks every new merge against every earlier one on its sheet (`_.each(this._merges)`, rebuilding `Object.keys` per call), so a sheet's cost is quadratic in its merge count (one sheet at the old 5,000 cap took 1.6 s in the worker harness, and 20 such sheets ran past the 20 s load timeout in Chromium; the new caps keep the worst case near 1.3 s). ⚠️ The styles counter reads every `` (cellXfs' and dxfs' alike; the lookahead skips ``) with `readTagAttributes()` and refuses, as `number-format`, a `formatCode` that is over 255 characters (Excel's own limit) or has a `[` after its last `]`, checked on the value as ExcelJS's XML parser hands it over (entities and character references decoded, so `[` cannot hide a `[`; ExcelJS's own backslash unescape only removes characters, so it cannot reopen a bracket). ExcelJS runs `utils.isDateFmt` once per numeric cell at load, and its `fmt.replace(/\[[^\]]*]/g, '')` rescans to the end of the code for every `[` with no later `]` (a 60,000-character code of `[` cost 2.2 s per numeric cell; even 255 unclosed characters add up at the cell cap), while a closed code is linear. The length cap also bounds the `Unsupported number format: ` notice, and the refusal messages never echo the code. ⚠️ Cell text reaching the page is bounded in the worker: `formatCellValue` caps every display string at `LIMITS.maxCellTextChars` (1,000; the last kept character is an ellipsis and a cut never splits a surrogate pair) for every value shape, strings, rich text (runs are joined only until the cap is passed), hyperlink text, formula source and results, and errors. Without it each tile cell carried its whole string and structured clone copied it once per cell, after the load timeout had already been cleared by the metadata reply: one 1 MB shared string over a 60 x 20 block left the page unresponsive for 150 s at 9.6 GB. A cell is one `nowrap` line with an ellipsis, so nothing past the column width was ever shown. A worker test asserts every tile cell's `text` is within the cap for a long shared string, rich text and a hyperlink. 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. ⚠️ Merges are also capped per sheet (`LIMITS.maxMergesPerSheet`, 2,000) and across the workbook (`LIMITS.maxMerges`, 10,000), both refused as `merge-limit` before ExcelJS loads: on top of the area each merge costs, ExcelJS's `Worksheet._mergeCellsInternal` checks every new merge against every earlier one on its sheet (`_.each(this._merges)`, rebuilding `Object.keys` per call), so a sheet's cost is quadratic in its merge count (one sheet at the old 5,000 cap took 1.6 s in the worker harness, and 20 such sheets ran past the 20 s load timeout in Chromium; the new caps keep the worst case near 1.3 s). ⚠️ The styles counter reads every `` (cellXfs' and dxfs' alike; the lookahead skips ``) with `readTagAttributes()` and refuses, as `number-format`, a `formatCode` that is over 255 characters (Excel's own limit) or has a `[` after its last `]`, checked on the value as ExcelJS's XML parser hands it over (entities and character references decoded, so `[` cannot hide a `[`; ExcelJS's own backslash unescape only removes characters, so it cannot reopen a bracket). ExcelJS runs `utils.isDateFmt` once per numeric cell at load, and its `fmt.replace(/\[[^\]]*]/g, '')` rescans to the end of the code for every `[` with no later `]` (a 60,000-character code of `[` cost 2.2 s per numeric cell; even 255 unclosed characters add up at the cell cap), while a closed code is linear. The length cap also bounds the `Unsupported number format: ` notice, and the refusal messages never echo the code. ⚠️ Cell text reaching the page is bounded in the worker: `formatCellValue` caps every display string at `LIMITS.maxCellTextChars` (1,000; the last kept character is an ellipsis and a cut never splits a surrogate pair) for every value shape, strings, rich text (runs are joined only until the cap is passed), hyperlink text, formula source and results, and errors. Without it each tile cell carried its whole string and structured clone copied it once per cell, after the load timeout had already been cleared by the metadata reply: one 1 MB shared string over a 60 x 20 block left the page unresponsive for 150 s at 9.6 GB. A cell is one `nowrap` line with an ellipsis, so nothing past the column width was ever shown. A worker test asserts every tile cell's `text` is within the cap for a long shared string, rich text and a hyperlink. ⚠️ Bounding the text kept is not enough for rich text: `richTextPrefix` also visits at most `LIMITS.maxCellTextChars + 1` runs, since any that many non-empty runs already pass the cap. An empty run (``, 4 bytes) adds no text, so a loop that stopped only on length walked every run, and ExcelJS hands every cell referencing a shared string the same `richText` array, so the walk repeated per cell on every tile: one shared string of 200,000 empty runs (a 96 KB file) referenced by a 50 x 50 block took 11.2 s per tile, a million runs 56 s, with tiles requested once per animation frame while scrolling. With the bound the same tiles take about 24 ms. A core test gives a 5,000-run array whose entries past the bound throw on read and asserts `formatCellValue` returns. Notices are bounded too: the worker folds two or more `Unsupported number format` warnings in a tile into one `N unsupported number formats` entry (`foldWarnings`), and `.spreadsheet-preview-notice` has a `max-height` and scrolls, so a sheet with a distinct code per cell cannot push the grid out of view. 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 16f1ebde..13da63b1 100644 --- a/src/web/public/spreadsheet-preview-worker.js +++ b/src/web/public/spreadsheet-preview-worker.js @@ -265,7 +265,8 @@ function sendTile(message) { sheetId: String(message.sheetId), cells, merges, - warnings: Array.from(warnings), + // One counted entry for many unsupported number formats keeps the notice bar short. + warnings: core.foldWarnings(Array.from(warnings)), }); } diff --git a/src/web/public/spreadsheet-preview.js b/src/web/public/spreadsheet-preview.js index eee04d92..95f06e71 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 = 'd83f632d5c69'; + const SPREADSHEET_ASSET_VERSION = '4d543b11c25c'; 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 15c2d994..dabe5d4f 100644 --- a/src/web/public/spreadsheet-xlsx-core.js +++ b/src/web/public/spreadsheet-xlsx-core.js @@ -569,7 +569,8 @@ } const DATE_FORMAT = /^[ymd\-/ ]+$/i; - const TIME_FORMAT = /^[hms: ]+$/i; + // An optional trailing AM/PM (built-in format 18 is `h:mm AM/PM`). + const TIME_FORMAT = /^[hms: ]+(?:AM\/PM)?$/i; const DATE_TIME_FORMAT = /^[ymdhis\-/: ]+$/i; function isFormulaValue(value) { @@ -588,16 +589,50 @@ } // Joins rich-text runs only until the cap is passed, so a long run is never - // copied whole once per cell. + // copied whole once per cell. It also visits at most maxCellTextChars + 1 + // runs: an empty run (``, 4 bytes) adds no text, so a length check alone + // walked every run, for every cell sharing the string, on every tile. Any + // maxCellTextChars + 1 non-empty runs already pass the cap. function richTextPrefix(runs) { let text = ''; - for (const run of runs) { + const visit = Math.min(runs.length, LIMITS.maxCellTextChars + 1); + for (let index = 0; index < visit; index += 1) { if (text.length > LIMITS.maxCellTextChars) break; + const run = runs[index]; if (typeof run?.text === 'string') text += run.text.slice(0, LIMITS.maxCellTextChars + 1); } return text; } + // Excel displays at most 15 significant digits, so `=0.1+0.2` shows 0.3, + // never the binary float's 0.30000000000000004. + function generalNumber(value) { + return Number.isFinite(value) ? String(Number(value.toPrecision(15))) : String(value); + } + + const UNSUPPORTED_FORMAT_PREFIX = 'Unsupported number format: '; + + /** + * Folds every unsupported number format warning into one counted entry when + * there is more than one, so a sheet with a code per cell cannot grow the + * notice bar without bound. A lone warning is kept as is; its code is at + * most 255 characters (admission refuses longer ones). Order is preserved. + */ + function foldWarnings(warnings) { + const formats = warnings.filter((warning) => String(warning).startsWith(UNSUPPORTED_FORMAT_PREFIX)); + if (formats.length < 2) return warnings.slice(); + const folded = []; + let placed = false; + for (const warning of warnings) { + if (!String(warning).startsWith(UNSUPPORTED_FORMAT_PREFIX)) folded.push(warning); + else if (!placed) { + folded.push(`${formats.length} unsupported number formats`); + placed = true; + } + } + return folded; + } + /** * A cell's display text and any warning. The text is ALWAYS at most * `LIMITS.maxCellTextChars` characters, whatever shape the value has. @@ -622,7 +657,7 @@ const formatted = formatCellValueUncapped(serial, fallback, date1904); return /^General$/i.test(code) ? formatted - : { text: formatted.text, warning: `Unsupported number format: ${code}` }; + : { text: formatted.text, warning: `${UNSUPPORTED_FORMAT_PREFIX}${code}` }; } if (typeof value === 'object') { if (isFormulaValue(value)) { @@ -642,7 +677,7 @@ } const code = String(format || 'General'); if (typeof value !== 'number') return { text: String(value) }; - if (/^General$/i.test(code)) return { text: String(value) }; + if (/^General$/i.test(code)) return { text: generalNumber(value) }; if (DATE_FORMAT.test(code)) { const date = excelDate(value, Boolean(date1904)); const yyyy = date.getUTCFullYear(); @@ -652,9 +687,16 @@ } if (TIME_FORMAT.test(code)) { const seconds = Math.round((value - Math.floor(value)) * 86400) % 86400; - const hh = String(Math.floor(seconds / 3600)).padStart(2, '0'); + const hours = Math.floor(seconds / 3600); const mm = String(Math.floor((seconds % 3600) / 60)).padStart(2, '0'); const ss = String(seconds % 60).padStart(2, '0'); + if (/AM\/PM$/i.test(code)) { + const hour12 = String(hours % 12 || 12); + const hh = /hh/i.test(code) ? hour12.padStart(2, '0') : hour12; + const clock = /s/i.test(code) ? `${hh}:${mm}:${ss}` : `${hh}:${mm}`; + return { text: `${clock} ${hours < 12 ? 'AM' : 'PM'}` }; + } + const hh = String(hours).padStart(2, '0'); return { text: `${hh}:${mm}:${ss}` }; } if (DATE_TIME_FORMAT.test(code)) { @@ -685,7 +727,7 @@ (percent ? '%' : ''), }; } - return { text: String(value), warning: `Unsupported number format: ${code}` }; + return { text: generalNumber(value), warning: `${UNSUPPORTED_FORMAT_PREFIX}${code}` }; } // Colour resolution -------------------------------------------------------- @@ -923,6 +965,7 @@ computeViewport, intersectingMerges, formatCellValue, + foldWarnings, DEFAULT_THEME_PALETTE, INDEXED_PALETTE, MIN_CONTRAST_RATIO, diff --git a/src/web/public/styles.css b/src/web/public/styles.css index 9ff199a6..5e8a7531 100644 --- a/src/web/public/styles.css +++ b/src/web/public/styles.css @@ -19318,6 +19318,10 @@ html[data-session-list="sidebar"][data-sidebar="collapsed"] .btn-sidebar-toggle .spreadsheet-preview-notice { flex: 0 0 auto; + /* A long warning list scrolls inside the bar instead of pushing the grid out of view. */ + max-height: 4.5em; + overflow-y: auto; + overflow-wrap: anywhere; padding: 5px 10px; color: var(--warning, #f59e0b); font-size: 12px; diff --git a/test/spreadsheet-preview-worker.test.ts b/test/spreadsheet-preview-worker.test.ts index cbf3f9f0..4df7b18f 100644 --- a/test/spreadsheet-preview-worker.test.ts +++ b/test/spreadsheet-preview-worker.test.ts @@ -986,3 +986,25 @@ describe('spreadsheet preview worker: number formats ExcelJS would rescan', () = expect(tile.warnings).toEqual([`Unsupported number format: ${code}`]); }, 60_000); }); + +describe('spreadsheet preview worker: the notice bar stays bounded', () => { + // Each distinct unsupported code was its own notice entry; a 40 x 20 sheet + // with a code per cell grew the bar to thousands of pixels. + it('folds many distinct unsupported number formats in one tile into one counted warning', async () => { + const workbook = new ExcelJS.Workbook(); + const sheet = workbook.addWorksheet('Formats'); + for (let row = 1; row <= 40; row += 1) { + for (let col = 1; col <= 20; col += 1) { + const cell = sheet.getCell(row, col); + cell.value = row * col + 0.5; + cell.numFmt = `"c${row}-${col}"0.00E+00`; + } + } + const harness = createHarness(); + const metadata = await loadMetadata(harness, await writeWorkbook(workbook)); + const tile = await requestTile(harness, metadata.sheets[0].id, { r1: 1, c1: 1, r2: 40, c2: 20 }); + expect(tile.type, JSON.stringify(tile).slice(0, 200)).toBe('tile'); + expect(tile.cells).toHaveLength(800); + expect(tile.warnings).toEqual(['800 unsupported number formats']); + }, 60_000); +}); diff --git a/test/spreadsheet-preview.test.ts b/test/spreadsheet-preview.test.ts index 1836e2f4..24263730 100644 --- a/test/spreadsheet-preview.test.ts +++ b/test/spreadsheet-preview.test.ts @@ -176,6 +176,15 @@ describe('spreadsheet preview renderer', () => { expect(document.body.textContent).toContain('Spreadsheet parser failed'); }); + // The bar sits above the grid in a flex column; unbounded, enough warnings + // pushed the grid out of view. + it('clamps the notice bar height and scrolls its overflow', () => { + const css = readFileSync(resolve(import.meta.dirname, '../src/web/public/styles.css'), 'utf8'); + const rule = /\.spreadsheet-preview-notice\s*\{([^}]*)\}/.exec(css)?.[1] || ''; + expect(rule).toMatch(/max-height:\s*\d/); + expect(rule).toMatch(/overflow-y:\s*auto/); + }); + it('shows an explicit empty-sheet state without dropping workbook warnings', async () => { const fetchMock = vi.fn(async () => ({ ok: true, arrayBuffer: async () => new ArrayBuffer(8) })); const renderer = loadRenderer(fetchMock); diff --git a/test/spreadsheet-xlsx-core.test.ts b/test/spreadsheet-xlsx-core.test.ts index 427a1f70..980d6741 100644 --- a/test/spreadsheet-xlsx-core.test.ts +++ b/test/spreadsheet-xlsx-core.test.ts @@ -26,6 +26,7 @@ type Core = { computeViewport(axis: unknown, offset: number, viewportSize: number, overscan?: number): [number, number]; intersectingMerges(merges: string[], range: { r1: number; c1: number; r2: number; c2: number }): string[]; formatCellValue(value: unknown, format: string, date1904?: boolean): { text: string; warning?: string }; + foldWarnings(warnings: string[]): string[]; DEFAULT_THEME_PALETTE: string[]; INDEXED_PALETTE: string[]; parseThemePalette(xml?: string): string[]; @@ -428,6 +429,25 @@ describe('spreadsheet XLSX core', () => { expect(/[\uD800-\uDBFF](?![\uDC00-\uDFFF])/.test(emoji)).toBe(false); }); + // An empty run adds no text, so stopping on the text length alone still + // visited every run, once per cell referencing the shared string, per tile. + it('visits at most maxCellTextChars + 1 rich-text runs, however many are empty', () => { + const bound = core.LIMITS.maxCellTextChars + 1; + const runs: Array<{ text: string }> = Array.from({ length: 5_000 }, () => ({ text: '' })); + runs[0] = { text: 'head' }; + for (let index = bound; index < runs.length; index += 1) { + Object.defineProperty(runs, index, { + get() { + throw new Error(`rich-text run ${index} visited`); + }, + }); + } + expect(core.formatCellValue({ richText: runs }, 'General')).toEqual({ text: 'head' }); + // A run inside the bound still contributes. + runs[bound - 1] = { text: 'tail' }; + expect(core.formatCellValue({ richText: runs }, 'General').text).toBe('headtail'); + }); + it('refuses an entry whose declared compressed size runs past the file', () => { const zip = workbookZip(); const view = new DataView(zip.buffer, zip.byteOffset, zip.byteLength); @@ -483,6 +503,38 @@ describe('spreadsheet XLSX core', () => { expect(core.formatCellValue({ formula: 'SUM(A1:A2)' }, 'General')).toMatchObject({ text: '=SUM(A1:A2)' }); }); + // Excel displays at most 15 significant digits; String() printed the binary noise. + it('shows General and unsupported-format numbers at 15 significant digits, as Excel does', () => { + expect(core.formatCellValue(0.1 + 0.2, 'General').text).toBe('0.3'); + expect(core.formatCellValue(10.1 * 3, 'General').text).toBe('30.3'); + expect(core.formatCellValue({ formula: '0.1+0.2', result: 0.1 + 0.2 }, 'General').text).toBe('0.3'); + expect(core.formatCellValue(0.1 + 0.2, '0.00E+00')).toEqual({ + text: '0.3', + warning: 'Unsupported number format: 0.00E+00', + }); + expect(core.formatCellValue(1234.5, 'General').text).toBe('1234.5'); + expect(core.formatCellValue(-42, 'General').text).toBe('-42'); + }); + + it('renders a time-only AM/PM format as a 12-hour time, not a date', () => { + expect(core.formatCellValue(14.5 / 24, 'h:mm AM/PM')).toEqual({ text: '2:30 PM' }); + expect(core.formatCellValue(0, 'h:mm AM/PM').text).toBe('12:00 AM'); + expect(core.formatCellValue(0.5, 'h:mm AM/PM').text).toBe('12:00 PM'); + expect(core.formatCellValue(9.25 / 24, 'hh:mm:ss AM/PM').text).toBe('09:15:00 AM'); + // ExcelJS loads a time-formatted cell as a Date on the 1899-12-31 epoch day. + expect(core.formatCellValue(new Date(Date.UTC(1899, 11, 31, 14, 30)), 'h:mm AM/PM')).toEqual({ + text: '2:30 PM', + }); + }); + + it('folds unsupported number format warnings into one counted entry', () => { + const many = Array.from({ length: 800 }, (_, index) => `Unsupported number format: 0.0${'0'.repeat(index)}E+0`); + const folded = core.foldWarnings(['charts', ...many, 'Formula has no cached result']); + expect(folded).toEqual(['charts', '800 unsupported number formats', 'Formula has no cached result']); + expect(core.foldWarnings(['Unsupported number format: 0.00E+00'])).toEqual(['Unsupported number format: 0.00E+00']); + expect(core.foldWarnings(['charts'])).toEqual(['charts']); + }); + // toLocaleString throws a RangeError above 100 fraction digits; Excel caps at 30. it('caps a number format at 30 decimals instead of throwing', () => { expect(core.formatCellValue(1.5, `0.${'0'.repeat(120)}`).text).toBe(`1.5${'0'.repeat(29)}`);