feat(xlsx-preview): add artifact spreadsheet preview#16712
feat(xlsx-preview): add artifact spreadsheet preview#16712zhangjiadi225 wants to merge 34 commits into
Conversation
There was a problem hiding this comment.
This review was translated automatically.
The scope of this change is primarily adding read-only preview for .xlsx artifacts: OfficePreviewPanel integration with xlsx, adding worker for parsing/rendering model, virtualized grid, chart rendering, formula/number format handling, i18n, and tests. The overall implementation and test coverage is fairly complete, and CI has passed. I've left 2 accessibility/design-system warnings, and I suggest addressing them before approval.
Original Content
本次改动范围主要是新增 artifact 的 .xlsx 只读预览:OfficePreviewPanel 接入 xlsx,新增 worker 解析/render model、虚拟化网格、图表渲染、公式/数字格式处理、i18n 与测试。整体实现和测试覆盖较完整,CI 已通过;我留了 2 个 accessibility/design-system warning,建议处理后再 approve。
There was a problem hiding this comment.
This review was translated automatically.
Findings:
- Important:
useXlsxWorkbookonly rejects the compressed.xlsxbyte length before handing the buffer toJSZip.loadAsyncandExcelJS.Workbook.xlsx.load; there is no entry-count, per-entry, or total uncompressed-size preflight. A crafted workbook can stay under the 20 MB source cap while expanding into very large worksheet XML/media in the parser worker, tying up memory/CPU before the preview can fail cleanly. Please add DOCX/PPTX-style ZIP central-directory limits beforeJSZip.loadAsync/ExcelJS and cover oversized entry/total-uncompressed cases in tests.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/useXlsxWorkbook.ts:50,src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/parseWorkbook.ts:394 - Important: Formula range evaluation materializes every cell in
onRangewith unbounded nested loops before the deadline is checked again. A tiny workbook can contain a range likeSUM(A1:XFD1048576)and hang or exhaust the worker while building the array. Please cap supported range dimensions and check the deadline inside the loop, returning unevaluated without materializing huge ranges.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/formulaEvaluator.ts:128 - Important: Chart cache parsing trusts
ptCountand sparsept idxvalues from chart XML when allocating/extending arrays. Malformed XML can declare a huge count or far-out index and force large allocations before the per-chart catch can recover. Please bound these values to finite non-negative integers within a reasonable chart/workbook limit, or skip the cache and fall back to the referenced cells with a warning.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/chartXmlParser.ts:298 - Important: Excel
percentStackedcharts are collapsed to the samestackedboolean as ordinary stacked charts, andbuildChartOptionthen renders raw values. That makes 100% stacked Excel charts show absolute totals rather than percentages, which materially misrepresents the workbook. Please preserve a distinct percent-stacked state and normalize per category, or mark it unsupported with a warning.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/chartXmlParser.ts:467,src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/charts/buildChartOption.ts:22 - Important:
formatCellValueparses any string as a date when the cell has a date-likenumFmt, so a literal text cell such as"1"can render as a date instead of the text the workbook stored. Please restrict the string date path to the strict ISO strings produced for real Date values, or carry Date identity explicitly.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/numberFormat.ts:53 - Important: The merged-cell overlay fixes the visual duplicate, but the base virtualized layer still exposes
role="gridcell"for every covered cell while the merge overlay exposes another gridcell for the same master coordinate. Assistive tech can see empty covered cells plus a second semantic master cell. Please make the merged cell the single semantic gridcell, or hide covered placeholders from the accessibility tree, and add a regression test for duplicate merged-cell coordinates.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/XlsxGrid.tsx:548,src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/XlsxGrid.tsx:587 - Suggestion:
SheetRenderModel.frozenis parsed and mocked but intentionally unused by the v1 grid. Since this is new render-model surface with zero real consumer demand today, please either wire frozen panes intoXlsxGridwith tests or delete the field/parser output until the UI consumes it.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/renderModel.ts:35
CI: GitHub checks are green for the reviewed head except skipped auxiliary jobs; local pnpm lint/test/format was not run per repository review rules.
After fixing, please request eeee0717 for follow-up review.
Original Content
Findings:
- Important:
useXlsxWorkbookonly rejects the compressed.xlsxbyte length before handing the buffer toJSZip.loadAsyncandExcelJS.Workbook.xlsx.load; there is no entry-count, per-entry, or total uncompressed-size preflight. A crafted workbook can stay under the 20 MB source cap while expanding into very large worksheet XML/media in the parser worker, tying up memory/CPU before the preview can fail cleanly. Please add DOCX/PPTX-style ZIP central-directory limits beforeJSZip.loadAsync/ExcelJS and cover oversized entry/total-uncompressed cases in tests.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/useXlsxWorkbook.ts:50,src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/parseWorkbook.ts:394 - Important: Formula range evaluation materializes every cell in
onRangewith unbounded nested loops before the deadline is checked again. A tiny workbook can contain a range likeSUM(A1:XFD1048576)and hang or exhaust the worker while building the array. Please cap supported range dimensions and check the deadline inside the loop, returning unevaluated without materializing huge ranges.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/formulaEvaluator.ts:128 - Important: Chart cache parsing trusts
ptCountand sparsept idxvalues from chart XML when allocating/extending arrays. Malformed XML can declare a huge count or far-out index and force large allocations before the per-chart catch can recover. Please bound these values to finite non-negative integers within a reasonable chart/workbook limit, or skip the cache and fall back to the referenced cells with a warning.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/chartXmlParser.ts:298 - Important: Excel
percentStackedcharts are collapsed to the samestackedboolean as ordinary stacked charts, andbuildChartOptionthen renders raw values. That makes 100% stacked Excel charts show absolute totals rather than percentages, which materially misrepresents the workbook. Please preserve a distinct percent-stacked state and normalize per category, or mark it unsupported with a warning.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/chartXmlParser.ts:467,src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/charts/buildChartOption.ts:22 - Important:
formatCellValueparses any string as a date when the cell has a date-likenumFmt, so a literal text cell such as"1"can render as a date instead of the text the workbook stored. Please restrict the string date path to the strict ISO strings produced for real Date values, or carry Date identity explicitly.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/numberFormat.ts:53 - Important: The merged-cell overlay fixes the visual duplicate, but the base virtualized layer still exposes
role="gridcell"for every covered cell while the merge overlay exposes another gridcell for the same master coordinate. Assistive tech can see empty covered cells plus a second semantic master cell. Please make the merged cell the single semantic gridcell, or hide covered placeholders from the accessibility tree, and add a regression test for duplicate merged-cell coordinates.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/XlsxGrid.tsx:548,src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/XlsxGrid.tsx:587 - Suggestion:
SheetRenderModel.frozenis parsed and mocked but intentionally unused by the v1 grid. Since this is new render-model surface with zero real consumer demand today, please either wire frozen panes intoXlsxGridwith tests or delete the field/parser output until the UI consumes it.src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/renderModel.ts:35
CI: GitHub checks are green for the reviewed head except skipped auxiliary jobs; local pnpm lint/test/format was not run per repository review rules.
修复后请 request eeee0717 再进行后续 review。
@eeee0717 Thanks for the review, all 7 findings have been fixed and pushed (one commit per finding, each with unit tests):
Supplementary note: Local full Original Content@eeee0717 感谢 review,7 条发现已全部修复并 push(每条一个 commit,均附单测):
补充说明:本地全量 |
eeee0717
left a comment
There was a problem hiding this comment.
Reviewed current head 167cc882e22432b40cd6e7d0935ee020d770e893 for the renewed review request. GitHub CI is passing (notify skipped), and I did not run local pnpm lint/test/format per repo review rules.
The seven issues from my previous review appear addressed by inspection: ZIP central-directory preflight now runs before JSZip/ExcelJS, formula ranges are capped/deadline-checked, chart cache counts/indices are bounded, percentStacked is preserved and normalized, string date parsing is strict, merged placeholders are hidden from the accessibility tree, and the unused frozen field was removed.
Blocking findings:
-
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/parseWorkbook.ts:765still lets chart reference fallback materialize unbounded ranges. When chart cache data is missing or rejected,chartXmlParsercallsdata.readRange(ref), and this implementation loops every row/col in the parsed A1 reference with no area cap. A tiny workbook can point a chart atSheet1!$A$1:$XFD$1048576and tie up or exhaust the worker despite the new cacheptCountguard and ZIP preflight. Please cap chart reference ranges before building the 2D array, similar to the formulaMAX_RANGE_CELLSguard, and skip/mark oversized references with a warning. -
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/chartXmlParser.ts:193feeds untrusted drawing anchor coordinates directly intolayout.colX(point.col + 1)/layout.rowY(point.row + 1), whose implementations inparseWorkbook.ts:748and:755loop up to the supplied coordinate. A crafted chart anchor with a huge<xdr:col>or<xdr:row>can pin the parser worker before chart-level recovery. Please validate/clamp anchor coordinates to the preview limits or make out-of-range offset calculation O(1) and mark the chart/sheet truncated. -
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/parseWorkbook.ts:538stores Date cells inrawValueTableas ISO strings, so uncached formulas that reference dates lose Excel serial-number semantics. For example, ifA1is a date,=A1+1should evaluate to the next day, but the formula parser receives an ISO string and returns a value error/unevaluated result. The render model can keep ISOraw, but the formula evaluation context needs a serial value that respects the workbook date system.
Non-blocking follow-ups:
src/renderer/components/chat/panes/ArtifactPane.tsx:374applies the generic 2 MB artifact cap before the office-preview branch, so artifact-pane.xlsxfiles between 2 MB and the new 20 MB xlsx limit never reachXlsxPreviewPanelor its office actions. Office previews should bypass the text cap like PDF does, or the advertised xlsx limit is not reachable from this UX.src/renderer/i18n/locales/zh-tw.json:7802adds primary Traditional Chinese locale strings as literal[to be translated]:...placeholders. Those are user-visible in zh-TW mode; please ship real zh-TW strings inlocales/zh-tw.jsonrather than placeholder text.
|
@eeee0717 Thanks for the second-round review. The three blocking findings are fixed (one commit each, all with unit tests); one non-blocking item is fixed and the other appears to be a false positive — details below. Blocking
Non-blocking
Tests: the Please request eeee0717 for follow-up review. Original Content@eeee0717 感谢第二轮 review。3 条 blocking 已修复(每条一个 commit,均附单测);1 条 non-blocking 已修,另 1 条经核查疑为误报,说明如下。 Blocking
Non-blocking
测试:本地 修复后请 request eeee0717 再进行后续 review。 |
Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Address review feedback on the spreadsheet preview: - Grid now renders blank rows/cols beyond the used range: it fills the viewport at default cell sizes plus a scrollable buffer, re-measured on panel resize via ResizeObserver (Excel-like empty area). - Drop the standalone status-bar row that duplicated the active sheet name; selected-cell address/formula now shows inline in the bottom bar only when a cell is selected. - Drop the floating zoom overlay; zoom controls move to the right of the sheet tabs in a single bottom bar, and the zoom-reset button is removed (refresh already lives in the top toolbar). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
… text mid-line Row heights were dropping in two ways: - sheetFormatPr defaultRowHeight/defaultColWidth were ignored — every non-overridden row/col rendered at the hardcoded 15pt/8.43ch global default. Now read per sheet (worksheet.properties) and threaded into the render model, image anchors, and chart layout. - Row definitions were read via worksheet.model.rows, whose serializer drops cell-less rows (e.g. <row r="7" hidden="1"/>), un-hiding them. Now read straight from the Row objects on the load path. Cell text display reworked to match normal spreadsheet behavior: - Wrapped cells clip to whole lines (-webkit-line-clamp derived from the cell height) instead of slicing the overflow line in half, so a default-height row shows exactly one clean line. - Clicking a cell opens an absolutely positioned overlay at the cell rect showing the full content (min cell size, max 480px wide); it covers neighbours without pushing the grid layout, replacing the old outline-only selection highlight. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Zooming previously multiplied layout sizes and font size, so elements reflowed at each zoom level: the ECharts container resized (relaying axes/legend or drifting relative to cells), wrapped text re-clamped to a different line count, and fixed-px details (borders, padding, header font) fell out of proportion. Zoom is now a pure visual magnification, like enlarging a rendered sheet from 400x400 to 800x800: - All layout (axis tables, cell rects, fonts, floating objects, the selection overlay) is built at zoom=1. - One content layer applies transform: scale(zoom) with top-left origin; sticky row/col header bars keep scroll-space boxes with a scaled inner layer so they stay aligned. - Virtualizers keep working in scroll space via estimateSize * zoom, and the merge-layer viewport divides scroll coordinates back into content space. - Chart containers no longer change layout size on zoom, so ECharts never re-lays-out; charts scale uniformly with the grid. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
echarts, exceljs, fast-formula-parser, jszip and numfmt are imported only from the renderer (XlsxPreviewPanel) and bundled by Vite, so they belong in devDependencies alongside react/mermaid/katex — not dependencies, which is for unbundled main-process runtime deps. Lockfile relocates the same versions; no resolution changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
- Reset XlsxGrid internal selection when switching sheets by keying the grid on sheet name, removing the stale selection overlay that lingered on the newly-activated sheet while the status bar was empty - Drop the write-only imageUrlsRef (URL revoke is handled by the effect cleanup) - Cap per-column width expansion at MAX_COLS so whole-sheet <col> spans don't allocate thousands of unused entries cloned over postMessage - Pass CellView position as primitive props so its React.memo can skip re-renders when a cell's position is unchanged Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
fast-formula-parser@1.0.19 registers MAX, MIN, MEDIAN, COUNTA, LARGE, SMALL, MODE.SNGL, STDEV.S/.P and VAR.S/.P as empty stubs that return undefined, so the library throws #NAME? "not implemented" and the preview shows "公式无法求值" (e.g. =MAX(C2:C8) in a summary row). Inject implementations via the FormulaParser `functions` option, which merges after the built-ins and overrides the stubs. Aggregation follows Excel semantics: text/blank cells are ignored, empty ranges yield 0 for MAX/MIN, and out-of-range/degenerate inputs surface #NUM!/#N/A as evaluated error results rather than "unevaluated". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Address PR review feedback on the xlsx artifact preview: - Replace the hand-written `role="tab"` buttons with the shared `@cherrystudio/ui` Tabs primitives (line variant), gaining Radix roving focus and Arrow/Home/End keyboard navigation. - Add keyboard cell selection to the grid: arrow keys move (clearing merged regions), Enter/Space select, Escape clears, and moves scrollToIndex the target so virtualized cells stay mounted. - Complete grid semantics: role="grid" with aria-row/colcount on the focusable container, role="row" wrappers, and aria-row/colindex on gridcells. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
SheetRenderModel.frozen was parsed and mocked but intentionally unused by the grid; remove the speculative surface until a consumer exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
The 20MB compressed-size cap cannot stop decompression bombs: a crafted workbook can expand into multi-GB worksheet XML inside the parser worker. Generalize the docx-only preflight into zipPreflight (format label in messages, same limits) and run it on the raw bytes before JSZip/ExcelJS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
onRange previously materialized every cell of a range with no area cap and no deadline check inside the loop, so a tiny workbook containing SUM(A1:XFD1048576) could hang or exhaust the worker. Reject ranges over 100k cells up front and honor the evaluation deadline mid-loop; both paths throw into the existing catch that classifies the formula as unevaluated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
readCache allocated arrays straight from ptCount and extended them to any sparse pt idx declared in chart XML, letting malformed input force huge allocations before the per-chart catch could recover. Bound both to finite non-negative integers within 10k points; out-of-range declarations drop the cache with a warning and fall back to the referenced cells. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
…tages percentStacked was collapsed into the same boolean as ordinary stacked grouping, so 100% stacked Excel charts displayed absolute totals. Preserve the grouping distinctly on ChartModel.stacking and have buildChartOption normalize each category to percentages with a 0-100% value axis. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
formatCellValue passed any string through new Date() when the cell had a date-like numFmt, so literal text such as "1" rendered as a date. Gate the string date path on the exact toISOString() shape that parseWorkbook produces for real Date values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
The base virtualized layer kept role=gridcell on every merge-covered placeholder while the merge overlay exposed a second gridcell for the master coordinate, so assistive tech saw empty covered cells plus a duplicate master. Hide covered placeholders from the accessibility tree; the merge overlay is now the only semantic gridcell for merged coordinates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
The barrel forwarded a bare default, which the barrel/named-only lint rule now rejects on CI. Name the re-export and update the lazy loader and its test mock accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Chart data references come from untrusted chart XML. When the cache is missing or rejected, chartXmlParser falls back to readRange(ref), which materialized every cell of the parsed A1 range with no area bound — a reference like Sheet1!$A$1:$XFD$1048576 could tie up the worker before the per-chart catch could recover. Extract the range read into readRangeFromValueTable and reject references above MAX_CHART_RANGE_CELLS by area, mirroring the formula evaluator's guard. safeReadRange's existing catch turns the throw into missing data. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Chart drawing anchors carry untrusted <xdr:col>/<xdr:row> coordinates. The layout colX/rowY closures summed track sizes by looping from 1 up to the supplied coordinate, so a crafted anchor with a huge col/row could pin the parser worker before the per-chart catch recovered. Replace the loops with axisOffsetPx, which assumes the default size for all preceding tracks and corrects only the custom-sized ones. Cost is bounded by the sheet's actual custom-track count, independent of the anchor coordinate; results are identical for in-range anchors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Date cells stored their ISO string in the raw value table, so an uncached formula referencing a date (e.g. =A1+1) received a string and returned a value error instead of the next day. date1904 context is available at read time and numfmt renders in the 1900 system, so a 1900-system serial is stored regardless of the workbook date system. The render model still keeps the ISO raw on the cell; only the formula evaluation table now holds the numeric serial, for both plain date cells and cached formula results that are dates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
The xlsx_preview keys shipped as literal "[to be translated]" English placeholders in the primary Traditional Chinese locale, so they were user-visible in zh-TW mode. Replace them with real Taiwan-terminology translations (檔案 / 支援). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
44788cb to
450edcf
Compare
There was a problem hiding this comment.
This review was translated automatically.
Findings:
- Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/parseWorkbook.ts:664still computes floating image anchors with localcolX/rowYhelpers that loop up to the untrusted drawing coordinate. The chart-anchor path was moved toaxisOffsetPx, but embedded images still callcolX(tl.nativeCol + 1),rowY(tl.nativeRow + 1), and the same forbr, so a small workbook with a tiny image and huge<xdr:col>/<xdr:row>can still pin the parser worker before the preview reaches a clean truncated/error state. Please reuse the boundedaxisOffsetPxapproach for image anchors too, or clamp/skip hostile image anchors before offset calculation, and add the corresponding hostile-coordinate regression test.
CI: GitHub checks are green for the reviewed head (changes, catalog-hand-edit-check, changeset-check, basic-checks, general-test, and render-test passed; notify skipped). Local pnpm lint/test/format was not run per repository review rules.
After fixing, please request eeee0717 for further review.
Original Content
Findings:
- Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/parseWorkbook.ts:664still computes floating image anchors with localcolX/rowYhelpers that loop up to the untrusted drawing coordinate. The chart-anchor path was moved toaxisOffsetPx, but embedded images still callcolX(tl.nativeCol + 1),rowY(tl.nativeRow + 1), and the same forbr, so a small workbook with a tiny image and huge<xdr:col>/<xdr:row>can still pin the parser worker before the preview reaches a clean truncated/error state. Please reuse the boundedaxisOffsetPxapproach for image anchors too, or clamp/skip hostile image anchors before offset calculation, and add the corresponding hostile-coordinate regression test.
CI: GitHub checks are green for the reviewed head (changes, catalog-hand-edit-check, changeset-check, basic-checks, general-test, and render-test passed; notify skipped). Local pnpm lint/test/format was not run per repository review rules.
修复后请 eeee0717 再进行后续 review。
There was a problem hiding this comment.
This review was translated automatically.
Found 1 blocker and 1 warning. The main risks are all in the new xlsx parsing/worker path: floating image anchor can still use untrusted large coordinates to cause long worker occupation; when switching files, the old parsing will also not be terminated. CI has checked and passed, but it is recommended to merge after fixing the blocker.
Original Content
发现 1 个 blocker 和 1 个 warning。主要风险都在新增 xlsx 解析/worker 路径:floating image anchor 仍可用不可信大坐标造成 worker 长时间占用;切换文件时旧解析也不会被终止。CI 已检查为通过,但建议修完 blocker 后再合并。
|
|
||
| // Floating images. | ||
| const floatingImages: FloatingObjectModel[] = [] | ||
| const colX = (col: number): number => { |
There was a problem hiding this comment.
This review comment was translated automatically.
[A5][Blocker] The floating image path reimplemented colX/rowY here, internally looping from 1 to the passed coordinate; however, tl.nativeCol/nativeRow and br.nativeCol/nativeRow come from the xlsx drawing XML, and like the previously fixed chart anchor, are untrusted input. A very small workbook with image anchor written as huge <xdr:col>/<xdr:row> values will cause prolonged parser worker occupation at colX(tl.nativeCol + 1) or rowY(...); the ZIP pre-check, row/column limits, and stale response discard mechanisms don't take effect in time. It's recommended that the image path also reuse axisOffsetPx(col, colWidthsPx, defaultColWidthPx) / axisOffsetPx(row, rowHeightsPx, defaultRowHeightPx), and add a floating image large coordinate regression test.
Original Content
[A5][Blocker] floating image 路径这里重新实现了 colX/rowY,内部从 1 循环到传入坐标;但 tl.nativeCol/nativeRow 和 br.nativeCol/nativeRow 来自 xlsx drawing XML,和前面已修的 chart anchor 一样是不可信输入。一个很小的 workbook 只要把图片 anchor 写成超大 <xdr:col>/<xdr:row>,就会在 colX(tl.nativeCol + 1) 或 rowY(...) 处长时间占用 parser worker;ZIP 预检、行列上限和 stale response discard 都来不及生效。建议图片路径也复用 axisOffsetPx(col, colWidthsPx, defaultColWidthPx) / axisOffsetPx(row, rowHeightsPx, defaultRowHeightPx),并补一个 floating image 大坐标回归测试。
| worker.postMessage(request, [bytes]) | ||
| })() | ||
|
|
||
| return () => { |
There was a problem hiding this comment.
This review comment was translated automatically.
[A5][Warning] When request is superseded by filePath/refreshKey/sourceSize, cleanup only sets cancelled = true, and the worker doesn't terminate until unmount. parseWorkbook may run for a while on complex but valid xlsx files; when the user switches to a new file, the new request goes to the same busy worker and gets queued, the old parsing continues occupying CPU, and if the old parsing crashes, it may set the new request to error through the current onerror (worker errors have no id for deduplication). Suggest using an independent worker for each parsing request, or terminating the current worker and creating a new one when a new request starts/cleanup; requestId can continue as a stale response fallback, while also updating the test expectations for the current "reusing worker" behavior.
Original Content
[A5][Warning] request 被 filePath/refreshKey/sourceSize supersede 时,cleanup 只设置 cancelled = true,worker 直到 unmount 才 terminate。parseWorkbook 对复杂但合法的 xlsx 可能运行较久;用户切换到新文件时,新 request 会发到同一个 busy worker 并排队,旧解析继续占用 CPU,旧解析崩溃还可能通过当前 onerror 把新 request 置为 error(worker error 没有 id 可去重)。建议每个解析请求使用独立 worker,或在新请求开始/cleanup 时 terminate 当前 worker 并创建新 worker;requestId 可继续作为 stale response 兜底,同时更新当前“reusing worker”的测试预期。
Floating-image anchors came from untrusted drawing XML but computed their pixel offset with local colX/rowY helpers that looped up to the anchor coordinate. A tiny image with a billion-scale <xdr:col>/<xdr:row> could pin the parser worker before the preview reached a clean truncated state. Route image anchors through the bounded axisOffsetPx (already used for chart anchors), whose cost depends only on the sheet's custom-track count, not the anchor magnitude. Add a hostile-coordinate regression test that injects a 1e9 anchor into the stored drawing XML and asserts it resolves near-instantly (tight timeout) with the sheet clamped and flagged truncated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
The hook reused a single lazily-created worker and, on supersede (filePath/refreshKey/sourceSize change), only set cancelled=true — terminating the worker just on unmount. A slow but valid parse could then pin CPU while the next file queued behind it on the same worker, and a crash from the old parse (whose ErrorEvent carries no request id) could flip the new request to error via the rebound onerror. Each request now owns a dedicated worker that the effect cleanup terminates on supersede/unmount, so switching files frees the old parse's CPU immediately and isolates its onerror. requestId stays as a stale-response fallback. Update the worker-reuse tests and add a regression test for superseded-worker crash isolation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
@eeee0717 @AtomsH4 Thanks for the two rounds of review. This round includes 2 [A5] fixes that have been pushed (one commit each, both with tests):
Regarding the first round's [C7] two a11y warnings: verified already handled in earlier commits, so not covered in this review —
Tests: All passed in After fixing, please @eeee0717 @AtomsH4 proceed with follow-up review, thank you. Original Content@eeee0717 @AtomsH4 感谢两轮 review。本轮 2 条 [A5] 已修复并推送(各一个 commit,均附单测):
关于首轮的 [C7] 两条 a11y warning:经核对已在早前提交处理,故本轮 review 未再涉及——
测试: |
There was a problem hiding this comment.
This review was translated automatically.
LGTM. The previous round of xlsx floating image anchor blocker has been fixed to reuse bounded axisOffsetPx, and added hostile image anchor regression test; worker supersede cleanup also changed to terminate the current worker, to avoid old parsing continuing to occupy CPU or affect new requests through id-less onerror. render-test passed; general-test currently fails on unrelated file watcher timeout (src/main/services/file/tests/watcher.test.ts), which does not belong to this PR's scope of changes.
Original Content
LGTM。上一轮的 xlsx floating image anchor blocker 已修复为复用 bounded axisOffsetPx,并补了 hostile image anchor 回归测试;worker supersede cleanup 也改为终止当前 worker,避免旧解析继续占用 CPU 或通过 id-less onerror 影响新请求。render-test 通过;general-test 当前失败在 unrelated file watcher timeout(src/main/services/file/tests/watcher.test.ts),不属于本 PR 改动范围。
Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
There was a problem hiding this comment.
This review was translated automatically.
LGTM. The old issues with xlsx floating image anchor and worker supersede cleanup have been fixed and have regression coverage; the latest commit only stabilizes the file watcher test wait order, and the current head's basic/general/render and other key CI all passed.
Original Content
LGTM。xlsx floating image anchor 与 worker supersede cleanup 的旧问题已修复并有回归覆盖;最新提交仅稳定 file watcher 测试等待顺序,当前 head 的 basic/general/render 等关键 CI 均通过。
This reverts commit ae89cbe. Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
…ender-discussion Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
There was a problem hiding this comment.
This review was translated automatically.
Reviewed current head 730890428ae08bcd12d0a7bbae7de86fe9f39390 for the renewed review request. The earlier ZIP preflight, unbounded range/anchor, worker supersede, merged-cell, and zh-TW placeholder issues appear fixed, and GitHub CI is green. I still found current-head issues:
Findings:
- Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/parseWorkbook.ts:540/:601still store date cells into the formula raw value table as 1900-system serials even whenworkbook.properties.date1904is true, whilenumberFormat.ts:36ignoresdate1904. In 1904 workbooks, Excel formula semantics use 1904 serials, so formulas/comparisons like=A1,=A1=44575, or downstream chart values derived from formula output are off by 1462 serial units. Date-formatted=A1+1can look correct only because the wrong 1900 serial is later formatted with a 1900 formatter. Please evaluate formulas with the workbook date system and convert only at display time, with a 1904 regression test. - Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/charts/buildChartOption.ts:17normalizespercentStackedby the signed sum of all series. All-negative categories become positive percentages, and mixed positive/negative categories can exceed 100 then clip against the fixed axis max. Please preserve sign and normalize positive/negative stacks separately, with an axis min when negative percentages exist. - Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/parseWorkbook.ts:197strips outer quotes from chart reference sheet names but does not unescape doubled apostrophes. A valid reference like'Bob''s Data'!$A$1looks upBob''s Data!1:1instead ofBob's Data!1:1, so no-cache chart fallback loses data for sheets with apostrophes. - Warning:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/XlsxGrid.tsx:620keys floating images only byimageId, butparseWorkbookintentionally reuses one render image id for multiple anchors of the same workbook image. Repeated placements of the same image produce duplicate React keys and can reconcile the wrong DOM node; include a per-anchor identifier such as rect coordinates or add a unique floating-object id. - Warning:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/XlsxPreviewPanel.tsx:146rendersstate.messagedirectly in the user-facing error state. The worker/parse path returns hardcoded English/internal messages from ZIP preflight, parse, and file-read failures, bypassing the repo i18n rule in non-English locales. Please map parse failures to i18n keys or a generic translated preview error and log the technical detail.
CI: GitHub checks are green for this head (changes, catalog-hand-edit-check, changeset-check, basic-checks, general-test, render-test, and translate passed; notify skipped). git diff --check passed. Local pnpm lint/test/format was not run per repository review rules.
After fixing, please request eeee0717 for further review.
Original Content
Reviewed current head 730890428ae08bcd12d0a7bbae7de86fe9f39390 for the renewed review request. The earlier ZIP preflight, unbounded range/anchor, worker supersede, merged-cell, and zh-TW placeholder issues appear fixed, and GitHub CI is green. I still found current-head issues:
Findings:
- Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/parseWorkbook.ts:540/:601still store date cells into the formula raw value table as 1900-system serials even whenworkbook.properties.date1904is true, whilenumberFormat.ts:36ignoresdate1904. In 1904 workbooks, Excel formula semantics use 1904 serials, so formulas/comparisons like=A1,=A1=44575, or downstream chart values derived from formula output are off by 1462 serial units. Date-formatted=A1+1can look correct only because the wrong 1900 serial is later formatted with a 1900 formatter. Please evaluate formulas with the workbook date system and convert only at display time, with a 1904 regression test. - Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/charts/buildChartOption.ts:17normalizespercentStackedby the signed sum of all series. All-negative categories become positive percentages, and mixed positive/negative categories can exceed 100 then clip against the fixed axis max. Please preserve sign and normalize positive/negative stacks separately, with an axis min when negative percentages exist. - Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/parseWorkbook.ts:197strips outer quotes from chart reference sheet names but does not unescape doubled apostrophes. A valid reference like'Bob''s Data'!$A$1looks upBob''s Data!1:1instead ofBob's Data!1:1, so no-cache chart fallback loses data for sheets with apostrophes. - Warning:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/XlsxGrid.tsx:620keys floating images only byimageId, butparseWorkbookintentionally reuses one render image id for multiple anchors of the same workbook image. Repeated placements of the same image produce duplicate React keys and can reconcile the wrong DOM node; include a per-anchor identifier such as rect coordinates or add a unique floating-object id. - Warning:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/XlsxPreviewPanel.tsx:146rendersstate.messagedirectly in the user-facing error state. The worker/parse path returns hardcoded English/internal messages from ZIP preflight, parse, and file-read failures, bypassing the repo i18n rule in non-English locales. Please map parse failures to i18n keys or a generic translated preview error and log the technical detail.
CI: GitHub checks are green for this head (changes, catalog-hand-edit-check, changeset-check, basic-checks, general-test, render-test, and translate passed; notify skipped). git diff --check passed. Local pnpm lint/test/format was not run per repository review rules.
修复后请 eeee0717 再进行后续 review。
… error i18n - Unescape doubled apostrophes in quoted chart-reference sheet names so `'Bob''s Data'!$A$1` resolves to the `Bob's Data` sheet in the no-cache chart fallback. - Disambiguate floating-image React keys by anchor index; one workbook image is intentionally reused across anchors, so keying by imageId alone collided. - Show a translated generic description in the xlsx error state instead of the raw worker message, and log the technical detail at the failure site. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Thanks for the review. The following changes are made for the current head (commit Fixed (all with regression tests):
Not handled for now (with reasons):
If needed, I can add a comment for #1/#2 each noting the known limitation. After the fix, please @eeee0717 for follow-up review, thanks. Original Content感谢 review。本次针对当前 head 处理如下(commit 已修复(均附回归测试):
暂不处理(附理由):
|
eeee0717
left a comment
There was a problem hiding this comment.
Reviewed the renewed request for current head 7308904. This is the same head I reviewed in my latest changes-requested review, so I am not duplicating the same findings here.
The prior current-head findings still need either fixes or an explicit maintainer decision before I can approve. Core GitHub CI checks for this head are green (changes, catalog-hand-edit-check, changeset-check, basic-checks, general-test, render-test; notify skipped). Local pnpm lint/test/format was not run per repository review rules.
There was a problem hiding this comment.
This review was translated automatically.
Reviewed current head cbe661cbfee4a34cb77476fc7ead172c79c33bcd for the renewed request. The prior ZIP/range/anchor, sheet-name apostrophe, duplicate floating-image key, raw error i18n, zh-TW placeholder, worker supersede, and merged-cell accessibility issues appear fixed. I still found current-head issues:
Findings:
- Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/parseWorkbook.ts:543/:605still store formula-context date values as 1900-system serials even whenworkbook.properties.date1904is true, andnumberFormat.ts:36discards thedate1904flag. Date-formatted=A1+1can look correct because it is later formatted through the same 1900 convention, but uncached formula numeric results, comparisons to workbook serial literals, and chart fallback values can be off by 1462 days in 1904 workbooks. Please evaluate formulas in the workbook date system and convert only at display time, with a 1904 uncached-formula regression test. - Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/charts/buildChartOption.ts:17still normalizespercentStackedby a single signed category total. All-negative categories become positive percentages, and mixed positive/negative categories can exceed the fixedmax: 100axis and clip. Please preserve sign by normalizing positive and negative stacks separately and set an axis minimum when negative percentages exist, or explicitly downgrade unsupported negative percent-stacked charts with coverage. - Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/chartXmlParser.ts:516/:518parse every untrusted chart anchor with no count cap, andXlsxGrid.tsx:636renders every parsed chart. A small drawing XML can repeat thousands of anchors pointing at the same chart relationship; ZIP size/range/coordinate caps still pass, but the worker repeatedly reads/parses the chart part and the renderer can create thousands of chart hosts/ECharts instances. Please add a per-sheet/workbook floating-object cap, memo parsed chart parts by path, and skip excess objects with a warning. The same object cap should cover floating images fromparseWorkbook.ts:670andXlsxGrid.tsx:615.
CI: GitHub checks are green for this head (changes, catalog-hand-edit-check, changeset-check, basic-checks, general-test, and render-test passed; notify skipped). Local pnpm lint/test/format was not run per repository review rules.
After fixing, please request eeee0717 for further review.
Original Content
Reviewed current head cbe661cbfee4a34cb77476fc7ead172c79c33bcd for the renewed request. The prior ZIP/range/anchor, sheet-name apostrophe, duplicate floating-image key, raw error i18n, zh-TW placeholder, worker supersede, and merged-cell accessibility issues appear fixed. I still found current-head issues:
Findings:
- Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/parseWorkbook.ts:543/:605still store formula-context date values as 1900-system serials even whenworkbook.properties.date1904is true, andnumberFormat.ts:36discards thedate1904flag. Date-formatted=A1+1can look correct because it is later formatted through the same 1900 convention, but uncached formula numeric results, comparisons to workbook serial literals, and chart fallback values can be off by 1462 days in 1904 workbooks. Please evaluate formulas in the workbook date system and convert only at display time, with a 1904 uncached-formula regression test. - Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/charts/buildChartOption.ts:17still normalizespercentStackedby a single signed category total. All-negative categories become positive percentages, and mixed positive/negative categories can exceed the fixedmax: 100axis and clip. Please preserve sign by normalizing positive and negative stacks separately and set an axis minimum when negative percentages exist, or explicitly downgrade unsupported negative percent-stacked charts with coverage. - Important:
src/renderer/components/ArtifactPreview/office/XlsxPreviewPanel/worker/chartXmlParser.ts:516/:518parse every untrusted chart anchor with no count cap, andXlsxGrid.tsx:636renders every parsed chart. A small drawing XML can repeat thousands of anchors pointing at the same chart relationship; ZIP size/range/coordinate caps still pass, but the worker repeatedly reads/parses the chart part and the renderer can create thousands of chart hosts/ECharts instances. Please add a per-sheet/workbook floating-object cap, memo parsed chart parts by path, and skip excess objects with a warning. The same object cap should cover floating images fromparseWorkbook.ts:670andXlsxGrid.tsx:615.
CI: GitHub checks are green for this head (changes, catalog-hand-edit-check, changeset-check, basic-checks, general-test, and render-test passed; notify skipped). Local pnpm lint/test/format was not run per repository review rules.
修复后请 request eeee0717 再进行后续 review。
What this PR does
Before this PR:
.xlsxartifacts had no inline preview —OfficePreviewPanelonly supporteddocx/pptx, and spreadsheets fell through to the unsupported-format state.After this PR:
The artifact preview pane renders
.xlsxworkbooks read-only:sheetFormatPr), floating images, and charts.fast-formula-parser(5s budget, memoized, cycle-safe); unevaluable ones show the formula source greyed out. Number formats are applied to displayed values.@tanstack/react-virtual) with sticky row/column headers, an independent merge layer, and a floating layer for images and ECharts-rendered charts (bar/line/pie/area; placeholder for unsupported types). The grid pads blank rows/cols beyond the used range so the viewport is always filled, Excel-style.Fixes: N/A
Why we need it and why it was done in this way
Spreadsheets are a common artifact output (LLM-generated reports), and users previously had to open them externally. A custom worker parser + virtualized DOM grid keeps the main thread responsive, gives full control over fidelity, and avoids shipping a heavyweight embedded spreadsheet app for a read-only preview.
The following tradeoffs were made:
The following alternatives were considered:
Links to places where the discussion took place: internal review session (Conductor).
Breaking changes
None.
Special notes for your reviewer
The worker→UI contract lives in
renderModel.ts; all types are structured-cloneable, image binaries transfer asArrayBuffer.ExcelJS quirks worked around (each documented at the call site): default-namespace
wsDrdrawings,worksheet.model.rowsdropping cell-less row definitions (read from the load-path Row objects instead),sheetFormatPrdefaults surfaced viaworksheet.properties.Coordinate systems: virtualizers work in scroll space (sizes × zoom); rendering, merge visibility, and the selection overlay work in zoom=1 content space inside the transform layer.
Tests: 218 cases across parser (values/styles/merges/sizing/formulas/number formats/theme colors/charts), layout pure functions, grid rendering (virtualization, merges, zoom, overlay, floating layer), and panel state machine.
New dependencies — this feature adds
echarts,exceljs,fast-formula-parser,jszip,numfmt, declared underdevDependencies(renderer libraries bundled by Vite, matching howreact/mermaid/katexare declared here — notdependencies, which is for unbundled main-process runtime deps). Net bundle cost is smaller than the count suggests, and each covers a distinct gap ExcelJS leaves:jszipis already in the tree (ExcelJS, docx, docx-preview, epub,@aiden0z/pptx-rendererall depend on it —pnpm why jszip→ a single3.10.1). Declaring it directly just avoids relying on a transitive dep; it adds zero bundle weight. Used to read raw chart XML (ExcelJS parses no charts) and to rewrite openpyxl default-namespace drawing parts before ExcelJS crashes on them.echartsdedupes with@aiden0z/pptx-renderer's copy — both range on^6, resolve to a single6.1.0, and import the same modularecharts/*subpaths, so one copy lands in the bundle. Draws the parsed charts.exceljs(structure + full cell styles),fast-formula-parser(evaluates uncached formulas — ExcelJS only exposes cached results), andnumfmt(renders values against Excel number-format codes — ExcelJS returns the code string, not the display text) each cover a capability ExcelJS does not provide and have no in-tree substitute.numfmtvs SheetJSSSF(investigated, keptnumfmt) — I checked whether@e965/xlsx(already a dependency, used byexportExcel.ts) could replacenumfmtvia itsSSFmodule. It is a functional drop-in: number/percent/date/date-time/boolean, error-string passthrough, and invalid-format fallback all match, verified againstparser.numberFormat.test.ts. Rejected on bundle size — the parser runs in an isolated Web Worker bundle that cannot share the SheetJS the main renderer already pulls in viaexportExcel.ts, andSSFis only reachable through the full, non-tree-shakeable SheetJS build (~952 KB min); the standalonexlsx.zahlbuild exports a string payload, not a callable API. That is ~5×numfmt's tree-shakeable ESM (~196 KB source). Keepingnumfmtis the leaner choice for the shipped worker bundle.Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
mainfor active development,v1for v1 maintenance fixes/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note