Skip to content

add xlsx export suppport to workspace-sheets blueprint - #433

Open
maxwellpeterson wants to merge 9 commits into
mainfrom
mpeterson/xlsx-export
Open

add xlsx export suppport to workspace-sheets blueprint#433
maxwellpeterson wants to merge 9 commits into
mainfrom
mpeterson/xlsx-export

Conversation

@maxwellpeterson

@maxwellpeterson maxwellpeterson commented Sep 3, 2026

Copy link
Copy Markdown
Member

update built-in sheets blueprint to support xlsx export

@github-actions github-actions Bot added the kernel Changes to the Workshop kernel label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

Preview: pr433-mpeterson-xlsx-export

https://pr433-mpeterson-xlsx-export-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

@ask-bonk

ask-bonk Bot commented Sep 3, 2026

Copy link
Copy Markdown

Findings

  • Medium xlsx.js:361: Cells outside declared rows/cols are silently omitted, although the server accepts and persists them and CSV exports them. Export all valid Excel references or reject/prune them when stored.
  • Medium xlsx.js:463: nf: "text" converts formulas to literal strings. The app evaluates formulas independently of number formatting, so XLSX export changes workbook behavior.
  • Medium xlsx.js:385: Each unmatched apostrophe rescans the remaining formula, producing quadratic work. Valid 8,192-character cells can exceed the 30-second export deadline with only a few hundred formulas. Parse quoted references in one pass.

github run

@maxwellpeterson
maxwellpeterson marked this pull request as ready for review September 3, 2026 03:11
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@ask-bonk

ask-bonk Bot commented Sep 3, 2026

Copy link
Copy Markdown

@maxwellpeterson Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

ask-bonk[bot]

This comment was marked as resolved.

@ask-bonk

ask-bonk Bot commented Sep 3, 2026

Copy link
Copy Markdown

Submitted one review with 3 actionable inline findings.

github run

ask-bonk[bot]

This comment was marked as resolved.

@ask-bonk

ask-bonk Bot commented Sep 3, 2026

Copy link
Copy Markdown

Submitted one review with 3 actionable inline findings.

github run

devin-ai-integration[bot]

This comment was marked as resolved.

ask-bonk[bot]

This comment was marked as resolved.

ask-bonk[bot]

This comment was marked as resolved.

@ask-bonk

ask-bonk Bot commented Sep 3, 2026

Copy link
Copy Markdown

Submitted one review with 2 actionable inline findings. Focused XLSX tests pass (24/24).

github run

ask-bonk[bot]

This comment was marked as resolved.

@ask-bonk

ask-bonk Bot commented Sep 3, 2026

Copy link
Copy Markdown

Submitted one review with 3 actionable inline findings.

github run

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>`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

client.js discards whitespace while tokenizing, so =SUM (1,2) is supported and evaluates to 3. This emits it verbatim as <f>SUM (1,2)</f>, but OOXML function-call syntax requires SUM to be immediately followed by its parameter list (a space is the reference-intersection operator), making the exported formula invalid when Excel recalculates it. Normalize function-call whitespace before emission, or export formulas with this app-only syntax as text.

@ask-bonk

ask-bonk Bot commented Sep 3, 2026

Copy link
Copy Markdown

Submitted 1 inline finding. Additional findings:

  • High server.js:102: Delayed broadcasts can overwrite a sender’s newer local state, while its own corrective event is ignored.
  • Medium xlsx.js:549: =2^3^2 changes from 512 locally to 64 in Excel due to associativity differences.
  • Medium xlsx.js:252: Font sizes are stored/rendered as pixels but exported as points, making custom fonts about 33% larger.

github run

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

devin-ai-integration[bot]

This comment was marked as resolved.

@ask-bonk

ask-bonk Bot commented Sep 4, 2026

Copy link
Copy Markdown

Findings

  • Medium xlsx.js:531: Malformed formulas accepted by the grid, such as =SUM(1,2 or ="abc, are emitted as invalid Excel formulas. Validate them or export as text.
  • Medium xlsx.js:531: Out-of-grid references remain formulas despite their cells being omitted. For example, =XFE1+1 evaluates locally but becomes #NAME? in Excel.

Focused tests pass (23/23).

github run

@ask-bonk

ask-bonk Bot commented Sep 4, 2026

Copy link
Copy Markdown

Findings

  • Medium xlsx.js:456: Names requiring Excel delimiters, such as R1C1 or $Data, remain unquoted, breaking cross-sheet formulas. Quote unchanged names when required.
  • Medium xlsx.js:311: Date serial 1 displays 12/31/1899 locally but 01/01/1900 in Excel due to incompatible epochs.
  • Medium xlsx.js:303: Integer formatting changes -1.5 from -1 locally to -2 in Excel because rounding semantics differ.

github run

@devin-ai-integration devin-ai-integration Bot 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.

Devin Review found 1 new potential issue.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

Comment on lines +217 to +218
return this.enqueueMutation(async () => {
const document = await this.assembleDocument(await this.loadMeta());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Failed subscriptions leak callback capabilities

When snapshot assembly fails, subscribe abandons the duplicated callback before registration. The caller gets an error, but its server capability remains allocated.

Prompt for agents
In packages/workshop-backend/format-blueprints/workspace-sheets/files/server.js, Gadget.subscribe duplicates the callback before enqueueing snapshot assembly. If loadMeta or assembleDocument rejects, the duplicate was never added to subscribers, so dropSubscriber cannot release it. Ensure every failure before successful registration disposes dup exactly once, while preserving the queued snapshot-and-registration ordering and the existing onRpcBroken cleanup.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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

Labels

kernel Changes to the Workshop kernel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant