add xlsx export suppport to workspace-sheets blueprint - #433
add xlsx export suppport to workspace-sheets blueprint#433maxwellpeterson wants to merge 3 commits into
Conversation
Preview:
|
|
Findings
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
26389f9 to
8b78cde
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| const text = formula.slice(offset, end); | ||
| const name = nameParts.join(""); | ||
| const malformed = offset > 0 && /[A-Za-z0-9_.$]/.test(formula[offset - 1]); | ||
| const external = formula[offset - 1] === "]" || /\[[^\]]*\]/.test(name); |
There was a problem hiding this comment.
🟡 Bracketed sheet references break
For a local sheet named Q[1], external misclassifies its quoted reference and skips the required rename. The exported formula points to a nonexistent sheet.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@maxwellpeterson Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| const row = Math.min(cellRow, heightRow); | ||
| const height = heightRow === row ? sheet.rowHeights[heightIndex++] : null; | ||
| yield `<row r="${row}"${height ? ` ht="${height.value}" customHeight="1"` : ""}>`; | ||
| while (sheet.cells[cellIndex]?.row === row) yield cellXml(sheet.cells[cellIndex++], formulaNames); |
There was a problem hiding this comment.
Large valid workbooks can time out before delivery. sanitizeCellMap() accepts roughly 200,000 cells and the export deadline is 30 seconds, but this yields a separate stream chunk for every cell (plus every row). Consuming a 200,000-cell, four-column workbook here took about 42 seconds even though the XLSX was under 1 MB, so the platform rejects it. Please batch XML fragments into reasonably sized chunks before feeding CompressionStream.
| function parsedCellValue(value, fmt, formulaNames) { | ||
| if (value[0] === "'") return {type: "text", value: value.slice(1)}; | ||
| if (value[0] === "=") return {type: "formula", value: rewriteFormula(value.slice(1), formulaNames)}; | ||
| if (fmt?.nf === "text") return {type: "text", value}; |
There was a problem hiding this comment.
nf: "text" is only display formatting in the app: literalValue() still parses 42 and TRUE as number/boolean values, so formulas see those types. This branch instead writes them as inline strings in XLSX; for example, ISNUMBER(A1) changes from true in Workspace Sheets to false after export when A1 contains 42 with text formatting. Preserve the parsed underlying literal type and use number format 49 only for display.
| const name = formula.slice(offset, end); | ||
| const normalized = names.get(name.toLowerCase()); | ||
| if (!normalized) return null; | ||
| return {end: end + 1, text: `'${normalized.replace(/'/g, "''")}'!`}; |
There was a problem hiding this comment.
Every recognized unquoted reference gains two quote characters even when its sheet name is already legal and unchanged. Stored cell values may be 8,192 characters (including =), while Excel limits formula contents to 8,192 characters, so an otherwise valid near-limit formula can exceed that limit after just one rewrite and be repaired or dropped on open. Preserve legal unquoted references when no rename is needed, or enforce the post-rewrite limit explicitly.
|
Submitted one review with 3 actionable inline findings. |
| if (cell.value === "") return `<c r="${cell.reference}"${style}/>`; | ||
| const parsed = parsedCellValue(cell.value, formulaNames); | ||
| if (parsed.type === "blank") return `<c r="${cell.reference}"${style}/>`; | ||
| if (parsed.type === "formula") return `<c r="${cell.reference}"${style}><f>${formulaXml(parsed.value)}</f></c>`; |
There was a problem hiding this comment.
Workspace Sheets supports IFS, IFNA, XOR, SWITCH, CONCAT, TEXTJOIN, UNICHAR, UNICODE, and DAYS, but OOXML defines these as future functions whose stored names require the _xlfn. prefix. This writes, for example, CONCAT(...) with no cached value, so Excel recalculates it as #NAME? instead of the result. Rewrite these supported function tokens to _xlfn.<name> when exporting.
|
|
||
| async getDocument() { | ||
| return this.assembleDocument(await this.loadMeta()); | ||
| return this.enqueueMutation(async () => |
There was a problem hiding this comment.
getDocument() now joins mutationQueue, but applyOperationLocked() awaits subscriber callbacks before releasing that queue. An async callback that re-reads the document therefore deadlocks: its read waits behind the mutation while the mutation waits for the callback. Custom gadget clients can validly do this through subscribe(). Move the broadcast outside the queued critical section, or provide a snapshot method used only by export.
| for (const sheet of sheets) { | ||
| const base = safeSheetName(sheet.sourceName); | ||
| let name = base; | ||
| for (let suffix = 2; used.has(name.toLowerCase()); ++suffix) { |
There was a problem hiding this comment.
Accepted structure state can repeat sheet IDs or names without a count limit. Each duplicate restarts this suffix search at 2, so N duplicates perform O(N²) Set lookups before the stream is returned; 15,000 repeated IDs took about 15 seconds locally and about 20,000 exhausts the 30-second export budget. Deduplicate or cap exported sheets, or track the next suffix per base name.
|
Submitted one review with 3 actionable inline findings. |
1 participant
update built-in sheets blueprint to support xlsx export