Fix slide import fidelity, editor glitches, and stuck run spinner - #2765
Fix slide import fidelity, editor glitches, and stuck run spinner#2765NKoech123 wants to merge 18 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
builder-io-integration
Bot
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Code Review Summary
PR #2765 addresses several real Slides and shared progress issues: non-16:9 import geometry and text sizing, PDF style-boundary spacing, large-deck thumbnail performance, stale active-run polling, editor resize behavior, source-preserving patch guards, and misleading Google Slides export fallback messaging. The implementation is generally well targeted, uses regression coverage for the core fixes, and the overall risk is Standard because it changes shared polling plus multiple import/editor paths.
Key Findings
🟡 MEDIUM
- The extended import timeout is not applied to the common uploaded-source generation path, which still uses the default 60-second client timeout for large PDF/PPTX imports.
- The multiline formatting of import calls breaks the existing source-based generation-flow test; the focused test currently fails three assertions.
The RunsTray polling, fidelity math, PDF spacing guard, and browser-editor source-preservation gating otherwise appear sound. I also verified the generation-flow test failure directly against the current checkout.
🧪 Browser testing: Will run after this review (PR touches UI code)
| const imported = (await callAction( | ||
| "import-file", | ||
| { | ||
| filePath: file.path, | ||
| format: "pdf", | ||
| deckId: deck.id, | ||
| importIntoDeck: true, | ||
| }, | ||
| { timeoutMs: IMPORT_ACTION_TIMEOUT_MS }, |
There was a problem hiding this comment.
🟡 Apply the extended timeout to source-deck imports used before generation
The five-minute timeout is only passed by the direct import/reference-picker calls in this component. The main create-a-deck-from-upload flow calls importUploadedDeckIntoDeck() before generation, and that helper invokes the same import-pptx / import-file actions without timeout options, so large/image-heavy source decks still hit the 60-second default and can be reported as failed while the server continues importing. Pass the shared timeout through that helper (covering both PDF and PPTX) so every source-deck import path gets the intended budget.
Additional Info
Found independently by 2 of 4 code-review agents; confirmed by surrounding call-path inspection.
| const imported = (await callAction("import-google-slides-reference", { | ||
| presentationUrl: selection.url, | ||
| })) as { id?: unknown }; | ||
| const imported = (await callAction( |
There was a problem hiding this comment.
🟡 Keep generation-flow assertions compatible with the reformatted calls
The existing Index.generation-flow.test.ts extracts this function from the source and asserts it contains the literal callAction("import-pptx" and callAction("import-file". Moving the action names onto the next line makes those assertions fail; the focused test currently reports three failures. Update the source-based assertions to tolerate whitespace/newlines or update them to assert the new timeout behavior.
Additional Info
Confirmed by running `pnpm --dir templates/slides exec vitest run app/pages/Index.generation-flow.test.ts` on HEAD.
Visual recap — screenshot failedA recap was published, but the PR-comment screenshot could not be captured or uploaded. Open the interactive recap directly: Open the full interactive recap Diagnostic: light: screenshot captured but image upload failed |
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest commit adds a “Revise with AI” action to the Slides inline formatting menu, including selection preview, instruction capture, a bounded revision prompt, and a new prompt-builder test. The earlier import-timeout and generation-flow-test comments remain unresolved and were intentionally not reposted. This incremental review is Standard risk because the new feature changes editor state, agent-chat delivery, and keyboard-visible controls.
New Findings
🟡 MEDIUM
- The AI revision can be overwritten by the still-dirty inline editor when the user later exits editing.
- The revision prompt does not retain the owning deck ID, so navigation before execution can pair the selected slide with the wrong deck.
- The UI reports successful delivery and clears the instruction without awaiting the confirmation-capable local chat API.
- Desktop keyboard users lose access to Duplicate/Delete thumbnail actions because
display: noneis only reversed by pointer hover.
The prompt construction tests are useful, and the selection snapshot is taken before the AI input receives focus. 🧪 Browser testing: Will run after this review (PR touches UI code)
| sendToAgentChat({ | ||
| message: buildReviseSelectionPrompt({ | ||
| selectedText: aiTargetText, | ||
| instruction, | ||
| slideId, | ||
| }), | ||
| submit: true, | ||
| chatTarget: "local", | ||
| }); | ||
| toast.success(t("raw.sentToAgent"), { description: instruction }); | ||
| setShowAiInput(false); |
There was a problem hiding this comment.
🟡 Commit or exit inline editing before sending a revision
Submitting the AI request only closes the prompt; the selected block remains a dirty contentEditable session. When the user later clicks away, the inline editor can serialize its stale DOM through onUpdateSlide and overwrite the agent's server-side revision. Persist the current inline edit and release the edit state before allowing the remote update to reconcile, and cover the send-then-exit interaction.
Additional Info
Found by 1 of 3 incremental code-review agents; follows the SlideEditor dirty/edit and exit paths.
| sendToAgentChat({ | ||
| message: buildReviseSelectionPrompt({ | ||
| selectedText: aiTargetText, | ||
| instruction, | ||
| slideId, |
There was a problem hiding this comment.
🟡 Retain the owning deck when targeting the revision
The prompt includes slideId but not the owning deck ID, and asks the agent to resolve the target through view-screen. If the user navigates to another deck before the local agent executes, it can combine the captured slide ID with the newly active deck and fail the update. Capture and include the exact deck/slide pair from the editor context.
Additional Info
Found by 1 of 3 incremental code-review agents; confirmed against the current target-capture flow.
| sendToAgentChat({ | ||
| message: buildReviseSelectionPrompt({ | ||
| selectedText: aiTargetText, | ||
| instruction, | ||
| slideId, | ||
| }), | ||
| submit: true, | ||
| chatTarget: "local", | ||
| }); | ||
| toast.success(t("raw.sentToAgent"), { description: instruction }); |
There was a problem hiding this comment.
🟡 Confirm local chat delivery before reporting success
sendToAgentChat is fire-and-forget, but this path immediately shows a success toast and clears the instruction. The local chat API exposes sendToAgentChatAndConfirm specifically for rejected, missing-engine, or timed-out delivery; use it and retain the instruction with an error toast when delivery is not confirmed.
Additional Info
Found by 1 of 3 incremental code-review agents; the confirmation API and failure cases exist in packages/core/src/client/agent-chat.ts.
| {/* Actions - always visible on touch devices */} | ||
| {!readOnly && ( | ||
| <div className="absolute top-2 right-2 flex gap-0.5 sm:opacity-0 sm:group-hover:opacity-100"> | ||
| <div className="absolute top-2 right-2 flex gap-0.5 sm:hidden sm:group-hover:flex"> |
There was a problem hiding this comment.
🟡 Keep thumbnail actions keyboard accessible
At desktop widths, sm:hidden sm:group-hover:flex removes Duplicate and Delete from the tab order until a pointer hover occurs. Keyboard focus does not satisfy group-hover, so desktop keyboard users cannot reach these controls. Add a focus-within visibility rule (or otherwise preserve focusability) while retaining the compositor optimization.
Additional Info
Found by 1 of 3 incremental code-review agents; direct regression from opacity-only hiding to display:none.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest commit adds Delete-key support for images embedded in flow-layout content, with focused interaction tests and a changelog entry. The new helper correctly avoids deleting arbitrary flow containers, and the targeted editor tests pass. The previously reported six issues remain unresolved and were intentionally not reposted. This remains Standard risk because the change mutates persisted slide HTML and selection state.
New Finding
🟡 MEDIUM
- Delete on an imported fidelity image can remove only the nested
<img>while leaving its absolute persistedfmd-pptx-imagewrapper and durable object ID behind. The new flow-image branch needs to resolve and remove the owning object for imported images.
🧪 Browser testing: Will run after this review (PR touches UI code)
| } else if (isDeletableFlowImage(element)) { | ||
| e.preventDefault(); | ||
| element.remove(); |
There was a problem hiding this comment.
🟡 Delete the containing image object rather than only its nested image
isDeletableFlowImage accepts every <img>, but imported fidelity images are nested inside an absolute fmd-pptx-image wrapper carrying the persisted object ID. Selecting that nested image and pressing Delete removes only the <img>, leaving an invisible wrapper and durable object metadata in the saved slide. Resolve the selected image to its owning persisted image object before removal, and add a regression asserting both wrapper and image are removed.
Additional Info
Found independently by 1 of 3 incremental agents; focused interaction tests pass but do not cover nested imported fidelity images.
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🔴
Review Details
Incremental Code Review Summary
The latest commit adds an authenticated image proxy for PDF export, URL/DNS validation, redirect checks, response caps, and temporary DOM image restoration around rasterization. The focused tests pass, and the earlier seven review comments remain unresolved and were intentionally not reposted. Because this increment introduces a network proxy and SSRF-sensitive URL handling, the risk level is now High.
New Findings
🔴 HIGH
- DNS validation is separate from the subsequent fetch, leaving a DNS-rebinding SSRF path.
- Hexadecimal IPv4-mapped IPv6 literals can bypass the private-address filter.
- The response-size limit is checked only after buffering the complete upstream body.
🟡 MEDIUM
- PDF rasterization explicitly omits credentials while re-fetching authenticated same-origin proxy URLs, so proxied images can still render blank.
The URL unit tests cover common private ranges and redirects, and the export code restores temporary DOM mutations correctly, but the new proxy needs hardened outbound fetching and bounded streaming before merge. 🧪 Browser testing: Will run after this review (PR touches UI code)
| for (let hop = 0; hop <= MAX_PROXY_REDIRECTS; hop++) { | ||
| if (!(await resolvesToPublicAddress(target.hostname))) { |
There was a problem hiding this comment.
🔴 Bind the fetch to the address validated against SSRF
resolvesToPublicAddress() performs a DNS lookup, but fetch(target) performs a separate hostname resolution. A DNS-rebinding host can answer with a public address for validation and then return a loopback, RFC1918, or metadata address for the fetch, bypassing the redirect checks. Resolve once and pin the outbound connection to the validated address (including independently at every redirect hop).
Additional Info
Found independently by 2 of 3 incremental agents.
| if (/^f[cd]/.test(addr) || addr.startsWith("fe8") || addr.startsWith("fe9")) | ||
| return true; | ||
| if (addr.startsWith("fea") || addr.startsWith("feb")) return true; | ||
| const mapped = /^::ffff:(\d+\.\d+\.\d+\.\d+)$/.exec(addr); |
There was a problem hiding this comment.
🔴 Reject hexadecimal IPv4-mapped IPv6 private addresses
The IPv4-mapped IPv6 check only accepts dotted IPv4 tails. Valid forms such as ::ffff:7f00:1 and ::ffff:a9fe:a9fe fall through as public even though they map to loopback and 169.254.169.254. Normalize mapped addresses regardless of textual representation and add regression cases for these forms.
Additional Info
Found by 1 of 3 incremental agents; the current tests cover dotted mapped IPv4 only.
| return { error: "Image too large" }; | ||
| } | ||
|
|
||
| const body = Buffer.from(await response.arrayBuffer()); |
There was a problem hiding this comment.
🔴 Enforce the image size cap while streaming the response
For chunked responses or an upstream that omits/lies about Content-Length, await response.arrayBuffer() buffers the entire body before the 15 MiB check runs. Read the response stream with a byte counter and abort/cancel as soon as the cap is exceeded, then construct the buffer only after the bounded read completes.
Additional Info
Found by 1 of 3 incremental agents; focused tests do not cover chunked oversized bodies.
| fetch: { | ||
| requestInit: { cache: "no-cache", mode: "cors", credentials: "omit" }, |
There was a problem hiding this comment.
🟡 Preserve session credentials when rasterizing proxied images
The new proxy requires a session cookie, but the modern-screenshot fetch options explicitly set credentials: "omit". After preload replaces a non-CORS image with the authenticated same-origin proxy URL, the screenshot library re-fetches it without the cookie and receives 401, so those images can still be blank in exported PDFs. Use same-origin credentials for proxy requests while continuing to omit credentials for direct third-party images.
Additional Info
Found by 1 of 3 incremental agents; proxy authentication and screenshot fetch options are visible in the changed code.
|
@builderio-bot verify the above issues exist and fix them. Feel free to be skeptical and let me know the changes you've made |
|
I verified all 11 findings against the code. All 11 were real — I found no false positives. Two were serious enough to be worth calling out. The two that mattered mostThe proxy I added last turn didn't actually work. My SSRF guard had a real bypass. I checked IPv4-mapped IPv6 with The rest
Two things to knowI hit two stale package builds unrelated to this PR ( Also, Final state: 761 tests pass, monorepo typecheck 0 errors, format and all guards clean. 23 new tests across the IPv6 classifier, connection pinning, image-owner lookup, and import timeouts. |
2 participants
Summary
Triages and fixes a batch of reported slides bugs: PDF/PPTX import fidelity issues, editor rendering glitches on large decks, a run indicator that could spin indefinitely, and a Google Slides export that silently reported success when it actually fell back to a PPTX download.
Problem
User feedback surfaced several issues: imports of non-16:9 PDFs/PPTX rendered distorted or mispositioned content and lost spaces between words when style changed mid-line, decks with many slides rendered slowly with flickering/glitchy thumbnails, editing an imported deck could fail to save, the runs tray spinner could stick "active" forever after a run died mid-flight, the deck editor could stay dimmed after resizing, and Google Slides export reported success even when it silently fell back to a PPTX download.
Solution
Added a triage plan (
plans/slides-feedback-2026-08-07.md) documenting each reported bug, verifying it against the code, and fixing the confirmed issues directly. Fixes span the import pipeline, the slide renderer, the editor sidebar/UI, export flow, and the core runs tray polling logic.Key Changes
pollMs={0}, so an abandoned run (budget exhausted, dead worker) can no longer spin forever; added regression tests inRunsTray.spec.tsx.html-converter.ts): scale element positions/sizes against the aspect-ratio preset closest to the source slide's own ratio instead of a fixed 16:9/960x540 box, fixing distorted/mispositioned content on portrait or custom-size pages; also scale font sizes by the same EMU-relative factor as positions instead of a fixed pt→px conversion.pdf-fidelity-parser.ts): preserve word-gap spacing across a mid-line style change (color/weight) so headings no longer get jammed together (e.g. "7 Airpurifying" → "7 Air purifying").IntersectionObserver, avoiding hundreds of reflows at once on large decks.backdrop-bluron thumbnail overlays (replaced with opaque scrims), usehidden/blockinstead of opacity toggling, and addcontent-visibility: autoplus fixedaspect-ratioon thumbnails to stop flickering/dark patches on long slide rails.sidebarOpenon window resize so the mobile dimming scrim doesn't get stuck covering the editor after narrowing the window.tool/mcp/a2a), not the browser editor, fixing human edits to imported decks failing to save.To clone this PR locally use the Github CLI with command
gh pr checkout 2765You can tag me at @BuilderIO for anything you want me to fix or change