feat: preview spreadsheets and slide decks#55
Conversation
|
Exact-head review request for
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Correction: bind review to the actual exact head @clawsweeper re-review |
34039c4 to
999784f
Compare
|
Exact-head review request for Maintainer-pattern hardening added after reviewing the landing work on #49, #50, and #51:
The shared autoreview helper completed the earlier source-review cycles and drove eight fixes; its final branch-mode closeout now fails closed because this PR intentionally contains binary proof images. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex review: needs maintainer review before merge. Reviewed July 12, 2026, 3:41 PM ET / 19:41 UTC. Summary Reproducibility: not applicable. This PR adds a new preview capability rather than fixing a defined current-main bug, and the only exact-head CI failure is an unrelated direct-message menu timeout. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Keep the strict text-only parser, hard resource limits, and authenticated fallback, then merge if maintainers accept synchronous client parsing; otherwise move parsing into a terminable worker rather than broadening server-side conversion or full Office rendering. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR adds a new preview capability rather than fixing a defined current-main bug, and the only exact-head CI failure is an unrelated direct-message menu timeout. Is this the best way to solve the issue? Yes, if maintainers accept the parser boundary: bounded text-only extraction with authenticated fallback is narrower and more maintainable than server conversion or full Office rendering, while worker isolation is the safer alternative for stronger availability containment. AGENTS.md: found, but no applicable review policy affected this item. Codex review notes: model internal, reasoning high; reviewed against 0090ef0791a5. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (6 earlier review cycles)
|
clawsweeper
Bot
added
proof: sufficient
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 999784ff16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| const cells: SpreadsheetCell[] = []; | ||
| let truncated = false; | ||
| for (const cell of Array.from(worksheet.getElementsByTagNameNS("*", "c"))) { |
There was a problem hiding this comment.
When an uploaded workbook has a worksheet with hundreds of thousands of tiny <c/> elements that still fit under the 4 MiB entry limit, this Array.from(...) materializes every cell before the remainingCells guard can break. That bypasses the intended 10,000-cell safety budget during parsing and can allocate/iterate enough nodes to hang the artifact viewer; iterate with an early break and/or reject excessive cell tags before creating an array.
Useful? React with 👍 / 👎.
| characters += text.length; | ||
| if (characters > PRESENTATION_TEXT_LIMIT) | ||
| throw new Error("A slide contains too much text to preview safely."); | ||
| paragraphs.push(text); |
There was a problem hiding this comment.
When a PPTX slide contains many one-character paragraphs, characters stays below the 64 KiB text limit while paragraphs can grow to tens of thousands of entries, and the Svelte viewer renders one DOM node per entry. This gives crafted decks a way to freeze the preview despite the text-size cap; add a paragraph/run count limit or coalesce text before pushing.
Useful? React with 👍 / 👎.
| const sharedStrings: string[] = []; | ||
| if (entries.has("xl/sharedStrings.xml")) { | ||
| const shared = xml(await entryText(bytes, entries, "xl/sharedStrings.xml", signal, budget)); | ||
| for (const item of Array.from(shared.getElementsByTagNameNS("*", "si"))) { |
There was a problem hiding this comment.
When an XLSX contains a large sharedStrings.xml with many tiny <si> entries, this loop materializes every shared string before the 10,000-cell cap is applied, even if only a few cells reference them. A crafted workbook can fit this under the 4 MiB entry limit while allocating hundreds of thousands of strings/nodes on the main thread; add an entry/character budget or resolve only the referenced indices.
Useful? React with 👍 / 👎.
| truncated = true; | ||
| break; | ||
| } | ||
| const reference = cell.getAttribute("r") || ""; |
There was a problem hiding this comment.
When an XLSX producer relies on row/cell order and omits the optional cell r attribute, every parsed value gets stored with an empty reference here, so spreadsheetGrid never maps those values to A1/B1/etc. Those valid sheets render as blank despite having cell data; derive the coordinate from the containing row and cell index when r is absent.
Useful? React with 👍 / 👎.
| if (kind === "spreadsheet") spreadsheet = await parseSpreadsheet(bytes, signal); | ||
| else presentation = await parsePresentation(bytes, signal); |
There was a problem hiding this comment.
When a user opens another artifact while a spreadsheet or deck is still in synchronous XML parsing, the abort signal may not be observed until after parsing returns, but these assignments still write the old preview data. If the newer artifact has already reached ready, the stale parse can overwrite it; store the parse result in a local, recheck signal.aborted/upload identity, and only then assign.
Useful? React with 👍 / 👎.
| const relationships = relationshipMap( | ||
| xml(await entryText(bytes, entries, "ppt/_rels/presentation.xml.rels", signal, budget)), | ||
| ); | ||
| const slideIDs = Array.from(presentation.getElementsByTagNameNS("*", "sldId")); |
There was a problem hiding this comment.
When presentation.xml contains a very large number of <sldId> elements, this Array.from(...) builds the full list before applying the 200-slide limit on the next line. A small crafted deck can therefore spend substantial time and memory enumerating slide IDs even though only 200 can be previewed; count/collect only up to the cap and mark the rest as truncated.
Useful? React with 👍 / 👎.
| } | ||
| const sheets: SpreadsheetSheet[] = []; | ||
| let remainingCells = SPREADSHEET_CELL_LIMIT; | ||
| const sheetDescriptors = Array.from(workbook.getElementsByTagNameNS("*", "sheet")); |
There was a problem hiding this comment.
When workbook.xml contains far more than 100 <sheet> entries, this line materializes every descriptor before the sheet-count guard rejects the workbook. That means the new worksheet cap still allows a crafted workbook to allocate and iterate a huge sheet list on the main thread; collect only up to the limit plus one or pre-scan the XML before building the array.
Useful? React with 👍 / 👎.
3941862 to
371d47e
Compare
|
@clawsweeper re-review PR 51 is merged, and this branch is now rebased onto latest |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
removed
status: 👀 ready for maintainer look
|
@clawsweeper review |
clawsweeper
Bot
added
proof: sufficient
371d47e to
b38580f
Compare
clawsweeper
Bot
added
rating: 🐚 platinum hermit
b38580f to
e8c383f
Compare
|
Merged using rebase. What changed after review:
Verification:
Landed on
Thanks @jjjhenriksen! |
2 participants
Summary
This builds directly on the multi-class artifact viewer introduced in #49. The exact head is rebased on current remote
mainat0090ef0791a56345496c6acf96d51e3e924bac39, including the merged work from #50 and #51.Safety boundaries
End-to-end behavior proof
The proof fixture is uploaded through ClickClack's real upload and attachment APIs, opened from the rendered message card, and exercised in the running Go-backed web app.
Spreadsheet preview
The browser opened
forecast.xlsx, rendered theForecastsheet as a scrollable grid, exposed both worksheet tabs, and switched toAssumptionswithout leaving the conversation.The same browser scenario also proves package-root relationship resolution, workbook-global cell limits, a bounded sparse 10,000-cell DOM, and successful text-only rendering when an unused slide media object exceeds the extraction limit.
Safety fallback
A crafted workbook with 101 worksheet descriptors is rejected before repeated worksheet extraction. The authenticated original remains available.
Slide deck preview
The browser opened
launch.pptx, rendered slide text in the artifact pane, exposed slide count and navigation, advanced to the second slide, and retained the authenticated download action.Verification
pnpm check— passed: Go, FakeCo AWS, desktop, TypeScript/Svelte typechecks, lint, formatting, production builds, and embedded web assetspnpm coverage— passed, 85.8% total Go coveragegit diff --checkpassedCAPTURE_OFFICE_PROOF=1 CLICKCLACK_E2E_PORT=18086 node_modules/.bin/playwright test tests/e2e/artifact-viewer.spec.ts— 12 passedReview closeout
Peter's structured autoreview process was run repeatedly before commit. Eight accepted findings were fixed: streamed decompression bounding, workbook-global cell accounting, stored-entry size validation, visible grid clipping disclosure, worksheet-count bounding, extracted-only expansion accounting, rectangular DOM-grid bounding, and package-absolute relationship resolution. The final exact-source rerun is currently blocked by a local Codex CLI structured-input truncation regression; no finding was emitted by that failed rerun. ClawSweeper found no actionable issues on the prior head. Seven subsequent ChatGPT Codex findings plus ClawSweeper’s pre-DOM structural-bound finding are fixed on exact head
b38580f: bounded worksheet, cell, shared-string, slide, and paragraph iteration; shared-text and paragraph limits; derived coordinates for omitted cell references; and abort-safe preview assignment.No database, SQL, OpenAPI, SDK, upload-storage, or public transport contract changes are included.