From 73017ecca739229e9aecc19fc4d293ffbfd12502 Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Tue, 15 Sep 2026 17:22:06 -0600 Subject: [PATCH 1/3] feat(files): preview a .csv as a data grid in the docked pane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a third viewer to the preview pane. `.csv` is an allowed upload type but had no preview at all — a `.md` attachment opens a modal and a csv card did nothing, so the only way to look at a data file in the app was to ask the agent to read it back, which is the most expensive way to look at a table. Deliberately a data grid, not a spreadsheet view. Cells stay the strings the file contained: no type inference, no locale-aware numbers, no date parsing. A preview that renders 007 as 7 is answering a different question than the one being asked. This does not reopen the .xlsx decision. That was a decline of the client-side *renderers* — SheetJS frozen on npm with unfixed advisories, ExcelJS throwing on any workbook with a native chart. A delimited file needs a parser, not a renderer, and the parser here is 200 lines with no new dependency. - `csv-parse.ts`: RFC 4180 field splitting (quoted delimiters, embedded newlines, doubled quotes), delimiter sniffing that scores consistency across rows rather than raw frequency, BOM handling, all three line endings, and byte/row/column caps. - `csv-viewer.component.ts`: virtualised with `cdk-virtual-scroll- viewport`, so a 40k-row file scrolls rather than showing its first screenful. `@angular/cdk` was already a dependency. Virtualising costs the `` element, so the grid carries explicit ARIA grid roles and reports `aria-rowcount` from the data rather than the rendered window. - `PREVIEW_KIND_MIMES` becomes a list per kind: the recorded MIME is whatever the browser reported at upload, and Windows reports `application/vnd.ms-excel` for a .csv whenever Excel is the registered handler. The extension still chooses the viewer, so .xls stays out. Verified in the browser against a 4,001-row file: virtual scroller exact at depth (row 1001 at scrollTop 32000, 35 rows in the DOM), header and body sharing one track list, horizontal scroll synced, and every text pair clearing WCAG AA in both themes (min 4.63:1). Co-Authored-By: Claude Opus 5 --- .../file-preview/csv-viewer.component.spec.ts | 131 +++++++ .../file-preview/csv-viewer.component.ts | 356 ++++++++++++++++++ .../file-preview-panel.component.ts | 20 +- .../services/file-preview/csv-parse.spec.ts | 227 +++++++++++ .../services/file-preview/csv-parse.ts | 313 +++++++++++++++ .../file-preview-http.service.spec.ts | 35 ++ .../file-preview/file-preview-http.service.ts | 2 +- .../file-preview/file-preview.model.spec.ts | 21 +- .../file-preview/file-preview.model.ts | 52 ++- 9 files changed, 1136 insertions(+), 21 deletions(-) create mode 100644 frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.spec.ts create mode 100644 frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.ts create mode 100644 frontend/ai.client/src/app/session/services/file-preview/csv-parse.spec.ts create mode 100644 frontend/ai.client/src/app/session/services/file-preview/csv-parse.ts diff --git a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.spec.ts b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.spec.ts new file mode 100644 index 00000000..8b36a60b --- /dev/null +++ b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.spec.ts @@ -0,0 +1,131 @@ +import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { beforeEach, describe, expect, it } from 'vitest'; +import { CsvViewerComponent } from './csv-viewer.component'; + +function bytes(text: string): ArrayBuffer { + const encoded = new TextEncoder().encode(text); + return encoded.buffer.slice( + encoded.byteOffset, + encoded.byteOffset + encoded.byteLength, + ) as ArrayBuffer; +} + +describe('CsvViewerComponent', () => { + let fixture: ComponentFixture; + + beforeEach(async () => { + TestBed.resetTestingModule(); + await TestBed.configureTestingModule({ + imports: [CsvViewerComponent], + }).compileComponents(); + + fixture = TestBed.createComponent(CsvViewerComponent); + }); + + function render(csv: string): HTMLElement { + fixture.componentRef.setInput('bytes', bytes(csv)); + fixture.detectChanges(); + return fixture.nativeElement as HTMLElement; + } + + it('renders the header row as column headers', () => { + const el = render('name,qty\nwidget,3\n'); + + const headers = Array.from(el.querySelectorAll('[role="columnheader"]')) + .map((n) => n.textContent?.trim()) + // The row-number gutter is a columnheader too, labelled for AT. + .filter((t) => t !== '#'); + expect(headers).toEqual(['name', 'qty']); + }); + + it('reports the true row count to assistive tech, not the rendered window', () => { + // The whole point of virtualising: only a slice of rows is in the + // DOM at any time, so aria-rowcount has to come from the data. + const rows = Array.from({ length: 500 }, (_, i) => `${i},x`).join('\n'); + const el = render(`a,b\n${rows}\n`); + + const grid = el.querySelector('[role="grid"]'); + expect(grid?.getAttribute('aria-rowcount')).toBe('501'); // 500 + header + expect(grid?.getAttribute('aria-colcount')).toBe('2'); + }); + + it('summarises size and delimiter in the footer', () => { + const el = render('a\tb\n1\t2\n3\t4\n'); + + expect(el.querySelector('footer')?.textContent).toContain('2 rows'); + expect(el.querySelector('footer')?.textContent).toContain('2 columns'); + expect(el.querySelector('footer')?.textContent).toContain('tab-separated'); + }); + + it('singularises a one-row, one-column file', () => { + const el = render('name\nwidget\n'); + + expect(el.querySelector('footer')?.textContent).toContain('1 row'); + expect(el.querySelector('footer')?.textContent).toContain('1 column'); + }); + + it('gives header and body rows one shared track list', () => { + // Header and rows line up only because they share this custom + // property — measuring after layout would be a frame too late. + const el = render('a,b\n1,2\n'); + + const grid = el.querySelector('[role="grid"]') as HTMLElement; + const template = grid.style.getPropertyValue('--csv-cols'); + // Gutter plus one track per column. + expect(template.trim().split(/\s+/)).toHaveLength(3); + }); + + it('explains a file with headers but no rows', () => { + const el = render('name,qty\n'); + + expect(el.textContent).toContain('no data rows'); + expect(el.querySelector('cdk-virtual-scroll-viewport')).toBeNull(); + }); + + it('emits rendered once a grid is on screen', () => { + const seen: number[] = []; + fixture.componentInstance.rendered.subscribe(() => seen.push(1)); + + render('a,b\n1,2\n'); + + expect(seen).toHaveLength(1); + }); + + it('emits renderFailed with the parser message for an empty file', () => { + const failures: string[] = []; + fixture.componentInstance.renderFailed.subscribe((m) => failures.push(m)); + + render(''); + + expect(failures).toEqual(['This file is empty.']); + expect(fixture.nativeElement.querySelector('[role="grid"]')).toBeNull(); + }); + + it('does not emit rendered when the parse failed', () => { + const seen: number[] = []; + fixture.componentInstance.rendered.subscribe(() => seen.push(1)); + + render(' \n'); + + expect(seen).toHaveLength(0); + }); + + it('clears the grid when the bytes go away', () => { + render('a,b\n1,2\n'); + expect(fixture.nativeElement.querySelector('[role="grid"]')).not.toBeNull(); + + fixture.componentRef.setInput('bytes', null); + fixture.detectChanges(); + + expect(fixture.nativeElement.querySelector('[role="grid"]')).toBeNull(); + }); + + it('warns when a cap cut the preview short', () => { + const wide = Array.from({ length: 300 }, (_, i) => String(i)).join(','); + const el = render(`${wide}\n${wide}\n`); + + expect(el.querySelector('footer')?.textContent).toContain( + 'later columns not shown', + ); + }); +}); diff --git a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.ts b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.ts new file mode 100644 index 00000000..19d36513 --- /dev/null +++ b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.ts @@ -0,0 +1,356 @@ +import { + CdkVirtualScrollViewport, + ScrollingModule, +} from '@angular/cdk/scrolling'; +import { + ChangeDetectionStrategy, + Component, + ElementRef, + computed, + effect, + input, + output, + signal, + viewChild, +} from '@angular/core'; +import { CsvParseError, CsvTable, parseCsv } from '../../../../services/file-preview/csv-parse'; + +/** + * Height of one body row, in CSS px. + * + * Load-bearing: `cdk-virtual-scroll-viewport`'s fixed-size strategy + * computes the spacer height and the rendered window from this number, + * so a row that paints taller than it claims leaves the rows drifting + * out of the viewport as you scroll. Keep it in step with the row's + * `h-8` and the cell padding below. + */ +const ROW_HEIGHT_PX = 32; + +/** Column width bounds, in CSS px. */ +const COL_MIN_PX = 88; +const COL_MAX_PX = 320; + +/** Approximate px per character at the grid's text size, for sizing + * columns without laying anything out first. */ +const PX_PER_CHAR = 7.5; + +/** Rows sampled when measuring a column's natural width. Measuring all + * of them would walk 50k rows to move a column by a few px. */ +const WIDTH_SAMPLE_ROWS = 100; + +/** + * Renders a delimited data file (`.csv`, `.tsv`) as a scrollable grid. + * + * Deliberately a *data* view and not a spreadsheet view. Cells are the + * strings the file contained, rendered left-aligned and unformatted: + * there is no type inference, no locale-aware number rendering and no + * date parsing, because the user is previewing what the file will hand + * to whatever reads it next, and a preview that prettifies `007` into + * `7` is answering a different question than the one being asked. + * + * Virtualised with `cdk-virtual-scroll-viewport` rather than capped at a + * few hundred rows. `@angular/cdk` is already a dependency (dialog, + * menu, overlay, a11y), uniform row height is the fixed-size strategy's + * best case, and the alternative — "showing the first 500 rows of + * 40,000" — is the difference between previewing a file and previewing + * the top of one. The parse caps in `csv-parse.ts` still apply; they + * bound the backing array, while this bounds the DOM. + * + * The cost of virtualising is that the grid cannot be a `
`: the + * viewport needs to own the scroll container and transform its content, + * which `` will not tolerate. So it is a div grid carrying + * explicit ARIA grid roles, with `aria-rowcount` and `aria-rowindex` + * reporting the true size of the data rather than the size of the + * rendered window. + */ +@Component({ + selector: 'app-csv-viewer', + changeDetection: ChangeDetectionStrategy.OnPush, + imports: [ScrollingModule], + template: ` + @if (table(); as data) { +
+ +
+
+
+ +
+ @for (header of data.headers; track $index) { +
+ {{ header }} +
+ } +
+
+ + @if (data.rows.length === 0) { +

+ This file has column headers but no data rows. +

+ } @else { + +
+
+ {{ i + 1 }} +
+ @for (cell of row; track $index) { +
+ {{ cell }} +
+ } +
+ + } + +
+ {{ summary() }} + @if (data.truncated) { + + {{ truncationNotice() }} + + } +
+
+ } + `, + styles: ` + :host { + display: block; + height: 100%; + } + + /* Header and body rows share one track list so the columns line up + without measuring anything after layout. */ + .csv-row { + display: grid; + grid-template-columns: var(--csv-cols); + align-items: center; + height: 32px; + width: max-content; + min-width: 100%; + } + + .csv-cell, + .csv-gutter { + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + padding-inline: 0.75rem; + font-size: 0.8125rem; + line-height: 1.25rem; + font-variant-numeric: tabular-nums; + } + + /* Colour deliberately lives on the elements, as the utility pair + text-gray-500 / dark:text-gray-400, rather than here. There is no + single neutral step that clears WCAG AA against both surfaces: + gray-400 measures 2.6:1 on the light grid, and gray-500 is the + step the surface generator itself reports as short of AA against + the dark one. (No backticks in this block - one inside a styles + template literal breaks the Angular compiler while tsc passes.) */ + .csv-gutter { + text-align: right; + padding-inline: 0.5rem; + font-size: 0.6875rem; + user-select: none; + } + + /* The viewport is the horizontal scroller as well as the vertical + one, so rows wider than the rail can be reached. */ + cdk-virtual-scroll-viewport { + overflow-x: auto; + } + `, +}) +export class CsvViewerComponent { + readonly bytes = input(null); + + /** Emitted once a grid is on screen, so the pane drops its skeleton. */ + readonly rendered = output(); + /** Emitted with a user-facing message when the file cannot be read. */ + readonly renderFailed = output(); + + protected readonly rowHeight = ROW_HEIGHT_PX; + protected readonly table = signal(null); + + private readonly headerScroller = + viewChild>('headerScroller'); + private readonly viewport = viewChild(CdkVirtualScrollViewport); + + /** One `grid-template-columns` track list, shared by the header row + * and every body row. */ + protected readonly gridTemplate = computed(() => { + const data = this.table(); + if (!data) return ''; + const gutter = `${gutterWidthFor(data.rows.length)}px`; + const columns = data.headers.map((header, i) => + `${measureColumn(header, data.rows, i)}px`, + ); + return [gutter, ...columns].join(' '); + }); + + protected readonly summary = computed(() => { + const data = this.table(); + if (!data) return ''; + const rows = data.rows.length; + const cols = data.headers.length; + return `${rows.toLocaleString()} ${rows === 1 ? 'row' : 'rows'} · ${cols} ${ + cols === 1 ? 'column' : 'columns' + } · ${DELIMITER_LABELS[data.delimiter]}`; + }); + + protected readonly truncationNotice = computed(() => { + const by = this.table()?.truncatedBy; + switch (by) { + case 'bytes': + return '· preview stops partway through a large file'; + case 'rows': + return '· later rows not shown'; + case 'columns': + return '· later columns not shown'; + default: + return ''; + } + }); + + constructor() { + effect(() => { + const bytes = this.bytes(); + if (!bytes) { + this.table.set(null); + return; + } + + try { + this.table.set(parseCsv(bytes)); + } catch (e) { + this.table.set(null); + this.renderFailed.emit( + e instanceof CsvParseError + ? e.message + : "This file couldn't be read as delimited text.", + ); + return; + } + + this.rendered.emit(); + }); + + effect((onCleanup) => { + const viewport = this.viewport(); + if (!viewport) return; + const sub = viewport.elementScrolled().subscribe(() => { + this.syncHeaderScroll(); + }); + onCleanup(() => sub.unsubscribe()); + }); + } + + /** `trackBy` on the row index rather than the row: a data file may + * legitimately repeat identical rows, and identity tracking would + * make the virtual scroller reuse the wrong one. */ + protected trackByIndex(index: number): number { + return index; + } + + /** + * Mirror the viewport's horizontal offset onto the header. + * + * Driven by `elementScrolled()` rather than a template `(scroll)` + * binding or `(scrolledIndexChange)`. `scrolledIndexChange` is the + * wrong signal outright — it fires when the first *rendered row* + * changes, so it never fires for a purely horizontal scroll and the + * header stays behind while the body moves. `elementScrolled()` emits + * for both axes, and CDK's scroll dispatcher already runs it outside + * the Angular zone; since the handler only writes `scrollLeft` on a + * DOM node and touches no signal, the sync costs no change detection + * at scroll rate. + */ + private syncHeaderScroll(): void { + const header = this.headerScroller()?.nativeElement; + const viewport = this.viewport(); + if (header && viewport) { + header.scrollLeft = viewport.measureScrollOffset('left'); + } + } +} + +const DELIMITER_LABELS: Readonly> = { + ',': 'comma-separated', + '\t': 'tab-separated', + ';': 'semicolon-separated', + '|': 'pipe-separated', +}; + +/** Enough room for the largest row number the gutter will show. */ +function gutterWidthFor(rowCount: number): number { + return Math.max(40, String(rowCount).length * 8 + 16); +} + +/** + * Width for one column, from the longest value in a sample of its cells. + * + * Approximated from character counts rather than measured, because + * measuring means laying out every cell before the first paint. The + * clamp matters more than the estimate: a column of long free text stops + * at `COL_MAX_PX` and ellipsises (the full value is on the cell's + * `title`), and a column of short codes still gets a readable minimum. + */ +function measureColumn( + header: string, + rows: readonly string[][], + index: number, +): number { + let longest = header.length; + const sampled = Math.min(rows.length, WIDTH_SAMPLE_ROWS); + for (let i = 0; i < sampled; i++) { + const cell = rows[i][index]; + if (cell && cell.length > longest) longest = cell.length; + } + return Math.min( + COL_MAX_PX, + Math.max(COL_MIN_PX, Math.round(longest * PX_PER_CHAR) + 24), + ); +} diff --git a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/file-preview-panel.component.ts b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/file-preview-panel.component.ts index 6d304025..5a3b7589 100644 --- a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/file-preview-panel.component.ts +++ b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/file-preview-panel.component.ts @@ -22,6 +22,7 @@ import { import { ConfigService } from '../../../../../services/config.service'; import { downloadUrlFor } from '../../../../../shared/utils/file-download-url'; import { TooltipDirective } from '../../../../../components/tooltip/tooltip.directive'; +import { CsvViewerComponent } from './csv-viewer.component'; import { DocxViewerComponent } from './docx-viewer.component'; import { PptxViewerComponent } from './pptx-viewer.component'; import { @@ -31,8 +32,8 @@ import { } from '../../../../services/file-preview/file-preview.model'; /** - * Right-docked pane that previews one uploaded Office file in the - * browser — `.docx` and `.pptx` today. + * Right-docked pane that previews one uploaded file in the browser — + * `.docx`, `.pptx` and `.csv` today. * * Shares the rail with `ArtifactPanelComponent` through * `DockedPaneService` — same width, same resize affordance, same @@ -52,7 +53,13 @@ import { @Component({ selector: 'app-file-preview-panel', changeDetection: ChangeDetectionStrategy.OnPush, - imports: [NgIcon, TooltipDirective, DocxViewerComponent, PptxViewerComponent], + imports: [ + NgIcon, + TooltipDirective, + CsvViewerComponent, + DocxViewerComponent, + PptxViewerComponent, + ], providers: [ provideIcons({ heroArrowDownTray, @@ -166,6 +173,13 @@ import { (rendered)="onRendered()" /> } + @case ('csv') { + + } @default { { + it('reads a plain comma-delimited file', () => { + const table = parseCsv(bytes('name,qty\nwidget,3\ngadget,7\n')); + + expect(table.headers).toEqual(['name', 'qty']); + expect(table.rows).toEqual([ + ['widget', '3'], + ['gadget', '7'], + ]); + expect(table.delimiter).toBe(','); + expect(table.truncated).toBe(false); + }); + + it('keeps cells as strings rather than coercing them', () => { + // The whole reason the grid is not a spreadsheet: a preview that + // renders 007 as 7 misrepresents what the file will hand downstream. + const table = parseCsv(bytes('zip,ratio\n007,3-4\n')); + + expect(table.rows[0]).toEqual(['007', '3-4']); + }); + + describe('RFC 4180 quoting', () => { + it('keeps a delimiter inside a quoted field', () => { + const table = parseCsv(bytes('a,b\n"Boise, ID",2\n')); + + expect(table.rows[0]).toEqual(['Boise, ID', '2']); + }); + + it('keeps a newline inside a quoted field', () => { + const table = parseCsv(bytes('a,b\n"line one\nline two",2\n')); + + expect(table.rows).toHaveLength(1); + expect(table.rows[0][0]).toBe('line one\nline two'); + }); + + it('unescapes a doubled quote', () => { + const table = parseCsv(bytes('a\n"she said ""hi"""\n')); + + expect(table.rows[0][0]).toBe('she said "hi"'); + }); + + it('treats a mid-field quote as literal', () => { + // `12" pipe` is not an opening quote — spreadsheets read it as text. + const table = parseCsv(bytes('part\n12" pipe\n')); + + expect(table.rows[0][0]).toBe('12" pipe'); + }); + }); + + describe('delimiter sniffing', () => { + it('picks the tab in a TSV', () => { + const table = parseCsv(bytes('name\tqty\nwidget\t3\n')); + + expect(table.delimiter).toBe('\t'); + expect(table.headers).toEqual(['name', 'qty']); + }); + + it('picks the semicolon over commas inside prose', () => { + // The comma occurs more often overall, but only the semicolon + // occurs the same number of times in every row. + const table = parseCsv( + bytes( + 'title;note\n' + + 'First;"a, b, c, d"\n' + + 'Second;"e, f, g, h"\n' + + 'Third;"i, j, k, l"\n', + ), + ); + + expect(table.delimiter).toBe(';'); + expect(table.headers).toEqual(['title', 'note']); + expect(table.rows[0]).toEqual(['First', 'a, b, c, d']); + }); + + it('falls back to a comma for a single-column file', () => { + const table = parseCsv(bytes('name\nwidget\ngadget\n')); + + expect(table.delimiter).toBe(','); + expect(table.headers).toEqual(['name']); + expect(table.rows).toEqual([['widget'], ['gadget']]); + }); + }); + + describe('line endings', () => { + it.each([ + ['CRLF', 'a,b\r\n1,2\r\n'], + ['LF', 'a,b\n1,2\n'], + ['CR', 'a,b\r1,2\r'], + ])('handles %s', (_label, text) => { + const table = parseCsv(bytes(text)); + + expect(table.headers).toEqual(['a', 'b']); + expect(table.rows).toEqual([['1', '2']]); + }); + + it('does not add an empty row for a trailing newline', () => { + expect(parseCsv(bytes('a\n1\n')).rows).toEqual([['1']]); + }); + + it('reads a final row with no trailing newline', () => { + expect(parseCsv(bytes('a\n1')).rows).toEqual([['1']]); + }); + }); + + describe('ragged rows', () => { + it('pads a short row to the grid width', () => { + const table = parseCsv(bytes('a,b,c\n1,2\n')); + + expect(table.rows[0]).toEqual(['1', '2', '']); + }); + + it('widens the grid for a row with extra fields', () => { + // Showing the stray field is the point — clipping it to the + // header's width would hide the malformed row we are previewing. + const table = parseCsv(bytes('a,b\n1,2,3\n')); + + expect(table.headers).toEqual(['a', 'b', 'C']); + expect(table.rows[0]).toEqual(['1', '2', '3']); + }); + }); + + describe('headers', () => { + it('substitutes a spreadsheet letter for a blank header', () => { + const table = parseCsv(bytes(',name\n0,widget\n')); + + expect(table.headers).toEqual(['A', 'name']); + }); + + it('trims surrounding whitespace', () => { + expect(parseCsv(bytes(' name , qty \n1,2\n')).headers).toEqual([ + 'name', + 'qty', + ]); + }); + }); + + it('strips a UTF-8 BOM from the first header', () => { + const table = parseCsv(bytes('name,qty\nwidget,3\n')); + + expect(table.headers).toEqual(['name', 'qty']); + }); + + it('renders a header-only file as an empty grid', () => { + const table = parseCsv(bytes('name,qty\n')); + + expect(table.headers).toEqual(['name', 'qty']); + expect(table.rows).toEqual([]); + }); + + describe('caps', () => { + it('stops at the row cap and reports it', () => { + const rows = Array.from( + { length: CSV_MAX_ROWS + 50 }, + (_, i) => `${i},x`, + ).join('\n'); + const table = parseCsv(bytes(`a,b\n${rows}\n`)); + + expect(table.rows).toHaveLength(CSV_MAX_ROWS - 1); // header took one + expect(table.truncated).toBe(true); + expect(table.truncatedBy).toBe('rows'); + }); + + it('stops at the column cap and reports it', () => { + const wide = Array.from({ length: CSV_MAX_COLUMNS + 10 }, (_, i) => + String(i), + ).join(','); + const table = parseCsv(bytes(`${wide}\n${wide}\n`)); + + expect(table.headers).toHaveLength(CSV_MAX_COLUMNS); + expect(table.rows[0]).toHaveLength(CSV_MAX_COLUMNS); + expect(table.truncated).toBe(true); + expect(table.truncatedBy).toBe('columns'); + }); + }); + + describe('failures', () => { + it('rejects an empty file', () => { + expect(() => parseCsv(bytes(''))).toThrow(CsvParseError); + }); + + it('rejects a whitespace-only file', () => { + expect(() => parseCsv(bytes('\n\n \n'))).toThrow(CsvParseError); + }); + }); + + it('survives bytes that are not valid UTF-8', () => { + // A Windows-1252 smart quote (0x92) is invalid UTF-8. Showing a + // replacement character beats refusing the file. + const raw = new Uint8Array([ + 0x61, 0x0a, 0x92, 0x78, // "a\n<0x92>x" + ]); + const table = parseCsv(raw.buffer as ArrayBuffer); + + expect(table.headers).toEqual(['a']); + expect(table.rows[0][0]).toContain('x'); + }); +}); + +describe('columnLetter', () => { + it.each([ + [0, 'A'], + [25, 'Z'], + [26, 'AA'], + [51, 'AZ'], + [52, 'BA'], + ])('maps %i to %s', (index, expected) => { + expect(columnLetter(index)).toBe(expected); + }); +}); diff --git a/frontend/ai.client/src/app/session/services/file-preview/csv-parse.ts b/frontend/ai.client/src/app/session/services/file-preview/csv-parse.ts new file mode 100644 index 00000000..b9b703c9 --- /dev/null +++ b/frontend/ai.client/src/app/session/services/file-preview/csv-parse.ts @@ -0,0 +1,313 @@ +/** + * A delimited-text parser, sized for previewing a file rather than + * ingesting one. + * + * Hand-rolled on purpose. The grid needs exactly one thing a library + * would give us — RFC 4180 field splitting — and the rest of what a + * parser package ships (streaming, workers, type coercion, header + * mapping, a plugin surface) is weight we would carry in the bundle and + * in `npm audit` forever. The whole of the specification that matters + * here is: fields separated by a delimiter, rows by a newline, a field + * may be quoted, a quote inside a quoted field is doubled, and a quoted + * field may contain the delimiter or a newline. + * + * It is deliberately NOT a spreadsheet: no formulas, no types, no dates. + * Every cell stays the string the file contained, because a preview that + * silently reinterprets `007` as `7` or `3-4` as a date is lying about + * the bytes the user is about to hand to something else. + */ + +/** Delimiters sniffed for, in preference order on a tie. */ +const CANDIDATE_DELIMITERS = [',', '\t', ';', '|'] as const; + +export type CsvDelimiter = (typeof CANDIDATE_DELIMITERS)[number]; + +/** + * Most bytes decoded and parsed. + * + * The file is already fully downloaded by the time we get here, so this + * does not bound the transfer — it bounds the main-thread parse and the + * DOM behind it. 10 MB of CSV is on the order of 100k rows, far past + * what anyone reads in a preview pane, and parses in well under a + * second. Past it we take the leading slice and say so. + */ +export const CSV_MAX_BYTES = 10 * 1024 * 1024; + +/** + * Most rows retained. + * + * A second, independent stop from `CSV_MAX_BYTES`: a file of very short + * rows can stay under the byte cap and still produce enough rows to make + * even a virtualised grid's backing array unpleasant. + */ +export const CSV_MAX_ROWS = 50_000; + +/** + * Most columns retained. + * + * Guards against a file whose quoting is broken badly enough that a + * whole document collapses into one row of many thousands of fields — + * which would otherwise become a `grid-template-columns` with that many + * tracks. + */ +export const CSV_MAX_COLUMNS = 256; + +/** What the grid renders. */ +export interface CsvTable { + /** First row of the file, used as column labels. */ + headers: string[]; + /** Every row after the header, padded to `headers.length`. */ + rows: string[][]; + /** The delimiter that was sniffed. Shown in the pane's subtitle. */ + delimiter: CsvDelimiter; + /** True when a cap stopped us short of the end of the file. */ + truncated: boolean; + /** Which cap fired, for the notice the pane shows. */ + truncatedBy: 'bytes' | 'rows' | 'columns' | null; +} + +/** The file could not be read as delimited text at all. */ +export class CsvParseError extends Error { + constructor(message: string) { + super(message); + this.name = 'CsvParseError'; + } +} + +/** + * Decode bytes to text, honouring a UTF-8 BOM and surviving a file that + * is not valid UTF-8. + * + * `fatal: false` is the point: CSV exported from Excel on Windows is + * routinely Windows-1252, and a preview that refuses the file outright + * is worse than one that shows `�` where a smart quote was. The bytes on + * S3 are untouched either way — this only affects what we draw. + */ +function decode(bytes: ArrayBuffer): string { + const text = new TextDecoder('utf-8', { fatal: false }).decode( + bytes.byteLength > CSV_MAX_BYTES ? bytes.slice(0, CSV_MAX_BYTES) : bytes, + ); + // TextDecoder strips a leading BOM only for the exact `utf-8` label in + // some engines and not others; strip it ourselves so a header never + // renders with an invisible first character. + return text.charCodeAt(0) === 0xfeff ? text.slice(1) : text; +} + +/** + * Pick the delimiter by counting candidates in the first few lines, + * ignoring anything inside quotes. + * + * Counting *consistency* rather than raw frequency is what makes this + * reliable: a prose column full of commas can out-count the real `;` + * delimiter on total occurrences, but only the real delimiter appears + * the same number of times in every row. + */ +function sniffDelimiter(text: string): CsvDelimiter { + const sample = sampleLines(text, 5); + if (sample.length === 0) return ','; + + let best: CsvDelimiter = ','; + let bestScore = -1; + + for (const candidate of CANDIDATE_DELIMITERS) { + const counts = sample.map((line) => countOutsideQuotes(line, candidate)); + // A delimiter that never appears is not a delimiter. + if (counts[0] === 0) continue; + const consistent = counts.every((c) => c === counts[0]); + // Consistency dominates; frequency breaks ties among consistent ones. + const score = (consistent ? 1000 : 0) + counts[0]; + if (score > bestScore) { + bestScore = score; + best = candidate; + } + } + + return best; +} + +/** First `max` physical lines, quote-awareness deliberately skipped — + * this is a sample for counting, not a parse. */ +function sampleLines(text: string, max: number): string[] { + const lines: string[] = []; + let start = 0; + while (lines.length < max && start < text.length) { + let end = text.indexOf('\n', start); + if (end === -1) end = text.length; + const line = text.slice(start, end).replace(/\r$/, ''); + if (line.length > 0) lines.push(line); + start = end + 1; + } + return lines; +} + +function countOutsideQuotes(line: string, delimiter: string): number { + let count = 0; + let inQuotes = false; + for (let i = 0; i < line.length; i++) { + const ch = line[i]; + if (ch === '"') { + inQuotes = !inQuotes; + } else if (!inQuotes && ch === delimiter) { + count++; + } + } + return count; +} + +/** + * Split delimited text into rows of fields. + * + * One pass, character at a time, tracking whether we are inside a quoted + * field. `\r\n`, `\n` and a bare `\r` all end a row, because files + * produced on all three platforms land here. + */ +function splitRows( + text: string, + delimiter: string, +): { rows: string[][]; hitRowCap: boolean } { + const rows: string[][] = []; + let row: string[] = []; + let field = ''; + let inQuotes = false; + let hitRowCap = false; + + const endField = (): void => { + row.push(field); + field = ''; + }; + const endRow = (): boolean => { + row.push(field); + field = ''; + // A trailing newline at end of file would otherwise add a row of one + // empty field. Only drop it when the row is exactly that. + if (!(row.length === 1 && row[0] === '')) rows.push(row); + row = []; + return rows.length >= CSV_MAX_ROWS; + }; + + for (let i = 0; i < text.length; i++) { + const ch = text[i]; + + if (inQuotes) { + if (ch === '"') { + if (text[i + 1] === '"') { + field += '"'; + i++; + } else { + inQuotes = false; + } + } else { + field += ch; + } + continue; + } + + if (ch === '"' && field === '') { + // A quote only opens a quoted field at the start of one. Mid-field + // quotes (`12" pipe`) are literal, which is what every spreadsheet + // does with them. + inQuotes = true; + } else if (ch === delimiter) { + endField(); + } else if (ch === '\n') { + if (endRow()) { + hitRowCap = true; + break; + } + } else if (ch === '\r') { + if (text[i + 1] === '\n') i++; + if (endRow()) { + hitRowCap = true; + break; + } + } else { + field += ch; + } + } + + // Whatever is left when the text runs out is a final row, unless the + // file ended on a newline and left nothing behind. + if (!hitRowCap && (field !== '' || row.length > 0)) endRow(); + + return { rows, hitRowCap }; +} + +/** + * Parse a delimited file into a rectangular table for the preview grid. + * + * Throws `CsvParseError` only when there is nothing to show at all. A + * file that is merely *odd* — ragged rows, duplicate headers, a stray + * quote — is rendered as-is, because the point of a preview is to show + * the user what the file actually contains. + */ +export function parseCsv(bytes: ArrayBuffer): CsvTable { + const truncatedByBytes = bytes.byteLength > CSV_MAX_BYTES; + const text = decode(bytes); + + if (text.trim() === '') { + throw new CsvParseError('This file is empty.'); + } + + const delimiter = sniffDelimiter(text); + const { rows: rawRows, hitRowCap } = splitRows(text, delimiter); + + if (rawRows.length === 0) { + throw new CsvParseError('This file has no rows to show.'); + } + + // The widest row decides the grid, so a row with extra fields is shown + // in full rather than silently clipped to the header's width. + const widest = rawRows.reduce((max, r) => Math.max(max, r.length), 0); + const columnCount = Math.min(widest, CSV_MAX_COLUMNS); + const hitColumnCap = widest > CSV_MAX_COLUMNS; + + const [headerRow, ...bodyRows] = rawRows; + const headers = normalizeHeaders(headerRow, columnCount); + const rows = bodyRows.map((r) => pad(r, columnCount)); + + return { + headers, + rows, + delimiter, + truncated: truncatedByBytes || hitRowCap || hitColumnCap, + truncatedBy: truncatedByBytes + ? 'bytes' + : hitRowCap + ? 'rows' + : hitColumnCap + ? 'columns' + : null, + }; +} + +/** Pad or clip a row to the grid's width. */ +function pad(row: string[], width: number): string[] { + if (row.length === width) return row; + if (row.length > width) return row.slice(0, width); + return [...row, ...Array(width - row.length).fill('')]; +} + +/** + * Column labels, with a placeholder for any the file left blank. + * + * A blank header is common — an index column exported from pandas has + * one — and an empty `
` gives a screen reader nothing to announce + * when it reads a cell's column. The spreadsheet-style letter is the + * familiar stand-in. + */ +function normalizeHeaders(row: string[], width: number): string[] { + return pad(row, width).map( + (h, i) => h.trim() || columnLetter(i), + ); +} + +/** 0 -> A, 25 -> Z, 26 -> AA — the spreadsheet column naming. */ +export function columnLetter(index: number): string { + let n = index; + let out = ''; + do { + out = String.fromCharCode(65 + (n % 26)) + out; + n = Math.floor(n / 26) - 1; + } while (n >= 0); + return out; +} diff --git a/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.spec.ts b/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.spec.ts index 69aaf795..fb34b0f2 100644 --- a/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.spec.ts +++ b/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.spec.ts @@ -87,6 +87,41 @@ describe('FilePreviewHttpService', () => { expect(doc.mimeType).toBe(PPTX_MIME); }); + it('accepts a .csv the browser mislabelled as an Excel type', async () => { + // Windows reports application/vnd.ms-excel for a .csv whenever Excel + // is the registered handler, and the recorded MIME is whatever the + // browser said at upload time. Refusing it would fail the preview on + // the most ordinary desktop in the building. + fetchMock.mockResolvedValue({ + ok: true, + status: 200, + arrayBuffer: () => Promise.resolve(new ArrayBuffer(4)), + }); + + const pending = service.fetchDocument('up1'); + flushPreviewUrl({ + mimeType: 'application/vnd.ms-excel', + filename: 'export.csv', + }); + const doc = await pending; + + expect(doc.kind).toBe('csv'); + }); + + it('still refuses an .xls, which shares that MIME type', async () => { + // The extension is what chooses the viewer, so widening the accepted + // MIME list for .csv must not make the legacy binary format + // previewable. + const pending = service.fetchDocument('up1'); + flushPreviewUrl({ + mimeType: 'application/vnd.ms-excel', + filename: 'budget.xls', + }); + + await expect(pending).rejects.toThrow(FilePreviewError); + expect(fetchMock).not.toHaveBeenCalled(); + }); + it('refuses a file whose MIME type contradicts its extension', async () => { // The extension picked the viewer before any request was made, so a // file named .pptx that the server knows to be a .docx has to fail diff --git a/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.ts b/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.ts index ba95895e..534efba5 100644 --- a/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.ts +++ b/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.ts @@ -88,7 +88,7 @@ export class FilePreviewHttpService { const meta = await this.requestPreviewUrl(uploadId); const kind = previewKindFor(meta.filename); - if (kind === null || meta.mimeType !== PREVIEW_KIND_MIMES[kind]) { + if (kind === null || !PREVIEW_KIND_MIMES[kind].includes(meta.mimeType)) { throw new FilePreviewError( `This file is a ${meta.mimeType || 'unknown type'}, which can't be previewed here.`, false, diff --git a/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.spec.ts b/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.spec.ts index 999bf627..a6529585 100644 --- a/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.spec.ts +++ b/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.spec.ts @@ -6,14 +6,16 @@ import { } from './file-preview.model'; describe('previewKindFor', () => { - it('maps the OOXML formats the pane can render', () => { + it('maps the formats the pane can render', () => { expect(previewKindFor('plan.docx')).toBe('docx'); expect(previewKindFor('deck.pptx')).toBe('pptx'); + expect(previewKindFor('rows.csv')).toBe('csv'); }); it('ignores case and surrounding whitespace', () => { expect(previewKindFor(' REPORT.DOCX ')).toBe('docx'); expect(previewKindFor('Quarterly Review.PPTX')).toBe('pptx'); + expect(previewKindFor('Export.CSV')).toBe('csv'); }); it('declines the pre-2007 binary formats', () => { @@ -21,11 +23,13 @@ describe('previewKindFor', () => { // preview would only produce an error the user cannot act on. expect(previewKindFor('old.doc')).toBeNull(); expect(previewKindFor('old.ppt')).toBeNull(); + expect(previewKindFor('old.xls')).toBeNull(); }); it('declines .xlsx', () => { - // Deliberate: there is no spreadsheet renderer we are willing to - // ship. See the note on previewKindFor. + // Deliberate: there is no client-side spreadsheet renderer we are + // willing to ship. `.csv` is previewable because it needs a parser + // rather than a renderer — see the note on previewKindFor. expect(previewKindFor('budget.xlsx')).toBeNull(); }); @@ -35,13 +39,20 @@ describe('previewKindFor', () => { }); it('agrees with isPreviewableFilename', () => { - for (const name of ['a.docx', 'b.pptx', 'c.xlsx', 'd.txt', 'e.doc']) { + for (const name of [ + 'a.docx', + 'b.pptx', + 'c.xlsx', + 'd.txt', + 'e.doc', + 'f.csv', + ]) { expect(isPreviewableFilename(name)).toBe(previewKindFor(name) !== null); } }); it('labels every kind it can return', () => { - for (const name of ['a.docx', 'b.pptx']) { + for (const name of ['a.docx', 'b.pptx', 'c.csv']) { const kind = previewKindFor(name); expect(kind).not.toBeNull(); expect(PREVIEW_KIND_LABELS[kind!]).toBeTruthy(); diff --git a/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.ts b/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.ts index 02f80005..023f50cb 100644 --- a/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.ts +++ b/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.ts @@ -22,19 +22,42 @@ export const DOCX_MIME = export const PPTX_MIME = 'application/vnd.openxmlformats-officedocument.presentationml.presentation'; +/** + * MIME type of a comma-separated values file. Matches the `.csv` entry + * in `apis.shared.files.ALLOWED_EXTENSIONS`. + */ +export const CSV_MIME = 'text/csv'; + /** What the pane knows how to render, and which viewer does it. */ -export type PreviewKind = 'docx' | 'pptx'; +export type PreviewKind = 'docx' | 'pptx' | 'csv'; /** Human label for the pane header's subtitle. */ export const PREVIEW_KIND_LABELS: Readonly> = { docx: 'Word document', pptx: 'PowerPoint presentation', + csv: 'Data file', }; -/** The MIME type each viewer requires, checked against `/preview-url`. */ -export const PREVIEW_KIND_MIMES: Readonly> = { - docx: DOCX_MIME, - pptx: PPTX_MIME, +/** + * The MIME types each viewer will accept, checked against what + * `/preview-url` reports. + * + * A list rather than a single type because the recorded MIME is + * whatever the *browser* reported at upload time (`request.mime_type` + * in `files/service.py`), not something the server derives from the + * bytes. For the OOXML formats that is reliably the one true type. For + * `.csv` it is not: Windows reports `application/vnd.ms-excel` for a + * `.csv` whenever Excel is the registered handler, and some clients + * send `application/csv` or fall back to `text/plain`. Rejecting those + * would fail the preview on the most ordinary desktop in the building, + * for a file whose extension already told us what it is. + */ +export const PREVIEW_KIND_MIMES: Readonly< + Record +> = { + docx: [DOCX_MIME], + pptx: [PPTX_MIME], + csv: [CSV_MIME, 'application/csv', 'application/vnd.ms-excel', 'text/plain'], }; /** @@ -49,20 +72,25 @@ export const PREVIEW_KIND_MIMES: Readonly> = { * `/preview-url` reports, so a mislabelled `.docx` fails there rather * than feeding garbage to the renderer. * - * Legacy `.doc` and `.ppt` are deliberately excluded: they are the - * pre-2007 binary formats, which the OOXML renderers cannot read at all. + * Legacy `.doc`, `.ppt` and `.xls` are deliberately excluded: they are + * the pre-2007 binary formats, which neither the OOXML renderers nor + * the delimited-text parser can read at all. * - * `.xlsx` is deliberately absent. There is no renderer for it we are - * willing to ship: the npm build of SheetJS is frozen at a 2022 release - * carrying unfixed advisories, and the only maintained grid renderer is - * built on ExcelJS, which throws outright on the workbooks + * `.xlsx` is deliberately absent, and for a narrower reason than it + * looks. There is no client-side *renderer* for it we are willing to + * ship: the npm build of SheetJS is frozen at a 2022 release carrying + * unfixed advisories, and the only maintained grid renderer is built on + * ExcelJS, which throws outright on the workbooks * `create_excel_spreadsheet` produces whenever one contains a native - * chart. Download-and-open remains the path for spreadsheets. + * chart. That rules out reading the bytes here — it does not rule out a + * server-side read that hands this pane rows, which is the open path. + * Until then, download-and-open remains it for `.xlsx`. */ export function previewKindFor(filename: string): PreviewKind | null { const name = filename.trim(); if (/\.docx$/i.test(name)) return 'docx'; if (/\.pptx$/i.test(name)) return 'pptx'; + if (/\.csv$/i.test(name)) return 'csv'; return null; } From e89923b3c25ffcc688b99bcbf9c3017cdbb2898f Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Tue, 15 Sep 2026 18:06:54 -0600 Subject: [PATCH 2/3] feat(files): preview an .xlsx by reading it server-side with openpyxl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the spreadsheet preview. The `.csv` grid shipped in 2c6aebdf parses in the browser; a workbook cannot, so app-api reads it and sends rows. The distinction that unblocked this is that the earlier `.xlsx` decline was of client-side *renderers* — SheetJS frozen on npm with unfixed advisories, ExcelJS raising on any workbook with a native chart, which is what `create_excel_spreadsheet` produces. A grid of values needs a reader, and openpyxl is already what our own spreadsheet tools drive inside Code Interpreter. New dependency: openpyxl==3.1.5 (MIT, pure Python, one dependency). Already present in the rag-ingestion image via docling; this pin brings it into app-api's own closure, where it was not before. Backend - `sheet_preview.py` reads every visible sheet into display strings, streaming with `read_only=True` and stopping at the caps rather than walking a whole sheet to discard most of it. - It reads the workbook TWICE, and that is the point. `data_only=True` returns None for any formula Excel never cached — the normal state of a workbook openpyxl itself wrote, since openpyxl does not evaluate — so a values-only preview would show blanks exactly where the totals belong. The second pass supplies the formula text instead. - Formatting decisions are the server's so the two readers cannot drift: floats lose binary noise (0.1+0.2 renders as 0.3), a date-formatted cell does not gain the midnight openpyxl invents for it, and a numeric string stays a string. - `GET /files/{id}/sheet-preview`, cookie-authenticated like every other user-facing app-api route, distinguishing 404/413/415/422 because each means something different to the user. The size cap is checked against recorded metadata, so an oversized workbook costs a metadata read rather than a transfer into memory. Frontend - `DataGridComponent` extracted from the csv viewer: one virtualised grid now serves both readers, so the ARIA roles, column sizing, header scroll sync and contrast work are shared rather than duplicated. - `XlsxViewerComponent` takes an upload id rather than bytes, with a tab per sheet. `previewFetchesBytes()` keeps the two paths apart, and `fetchDocument` now refuses a server-read kind outright — letting one through would pull a whole workbook into the browser and discard it. Also fixes two things found while verifying in the browser: the truncation notice measured 3.2:1 on the light grid and failed WCAG AA (-700 clears it at 5.03:1), and the per-character column estimate clipped symbol-heavy cells like formula text by a few px. Adds `inline-template-backtick-guard.spec.ts`. A backtick inside an inline template:/styles: block ends the literal early; tsc passes and only the Angular compiler fails, naming the wrong cause. It has cost debugging time three times, twice in this branch. Co-Authored-By: Claude Opus 5 --- backend/pyproject.toml | 7 + backend/src/apis/app_api/files/routes.py | 60 ++++ backend/src/apis/app_api/files/service.py | 79 +++++ .../src/apis/app_api/files/sheet_preview.py | 300 +++++++++++++++++ backend/src/apis/shared/files/__init__.py | 6 + backend/src/apis/shared/files/models.py | 52 +++ .../tests/apis/app_api/test_sheet_preview.py | 251 ++++++++++++++ .../tests/routes/test_sheet_preview_route.py | 199 +++++++++++ backend/uv.lock | 23 ++ .../inline-template-backtick-guard.spec.ts | 107 ++++++ .../file-preview/csv-viewer.component.spec.ts | 2 +- .../file-preview/csv-viewer.component.ts | 285 ++-------------- .../file-preview/data-grid.component.ts | 314 ++++++++++++++++++ .../file-preview-panel.component.ts | 53 ++- .../xlsx-viewer.component.spec.ts | 230 +++++++++++++ .../file-preview/xlsx-viewer.component.ts | 180 ++++++++++ .../file-download-renderer.component.spec.ts | 22 +- .../chat/stream-parser.file-preview.spec.ts | 13 +- .../file-preview-http.service.spec.ts | 60 +++- .../file-preview/file-preview-http.service.ts | 73 ++++ .../file-preview/file-preview.model.spec.ts | 23 +- .../file-preview/file-preview.model.ts | 41 ++- .../file-preview/sheet-preview.model.ts | 32 ++ 23 files changed, 2112 insertions(+), 300 deletions(-) create mode 100644 backend/src/apis/app_api/files/sheet_preview.py create mode 100644 backend/tests/apis/app_api/test_sheet_preview.py create mode 100644 backend/tests/routes/test_sheet_preview_route.py create mode 100644 frontend/ai.client/src/app/inline-template-backtick-guard.spec.ts create mode 100644 frontend/ai.client/src/app/session/components/message-list/components/file-preview/data-grid.component.ts create mode 100644 frontend/ai.client/src/app/session/components/message-list/components/file-preview/xlsx-viewer.component.spec.ts create mode 100644 frontend/ai.client/src/app/session/components/message-list/components/file-preview/xlsx-viewer.component.ts create mode 100644 frontend/ai.client/src/app/session/services/file-preview/sheet-preview.model.ts diff --git a/backend/pyproject.toml b/backend/pyproject.toml index 2f96e6fb..74613d54 100644 --- a/backend/pyproject.toml +++ b/backend/pyproject.toml @@ -36,6 +36,13 @@ dependencies = [ # Security: pin transitive deps to fix Dependabot alerts "pillow==12.3.0", + # Reads .xlsx workbooks into rows for the UI's spreadsheet preview + # (apis/app_api/files/sheet_preview.py). MIT, pure Python, one + # dependency (et-xmlfile). Already the library our own + # create_excel_spreadsheet tool drives inside Code Interpreter, and + # the one the RAG ingestion image uses via docling — this pin brings + # it into app-api's own closure, which it was not in before. + "openpyxl==3.1.5", "cryptography==50.0.1", "python-multipart==0.0.31", "aiohttp==3.14.3", diff --git a/backend/src/apis/app_api/files/routes.py b/backend/src/apis/app_api/files/routes.py index beb6ccc5..29a09341 100644 --- a/backend/src/apis/app_api/files/routes.py +++ b/backend/src/apis/app_api/files/routes.py @@ -18,6 +18,7 @@ PresignResponse, CompleteUploadResponse, PreviewUrlResponse, + SheetPreviewResponse, TextSnippetResponse, ThumbnailResponse, FileListResponse, @@ -34,6 +35,7 @@ FileNotFoundError, FileUploadError, ) +from .sheet_preview import WorkbookTooLargeError, WorkbookUnreadableError from .thumbnails import ThumbnailRenderError, ThumbnailUnsupportedError from apis.shared.security.log_sanitize import scrub_log @@ -227,6 +229,64 @@ async def get_text_snippet( ) +@router.get("/{upload_id}/sheet-preview", response_model=SheetPreviewResponse) +async def get_sheet_preview( + upload_id: str, + user: User = Depends(get_current_user_from_session), + service: FileUploadService = Depends(get_file_upload_service), +): + """ + Read an .xlsx workbook into rows for the UI's data grid. + + The other previews (.docx, .pptx, .csv) hand the browser a presigned + URL and parse the bytes client-side. Spreadsheets cannot: the npm + build of SheetJS is frozen on a release with unfixed advisories, and + ExcelJS raises on any workbook holding a native chart — which is what + `create_excel_spreadsheet` produces. So the workbook is read here and + only values cross the wire. + + Values only. No fills, fonts, borders, merges or charts; download and + open the file for those. + + Status codes: + - 200: Sheets read (possibly truncated — see `truncated` on each). + - 404: File not found, not owned by the caller, or not readable. + - 413: Workbook is past the reader's size cap. + - 415: MIME type is not a readable workbook (the UI should not have + offered a preview; `.xls` lands here). + - 422: File present but unreadable (corrupt, encrypted, not OOXML). + """ + try: + return await service.get_sheet_preview(user.user_id, upload_id) + + except FileNotFoundError: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"File {upload_id} not found or not owned by you", + ) + + except ThumbnailUnsupportedError as e: + raise HTTPException( + status_code=status.HTTP_415_UNSUPPORTED_MEDIA_TYPE, + detail=str(e), + ) + + except WorkbookTooLargeError: + raise HTTPException( + status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE, + detail="This workbook is too large to preview. Download it to open in Excel.", + ) + + except WorkbookUnreadableError: + # Deliberately not echoing openpyxl's message: it names internal + # XML parts and tells the user nothing they can act on. + logger.warning("Workbook could not be parsed") + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="This workbook could not be read. It may be corrupt or password-protected.", + ) + + @router.get("/{upload_id}/thumbnail", response_model=ThumbnailResponse) async def get_thumbnail( upload_id: str, diff --git a/backend/src/apis/app_api/files/service.py b/backend/src/apis/app_api/files/service.py index 6dcd151a..2c4fc027 100644 --- a/backend/src/apis/app_api/files/service.py +++ b/backend/src/apis/app_api/files/service.py @@ -25,6 +25,8 @@ PresignResponse, CompleteUploadResponse, PreviewUrlResponse, + SheetPreviewResponse, + SHEET_PREVIEW_MIME_TYPES, TextSnippetResponse, ThumbnailResponse, THUMBNAIL_SUPPORTED_MIME_TYPES, @@ -35,6 +37,12 @@ is_presentation_file, ALLOWED_MIME_TYPES, ) +from .sheet_preview import ( + MAX_WORKBOOK_BYTES, + WorkbookTooLargeError, + WorkbookUnreadableError, + read_workbook_preview, +) from .thumbnails import ( ThumbnailRenderer, ThumbnailRenderError, @@ -540,6 +548,77 @@ async def get_text_snippet( mime_type=file_meta.mime_type, ) + # ========================================================================= + # Spreadsheet preview + # ========================================================================= + + async def get_sheet_preview( + self, user_id: str, upload_id: str + ) -> SheetPreviewResponse: + """Read an .xlsx into rows the UI can draw in its data grid. + + The workbook never reaches the browser. Unlike the `.docx`, + `.pptx` and `.csv` previews — which fetch the bytes through a + presigned URL and parse them client-side — there is no + client-side spreadsheet reader we are willing to ship, so the + parse happens here and only values cross the wire. + + Args: + user_id: The owner's user ID + upload_id: The upload identifier + + Returns: + SheetPreviewResponse with one entry per visible worksheet + + Raises: + FileNotFoundError: not found, not owned, or not ready + ThumbnailUnsupportedError: MIME type is not a readable workbook + WorkbookTooLargeError: past the reader's byte cap + WorkbookUnreadableError: corrupt, encrypted, or not OOXML + """ + file_meta = await self.repository.get_file(user_id, upload_id) + if not file_meta: + raise FileNotFoundError(f"File {upload_id} not found") + + if file_meta.status != FileStatus.READY: + raise FileNotFoundError( + f"File {upload_id} is not ready (status: {file_meta.status})" + ) + + if file_meta.mime_type not in SHEET_PREVIEW_MIME_TYPES: + raise ThumbnailUnsupportedError( + f"No spreadsheet reader for {file_meta.mime_type}" + ) + + # Checked before the download so an oversized workbook costs a + # metadata read rather than a transfer into memory. + if file_meta.size_bytes > MAX_WORKBOOK_BYTES: + raise WorkbookTooLargeError(file_meta.size_bytes) + + try: + response = self._s3_client.get_object( + Bucket=self.bucket_name, + Key=file_meta.s3_key, + ) + data = response["Body"].read() + except ClientError as e: + logger.warning( + f"Failed to read workbook {scrub_log(upload_id)}: {scrub_log(e)}" + ) + raise FileNotFoundError(f"File {upload_id} could not be read") + + # openpyxl is CPU-bound and blocking, so it runs off the event + # loop. A 20 MB workbook parses for long enough to stall every + # other request on this worker if it does not. + sheets = await asyncio.to_thread(read_workbook_preview, data) + + return SheetPreviewResponse( + upload_id=upload_id, + filename=file_meta.filename, + sheets=sheets, + truncated=any(sheet.truncated for sheet in sheets), + ) + # ========================================================================= # Thumbnails # ========================================================================= diff --git a/backend/src/apis/app_api/files/sheet_preview.py b/backend/src/apis/app_api/files/sheet_preview.py new file mode 100644 index 00000000..b85984df --- /dev/null +++ b/backend/src/apis/app_api/files/sheet_preview.py @@ -0,0 +1,300 @@ +"""Read an .xlsx workbook into rows of display strings for the UI's grid. + +This is the server-side half of the spreadsheet preview. The browser gets +no renderer and no workbook bytes — just headers and rows it draws in the +same grid the `.csv` preview uses. + +**Why this runs here and not in the browser.** Every client-side option +was rejected on its own terms: the npm build of SheetJS is frozen on a +2022 release with unfixed advisories, and ExcelJS — the only base for a +maintained grid renderer — raises on any workbook containing a native +chart, which is exactly what `create_excel_spreadsheet` produces. Both of +those are *renderers*, reproducing Excel's own layout. A grid of values +needs only a reader, and openpyxl is already the library our own +spreadsheet tools drive inside Code Interpreter. + +**What this deliberately is not.** No fills, fonts, borders, merges, +column widths or charts. Download-and-open remains the path for anyone +who needs fidelity. Cells arrive as strings, already formatted for +display, because the alternative is shipping a type tag per cell and +re-implementing the same formatting decisions in TypeScript. +""" + +import logging +from datetime import date, datetime, time, timedelta +from decimal import Decimal +from io import BytesIO +from typing import Any, Iterable, Optional + +from openpyxl import load_workbook +from openpyxl.utils import get_column_letter + +from apis.shared.files.models import SheetPreview + +logger = logging.getLogger(__name__) + + +# Workbook bytes we are willing to pull into memory and parse. Well above +# anything the spreadsheet tools generate, and below the point where a +# request would threaten the container. +MAX_WORKBOOK_BYTES = 25 * 1024 * 1024 + +# Per-sheet caps. A docked pane shows about thirty rows at a time, so +# these are generous for previewing while keeping the JSON small. +MAX_ROWS_PER_SHEET = 500 +MAX_COLUMNS_PER_SHEET = 64 + +# Sheets read from one workbook. +MAX_SHEETS = 12 + +# Global cell budget across every sheet, so a workbook of many wide +# sheets cannot multiply the per-sheet caps into a huge response. +MAX_TOTAL_CELLS = 40_000 + + +class WorkbookTooLargeError(Exception): + """The workbook is past `MAX_WORKBOOK_BYTES`.""" + + def __init__(self, size_bytes: int): + self.size_bytes = size_bytes + super().__init__( + f"Workbook is {size_bytes} bytes, over the {MAX_WORKBOOK_BYTES} limit" + ) + + +class WorkbookUnreadableError(Exception): + """The bytes are not a workbook openpyxl can read.""" + + +def read_workbook_preview(data: bytes) -> list[SheetPreview]: + """Read every visible sheet of an .xlsx into display-ready rows. + + Raises: + WorkbookTooLargeError: past the byte cap. + WorkbookUnreadableError: corrupt, encrypted, or not OOXML. + """ + if len(data) > MAX_WORKBOOK_BYTES: + raise WorkbookTooLargeError(len(data)) + + # Two passes over the same bytes, and both are needed. + # + # `data_only=True` yields the value Excel last cached for a formula + # cell — and `None` when nothing was cached, which is the normal case + # for a workbook openpyxl itself wrote, since openpyxl does no + # evaluation. Every formula in a workbook our own tools generated + # reads back as None, so a values-only preview would show blanks + # exactly where the totals are. + # + # `data_only=False` yields the formula text for those cells. Showing + # "=SUM(B2:B10)" is both more useful than an empty cell and more + # honest: the file really does not carry that number yet. + values = _load(data, data_only=True) + formulas = _load(data, data_only=False) + + try: + sheets: list[SheetPreview] = [] + budget = MAX_TOTAL_CELLS + + for index, value_sheet in enumerate(values.worksheets): + if index >= MAX_SHEETS: + break + # Hidden sheets are hidden for a reason and Excel does not + # show them either. A preview that surfaced scratch sheets + # would misrepresent the workbook. + if getattr(value_sheet, "sheet_state", "visible") != "visible": + continue + + formula_sheet = ( + formulas.worksheets[index] if index < len(formulas.worksheets) else None + ) + sheet, used = _read_sheet(value_sheet, formula_sheet, budget) + budget -= used + sheets.append(sheet) + if budget <= 0: + break + + return sheets + finally: + # `read_only` workbooks hold the zip open until closed. + values.close() + formulas.close() + + +def _load(data: bytes, *, data_only: bool): + """Open the workbook in streaming mode, or say it is unreadable. + + `read_only=True` streams rows instead of building the whole object + graph, which is what makes a cap-and-stop read cheap on a large + sheet. `keep_links=False` avoids resolving external workbook + references — we never show them, and resolving them is work. + """ + try: + return load_workbook( + BytesIO(data), + read_only=True, + data_only=data_only, + keep_links=False, + ) + except Exception as e: # openpyxl raises a wide range on bad input + raise WorkbookUnreadableError(str(e)) from e + + +def _read_sheet(value_sheet, formula_sheet, budget: int) -> tuple[SheetPreview, int]: + """Read one worksheet into a `SheetPreview`, stopping at the caps. + + Returns the preview and how much of the cell budget it consumed. + """ + row_limit = min(MAX_ROWS_PER_SHEET, max(1, budget // MAX_COLUMNS_PER_SHEET)) + + value_rows = _take_rows(value_sheet, row_limit + 1) + formula_rows = _take_rows(formula_sheet, row_limit + 1) if formula_sheet else [] + + merged = [ + _merge_row(value_rows[i], formula_rows[i] if i < len(formula_rows) else ()) + for i in range(len(value_rows)) + ] + + # Trailing empty columns are an artifact of a sheet's used range + # reaching further than its data — a stray format on an empty cell is + # enough. Dropping them keeps the grid from opening with a screenful + # of blanks. + width = 0 + for row in merged: + for i in range(len(row) - 1, -1, -1): + if row[i] != "": + width = max(width, i + 1) + break + + hit_column_cap = width > MAX_COLUMNS_PER_SHEET + width = min(width, MAX_COLUMNS_PER_SHEET) + + # `max_row` is what the file claims, which is the honest denominator + # for "showing 500 of N" even though we stopped reading early. + declared_rows = value_sheet.max_row or 0 + + if width == 0 or not merged: + return ( + SheetPreview( + name=str(value_sheet.title), + headers=[], + rows=[], + total_rows=max(0, declared_rows - 1) if declared_rows else 0, + truncated=False, + truncated_by=None, + ), + 0, + ) + + hit_row_cap = len(merged) > row_limit + body_source = merged[1 : row_limit + 1] + + headers = _normalize_headers(_fit(merged[0], width)) + rows = [_fit(row, width) for row in body_source] + + total_rows = max(len(rows), declared_rows - 1 if declared_rows else 0) + + return ( + SheetPreview( + name=str(value_sheet.title), + headers=headers, + rows=rows, + total_rows=total_rows, + truncated=hit_row_cap or hit_column_cap, + truncated_by="columns" if hit_column_cap else ("rows" if hit_row_cap else None), + ), + len(rows) * width, + ) + + +def _take_rows(sheet, limit: int) -> list[tuple[Any, ...]]: + """Pull at most `limit` rows, stopping the stream rather than reading + the whole sheet to throw most of it away.""" + out: list[tuple[Any, ...]] = [] + try: + for row in sheet.iter_rows(values_only=True): + out.append(row) + if len(out) >= limit: + break + except Exception as e: + # A sheet can fail mid-stream on a malformed shared-string or + # date. Keep whatever was read — a partial preview beats none. + logger.warning("Stopped reading sheet early: %s", e) + return out + + +def _merge_row(value_row: Iterable[Any], formula_row: Iterable[Any]) -> list[str]: + """Format one row, falling back to formula text where no value was cached.""" + values = list(value_row) + formulas = list(formula_row) + out: list[str] = [] + for i, value in enumerate(values): + if value is None and i < len(formulas): + candidate = formulas[i] + if isinstance(candidate, str) and candidate.startswith("="): + out.append(candidate) + continue + out.append(_format_cell(value)) + return out + + +def _fit(row: list[str], width: int) -> list[str]: + """Pad or clip a row to the sheet's grid width.""" + if len(row) == width: + return row + if len(row) > width: + return row[:width] + return row + [""] * (width - len(row)) + + +def _normalize_headers(row: list[str]) -> list[str]: + """Column labels, with the spreadsheet letter standing in for a blank.""" + return [h.strip() or get_column_letter(i + 1) for i, h in enumerate(row)] + + +def _format_cell(value: Any) -> str: + """Render one cell value the way it should read in the grid. + + The formatting rules a spreadsheet applies (currency, percentages, + thousands separators) live in the cell's `number_format`, which + `read_only` + `values_only` does not carry. Rather than half-apply + them, values are rendered plainly and predictably. The one thing + worth care is float noise: a cell holding 0.1 + 0.2 must not preview + as 0.30000000000000004. + """ + if value is None: + return "" + if isinstance(value, bool): + # Checked before int: bool is a subclass of int in Python, and + # Excel shows these as TRUE/FALSE. + return "TRUE" if value else "FALSE" + if isinstance(value, int): + return str(value) + if isinstance(value, (float, Decimal)): + return _format_number(float(value)) + if isinstance(value, datetime): + # openpyxl returns a datetime for every date-formatted cell, so a + # plain date arrives with a midnight time that was never in the + # file. Showing it would invent precision. + if value.hour == value.minute == value.second == value.microsecond == 0: + return value.date().isoformat() + return value.isoformat(sep=" ") + if isinstance(value, date): + return value.isoformat() + if isinstance(value, time): + return value.isoformat() + if isinstance(value, timedelta): + return str(value) + return str(value) + + +def _format_number(value: float) -> str: + """Format a float without exposing binary-float noise.""" + if value != value or value in (float("inf"), float("-inf")): + return str(value) + if value.is_integer() and abs(value) < 1e15: + return str(int(value)) + # 15 significant digits is the most a float carries; formatting to it + # collapses 0.30000000000000004 to 0.3 while leaving genuine + # precision alone. + text = f"{value:.15g}" + return text diff --git a/backend/src/apis/shared/files/__init__.py b/backend/src/apis/shared/files/__init__.py index 9a12684b..6fa4213e 100644 --- a/backend/src/apis/shared/files/__init__.py +++ b/backend/src/apis/shared/files/__init__.py @@ -21,6 +21,9 @@ TABULAR_EXTENSIONS, PRESENTATION_MIME_TYPES, PRESENTATION_EXTENSIONS, + SHEET_PREVIEW_MIME_TYPES, + SheetPreview, + SheetPreviewResponse, INLINE_DOCUMENT_MAX_BYTES, get_file_format, is_allowed_mime_type, @@ -69,6 +72,9 @@ "TABULAR_EXTENSIONS", "PRESENTATION_MIME_TYPES", "PRESENTATION_EXTENSIONS", + "SHEET_PREVIEW_MIME_TYPES", + "SheetPreview", + "SheetPreviewResponse", "INLINE_DOCUMENT_MAX_BYTES", "get_file_format", "is_allowed_mime_type", diff --git a/backend/src/apis/shared/files/models.py b/backend/src/apis/shared/files/models.py index ae0b7459..18cf7934 100644 --- a/backend/src/apis/shared/files/models.py +++ b/backend/src/apis/shared/files/models.py @@ -377,6 +377,58 @@ class TextSnippetResponse(BaseModel): model_config = ConfigDict(populate_by_name=True) +class SheetPreview(BaseModel): + """One worksheet, read into display-ready strings.""" + + name: str = Field(..., description="Worksheet name as it appears on the tab") + headers: List[str] = Field( + ..., description="First row of the sheet, used as column labels" + ) + rows: List[List[str]] = Field( + ..., description="Body rows, each padded to len(headers)" + ) + total_rows: int = Field( + ..., + alias="totalRows", + description="Body rows the sheet claims to have, which may exceed len(rows)", + ) + truncated: bool = Field( + ..., description="True when a cap stopped the read short of the sheet's end" + ) + truncated_by: Optional[str] = Field( + None, + alias="truncatedBy", + description="Which cap fired: 'rows' or 'columns'", + ) + + model_config = ConfigDict(populate_by_name=True) + + +class SheetPreviewResponse(BaseModel): + """Response for GET /api/files/{uploadId}/sheet-preview.""" + + upload_id: str = Field(..., alias="uploadId") + filename: str + sheets: List[SheetPreview] = Field( + ..., description="Visible worksheets, in workbook order" + ) + truncated: bool = Field( + ..., + description="True when any sheet was cut short or sheets were dropped", + ) + + model_config = ConfigDict(populate_by_name=True) + + +# MIME types the spreadsheet reader can turn into a grid. Deliberately +# only OOXML: the pre-2007 .xls binary format needs a different library +# (xlrd), and it is not worth one for a format nothing in the product +# generates. +SHEET_PREVIEW_MIME_TYPES = frozenset({ + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", +}) + + # MIME types the thumbnail renderer can currently produce a preview image for. # Callers should consult this set before invoking the thumbnail endpoint to # avoid hammering the service for unsupported types. diff --git a/backend/tests/apis/app_api/test_sheet_preview.py b/backend/tests/apis/app_api/test_sheet_preview.py new file mode 100644 index 00000000..ea935694 --- /dev/null +++ b/backend/tests/apis/app_api/test_sheet_preview.py @@ -0,0 +1,251 @@ +"""Tests for the .xlsx reader behind GET /files/{id}/sheet-preview.""" + +from datetime import datetime +from io import BytesIO + +import pytest +from openpyxl import Workbook + +from apis.app_api.files.sheet_preview import ( + MAX_COLUMNS_PER_SHEET, + MAX_ROWS_PER_SHEET, + MAX_SHEETS, + MAX_WORKBOOK_BYTES, + WorkbookTooLargeError, + WorkbookUnreadableError, + read_workbook_preview, +) + + +def build(*, sheets: dict[str, list[list]] | None = None) -> bytes: + """Serialise a workbook the way openpyxl itself would write one.""" + wb = Workbook() + wb.remove(wb.active) + for name, rows in (sheets or {"Sheet1": [["a"], [1]]}).items(): + ws = wb.create_sheet(title=name) + for row in rows: + ws.append(row) + buffer = BytesIO() + wb.save(buffer) + return buffer.getvalue() + + +class TestBasicReading: + def test_reads_headers_and_rows(self): + data = build(sheets={"Budget": [["Item", "Cost"], ["Rent", 1200], ["Food", 300]]}) + + [sheet] = read_workbook_preview(data) + + assert sheet.name == "Budget" + assert sheet.headers == ["Item", "Cost"] + assert sheet.rows == [["Rent", "1200"], ["Food", "300"]] + assert sheet.truncated is False + + def test_reads_every_visible_sheet_in_order(self): + data = build( + sheets={ + "First": [["a"], ["1"]], + "Second": [["b"], ["2"]], + } + ) + + sheets = read_workbook_preview(data) + + assert [s.name for s in sheets] == ["First", "Second"] + + def test_skips_hidden_sheets(self): + # Excel does not show them either; surfacing a scratch sheet would + # misrepresent the workbook. + wb = Workbook() + wb.remove(wb.active) + visible = wb.create_sheet(title="Visible") + visible.append(["a"]) + visible.append([1]) + hidden = wb.create_sheet(title="Scratch") + hidden.append(["secret"]) + hidden.sheet_state = "hidden" + buffer = BytesIO() + wb.save(buffer) + + sheets = read_workbook_preview(buffer.getvalue()) + + assert [s.name for s in sheets] == ["Visible"] + + def test_substitutes_a_column_letter_for_a_blank_header(self): + data = build(sheets={"S": [[None, "name"], [1, "widget"]]}) + + [sheet] = read_workbook_preview(data) + + assert sheet.headers == ["A", "name"] + + def test_pads_a_short_row_to_the_grid_width(self): + data = build(sheets={"S": [["a", "b", "c"], [1, 2]]}) + + [sheet] = read_workbook_preview(data) + + assert sheet.rows == [["1", "2", ""]] + + def test_reports_an_empty_sheet_without_failing(self): + data = build(sheets={"Empty": []}) + + [sheet] = read_workbook_preview(data) + + assert sheet.headers == [] + assert sheet.rows == [] + + +class TestFormulas: + """The trap that motivated reading the workbook twice.""" + + def test_shows_formula_text_when_no_value_was_cached(self): + # openpyxl performs no evaluation, so every formula in a workbook + # it wrote reads back as None under data_only=True. A values-only + # preview would show a blank exactly where the total belongs. + wb = Workbook() + ws = wb.active + ws.append(["Item", "Cost"]) + ws.append(["Rent", 1200]) + ws.append(["Total", "=SUM(B2:B2)"]) + buffer = BytesIO() + wb.save(buffer) + + [sheet] = read_workbook_preview(buffer.getvalue()) + + assert sheet.rows[1] == ["Total", "=SUM(B2:B2)"] + + def test_prefers_a_cached_value_over_the_formula(self): + # A workbook saved by Excel carries the computed value, which is + # what the user should see. + wb = Workbook() + wb.active.append(["total"]) + wb.active.append(["=SUM(A1:A1)"]) + buffer = BytesIO() + wb.save(buffer) + without_cache = read_workbook_preview(buffer.getvalue())[0] + assert without_cache.rows == [["=SUM(A1:A1)"]] + + # Simulate the cached-value case by writing the literal instead. + cached = read_workbook_preview(build(sheets={"Sheet": [["total"], [42]]}))[0] + assert cached.rows == [["42"]] + + +class TestCellFormatting: + @pytest.mark.parametrize( + "value,expected", + [ + (None, ""), + (True, "TRUE"), + (False, "FALSE"), + (7, "7"), + (-3, "-3"), + ("plain", "plain"), + ], + ) + def test_scalar_values(self, value, expected): + data = build(sheets={"S": [["h"], [value]]}) + + [sheet] = read_workbook_preview(data) + + assert sheet.rows == [[expected]] + + def test_a_whole_float_loses_its_trailing_zero(self): + data = build(sheets={"S": [["h"], [3.0]]}) + + [sheet] = read_workbook_preview(data) + + assert sheet.rows == [["3"]] + + def test_float_noise_is_not_shown(self): + # 0.1 + 0.2 must not preview as 0.30000000000000004. + data = build(sheets={"S": [["h"], [0.1 + 0.2]]}) + + [sheet] = read_workbook_preview(data) + + assert sheet.rows == [["0.3"]] + + def test_a_date_does_not_gain_a_midnight_it_never_had(self): + # openpyxl returns a datetime for every date-formatted cell, so + # rendering it whole would invent precision the file lacks. + data = build(sheets={"S": [["when"], [datetime(2026, 3, 15)]]}) + + [sheet] = read_workbook_preview(data) + + assert sheet.rows == [["2026-03-15"]] + + def test_a_real_timestamp_keeps_its_time(self): + data = build(sheets={"S": [["when"], [datetime(2026, 3, 15, 9, 30)]]}) + + [sheet] = read_workbook_preview(data) + + assert sheet.rows == [["2026-03-15 09:30:00"]] + + def test_a_numeric_string_stays_a_string(self): + # Same contract as the csv grid: the preview must not reinterpret + # what the file will hand downstream. + data = build(sheets={"S": [["zip"], ["007"]]}) + + [sheet] = read_workbook_preview(data) + + assert sheet.rows == [["007"]] + + +class TestCaps: + def test_stops_at_the_row_cap_and_reports_the_real_total(self): + rows = [["n"]] + [[i] for i in range(MAX_ROWS_PER_SHEET + 50)] + data = build(sheets={"S": rows}) + + [sheet] = read_workbook_preview(data) + + assert len(sheet.rows) == MAX_ROWS_PER_SHEET + assert sheet.truncated is True + assert sheet.truncated_by == "rows" + # The denominator is what the file claims, not what we read. + assert sheet.total_rows == MAX_ROWS_PER_SHEET + 50 + + def test_stops_at_the_column_cap(self): + wide = list(range(MAX_COLUMNS_PER_SHEET + 10)) + data = build(sheets={"S": [wide, wide]}) + + [sheet] = read_workbook_preview(data) + + assert len(sheet.headers) == MAX_COLUMNS_PER_SHEET + assert len(sheet.rows[0]) == MAX_COLUMNS_PER_SHEET + assert sheet.truncated_by == "columns" + + def test_stops_at_the_sheet_cap(self): + data = build( + sheets={f"S{i}": [["a"], [i]] for i in range(MAX_SHEETS + 5)} + ) + + sheets = read_workbook_preview(data) + + assert len(sheets) <= MAX_SHEETS + + def test_drops_trailing_empty_columns(self): + # A stray format on an empty cell is enough to stretch a sheet's + # used range, and the grid should not open on a screen of blanks. + wb = Workbook() + ws = wb.active + ws.append(["a", "b"]) + ws.append([1, 2]) + ws["H5"] = None + buffer = BytesIO() + wb.save(buffer) + + [sheet] = read_workbook_preview(buffer.getvalue()) + + assert sheet.headers == ["a", "b"] + + def test_refuses_a_workbook_past_the_byte_cap(self): + with pytest.raises(WorkbookTooLargeError): + read_workbook_preview(b"x" * (MAX_WORKBOOK_BYTES + 1)) + + +class TestFailures: + def test_rejects_bytes_that_are_not_a_workbook(self): + with pytest.raises(WorkbookUnreadableError): + read_workbook_preview(b"this is not a zip file") + + def test_rejects_an_empty_body(self): + with pytest.raises(WorkbookUnreadableError): + read_workbook_preview(b"") diff --git a/backend/tests/routes/test_sheet_preview_route.py b/backend/tests/routes/test_sheet_preview_route.py new file mode 100644 index 00000000..2623e37e --- /dev/null +++ b/backend/tests/routes/test_sheet_preview_route.py @@ -0,0 +1,199 @@ +"""End-to-end tests for GET /files/{upload_id}/sheet-preview. + +Unlike `tests/apis/app_api/test_sheet_preview.py`, which exercises the +reader on its own, these drive the whole chain: route -> auth dependency +-> FileUploadService -> S3 body -> openpyxl -> response model. The +service is real; only the repository and the S3 client are stubs, so a +mismatch between what openpyxl returns and what the response model +accepts fails here rather than in production. +""" + +from datetime import datetime, timezone +from io import BytesIO +from unittest.mock import AsyncMock, MagicMock + +import pytest +from fastapi import FastAPI +from openpyxl import Workbook + +from apis.app_api.files.routes import router +from apis.app_api.files.service import FileUploadService, get_file_upload_service +from apis.app_api.files.sheet_preview import MAX_WORKBOOK_BYTES +from apis.shared.files.models import FileMetadata, FileStatus + +from tests.routes.conftest import mock_service + +XLSX_MIME = "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" + + +def workbook_bytes(rows=None, *, sheet_name="Sheet1") -> bytes: + wb = Workbook() + ws = wb.active + ws.title = sheet_name + for row in rows if rows is not None else [["Item", "Cost"], ["Rent", 1200]]: + ws.append(row) + buffer = BytesIO() + wb.save(buffer) + return buffer.getvalue() + + +def metadata(**overrides) -> FileMetadata: + base = dict( + upload_id="up1", + user_id="user-1", + session_id="sess-1", + filename="budget.xlsx", + mime_type=XLSX_MIME, + size_bytes=4096, + s3_key="users/user-1/up1/budget.xlsx", + s3_bucket="test-bucket", + status=FileStatus.READY, + created_at=datetime.now(timezone.utc), + ) + base.update(overrides) + return FileMetadata(**base) + + +@pytest.fixture +def s3_body(): + """Mutable holder for whatever the stub S3 client returns.""" + return {"data": workbook_bytes()} + + +@pytest.fixture +def file_meta(): + return {"value": metadata()} + + +@pytest.fixture +def app(s3_body, file_meta): + s3 = MagicMock() + s3.get_object.side_effect = lambda **_: { + "Body": BytesIO(s3_body["data"]) + } + + repository = MagicMock() + repository.get_file = AsyncMock(side_effect=lambda *_: file_meta["value"]) + + service = FileUploadService( + repository=repository, + s3_client=s3, + bucket_name="test-bucket", + thumbnail_renderer=MagicMock(), + ) + + _app = FastAPI() + _app.include_router(router) + mock_service(_app, get_file_upload_service, service) + return _app + + +class TestSuccess: + def test_returns_sheets_as_rows_of_strings( + self, app, make_user, authenticated_client + ): + client = authenticated_client(app, make_user(user_id="user-1")) + + response = client.get("/files/up1/sheet-preview") + + assert response.status_code == 200 + body = response.json() + assert body["filename"] == "budget.xlsx" + assert body["truncated"] is False + [sheet] = body["sheets"] + assert sheet["name"] == "Sheet1" + assert sheet["headers"] == ["Item", "Cost"] + assert sheet["rows"] == [["Rent", "1200"]] + + def test_serialises_with_camelCase_aliases( + self, app, make_user, authenticated_client + ): + # The SPA's SheetPreview interface reads these names directly. + client = authenticated_client(app, make_user(user_id="user-1")) + + [sheet] = client.get("/files/up1/sheet-preview").json()["sheets"] + + assert "totalRows" in sheet + assert "truncatedBy" in sheet + assert "total_rows" not in sheet + + def test_the_workbook_bytes_never_leave_the_server( + self, app, make_user, authenticated_client + ): + # The whole point of this route: no presigned URL, no bytes. + client = authenticated_client(app, make_user(user_id="user-1")) + + raw = client.get("/files/up1/sheet-preview").content + + assert b"PK\x03\x04" not in raw # the zip magic an .xlsx starts with + + def test_shows_formula_text_where_no_value_was_cached( + self, app, make_user, authenticated_client, s3_body + ): + # openpyxl does not evaluate, so a workbook it wrote carries no + # cached value — a values-only read would show a blank total. + s3_body["data"] = workbook_bytes( + [["Item", "Cost"], ["Rent", 1200], ["Total", "=SUM(B2:B2)"]] + ) + client = authenticated_client(app, make_user(user_id="user-1")) + + [sheet] = client.get("/files/up1/sheet-preview").json()["sheets"] + + assert sheet["rows"][1] == ["Total", "=SUM(B2:B2)"] + + +class TestRejections: + def test_401_without_a_session(self, app, unauthenticated_client): + client = unauthenticated_client(app) + + assert client.get("/files/up1/sheet-preview").status_code == 401 + + def test_404_when_the_file_is_not_the_callers( + self, app, make_user, authenticated_client, file_meta + ): + file_meta["value"] = None + client = authenticated_client(app, make_user(user_id="user-1")) + + assert client.get("/files/up1/sheet-preview").status_code == 404 + + def test_404_while_the_upload_is_still_pending( + self, app, make_user, authenticated_client, file_meta + ): + file_meta["value"] = metadata(status=FileStatus.PENDING) + client = authenticated_client(app, make_user(user_id="user-1")) + + assert client.get("/files/up1/sheet-preview").status_code == 404 + + def test_415_for_a_format_that_is_not_a_workbook( + self, app, make_user, authenticated_client, file_meta + ): + # .xls shares nothing with OOXML; the UI should never have asked. + file_meta["value"] = metadata( + filename="old.xls", mime_type="application/vnd.ms-excel" + ) + client = authenticated_client(app, make_user(user_id="user-1")) + + assert client.get("/files/up1/sheet-preview").status_code == 415 + + def test_413_before_downloading_an_oversized_workbook( + self, app, make_user, authenticated_client, file_meta + ): + # Checked against recorded metadata, so an oversized file costs a + # metadata read rather than a transfer into memory. + file_meta["value"] = metadata(size_bytes=MAX_WORKBOOK_BYTES + 1) + client = authenticated_client(app, make_user(user_id="user-1")) + + assert client.get("/files/up1/sheet-preview").status_code == 413 + + def test_422_for_bytes_that_are_not_a_workbook( + self, app, make_user, authenticated_client, s3_body + ): + s3_body["data"] = b"not a workbook at all" + client = authenticated_client(app, make_user(user_id="user-1")) + + response = client.get("/files/up1/sheet-preview") + + assert response.status_code == 422 + # openpyxl's own message names internal XML parts; the user gets + # something they can act on instead. + assert "corrupt or password-protected" in response.json()["detail"] diff --git a/backend/uv.lock b/backend/uv.lock index a0c57cb5..089ef1ea 100644 --- a/backend/uv.lock +++ b/backend/uv.lock @@ -28,6 +28,7 @@ dependencies = [ { name = "fastapi" }, { name = "httpx" }, { name = "idna" }, + { name = "openpyxl" }, { name = "pillow" }, { name = "pyasn1" }, { name = "pyjwt", extra = ["crypto"] }, @@ -115,6 +116,7 @@ requires-dist = [ { name = "mypy", marker = "extra == 'dev'", specifier = "==1.20.2" }, { name = "numpy", marker = "extra == 'dev'", specifier = "==2.2.6" }, { name = "openai", marker = "extra == 'agentcore'", specifier = "==2.32.0" }, + { name = "openpyxl", specifier = "==3.1.5" }, { name = "pandas", marker = "extra == 'dev'", specifier = "==2.3.3" }, { name = "pillow", specifier = "==12.3.0" }, { name = "pyasn1", specifier = "==0.6.4" }, @@ -1168,6 +1170,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/55/e2/2537ebcff11c1ee1ff17d8d0b6f4db75873e3b0fb32c2d4a2ee31ecb310a/docstring_parser-0.17.0-py3-none-any.whl", hash = "sha256:cf2569abd23dce8099b300f9b4fa8191e9582dda731fd533daf54c4551658708", size = 36896, upload-time = "2025-07-21T07:35:00.684Z" }, ] +[[package]] +name = "et-xmlfile" +version = "2.0.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/d3/38/af70d7ab1ae9d4da450eeec1fa3918940a5fafb9055e934af8d6eb0c2313/et_xmlfile-2.0.0.tar.gz", hash = "sha256:dab3f4764309081ce75662649be815c4c9081e88f0837825f90fd28317d4da54", size = 17234, upload-time = "2024-10-25T17:25:40.039Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/c1/8b/5fe2cc11fee489817272089c4203e679c63b570a5aaeb18d852ae3cbba6a/et_xmlfile-2.0.0-py3-none-any.whl", hash = "sha256:7a91720bc756843502c3b7504c77b8fe44217c85c537d85037f0f536151b2caa", size = 18059, upload-time = "2024-10-25T17:25:39.051Z" }, +] + [[package]] name = "exceptiongroup" version = "1.3.1" @@ -2522,6 +2533,18 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/1e/c1/d6e64ccd0536bf616556f0cad2b6d94a8125f508d25cfd814b1d2db4e2f1/openai-2.32.0-py3-none-any.whl", hash = "sha256:4dcc9badeb4bf54ad0d187453742f290226d30150890b7890711bda4f32f192f", size = 1162570, upload-time = "2026-04-15T22:28:17.714Z" }, ] +[[package]] +name = "openpyxl" +version = "3.1.5" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "et-xmlfile" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/3d/f9/88d94a75de065ea32619465d2f77b29a0469500e99012523b91cc4141cd1/openpyxl-3.1.5.tar.gz", hash = "sha256:cf0e3cf56142039133628b5acffe8ef0c12bc902d2aadd3e0fe5878dc08d1050", size = 186464, upload-time = "2024-06-28T14:03:44.161Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/c0/da/977ded879c29cbd04de313843e76868e6e13408a94ed6b987245dc7c8506/openpyxl-3.1.5-py2.py3-none-any.whl", hash = "sha256:5282c12b107bffeef825f4617dc029afaf41d0ea60823bbb665ef3079dc79de2", size = 250910, upload-time = "2024-06-28T14:03:41.161Z" }, +] + [[package]] name = "opentelemetry-api" version = "1.44.0" diff --git a/frontend/ai.client/src/app/inline-template-backtick-guard.spec.ts b/frontend/ai.client/src/app/inline-template-backtick-guard.spec.ts new file mode 100644 index 00000000..5903177e --- /dev/null +++ b/frontend/ai.client/src/app/inline-template-backtick-guard.spec.ts @@ -0,0 +1,107 @@ +// @vitest-environment node +// +// Guards the one Angular mistake in this codebase that `tsc --noEmit` +// cannot see. +// +// A backtick inside a component's inline `template:` or `styles:` block +// closes the template literal early. TypeScript still typechecks the +// file — the truncated literal is a valid string, and the prose after it +// parses as *something* — so `npx tsc --noEmit` reports success and only +// the Angular compiler fails, with errors that point at the wrong thing +// entirely ("Cannot find name 'text'", "Expected 1 arguments, but got 3", +// "Incorrect number of arguments to @Component decorator"). +// +// It is an easy mistake because the natural way to name a CSS class or a +// method in a comment is to quote it in backticks, exactly as one would +// anywhere else in the file. It has cost debugging time three separate +// times. This spec makes it a test failure with a message that names the +// real cause. +// +// The fix is always the same: write the identifier without backticks. +import { describe, expect, it } from 'vitest'; +import * as fs from 'fs'; +import * as path from 'path'; +import { fromProjectRoot } from '../testing/project-root'; + +/** + * Walk `dir` for component sources, skipping generated output and specs. + * + * Specs are excluded for the same reason `surface-literal-guard.spec.ts` + * excludes them: they are not compiled by the Angular compiler, and a + * spec that quotes the pattern it looks for — this one does — would + * otherwise report itself. + */ +function collect(dir: string, out: string[] = []): string[] { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === 'node_modules' || entry.name === 'generated') continue; + collect(full, out); + } else if (entry.name.endsWith('.ts') && !entry.name.endsWith('.spec.ts')) { + out.push(full); + } + } + return out; +} + +/** + * Index just past the backtick that closes the literal starting at + * `start`, honouring backslash escapes. + */ +function endOfLiteral(source: string, start: number): number { + for (let i = start; i < source.length; i++) { + if (source[i] === '\\') { + i++; + continue; + } + if (source[i] === '`') return i; + } + return -1; +} + +describe('inline template hygiene', () => { + it('no component template: or styles: block contains a backtick', () => { + const offenders: string[] = []; + + for (const file of collect(fromProjectRoot('src/app'))) { + const source = fs.readFileSync(file, 'utf8'); + + for (const key of ['template: `', 'styles: `']) { + let cursor = 0; + while (true) { + const at = source.indexOf(key, cursor); + if (at === -1) break; + + const literalStart = at + key.length; + const close = endOfLiteral(source, literalStart); + cursor = close === -1 ? source.length : close + 1; + if (close === -1) continue; + + // A literal that really is the whole block is followed by the + // decorator's next property or its closing brace. Anything + // else means the backtick we found was a stray one inside the + // block, and the "literal" stopped short. + const after = source.slice(close + 1).trimStart(); + if (after.startsWith(',') || after.startsWith('}') || after.startsWith(';')) { + continue; + } + + const line = source.slice(0, close).split('\n').length; + offenders.push( + `${path.relative(fromProjectRoot('.'), file)}:${line} — ` + + `stray backtick inside a ${key.replace(': `', '')} block ` + + `(near: ${source.slice(Math.max(0, close - 40), close + 20).replace(/\n/g, ' ')})`, + ); + } + } + } + + expect( + offenders, + 'A backtick inside an inline template:/styles: block ends the ' + + 'template literal early. tsc passes; only `ng build` fails, and ' + + 'its errors name the wrong cause. Drop the backticks:\n' + + offenders.join('\n'), + ).toEqual([]); + }); +}); diff --git a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.spec.ts b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.spec.ts index 8b36a60b..378bf5fa 100644 --- a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.spec.ts +++ b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.spec.ts @@ -70,7 +70,7 @@ describe('CsvViewerComponent', () => { const el = render('a,b\n1,2\n'); const grid = el.querySelector('[role="grid"]') as HTMLElement; - const template = grid.style.getPropertyValue('--csv-cols'); + const template = grid.style.getPropertyValue('--grid-cols'); // Gutter plus one track per column. expect(template.trim().split(/\s+/)).toHaveLength(3); }); diff --git a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.ts b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.ts index 19d36513..2dda6c59 100644 --- a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.ts +++ b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/csv-viewer.component.ts @@ -1,158 +1,44 @@ -import { - CdkVirtualScrollViewport, - ScrollingModule, -} from '@angular/cdk/scrolling'; import { ChangeDetectionStrategy, Component, - ElementRef, computed, effect, input, output, signal, - viewChild, } from '@angular/core'; -import { CsvParseError, CsvTable, parseCsv } from '../../../../services/file-preview/csv-parse'; - -/** - * Height of one body row, in CSS px. - * - * Load-bearing: `cdk-virtual-scroll-viewport`'s fixed-size strategy - * computes the spacer height and the rendered window from this number, - * so a row that paints taller than it claims leaves the rows drifting - * out of the viewport as you scroll. Keep it in step with the row's - * `h-8` and the cell padding below. - */ -const ROW_HEIGHT_PX = 32; - -/** Column width bounds, in CSS px. */ -const COL_MIN_PX = 88; -const COL_MAX_PX = 320; - -/** Approximate px per character at the grid's text size, for sizing - * columns without laying anything out first. */ -const PX_PER_CHAR = 7.5; - -/** Rows sampled when measuring a column's natural width. Measuring all - * of them would walk 50k rows to move a column by a few px. */ -const WIDTH_SAMPLE_ROWS = 100; +import { + CsvParseError, + CsvTable, + parseCsv, +} from '../../../../services/file-preview/csv-parse'; +import { DataGridComponent } from './data-grid.component'; /** - * Renders a delimited data file (`.csv`, `.tsv`) as a scrollable grid. - * - * Deliberately a *data* view and not a spreadsheet view. Cells are the - * strings the file contained, rendered left-aligned and unformatted: - * there is no type inference, no locale-aware number rendering and no - * date parsing, because the user is previewing what the file will hand - * to whatever reads it next, and a preview that prettifies `007` into - * `7` is answering a different question than the one being asked. + * Renders a delimited data file (`.csv`) as a scrollable grid. * - * Virtualised with `cdk-virtual-scroll-viewport` rather than capped at a - * few hundred rows. `@angular/cdk` is already a dependency (dialog, - * menu, overlay, a11y), uniform row height is the fixed-size strategy's - * best case, and the alternative — "showing the first 500 rows of - * 40,000" — is the difference between previewing a file and previewing - * the top of one. The parse caps in `csv-parse.ts` still apply; they - * bound the backing array, while this bounds the DOM. + * Parsing happens in the browser: a delimited file needs nothing but + * field splitting, and `csv-parse.ts` is a couple of hundred lines with + * no dependency. That is the line between this viewer and the `.xlsx` + * one, which has to ask the server — not because spreadsheets are + * bigger, but because no client-side workbook *reader* was shippable. * - * The cost of virtualising is that the grid cannot be a ``: the - * viewport needs to own the scroll container and transform its content, - * which `` will not tolerate. So it is a div grid carrying - * explicit ARIA grid roles, with `aria-rowcount` and `aria-rowindex` - * reporting the true size of the data rather than the size of the - * rendered window. + * Everything below the data is `DataGridComponent`'s job. */ @Component({ selector: 'app-csv-viewer', changeDetection: ChangeDetectionStrategy.OnPush, - imports: [ScrollingModule], + imports: [DataGridComponent], template: ` @if (table(); as data) { -
- -
-
-
- -
- @for (header of data.headers; track $index) { -
- {{ header }} -
- } -
-
- - @if (data.rows.length === 0) { -

- This file has column headers but no data rows. -

- } @else { - -
-
- {{ i + 1 }} -
- @for (cell of row; track $index) { -
- {{ cell }} -
- } -
- - } - -
- {{ summary() }} - @if (data.truncated) { - - {{ truncationNotice() }} - - } -
-
+ } `, styles: ` @@ -160,48 +46,6 @@ const WIDTH_SAMPLE_ROWS = 100; display: block; height: 100%; } - - /* Header and body rows share one track list so the columns line up - without measuring anything after layout. */ - .csv-row { - display: grid; - grid-template-columns: var(--csv-cols); - align-items: center; - height: 32px; - width: max-content; - min-width: 100%; - } - - .csv-cell, - .csv-gutter { - overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; - padding-inline: 0.75rem; - font-size: 0.8125rem; - line-height: 1.25rem; - font-variant-numeric: tabular-nums; - } - - /* Colour deliberately lives on the elements, as the utility pair - text-gray-500 / dark:text-gray-400, rather than here. There is no - single neutral step that clears WCAG AA against both surfaces: - gray-400 measures 2.6:1 on the light grid, and gray-500 is the - step the surface generator itself reports as short of AA against - the dark one. (No backticks in this block - one inside a styles - template literal breaks the Angular compiler while tsc passes.) */ - .csv-gutter { - text-align: right; - padding-inline: 0.5rem; - font-size: 0.6875rem; - user-select: none; - } - - /* The viewport is the horizontal scroller as well as the vertical - one, so rows wider than the rail can be reached. */ - cdk-virtual-scroll-viewport { - overflow-x: auto; - } `, }) export class CsvViewerComponent { @@ -212,25 +56,8 @@ export class CsvViewerComponent { /** Emitted with a user-facing message when the file cannot be read. */ readonly renderFailed = output(); - protected readonly rowHeight = ROW_HEIGHT_PX; protected readonly table = signal(null); - private readonly headerScroller = - viewChild>('headerScroller'); - private readonly viewport = viewChild(CdkVirtualScrollViewport); - - /** One `grid-template-columns` track list, shared by the header row - * and every body row. */ - protected readonly gridTemplate = computed(() => { - const data = this.table(); - if (!data) return ''; - const gutter = `${gutterWidthFor(data.rows.length)}px`; - const columns = data.headers.map((header, i) => - `${measureColumn(header, data.rows, i)}px`, - ); - return [gutter, ...columns].join(' '); - }); - protected readonly summary = computed(() => { const data = this.table(); if (!data) return ''; @@ -277,43 +104,6 @@ export class CsvViewerComponent { this.rendered.emit(); }); - - effect((onCleanup) => { - const viewport = this.viewport(); - if (!viewport) return; - const sub = viewport.elementScrolled().subscribe(() => { - this.syncHeaderScroll(); - }); - onCleanup(() => sub.unsubscribe()); - }); - } - - /** `trackBy` on the row index rather than the row: a data file may - * legitimately repeat identical rows, and identity tracking would - * make the virtual scroller reuse the wrong one. */ - protected trackByIndex(index: number): number { - return index; - } - - /** - * Mirror the viewport's horizontal offset onto the header. - * - * Driven by `elementScrolled()` rather than a template `(scroll)` - * binding or `(scrolledIndexChange)`. `scrolledIndexChange` is the - * wrong signal outright — it fires when the first *rendered row* - * changes, so it never fires for a purely horizontal scroll and the - * header stays behind while the body moves. `elementScrolled()` emits - * for both axes, and CDK's scroll dispatcher already runs it outside - * the Angular zone; since the handler only writes `scrollLeft` on a - * DOM node and touches no signal, the sync costs no change detection - * at scroll rate. - */ - private syncHeaderScroll(): void { - const header = this.headerScroller()?.nativeElement; - const viewport = this.viewport(); - if (header && viewport) { - header.scrollLeft = viewport.measureScrollOffset('left'); - } } } @@ -323,34 +113,3 @@ const DELIMITER_LABELS: Readonly> = { ';': 'semicolon-separated', '|': 'pipe-separated', }; - -/** Enough room for the largest row number the gutter will show. */ -function gutterWidthFor(rowCount: number): number { - return Math.max(40, String(rowCount).length * 8 + 16); -} - -/** - * Width for one column, from the longest value in a sample of its cells. - * - * Approximated from character counts rather than measured, because - * measuring means laying out every cell before the first paint. The - * clamp matters more than the estimate: a column of long free text stops - * at `COL_MAX_PX` and ellipsises (the full value is on the cell's - * `title`), and a column of short codes still gets a readable minimum. - */ -function measureColumn( - header: string, - rows: readonly string[][], - index: number, -): number { - let longest = header.length; - const sampled = Math.min(rows.length, WIDTH_SAMPLE_ROWS); - for (let i = 0; i < sampled; i++) { - const cell = rows[i][index]; - if (cell && cell.length > longest) longest = cell.length; - } - return Math.min( - COL_MAX_PX, - Math.max(COL_MIN_PX, Math.round(longest * PX_PER_CHAR) + 24), - ); -} diff --git a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/data-grid.component.ts b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/data-grid.component.ts new file mode 100644 index 00000000..1695bf85 --- /dev/null +++ b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/data-grid.component.ts @@ -0,0 +1,314 @@ +import { + CdkVirtualScrollViewport, + ScrollingModule, +} from '@angular/cdk/scrolling'; +import { + ChangeDetectionStrategy, + Component, + ElementRef, + computed, + effect, + input, + viewChild, +} from '@angular/core'; + +/** + * Height of one body row, in CSS px. + * + * Load-bearing: `cdk-virtual-scroll-viewport`'s fixed-size strategy + * computes the spacer height and the rendered window from this number, + * so a row that paints taller than it claims leaves the rows drifting + * out of the viewport as you scroll. Keep it in step with the row height + * in the styles below. + */ +const ROW_HEIGHT_PX = 32; + +/** Column width bounds, in CSS px. */ +const COL_MIN_PX = 88; +const COL_MAX_PX = 320; + +/** + * Approximate px per character at the grid's text size, for sizing + * columns without laying anything out first. + * + * Measured against the real font at 13px: mixed-case words average + * about 6.7px, dates 7.3, digits 7.8, and formula text like + * "=SUM(B2:C2)" close to 8.1 — the wide glyphs are exactly the ones + * spreadsheet content is full of. An estimate tuned to the average + * clipped those by a few px, which reads as a bug rather than as the + * deliberate ellipsis at COL_MAX_PX, so this sits at the top of the + * range instead. Over-estimating only costs a slightly wider column. + */ +const PX_PER_CHAR = 8.2; + +/** Rows sampled when measuring a column's natural width. Measuring all + * of them would walk 50k rows to move a column by a few px. */ +const WIDTH_SAMPLE_ROWS = 100; + +/** + * A scrollable grid of strings — the shared body of every data preview. + * + * Purely presentational: it takes headers and rows and draws them. Where + * those came from is the caller's business, which is what lets one grid + * serve both a `.csv` parsed in the browser and an `.xlsx` read by + * app-api. Cells are rendered exactly as handed over, with no type + * inference, number formatting or date parsing — a preview that + * prettifies `007` into `7` is answering a different question than the + * one being asked, and that has to stay true no matter which reader + * produced the strings. + * + * Virtualised with `cdk-virtual-scroll-viewport`. `@angular/cdk` is + * already a dependency (dialog, menu, overlay, a11y), uniform row height + * is the fixed-size strategy's best case, and the alternative — + * "showing the first 500 rows of 40,000" — is the difference between + * previewing a file and previewing the top of one. + * + * The cost of virtualising is that the grid cannot be a `
`: the + * viewport needs to own the scroll container and transform its content, + * which `` will not tolerate. So it is a div grid carrying + * explicit ARIA grid roles, with `aria-rowcount` and `aria-rowindex` + * reporting the true size of the data rather than the size of the + * rendered window. + */ +@Component({ + selector: 'app-data-grid', + changeDetection: ChangeDetectionStrategy.OnPush, + imports: [ScrollingModule], + template: ` +
+ +
+
+
+ +
+ @for (header of headers(); track $index) { +
+ {{ header }} +
+ } +
+
+ + @if (rows().length === 0) { +

+ {{ emptyMessage() }} +

+ } @else { + +
+
+ {{ i + 1 }} +
+ @for (cell of row; track $index) { +
+ {{ cell }} +
+ } +
+ + } + +
+ {{ summary() }} + @if (notice()) { + + + {{ notice() }} + + } +
+
+ `, + styles: ` + :host { + display: block; + height: 100%; + } + + /* Header and body rows share one track list so the columns line up + without measuring anything after layout. */ + .grid-row { + display: grid; + grid-template-columns: var(--grid-cols); + align-items: center; + height: 32px; + width: max-content; + min-width: 100%; + } + + .grid-cell, + .grid-gutter { + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + padding-inline: 0.75rem; + font-size: 0.8125rem; + line-height: 1.25rem; + font-variant-numeric: tabular-nums; + } + + /* Colour deliberately lives on the elements, as the utility pair + text-gray-500 / dark:text-gray-400, rather than here. There is no + single neutral step that clears WCAG AA against both surfaces: + gray-400 measures 2.6:1 on the light grid, and gray-500 is the + step the surface generator itself reports as short of AA against + the dark one. (No backticks in this block - one inside a styles + template literal breaks the Angular compiler while tsc passes.) */ + .grid-gutter { + text-align: right; + padding-inline: 0.5rem; + font-size: 0.6875rem; + user-select: none; + } + + /* The viewport is the horizontal scroller as well as the vertical + one, so rows wider than the rail can be reached. */ + cdk-virtual-scroll-viewport { + overflow-x: auto; + } + `, +}) +export class DataGridComponent { + readonly headers = input.required(); + readonly rows = input.required(); + /** Footer line: size, and whatever else the reader knows. */ + readonly summary = input(''); + /** Footer warning, shown after the summary when a cap cut the data. */ + readonly notice = input(''); + /** Shown in place of the body when there are headers but no rows. */ + readonly emptyMessage = input('No data rows to show.'); + readonly ariaLabel = input('Data preview'); + + protected readonly rowHeight = ROW_HEIGHT_PX; + + private readonly headerScroller = + viewChild>('headerScroller'); + private readonly viewport = viewChild(CdkVirtualScrollViewport); + + /** One `grid-template-columns` track list, shared by the header row + * and every body row. */ + protected readonly gridTemplate = computed(() => { + const headers = this.headers(); + const rows = this.rows(); + const gutter = `${gutterWidthFor(rows.length)}px`; + const columns = headers.map( + (header, i) => `${measureColumn(header, rows, i)}px`, + ); + return [gutter, ...columns].join(' '); + }); + + constructor() { + effect((onCleanup) => { + const viewport = this.viewport(); + if (!viewport) return; + const sub = viewport.elementScrolled().subscribe(() => { + this.syncHeaderScroll(); + }); + onCleanup(() => sub.unsubscribe()); + }); + } + + /** `trackBy` on the row index rather than the row: a data file may + * legitimately repeat identical rows, and identity tracking would + * make the virtual scroller reuse the wrong one. */ + protected trackByIndex(index: number): number { + return index; + } + + /** + * Mirror the viewport's horizontal offset onto the header. + * + * Driven by `elementScrolled()` rather than a template `(scroll)` + * binding or `(scrolledIndexChange)`. `scrolledIndexChange` is the + * wrong signal outright — it fires when the first *rendered row* + * changes, so it never fires for a purely horizontal scroll and the + * header stays behind while the body moves. `elementScrolled()` emits + * for both axes, and CDK's scroll dispatcher already runs it outside + * the Angular zone; since the handler only writes `scrollLeft` on a + * DOM node and touches no signal, the sync costs no change detection + * at scroll rate. + */ + private syncHeaderScroll(): void { + const header = this.headerScroller()?.nativeElement; + const viewport = this.viewport(); + if (header && viewport) { + header.scrollLeft = viewport.measureScrollOffset('left'); + } + } +} + +/** Enough room for the largest row number the gutter will show. */ +function gutterWidthFor(rowCount: number): number { + return Math.max(40, String(rowCount).length * 8 + 16); +} + +/** + * Width for one column, from the longest value in a sample of its cells. + * + * Approximated from character counts rather than measured, because + * measuring means laying out every cell before the first paint. The + * clamp matters more than the estimate: a column of long free text stops + * at `COL_MAX_PX` and ellipsises (the full value is on the cell's + * `title`), and a column of short codes still gets a readable minimum. + */ +function measureColumn( + header: string, + rows: readonly (readonly string[])[], + index: number, +): number { + let longest = header.length; + const sampled = Math.min(rows.length, WIDTH_SAMPLE_ROWS); + for (let i = 0; i < sampled; i++) { + const cell = rows[i][index]; + if (cell && cell.length > longest) longest = cell.length; + } + return Math.min( + COL_MAX_PX, + Math.max(COL_MIN_PX, Math.round(longest * PX_PER_CHAR) + 24), + ); +} diff --git a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/file-preview-panel.component.ts b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/file-preview-panel.component.ts index 5a3b7589..65d76840 100644 --- a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/file-preview-panel.component.ts +++ b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/file-preview-panel.component.ts @@ -25,15 +25,23 @@ import { TooltipDirective } from '../../../../../components/tooltip/tooltip.dire import { CsvViewerComponent } from './csv-viewer.component'; import { DocxViewerComponent } from './docx-viewer.component'; import { PptxViewerComponent } from './pptx-viewer.component'; +import { XlsxViewerComponent } from './xlsx-viewer.component'; import { PREVIEW_KIND_LABELS, PreviewKind, + previewFetchesBytes, previewKindFor, } from '../../../../services/file-preview/file-preview.model'; /** * Right-docked pane that previews one uploaded file in the browser — - * `.docx`, `.pptx` and `.csv` today. + * `.docx`, `.pptx`, `.csv` and `.xlsx` today. + * + * Three of those four are read here from bytes the pane fetched through + * a presigned URL. `.xlsx` is not: no client-side workbook reader was + * shippable, so app-api reads it and its viewer takes an upload id + * instead. `previewFetchesBytes` is what keeps the two paths from + * treading on each other. * * Shares the rail with `ArtifactPanelComponent` through * `DockedPaneService` — same width, same resize affordance, same @@ -59,6 +67,7 @@ import { CsvViewerComponent, DocxViewerComponent, PptxViewerComponent, + XlsxViewerComponent, ], providers: [ provideIcons({ @@ -180,6 +189,19 @@ import { (rendered)="onRendered()" /> } + @case ('xlsx') { + + + } @default { { const ref = this.open(); - if (!ref) { - this.requestSeq++; - this.reset(); - return; - } + this.requestSeq++; + this.reset(); + if (!ref) return; + + // An .xlsx is read by app-api, so there is nothing to fetch here + // — running the presigned-URL leg anyway would pull the whole + // workbook into the browser only to ignore it. Its viewer owns + // both the request and the failure, and reports them through the + // same (rendered)/(renderFailed) pair as every other viewer. + const kind = previewKindFor(ref.filename); + if (kind && !previewFetchesBytes(kind)) return; + void this.load(ref.uploadId); }); } @@ -302,7 +331,17 @@ export class FilePreviewPanelComponent { protected retry(): void { const ref = this.open(); - if (ref) void this.load(ref.uploadId); + if (!ref) return; + + const kind = previewKindFor(ref.filename); + if (kind && !previewFetchesBytes(kind)) { + // Nothing to refetch here — clearing the error re-creates the + // viewer, whose own effect issues the request again. + this.reset(); + return; + } + + void this.load(ref.uploadId); } protected onRendered(): void { diff --git a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/xlsx-viewer.component.spec.ts b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/xlsx-viewer.component.spec.ts new file mode 100644 index 00000000..4033873b --- /dev/null +++ b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/xlsx-viewer.component.spec.ts @@ -0,0 +1,230 @@ +import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { XlsxViewerComponent } from './xlsx-viewer.component'; +import { + FilePreviewError, + FilePreviewHttpService, +} from '../../../../services/file-preview/file-preview-http.service'; +import { + SheetPreview, + SheetPreviewResponse, +} from '../../../../services/file-preview/sheet-preview.model'; + +function sheet(overrides: Partial = {}): SheetPreview { + return { + name: 'Sheet1', + headers: ['Item', 'Cost'], + rows: [['Rent', '1200']], + totalRows: 1, + truncated: false, + truncatedBy: null, + ...overrides, + }; +} + +function response(sheets: SheetPreview[]): SheetPreviewResponse { + return { + uploadId: 'up1', + filename: 'budget.xlsx', + sheets, + truncated: sheets.some((s) => s.truncated), + }; +} + +describe('XlsxViewerComponent', () => { + let fixture: ComponentFixture; + let fetchSheets: ReturnType; + + beforeEach(async () => { + fetchSheets = vi.fn().mockResolvedValue(response([sheet()])); + + TestBed.resetTestingModule(); + await TestBed.configureTestingModule({ + imports: [XlsxViewerComponent], + providers: [ + { provide: FilePreviewHttpService, useValue: { fetchSheets } }, + ], + }).compileComponents(); + + fixture = TestBed.createComponent(XlsxViewerComponent); + }); + + /** Set the id and drain the fetch promise. */ + async function open(uploadId = 'up1'): Promise { + fixture.componentRef.setInput('uploadId', uploadId); + fixture.detectChanges(); + for (let i = 0; i < 10; i++) await Promise.resolve(); + fixture.detectChanges(); + return fixture.nativeElement as HTMLElement; + } + + it('asks the server for rows rather than fetching the workbook', async () => { + await open(); + + expect(fetchSheets).toHaveBeenCalledExactlyOnceWith('up1'); + }); + + it('renders the sheet it was given', async () => { + // Asserted through the header row and the grid's own row count + // rather than cell text: the CDK viewport has zero height under + // jsdom, so no body rows are ever in the DOM. What matters here is + // that the sheet reached the grid intact. + const el = await open(); + + const headers = Array.from(el.querySelectorAll('[role="columnheader"]')) + .map((n) => n.textContent?.trim()) + .filter((t) => t !== '#'); + expect(headers).toEqual(['Item', 'Cost']); + expect(el.querySelector('[role="grid"]')?.getAttribute('aria-rowcount')).toBe( + '2', + ); + }); + + it('emits rendered once the grid is on screen', async () => { + const seen: number[] = []; + fixture.componentInstance.rendered.subscribe(() => seen.push(1)); + + await open(); + + expect(seen).toHaveLength(1); + }); + + describe('multiple sheets', () => { + beforeEach(() => { + // The sheets differ in their HEADERS, not just their cells, so a + // tab switch is observable in the DOM that jsdom actually renders. + fetchSheets.mockResolvedValue( + response([ + sheet({ name: 'Q1', headers: ['Item', 'Q1 Cost'] }), + sheet({ name: 'Q2', headers: ['Item', 'Q2 Cost'] }), + ]), + ); + }); + + it('offers a tab per sheet and opens on the first', async () => { + const el = await open(); + + const tabs = Array.from(el.querySelectorAll('[role="tab"]')); + expect(tabs.map((t) => t.textContent?.trim())).toEqual(['Q1', 'Q2']); + expect(tabs[0].getAttribute('aria-selected')).toBe('true'); + expect(el.textContent).toContain('Q1 Cost'); + }); + + it('switches the grid when another tab is chosen', async () => { + const el = await open(); + + const tabs = el.querySelectorAll('[role="tab"]'); + (tabs[1] as HTMLElement).click(); + fixture.detectChanges(); + + expect(el.textContent).toContain('Q2 Cost'); + expect(el.textContent).not.toContain('Q1 Cost'); + expect(tabs[1].getAttribute('aria-selected')).toBe('true'); + }); + }); + + it('shows no tab strip for a single-sheet workbook', async () => { + const el = await open(); + + expect(el.querySelector('[role="tablist"]')).toBeNull(); + }); + + describe('the footer summary', () => { + it('names both counts when the sheet was cut short', async () => { + // "500 of 12,480 rows" is the honest form — the grid holds 500 but + // the sheet has far more. + fetchSheets.mockResolvedValue( + response([ + sheet({ + rows: [['a', 'b']], + totalRows: 12480, + truncated: true, + truncatedBy: 'rows', + }), + ]), + ); + + const el = await open(); + + expect(el.querySelector('footer')?.textContent).toContain('1 of 12,480 rows'); + expect(el.querySelector('footer')?.textContent).toContain( + 'later rows not shown', + ); + }); + + it('collapses to one count when nothing was cut', async () => { + const el = await open(); + + const footer = el.querySelector('footer')?.textContent ?? ''; + expect(footer).toContain('1 row'); + expect(footer).not.toContain(' of '); + }); + + it('says values only, because that is what was read', async () => { + const el = await open(); + + expect(el.querySelector('footer')?.textContent).toContain('values only'); + }); + }); + + describe('failures', () => { + it('reports a workbook with no visible sheets rather than a blank grid', async () => { + fetchSheets.mockResolvedValue(response([])); + const failures: string[] = []; + fixture.componentInstance.renderFailed.subscribe((m) => failures.push(m)); + + await open(); + + expect(failures).toEqual(['This workbook has no visible sheets to show.']); + }); + + it('surfaces the message the http service chose', async () => { + fetchSheets.mockRejectedValue( + new FilePreviewError( + 'This workbook is too large to preview. Download it to open in Excel.', + false, + ), + ); + const failures: string[] = []; + fixture.componentInstance.renderFailed.subscribe((m) => failures.push(m)); + + await open(); + + expect(failures).toEqual([ + 'This workbook is too large to preview. Download it to open in Excel.', + ]); + }); + + it('does not emit rendered when the read failed', async () => { + fetchSheets.mockRejectedValue(new FilePreviewError('nope', false)); + const seen: number[] = []; + fixture.componentInstance.rendered.subscribe(() => seen.push(1)); + + await open(); + + expect(seen).toHaveLength(0); + }); + }); + + it('discards a response for a file the pane has moved off', async () => { + // A slow first workbook must not paint over the second one. + let resolveFirst: (r: SheetPreviewResponse) => void = () => undefined; + fetchSheets.mockImplementationOnce( + () => new Promise((r) => (resolveFirst = r)), + ); + fetchSheets.mockResolvedValueOnce( + response([sheet({ headers: ['Second'] })]), + ); + + fixture.componentRef.setInput('uploadId', 'up1'); + fixture.detectChanges(); + await open('up2'); + + resolveFirst(response([sheet({ headers: ['First'] })])); + for (let i = 0; i < 10; i++) await Promise.resolve(); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toContain('Second'); + expect(fixture.nativeElement.textContent).not.toContain('First'); + }); +}); diff --git a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/xlsx-viewer.component.ts b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/xlsx-viewer.component.ts new file mode 100644 index 00000000..a6d77159 --- /dev/null +++ b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/xlsx-viewer.component.ts @@ -0,0 +1,180 @@ +import { + ChangeDetectionStrategy, + Component, + computed, + effect, + inject, + input, + output, + signal, +} from '@angular/core'; +import { + FilePreviewError, + FilePreviewHttpService, +} from '../../../../services/file-preview/file-preview-http.service'; +import { SheetPreview } from '../../../../services/file-preview/sheet-preview.model'; +import { DataGridComponent } from './data-grid.component'; + +/** + * Renders an `.xlsx` as a grid of values, read by app-api. + * + * The one viewer in the pane that does not parse anything. `.docx`, + * `.pptx` and `.csv` all fetch bytes through a presigned URL and read + * them in the browser; this one takes `uploadId` and asks + * `/files/{id}/sheet-preview` for rows, because every client-side + * workbook reader was rejected on its own terms — the npm build of + * SheetJS is frozen on a 2022 release with unfixed advisories, and + * ExcelJS raises outright on a workbook containing a native chart, which + * is exactly what `create_excel_spreadsheet` produces. The distinction + * that unblocked this was that a grid needs a *reader*, not a + * *renderer*, and openpyxl is already what our own spreadsheet tools + * drive. + * + * What the user does not get is fidelity: no fills, fonts, borders, + * merges, column widths or charts. Download-and-open remains the path + * for those, and the pane keeps its download button throughout. + * + * A formula whose value Excel never cached shows as its formula text + * ("=SUM(B2:B10)") rather than as a blank. That is not a flourish — it + * is the normal state of any workbook openpyxl wrote, since openpyxl + * does not evaluate, so a values-only preview would show holes exactly + * where the totals belong. + */ +@Component({ + selector: 'app-xlsx-viewer', + changeDetection: ChangeDetectionStrategy.OnPush, + imports: [DataGridComponent], + template: ` + @if (active(); as sheet) { +
+ @if (sheets().length > 1) { +
+ @for (candidate of sheets(); track candidate.name; let i = $index) { + + } +
+ } + +
+ +
+
+ } + `, + styles: ` + :host { + display: block; + height: 100%; + } + `, +}) +export class XlsxViewerComponent { + /** The file to read. Unlike its sibling viewers this takes an id, not + * bytes — the workbook is never fetched into the browser. */ + readonly uploadId = input(null); + + readonly rendered = output(); + readonly renderFailed = output(); + + private readonly previewHttp = inject(FilePreviewHttpService); + + protected readonly sheets = signal([]); + protected readonly selected = signal(0); + + protected readonly active = computed( + () => this.sheets()[this.selected()] ?? null, + ); + + protected readonly summary = computed(() => { + const sheet = this.active(); + if (!sheet) return ''; + const shown = sheet.rows.length; + const cols = sheet.headers.length; + // `totalRows` is what the sheet claims; `rows.length` is what the + // cap let through. Saying "500 of 12,480" is the honest form, and + // collapsing to one number when they agree keeps the common case + // quiet. + const rowText = + sheet.totalRows > shown + ? `${shown.toLocaleString()} of ${sheet.totalRows.toLocaleString()} rows` + : `${shown.toLocaleString()} ${shown === 1 ? 'row' : 'rows'}`; + return `${rowText} · ${cols} ${cols === 1 ? 'column' : 'columns'} · values only`; + }); + + protected readonly truncationNotice = computed(() => { + switch (this.active()?.truncatedBy) { + case 'rows': + return '· later rows not shown'; + case 'columns': + return '· later columns not shown'; + default: + return ''; + } + }); + + /** Bumped per load so a slow response for a file the pane has since + * moved off is discarded rather than painted. */ + private requestSeq = 0; + + constructor() { + effect(() => { + const uploadId = this.uploadId(); + this.requestSeq++; + this.sheets.set([]); + this.selected.set(0); + if (uploadId) void this.load(uploadId, this.requestSeq); + }); + } + + private async load(uploadId: string, seq: number): Promise { + try { + const response = await this.previewHttp.fetchSheets(uploadId); + if (seq !== this.requestSeq) return; + + if (response.sheets.length === 0) { + // Every sheet hidden, or none readable. Blank grids would be a + // worse answer than saying so. + this.renderFailed.emit('This workbook has no visible sheets to show.'); + return; + } + + this.sheets.set(response.sheets); + this.rendered.emit(); + } catch (e) { + if (seq !== this.requestSeq) return; + this.renderFailed.emit( + e instanceof FilePreviewError + ? e.message + : "This workbook couldn't be read.", + ); + } + } + + protected select(index: number): void { + this.selected.set(index); + } +} diff --git a/frontend/ai.client/src/app/session/components/message-list/components/inline-visual/renderers/file-download-renderer.component.spec.ts b/frontend/ai.client/src/app/session/components/message-list/components/inline-visual/renderers/file-download-renderer.component.spec.ts index c3b04b25..f42df958 100644 --- a/frontend/ai.client/src/app/session/components/message-list/components/inline-visual/renderers/file-download-renderer.component.spec.ts +++ b/frontend/ai.client/src/app/session/components/message-list/components/inline-visual/renderers/file-download-renderer.component.spec.ts @@ -90,21 +90,21 @@ describe('FileDownloadRendererComponent', () => { }); }); - it('offers a preview for a .pptx as well as a .docx', () => { - expect(render({ filename: 'deck.pptx', upload_id: 'up2' })).not.toBeNull(); + it.each([ + ['deck.pptx'], + ['rows.csv'], + ['budget.xlsx'], + ])('offers a preview for %s as well as a .docx', (filename) => { + expect(render({ filename, upload_id: 'up2' })).not.toBeNull(); expect(previewButton()).not.toBeNull(); }); it('offers no preview for formats the pane cannot render', () => { - // .xlsx has no renderer we are willing to ship, and the legacy - // binary .doc/.ppt formats are not OOXML at all. All of them still - // get their download link; only the button is withheld. - for (const filename of [ - 'budget.xlsx', - 'old.doc', - 'old.ppt', - 'notes.txt', - ]) { + // The legacy binary formats are not OOXML at all, and .xls would + // need a library we did not take on for a format nothing in the + // product generates. All of them still get their download link; + // only the button is withheld. + for (const filename of ['old.doc', 'old.ppt', 'old.xls', 'notes.txt']) { expect(render({ filename, upload_id: 'up1' })).not.toBeNull(); expect(previewButton()).toBeNull(); } diff --git a/frontend/ai.client/src/app/session/services/chat/stream-parser.file-preview.spec.ts b/frontend/ai.client/src/app/session/services/chat/stream-parser.file-preview.spec.ts index ce55496a..c1c0ffa9 100644 --- a/frontend/ai.client/src/app/session/services/chat/stream-parser.file-preview.spec.ts +++ b/frontend/ai.client/src/app/session/services/chat/stream-parser.file-preview.spec.ts @@ -85,10 +85,19 @@ describe('StreamParserService — auto-opening the file preview pane', () => { expect(preview.openFile()?.filename).toBe('report.docx'); }); + it('opens a generated .xlsx too, now that the pane can read one', () => { + // The pane reads a workbook server-side, so a spreadsheet the agent + // just produced opens on the same terms as a .docx or .pptx. + streamFile({ filename: 'budget.xlsx', upload_id: 'up3' }); + + expect(preview.openFile()?.filename).toBe('budget.xlsx'); + }); + it('leaves a format the pane cannot render alone', () => { // Opening a pane that could only show an error is worse than letting - // the download card speak for itself. - streamFile({ filename: 'budget.xlsx', upload_id: 'up3' }); + // the download card speak for itself. .xls is the pre-2007 binary + // format, which no reader here can open. + streamFile({ filename: 'legacy.xls', upload_id: 'up3b' }); expect(preview.openFile()).toBeNull(); }); diff --git a/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.spec.ts b/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.spec.ts index fb34b0f2..99f91f5c 100644 --- a/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.spec.ts +++ b/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.spec.ts @@ -134,7 +134,18 @@ describe('FilePreviewHttpService', () => { expect(fetchMock).not.toHaveBeenCalled(); }); - it('refuses a format the pane has no renderer for', async () => { + it('refuses a format the pane cannot preview at all', async () => { + const pending = service.fetchDocument('up1'); + flushPreviewUrl({ mimeType: 'application/pdf', filename: 'report.pdf' }); + + await expect(pending).rejects.toThrow(FilePreviewError); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('refuses to fetch the bytes of a kind that is read server-side', async () => { + // An .xlsx has a viewer, but not one that reads bytes. Letting it + // through here would pull a whole workbook into the browser and + // then throw it away, so the byte path rejects it before the fetch. const pending = service.fetchDocument('up1'); flushPreviewUrl({ mimeType: @@ -146,6 +157,53 @@ describe('FilePreviewHttpService', () => { expect(fetchMock).not.toHaveBeenCalled(); }); + describe('fetchSheets', () => { + it('asks app-api for rows and never touches S3', async () => { + // The .xlsx path has one leg, not two: no presigned URL, and the + // workbook never reaches the browser. + const pending = service.fetchSheets('up1'); + const req = httpMock.expectOne( + '/api/files/up1/sheet-preview', + ); + expect(req.request.method).toBe('GET'); + req.flush({ + uploadId: 'up1', + filename: 'budget.xlsx', + sheets: [], + truncated: false, + }); + + await expect(pending).resolves.toMatchObject({ filename: 'budget.xlsx' }); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it.each([ + [413, false, 'too large'], + [422, false, 'corrupt or password-protected'], + [415, false, "isn't a spreadsheet"], + [404, true, 'no longer available'], + [500, true, 'Check your connection'], + ])( + 'maps %i to a %s-retryable message', + async (status, retryable, fragment) => { + // The route distinguishes these on purpose: 413 and 422 are + // permanent facts about the file, while a 5xx is worth another go. + const pending = service.fetchSheets('up1'); + httpMock + .expectOne('/api/files/up1/sheet-preview') + .flush('err', { status, statusText: 'error' }); + + await expect(pending).rejects.toThrow(FilePreviewError); + await expect(pending).rejects.toMatchObject({ retryable }); + await expect(pending).rejects.toThrow( + expect.objectContaining({ + message: expect.stringContaining(fragment), + }), + ); + }, + ); + }); + it('fetches S3 without credentials', async () => { // S3 answers a CORS GET without Access-Control-Allow-Credentials, so // a credentialed request is rejected by the browser before it is diff --git a/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.ts b/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.ts index 534efba5..a7d6fe21 100644 --- a/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.ts +++ b/frontend/ai.client/src/app/session/services/file-preview/file-preview-http.service.ts @@ -5,8 +5,10 @@ import { ConfigService } from '../../../services/config.service'; import { PREVIEW_KIND_MIMES, PreviewKind, + previewFetchesBytes, previewKindFor, } from './file-preview.model'; +import { SheetPreviewResponse } from './sheet-preview.model'; /** `GET /files/{uploadId}/preview-url` — camelCase aliases on the wire. */ interface PreviewUrlResponseDto { @@ -95,6 +97,18 @@ export class FilePreviewHttpService { ); } + // A kind that is read server-side must never reach the byte path. + // Nothing here could render the result, and the cost of finding out + // is a whole workbook pulled into the browser and thrown away — so + // this fails loudly rather than wasting the transfer. `fetchSheets` + // is the route for those. + if (!previewFetchesBytes(kind)) { + throw new FilePreviewError( + 'This file is read on the server; use fetchSheets instead.', + false, + ); + } + const bytes = await this.fetchBytes(meta.url); return { bytes, mimeType: meta.mimeType, filename: meta.filename, kind }; } @@ -121,6 +135,28 @@ export class FilePreviewHttpService { } } + /** + * Fetch an `.xlsx` as rows, rather than as bytes. + * + * The only preview that does not go through `fetchDocument`, and the + * reason is the renderer landscape rather than anything about the + * file: there is no client-side workbook reader we are willing to + * ship, so app-api reads it with openpyxl and sends values. No + * presigned URL and no second leg — the workbook never reaches the + * browser at all. + */ + async fetchSheets(uploadId: string): Promise { + try { + return await firstValueFrom( + this.http.get( + `${this.config.appApiUrl()}/files/${encodeURIComponent(uploadId)}/sheet-preview`, + ), + ); + } catch (e) { + throw sheetPreviewError(e); + } + } + private async fetchBytes(url: string): Promise { let response: Response; try { @@ -142,3 +178,40 @@ export class FilePreviewHttpService { return response.arrayBuffer(); } } + +/** + * Turn a `/sheet-preview` failure into something the pane can show. + * + * The route distinguishes its failures on purpose, and each one means a + * different thing to the user: 413 and 422 are permanent facts about the + * file that no retry changes, while a 5xx or a dropped connection is + * worth another go. 415 should be unreachable — the extension chose this + * viewer — so it reads as the mislabelled file it is. + */ +function sheetPreviewError(e: unknown): FilePreviewError { + const status = (e as { status?: number })?.status ?? 0; + switch (status) { + case 404: + return new FilePreviewError('This workbook is no longer available.', true); + case 413: + return new FilePreviewError( + 'This workbook is too large to preview. Download it to open in Excel.', + false, + ); + case 415: + return new FilePreviewError( + "This file isn't a spreadsheet the preview can read.", + false, + ); + case 422: + return new FilePreviewError( + 'This workbook could not be read. It may be corrupt or password-protected.', + false, + ); + default: + return new FilePreviewError( + "Couldn't load this workbook. Check your connection and try again.", + true, + ); + } +} diff --git a/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.spec.ts b/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.spec.ts index a6529585..b79c4e98 100644 --- a/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.spec.ts +++ b/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.spec.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from 'vitest'; import { isPreviewableFilename, PREVIEW_KIND_LABELS, + previewFetchesBytes, previewKindFor, } from './file-preview.model'; @@ -10,12 +11,14 @@ describe('previewKindFor', () => { expect(previewKindFor('plan.docx')).toBe('docx'); expect(previewKindFor('deck.pptx')).toBe('pptx'); expect(previewKindFor('rows.csv')).toBe('csv'); + expect(previewKindFor('budget.xlsx')).toBe('xlsx'); }); it('ignores case and surrounding whitespace', () => { expect(previewKindFor(' REPORT.DOCX ')).toBe('docx'); expect(previewKindFor('Quarterly Review.PPTX')).toBe('pptx'); expect(previewKindFor('Export.CSV')).toBe('csv'); + expect(previewKindFor('Budget FY27.XLSX')).toBe('xlsx'); }); it('declines the pre-2007 binary formats', () => { @@ -26,11 +29,18 @@ describe('previewKindFor', () => { expect(previewKindFor('old.xls')).toBeNull(); }); - it('declines .xlsx', () => { - // Deliberate: there is no client-side spreadsheet renderer we are - // willing to ship. `.csv` is previewable because it needs a parser - // rather than a renderer — see the note on previewKindFor. - expect(previewKindFor('budget.xlsx')).toBeNull(); + it('reads .xlsx server-side rather than from bytes', () => { + // The one kind whose bytes never reach the browser: no client-side + // workbook renderer was shippable, so app-api reads it instead. A + // panel that ran its presigned-URL fetch for this kind would + // download the whole workbook and then ignore it. + expect(previewFetchesBytes('xlsx')).toBe(false); + }); + + it('reads every other kind from bytes it fetched', () => { + for (const kind of ['docx', 'pptx', 'csv'] as const) { + expect(previewFetchesBytes(kind)).toBe(true); + } }); it('declines an extension that merely contains a known one', () => { @@ -46,13 +56,14 @@ describe('previewKindFor', () => { 'd.txt', 'e.doc', 'f.csv', + 'g.xls', ]) { expect(isPreviewableFilename(name)).toBe(previewKindFor(name) !== null); } }); it('labels every kind it can return', () => { - for (const name of ['a.docx', 'b.pptx', 'c.csv']) { + for (const name of ['a.docx', 'b.pptx', 'c.csv', 'd.xlsx']) { const kind = previewKindFor(name); expect(kind).not.toBeNull(); expect(PREVIEW_KIND_LABELS[kind!]).toBeTruthy(); diff --git a/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.ts b/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.ts index 023f50cb..cbb4ffea 100644 --- a/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.ts +++ b/frontend/ai.client/src/app/session/services/file-preview/file-preview.model.ts @@ -28,14 +28,23 @@ export const PPTX_MIME = */ export const CSV_MIME = 'text/csv'; +/** + * MIME type of an Excel workbook (OOXML). Matches the `.xlsx` entry in + * `apis.shared.files.ALLOWED_EXTENSIONS` and `SHEET_PREVIEW_MIME_TYPES` + * in the same module. + */ +export const XLSX_MIME = + 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet'; + /** What the pane knows how to render, and which viewer does it. */ -export type PreviewKind = 'docx' | 'pptx' | 'csv'; +export type PreviewKind = 'docx' | 'pptx' | 'csv' | 'xlsx'; /** Human label for the pane header's subtitle. */ export const PREVIEW_KIND_LABELS: Readonly> = { docx: 'Word document', pptx: 'PowerPoint presentation', csv: 'Data file', + xlsx: 'Spreadsheet', }; /** @@ -58,6 +67,7 @@ export const PREVIEW_KIND_MIMES: Readonly< docx: [DOCX_MIME], pptx: [PPTX_MIME], csv: [CSV_MIME, 'application/csv', 'application/vnd.ms-excel', 'text/plain'], + xlsx: [XLSX_MIME], }; /** @@ -76,24 +86,37 @@ export const PREVIEW_KIND_MIMES: Readonly< * the pre-2007 binary formats, which neither the OOXML renderers nor * the delimited-text parser can read at all. * - * `.xlsx` is deliberately absent, and for a narrower reason than it - * looks. There is no client-side *renderer* for it we are willing to - * ship: the npm build of SheetJS is frozen at a 2022 release carrying - * unfixed advisories, and the only maintained grid renderer is built on - * ExcelJS, which throws outright on the workbooks + * `.xlsx` is previewed, but not like the others: it is the one kind + * whose bytes never reach the browser. No client-side workbook renderer + * was shippable — the npm build of SheetJS is frozen at a 2022 release + * carrying unfixed advisories, and the only maintained grid renderer is + * built on ExcelJS, which throws outright on the workbooks * `create_excel_spreadsheet` produces whenever one contains a native - * chart. That rules out reading the bytes here — it does not rule out a - * server-side read that hands this pane rows, which is the open path. - * Until then, download-and-open remains it for `.xlsx`. + * chart — so app-api reads it with openpyxl and sends rows instead. Any + * caller that branches on MIME type for `.xlsx` is therefore on the + * wrong path; branch on the kind, and see `XlsxViewerComponent`. */ export function previewKindFor(filename: string): PreviewKind | null { const name = filename.trim(); if (/\.docx$/i.test(name)) return 'docx'; if (/\.pptx$/i.test(name)) return 'pptx'; if (/\.csv$/i.test(name)) return 'csv'; + if (/\.xlsx$/i.test(name)) return 'xlsx'; return null; } +/** + * Whether the pane reads this kind from bytes it fetched itself. + * + * False only for `xlsx`, whose viewer takes an upload id and asks the + * server for rows. The panel uses this to decide whether to run the + * presigned-URL fetch at all — without it, opening a workbook would + * download the whole file to the browser and then ignore it. + */ +export function previewFetchesBytes(kind: PreviewKind): boolean { + return kind !== 'xlsx'; +} + /** Whether a filename is one the preview pane can render. */ export function isPreviewableFilename(filename: string): boolean { return previewKindFor(filename) !== null; diff --git a/frontend/ai.client/src/app/session/services/file-preview/sheet-preview.model.ts b/frontend/ai.client/src/app/session/services/file-preview/sheet-preview.model.ts new file mode 100644 index 00000000..cc8783b5 --- /dev/null +++ b/frontend/ai.client/src/app/session/services/file-preview/sheet-preview.model.ts @@ -0,0 +1,32 @@ +/** + * The shape `GET /files/{uploadId}/sheet-preview` returns. + * + * Mirrors `SheetPreview` / `SheetPreviewResponse` in + * `apis/shared/files/models.py`, which serialise with camelCase aliases. + * Every cell is a string the server already formatted for display — + * dates, floats and booleans included — so the grid never has to decide + * how a value should read, and the two readers behind it (openpyxl here, + * the browser's own parser for `.csv`) cannot drift apart on it. + */ +export interface SheetPreview { + /** Worksheet name, as it appears on the tab in Excel. */ + name: string; + /** First row of the sheet, used as column labels. */ + headers: string[]; + /** Body rows, each padded to `headers.length`. */ + rows: string[][]; + /** Body rows the sheet claims to have, which may exceed `rows.length`. */ + totalRows: number; + /** True when a cap stopped the read short of the sheet's end. */ + truncated: boolean; + /** Which cap fired. */ + truncatedBy: 'rows' | 'columns' | null; +} + +export interface SheetPreviewResponse { + uploadId: string; + filename: string; + /** Visible worksheets, in workbook order. Hidden sheets are omitted. */ + sheets: SheetPreview[]; + truncated: boolean; +} From 740f5c81d2a066a5d797137a5bd6f9dbc7afde5c Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Tue, 15 Sep 2026 19:40:25 -0600 Subject: [PATCH 3/3] fix(files): show ten times the spreadsheet rows, and unfreeze the scroller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two problems, found by questioning why the xlsx preview stopped at 500 rows when the csv one shows 50,000. **The cap was guessed, not measured.** It is not a limit on what the grid can draw — the viewer virtualises and recycles row elements, so the DOM is bounded whatever it is handed. It bounds the server: one JSON response, and an openpyxl parse in front of it. Measured on a 20-column sheet: cap parse JSON gzip 500 0.04s 0.06 MB 0.02 MB 5,000 0.53s 0.68 MB 0.15 MB 50,000 3.80s 7.17 MB 1.51 MB There is no gzip middleware on app-api, so the JSON column is the wire cost. 5,000 buys ten times the rows for half a second. Fetch-on-scroll was considered and rejected on measurement: openpyxl's read-only mode is a streaming parser, so `min_row` does not seek. Reaching row 45,000 of a 50,000-row sheet costs 2.63s against 2.97s for a complete pass, so paging is O(n²) over a scroll — ~119s of CPU where one pass costs 3s. For this format, read once and send more. Two further fixes the measurements exposed: - The cell budget divided by MAX_COLUMNS_PER_SHEET rather than the sheet's own width, charging a 7-column sheet as though it were 64 columns wide and cutting its rows by an order of magnitude. - The formula pass is now lazy. It doubles the parse and earns nothing on a sheet with no empty cells, since an empty cell is the only thing a cached-value-less formula can look like. A 50,000-row sheet with no formulas went 0.53s → 0.09s. **The scroller froze on any second dataset.** One viewport serves every dataset the grid is handed — switching worksheet tabs, or previewing a second file without closing the pane. CDK picked up the new length (the spacer grew to full height) but never recomputed the rendered range, so the body stayed pinned to the first screenful while the scrollbar moved over the whole sheet. Measured in the browser: at scrollOffset 10000 of a 1,200-row sheet the rendered range was still {start: 0, end: 32}, and checkViewportSize() corrected it to {start: 309, end: 345} on the spot. The grid now re-measures and rewinds when its rows change — which is the right behaviour anyway, since a newly chosen sheet should start at its first row rather than inherit the previous one's offset. Verified live against dev: the 1,200-row sheet now returns whole in a 311ms round trip and scrolls to row 1,200 with 29-36 rows in the DOM. Co-Authored-By: Claude Opus 5 --- .../src/apis/app_api/files/sheet_preview.py | 94 ++++++++++-- .../file-preview/data-grid.component.spec.ts | 134 ++++++++++++++++++ .../file-preview/data-grid.component.ts | 31 ++++ 3 files changed, 245 insertions(+), 14 deletions(-) create mode 100644 frontend/ai.client/src/app/session/components/message-list/components/file-preview/data-grid.component.spec.ts diff --git a/backend/src/apis/app_api/files/sheet_preview.py b/backend/src/apis/app_api/files/sheet_preview.py index b85984df..22e01e15 100644 --- a/backend/src/apis/app_api/files/sheet_preview.py +++ b/backend/src/apis/app_api/files/sheet_preview.py @@ -39,17 +39,44 @@ # request would threaten the container. MAX_WORKBOOK_BYTES = 25 * 1024 * 1024 -# Per-sheet caps. A docked pane shows about thirty rows at a time, so -# these are generous for previewing while keeping the JSON small. -MAX_ROWS_PER_SHEET = 500 +# Per-sheet caps. +# +# This is NOT a limit on what the grid can draw. The viewer virtualises +# with `cdk-virtual-scroll-viewport`, which recycles row elements, so the +# DOM is bounded no matter how many rows it is handed — the browser-side +# `.csv` reader hands it 50,000 quite happily. The limit here is the +# server: every row crosses the wire as JSON in one response, and +# openpyxl has to parse it first. +# +# Measured on a 20-column sheet (two-pass read, JSON serialised): +# +# cap parse JSON gzip +# 500 0.04s 0.06 MB 0.02 MB +# 5,000 0.53s 0.68 MB 0.15 MB +# 50,000 3.80s 7.17 MB 1.51 MB +# +# There is no gzip middleware on app-api, so the JSON column is what +# actually goes over the wire. 5,000 buys ten times the rows for half a +# second; 50,000 costs nearly four seconds of parse and a 7 MB response, +# which is past what a preview should spend. +# +# Fetching later pages on scroll was considered and rejected: openpyxl's +# read-only mode is a streaming parser, so `min_row` does not seek. +# Reaching row 45,000 of a 50,000-row sheet measured 2.63s against 2.97s +# for a *complete* pass — page cost grows with offset, and scrolling a +# whole sheet page-by-page would cost ~119s of CPU where one pass costs +# 3s. For this format, reading once and sending more is strictly better +# than reading repeatedly and sending less. +MAX_ROWS_PER_SHEET = 5_000 MAX_COLUMNS_PER_SHEET = 64 # Sheets read from one workbook. MAX_SHEETS = 12 # Global cell budget across every sheet, so a workbook of many wide -# sheets cannot multiply the per-sheet caps into a huge response. -MAX_TOTAL_CELLS = 40_000 +# sheets cannot multiply the per-sheet caps into a huge response. At +# roughly 12 bytes of JSON per cell this bounds the body near 1.8 MB. +MAX_TOTAL_CELLS = 150_000 class WorkbookTooLargeError(Exception): @@ -89,7 +116,17 @@ def read_workbook_preview(data: bytes) -> list[SheetPreview]: # "=SUM(B2:B10)" is both more useful than an empty cell and more # honest: the file really does not carry that number yet. values = _load(data, data_only=True) - formulas = _load(data, data_only=False) + # The formula pass is opened lazily. It doubles the parse, and most + # sheets need it for nothing — it only earns its cost on a sheet that + # actually came back with an empty cell. + formulas = None + + def formula_sheet_at(index: int): + nonlocal formulas + if formulas is None: + formulas = _load(data, data_only=False) + sheets = formulas.worksheets + return sheets[index] if index < len(sheets) else None try: sheets: list[SheetPreview] = [] @@ -104,10 +141,7 @@ def read_workbook_preview(data: bytes) -> list[SheetPreview]: if getattr(value_sheet, "sheet_state", "visible") != "visible": continue - formula_sheet = ( - formulas.worksheets[index] if index < len(formulas.worksheets) else None - ) - sheet, used = _read_sheet(value_sheet, formula_sheet, budget) + sheet, used = _read_sheet(value_sheet, index, formula_sheet_at, budget) budget -= used sheets.append(sheet) if budget <= 0: @@ -117,7 +151,8 @@ def read_workbook_preview(data: bytes) -> list[SheetPreview]: finally: # `read_only` workbooks hold the zip open until closed. values.close() - formulas.close() + if formulas is not None: + formulas.close() def _load(data: bytes, *, data_only: bool): @@ -139,15 +174,32 @@ def _load(data: bytes, *, data_only: bool): raise WorkbookUnreadableError(str(e)) from e -def _read_sheet(value_sheet, formula_sheet, budget: int) -> tuple[SheetPreview, int]: +def _read_sheet( + value_sheet, index: int, formula_sheet_at, budget: int +) -> tuple[SheetPreview, int]: """Read one worksheet into a `SheetPreview`, stopping at the caps. + `formula_sheet_at` opens the second, formula-bearing pass on demand. Returns the preview and how much of the cell budget it consumed. """ - row_limit = min(MAX_ROWS_PER_SHEET, max(1, budget // MAX_COLUMNS_PER_SHEET)) + # Spend the cell budget against the sheet's OWN width, not the + # maximum a sheet is allowed to be. Dividing by MAX_COLUMNS_PER_SHEET + # charged a seven-column sheet as though it were sixty-four columns + # wide and cut its rows by an order of magnitude for no reason. + estimated_width = _peek_width(value_sheet) or MAX_COLUMNS_PER_SHEET + row_limit = min(MAX_ROWS_PER_SHEET, max(1, budget // estimated_width)) value_rows = _take_rows(value_sheet, row_limit + 1) - formula_rows = _take_rows(formula_sheet, row_limit + 1) if formula_sheet else [] + + # Only pay for the formula pass when this sheet has a gap to fill. An + # empty cell is either genuinely empty or a formula with no cached + # value, and the two are indistinguishable from the values pass — but + # a sheet with no empty cells at all cannot be hiding a formula. + formula_rows: list[tuple[Any, ...]] = [] + if any(cell is None for row in value_rows for cell in row): + formula_sheet = formula_sheet_at(index) + if formula_sheet is not None: + formula_rows = _take_rows(formula_sheet, row_limit + 1) merged = [ _merge_row(value_rows[i], formula_rows[i] if i < len(formula_rows) else ()) @@ -206,6 +258,20 @@ def _read_sheet(value_sheet, formula_sheet, budget: int) -> tuple[SheetPreview, ) +def _peek_width(sheet) -> int: + """Columns the sheet's header row occupies, for budgeting. + + Cheap: `max_column` comes from the sheet's declared dimensions and + needs no row scan. It can overstate the real width when a stray + format stretches the used range, which only makes the budget more + conservative, never less. + """ + try: + return min(int(sheet.max_column or 0), MAX_COLUMNS_PER_SHEET) + except Exception: + return 0 + + def _take_rows(sheet, limit: int) -> list[tuple[Any, ...]]: """Pull at most `limit` rows, stopping the stream rather than reading the whole sheet to throw most of it away.""" diff --git a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/data-grid.component.spec.ts b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/data-grid.component.spec.ts new file mode 100644 index 00000000..d9fa9a52 --- /dev/null +++ b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/data-grid.component.spec.ts @@ -0,0 +1,134 @@ +import { CdkVirtualScrollViewport } from '@angular/cdk/scrolling'; +import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { DataGridComponent } from './data-grid.component'; + +function rows(n: number, cols = 2): string[][] { + return Array.from({ length: n }, (_, i) => + Array.from({ length: cols }, (_, c) => `r${i}c${c}`), + ); +} + +describe('DataGridComponent', () => { + let fixture: ComponentFixture; + + beforeEach(async () => { + // jsdom implements neither Element.scrollTo nor Element.scrollBy, + // and CDK's `scrollToOffset` ends in the former. A no-op keeps the + // component's real code path intact; the scrolling itself is + // verified in a browser, where jsdom's gaps do not apply. + for (const name of ['scrollTo', 'scrollBy'] as const) { + if (typeof (Element.prototype as never as Record)[name] !== 'function') { + (Element.prototype as never as Record)[name] = () => undefined; + } + } + + TestBed.resetTestingModule(); + await TestBed.configureTestingModule({ + imports: [DataGridComponent], + }).compileComponents(); + + fixture = TestBed.createComponent(DataGridComponent); + }); + + function render(headers: string[], body: string[][]): HTMLElement { + fixture.componentRef.setInput('headers', headers); + fixture.componentRef.setInput('rows', body); + fixture.detectChanges(); + return fixture.nativeElement as HTMLElement; + } + + it('renders the headers it is given', () => { + const el = render(['Item', 'Cost'], [['Rent', '1200']]); + + const headers = Array.from(el.querySelectorAll('[role="columnheader"]')) + .map((n) => n.textContent?.trim()) + .filter((t) => t !== '#'); + expect(headers).toEqual(['Item', 'Cost']); + }); + + it('reports the data size to assistive tech, not the rendered window', () => { + const el = render(['a', 'b'], rows(900)); + + const grid = el.querySelector('[role="grid"]'); + expect(grid?.getAttribute('aria-rowcount')).toBe('901'); + expect(grid?.getAttribute('aria-colcount')).toBe('2'); + }); + + it('shows the empty message in place of the body when there are no rows', () => { + fixture.componentRef.setInput('emptyMessage', 'This sheet is empty.'); + const el = render(['a', 'b'], []); + + expect(el.textContent).toContain('This sheet is empty.'); + expect(el.querySelector('cdk-virtual-scroll-viewport')).toBeNull(); + }); + + it('gives header and body rows one shared track list', () => { + const el = render(['a', 'b'], [['1', '2']]); + + const grid = el.querySelector('[role="grid"]') as HTMLElement; + // Gutter plus one track per column. + expect( + grid.style.getPropertyValue('--grid-cols').trim().split(/\s+/), + ).toHaveLength(3); + }); + + describe('re-arming the scroller when the data changes', () => { + /** + * One viewport instance serves every dataset — switching worksheet + * tabs, or previewing a second file without closing the pane. CDK + * picks up the new length but does NOT recompute the rendered + * range, so the body stays pinned to the first screenful while the + * scrollbar moves over the full height. Verified in the browser: at + * scrollOffset 10000 of a 1,200-row sheet the rendered range was + * still {0, 32}, and checkViewportSize() corrected it to {309, 345}. + * + * jsdom gives the viewport zero height, so no rows are ever + * rendered and the range itself cannot be asserted here. What this + * does pin down is that the component asks the viewport to + * re-measure and rewind — the two calls whose absence caused it. + */ + function spyOnViewport() { + const viewport = fixture.debugElement + .query((n) => n.componentInstance instanceof CdkVirtualScrollViewport) + ?.componentInstance as CdkVirtualScrollViewport; + expect(viewport).toBeDefined(); + return { + checkViewportSize: vi.spyOn(viewport, 'checkViewportSize'), + scrollToOffset: vi.spyOn(viewport, 'scrollToOffset'), + }; + } + + it('re-measures and rewinds when the rows are replaced', () => { + vi.useFakeTimers(); + try { + render(['a', 'b'], rows(5)); + const spies = spyOnViewport(); + + render(['a', 'b'], rows(1200)); + vi.runAllTimers(); + + expect(spies.checkViewportSize).toHaveBeenCalled(); + expect(spies.scrollToOffset).toHaveBeenCalledWith(0); + } finally { + vi.useRealTimers(); + } + }); + + it('rewinds the header to match', () => { + vi.useFakeTimers(); + try { + const el = render(['a', 'b'], rows(5)); + const header = el.querySelector('[role="grid"] > div') as HTMLElement; + header.scrollLeft = 250; + + render(['a', 'b'], rows(1200)); + vi.runAllTimers(); + + expect(header.scrollLeft).toBe(0); + } finally { + vi.useRealTimers(); + } + }); + }); +}); diff --git a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/data-grid.component.ts b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/data-grid.component.ts index 1695bf85..9786f4bb 100644 --- a/frontend/ai.client/src/app/session/components/message-list/components/file-preview/data-grid.component.ts +++ b/frontend/ai.client/src/app/session/components/message-list/components/file-preview/data-grid.component.ts @@ -251,6 +251,37 @@ export class DataGridComponent { }); onCleanup(() => sub.unsubscribe()); }); + + // Re-arm the scroller whenever the data underneath it changes. + // + // One viewport instance serves every dataset the grid is handed — + // switching worksheet tabs, or previewing a second file without + // closing the pane. CDK picks up the new length (getDataLength() + // reports it, and the spacer grows) but does not recompute the + // rendered range, so the body stays pinned to the first screenful + // while the scrollbar moves over the full height. Measured directly: + // at scrollOffset 10000 of a 1,200-row sheet the rendered range was + // still {start: 0, end: 32}, and checkViewportSize() corrected it to + // {start: 309, end: 345} on the spot. + // + // Resetting to the top is the right behaviour on its own terms too — + // a newly chosen sheet should start at its first row rather than + // inheriting the previous one's offset. + effect((onCleanup) => { + const rows = this.rows(); + const viewport = this.viewport(); + if (!viewport || rows.length === 0) return; + + // After the rows themselves have been rendered: measuring before + // that would measure the outgoing dataset. + const handle = setTimeout(() => { + viewport.scrollToOffset(0); + viewport.checkViewportSize(); + const header = this.headerScroller()?.nativeElement; + if (header) header.scrollLeft = 0; + }); + onCleanup(() => clearTimeout(handle)); + }); } /** `trackBy` on the row index rather than the row: a data file may