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.
This commit is contained in:
Aamer Akhter
2026-10-05 09:51:04 -04:00
parent 0ae5ce017a
commit 51b6be3e7a
9 changed files with 142 additions and 11 deletions
+1 -1
View File
@@ -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 `<col>` 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 `<row r>` outside 1-1048576 and an `xl/workbook.xml` `<sheet sheetId>` 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 `<numFmt>` 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<N>.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 `<col>` 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 `<row r>` outside 1-1048576 and an `xl/workbook.xml` `<sheet sheetId>` 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 `<numFmt>` 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 `<r/>` 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<N>.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)
File diff suppressed because one or more lines are too long
+2 -1
View File
@@ -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)),
});
}
+1 -1
View File
@@ -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;
+50 -7
View File
@@ -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 (`<r/>`, 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,
+4
View File
@@ -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;
+22
View File
@@ -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);
});
+9
View File
@@ -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);
+52
View File
@@ -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)}`);