Skip to content

fix(frontend): prevent large spreadsheet preview crashes - #3592

Open
dvd233 wants to merge 4 commits into
Tencent:mainfrom
dvd233:contrib/issue-3586-excel-preview
Open

dvd233 wants to merge 4 commits into
Tencent:mainfrom
dvd233:contrib/issue-3586-excel-preview

Conversation

@dvd233

@dvd233 dvd233 commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Guard client-side Excel previews before loading SheetJS when the source file exceeds 8 MiB.
  • Bound SheetJS parsing to 5,001 rows and reject truncated/oversized worksheets before generating a full HTML table.
  • Keep the document detail view usable with a localized safe-preview notice so the existing download action and metadata remain available.
  • Add regression coverage for file, row, cell, and real SheetJS parsing limits.

Fixes #3586

Why

renderExcel() previously parsed every worksheet and passed every row to sheet_to_html(). A 9.28 MiB workbook reported in #3586 could block the browser main thread and crash the renderer before the document details were usable.

Validation

On synchronization commit 1237f1654584821f829c20fcdb9eb9fcb200eaae:

  • 26 targeted tests passed: real SheetJS row-limit regression, fullscreen/focus regressions and file-preview utilities.
  • npm run check-i18n: 14 passed, including the two notice keys in the newly added Traditional Chinese locale.
  • npm run type-check and npm run build passed; existing chunk-size warnings remain.
  • The contribution diff against merged main 2fbfb447 passes git diff --check; the signed merge preserves the original PR head as its first parent.
  • Main subsequently advanced to f5cfa0ff. A file-level three-way comparison of all 11 contribution paths found no conflicts; package.json and package-lock.json blobs are unchanged. The Excel cap, focus/fullscreen/diagram functions and Vue template retain the verified behavior. This comparison is not a new full-main test run.
  • The full frontend suite, deployed-browser/real large workbook checks and Go/DocReader checks were not rerun for this merge. The earlier 1,174-pass / 1-skip / 5-failure full-suite result belongs to the earlier head and is not presented as current-commit success.

CI for the new head is reported separately on this PR.

AI-assisted follow-up implementation, validation and summary (GPT-6).

Copilot AI lite review requested due to automatic review settings September 22, 2026 21:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@vicfei

vicfei commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Reporter of #3586 here — confirming the mechanism against the original environment, and reporting where I tried and failed to break the guard.

Real-world confirmation (v0.8.0 docker, Chromium-based browser on UOS): the freezing document was a 9.28 MB xlsx that parsed into 24,283 chunks. Opening the detail drawer rendered the metadata header (file type, chunk count) but the content area stayed blank, and the "Page unresponsive" dialog appeared within seconds; after a reload, the next open killed the renderer outright (Aw, Snap — SIGTRAP). Notably the behavior was identical while the document was still parsing and after it completed — consistent with the preview path fetching the raw source bytes regardless of parse status, which is exactly why the guard belongs in the preview path and before SheetJS loads. The proposed notice-plus-download fallback matches the acceptance criterion from the report (metadata and chunk view must stay reachable).

Attempted holes in the guard, for the record:

  • Wide sheets (e.g. !ref spanning 16k columns): rows are capped at the 5001-row sentinel and the rows × columns product is capped at 50k cells computed from decode_range, so sheet_to_html never sees more than 50k cells regardless of range claims. Covered.
  • Full-range refs (A1:XFD1048576 written by some exporters): whether !ref reflects the truncated range or the full range, rows ends up > 5000 → rejected. Covered either way.
  • Many small sheets: the cell budget accumulates across sheets. Covered.
  • Residual main-thread cost: a dense-but-under-8MiB workbook can still materialize on the order of ~1M sparse cells inside XLSX.read before the metrics rejection — bounded by the byte guard, sub-second by my read, and far from the original freeze. Acceptable in my view; no change requested.

The sheetRows = MAX + 1 sentinel is a nice touch — a workbook that SheetJS truncated can't be rendered as if it were complete.

From the reporter's side this fully addresses #3586 as scoped; the freeze-then-SIGTRAP chain requires exactly the unbounded XLSX.read → sheet_to_html → v-html sequence this PR bounds.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants