Fix Slides new-slide button placement, insert flow, and add slide duplication shortcut - #2816
Conversation
…mpt, and Cmd+C/V duplicate Moves the "New Slide" trigger from the top toolbar into the left slide rail as an outline button. Clicking it now inserts a blank slide directly below the active one and opens the describe-slide prompt beside that new thumbnail, with an explicit close button; typing a description tells the agent to fill in the existing placeholder slide instead of adding a duplicate. Adds a Cmd/Ctrl+C then Cmd/Ctrl+V shortcut that duplicates the selected slide directly below itself when no slide element is selected.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
|
@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable) |
|
Checked latest PR feedback. No new human/bot review comments beyond the deploy-preview bot notices. The only actionable item was a failing |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Reset addSlideGenerating when the agent run finishes so the New slide button doesn't stay disabled for the rest of the session - Clamp the right-anchored AddSlidePopover using its measured height so it doesn't render off-screen near the bottom of the slide rail - Persist a new blank slide immediately (skip the 500ms debounce) since it's immediately followed by an agent update-slide request that can otherwise race the add-slide persistence
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ate-slide Immediate persistence via enqueueDeckOp only kicks off drainPendingDeckOps without blocking the caller, so the earlier fix still had a (much narrower) race window between the fire-and-forget add-slide save and the agent's update-slide request. Add flushDeckSave to await the in-flight save chain (including any follow-up drain queued while a save is already running), and have EditorSidebar await it right before submitting the agent request.
|
I encountered an error while processing your request. Please try again. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… new-slide flow - flushDeckSave previously resolved even when drainPendingDeckOps swallowed a save failure internally to drive its own retry loop; it now polls through requeued retries and throws once retries are exhausted, so the caller learns the placeholder never persisted - EditorSidebar's addSlideGenerating reset effect fired while onAwaitAddSlidePersisted() was still pending (agentGenerating was still false at that point), re-enabling New slide mid-save; gated the reset on having actually observed a run start via sawAgentGeneratingRef - Surface a toast and reset the generating flag when persistence fails so the button doesn't stay stuck disabled
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ialog-safe paste shortcut - Move the addSlideGenerating completion tracking from EditorSidebar (which unmounts when the rail closes on narrow viewports) up to DeckEditor, which already owns the state and never unmounts, using its existing top-level useAgentGenerating() call - Remove the orphaned blank placeholder slide when flushDeckSave ultimately fails, since AddSlidePopover closes immediately without waiting on the async submit callback and the typed prompt is lost either way - The new Cmd/Ctrl+C/V slide-duplication shortcut didn't check for an open dialog, sheet, menu, or popover before claiming the keystroke; extended its safe-zone checks to cover role=dialog/alertdialog content, Radix popper-positioned content, and AddSlidePopover
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The remount-safe reset effect keyed off the general useAgentGenerating() 'generating' value, which reflects ANY agent chat activity in the deck editor. An unrelated concurrent run finishing could be mistaken for the add-slide run completing and clear addSlideGenerating early (or while its own run is still active). Give the add-slide flow its own useAgentGenerating() instance owned by DeckEditor, and have EditorSidebar call the submit function passed down as a prop instead of instantiating its own hook, so the run stays correctly scoped and remount-safe at the same time.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Summary of review passAddressed 8 review findings from the Builder review bot across 5 commits (930161a → d78dd74):
Every comment thread has an inline reply explaining the fix (or, for one, why no change was needed after re-checking the retry-budget math). Remaining item: the |
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🔴
Review Details
Code Review Summary
This incremental review evaluated the latest scoped agent-submit change and the new failure cleanup and keyboard-overlay handling. The prior generation-lock issue was fixed: the add-slide request now uses a DeckEditor-owned useAgentGenerating instance, and the resolved thread was cleared. The persistence cleanup and broader overlay guards improve the failure and interaction paths, but they introduce or expose additional user-facing edge cases.
Key Findings
- 🔴 HIGH — Failed-save cleanup can delete user edits made to the placeholder while persistence retries are in progress.
- 🟡 MEDIUM — Unmounting the mobile sidebar can abandon the prompt while leaving the optimistic blank slide behind.
- 🟡 MEDIUM — Ordinary focused editor controls can still trigger slide copy/paste duplication.
- 🟡 MEDIUM — Treating every Radix popper as blocking disables slide duplication whenever an unrelated tooltip is visible.
🧪 Browser testing: Attempted full verification of 17 cases, but all were blocked because browser automation tools were unavailable; the dev server was healthy.
| // The popover already closed (AddSlidePopover doesn't wait on | ||
| // this async callback), so the typed prompt is gone either | ||
| // way — remove the orphaned blank placeholder rather than | ||
| // leaving a stray empty slide the user never asked for. | ||
| onRemoveFailedSlide?.(describeSlideId); |
There was a problem hiding this comment.
🔴 Do not delete user edits during failed-save cleanup
After persistence retries fail, this unconditionally queues deleteSlide for the placeholder. The slide remains editable while retries are pending, so a user can make manual changes that are queued behind the failed add; when connectivity returns, the cleanup delete can persist after those edits and erase the user's work. Retain the slide on failure or verify it is still untouched before deleting it.
There was a problem hiding this comment.
Confirmed, good catch — a real risk of destroying user work, not just a stray-slide cosmetic issue. Fixed in 061c719: the cleanup now only deletes the placeholder if its content/notes still match the untouched blank-layout default; if the user has started editing it directly on the canvas during the retry window, it's left in place (a later edit will re-enqueue and self-heal the original failed add-slide op alongside the new patch, since enqueueDeckOp clears failedSaveDecks/retry state on the next op for that deck).
| addSlideAgentSubmit, | ||
| }: EditorSidebarProps) { | ||
| const t = useT(); | ||
| const [describeSlideId, setDescribeSlideId] = useState<string | null>(null); |
There was a problem hiding this comment.
🟡 Cancel or preserve the placeholder when the sidebar unmounts
The prompt and describeSlideId are owned by this conditionally mounted sidebar, while the placeholder is inserted by DeckEditor. Closing the mobile drawer unmounts this component and removes the prompt without invoking either submit or failure cleanup, leaving an orphaned blank slide in the deck. Keep the prompt transaction in the parent or explicitly cancel and remove the placeholder on sidebar unmount.
There was a problem hiding this comment.
Skipping this one — closing the popover without describing the slide (via the X button or Escape) already leaves the same blank placeholder behind today, by design (identical outcome to using "Add empty slide" directly). Unmounting the sidebar mid-flow produces the exact same end state, not a worse one, so I don't think this needs separate handling. Let me know if you see a scenario where the outcome actually differs.
| const isInsideSafeZone = (el: Element | null) => { | ||
| if (!el) return false; | ||
| if (el instanceof HTMLInputElement) return true; | ||
| if (el instanceof HTMLTextAreaElement) return true; | ||
| if (el instanceof HTMLElement) { | ||
| if (el.isContentEditable) return true; | ||
| if (el.closest("[contenteditable='true']")) return true; | ||
| if (el.closest("input, textarea, [role='textbox']")) return true; | ||
| if (el.closest("[data-pin-popover]")) return true; | ||
| if (el.closest("[data-add-slide-popover]")) return true; | ||
| if (el.closest(".agent-panel-root")) return true; | ||
| if (el.closest("[role='dialog'], [role='alertdialog']")) return true; | ||
| if (isInsidePortaledLayer(el)) return true; | ||
| } | ||
| return false; | ||
| }; | ||
| if (isInsideSafeZone(e.target as Element | null)) return; |
There was a problem hiding this comment.
🟡 Ignore copy/paste shortcuts from ordinary editor controls
The safe-zone predicate excludes text fields and overlays, but not ordinary focused controls such as toolbar buttons, sidebar toggles, or export buttons. After Cmd/Ctrl+C records a slide, pressing Cmd/Ctrl+V while operating one of those controls can still duplicate the slide even though the shortcut is documented as a slide-rail action. Gate the handler to the rail/canvas context or exclude non-thumbnail interactive controls.
There was a problem hiding this comment.
Skipping this one — the existing pattern here (claim Cmd/Ctrl+C/V for canvas-level actions whenever no text-entry element has focus) matches established canvas-app convention (Figma, Google Slides use the same heuristic for object/slide copy-paste). A toolbar button having focus isn't a text-entry context, so treating it as fair game for the slide-duplication shortcut seems intentional rather than a gap, and the action is non-destructive/undoable if it ever surprises someone. Happy to revisit if there's a specific control where this causes real friction.
| // shortcut duplicate the slide underneath it. | ||
| if (document.querySelector("[role='dialog'], [role='alertdialog']")) | ||
| return; | ||
| if (document.querySelector("[data-radix-popper-content-wrapper]")) return; |
There was a problem hiding this comment.
🟡 Do not block slide shortcuts for unrelated tooltips
[data-radix-popper-content-wrapper] also wraps Radix tooltip content, not just blocking menus and popovers. Hovering a toolbar control can therefore mount a tooltip and cause this handler to ignore Cmd/Ctrl+C and Cmd/Ctrl+V, making slide duplication fail unexpectedly. Restrict this document-level check to interactive blocking overlays rather than every Radix popper.
There was a problem hiding this comment.
Confirmed — Radix Tooltip content uses the same Popper positioning primitive as Popover/DropdownMenu/Select, so it renders inside the identical [data-radix-popper-content-wrapper], and tooltips open on plain hover with no click needed. That would've silently broken the shortcut just by mousing near any toolbar button. Fixed in 061c719: narrowed the check to only treat a popper wrapper as blocking when it doesn't contain the existing data-agent-native-tooltip marker, so real menus/popovers/selects still block the shortcut but hover tooltips don't.
|
@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable) |
|
@SajalChaplot this button feels way more big and prominent than googles. can we take a nother rev at what a solution could look like here? 
|
…ck slide shortcuts - The failed-save cleanup unconditionally deleted the blank placeholder slide, but retries can take long enough for the user to have started editing it directly on the canvas in the meantime. Only delete it if it still matches the untouched blank-layout default. - data-radix-popper-content-wrapper also wraps Tooltip content, which opens on plain hover. The Cmd/Ctrl+C/V slide-duplication shortcut was treating any such wrapper as an open menu/popover, so hovering a toolbar button silently disabled the shortcut. Narrowed the check to exclude wrappers that only contain tooltip content (marked by the existing data-agent-native-tooltip attribute).
|
Yeah, gonna try something tomorrow |
|
I've been monitoring for the full window. Here's where things stand: All review feedback is fully addressed and replied to — 12 findings from the Builder review bot across 6 commits, with inline replies explaining each fix (or why a couple were intentionally left as-is). No merge conflicts; the PR is New CI failures since my last update are NOT caused by this PR:
I don't have permission to trigger a job re-run or I'm not merging ⠀ |
|
I tried bunch of UI variants. The best options out of those were these two. Figma slides like
Google slides likeThis requires using a second pane for toolbar, where add slides can be added. 
|
…bottom Switches the sidebar "New slide" trigger to a compact bordered ghost button (matches the slide list's own row padding) instead of the solid filled outline variant. Fixes the "Describe this slide" popover clipping off the bottom of the viewport when the newly inserted slide lands near the end of a long deck: the new slide's thumbnail is scrolled into view before the popover anchors to it, and the popover's height-based vertical clamp now re-measures via a ResizeObserver so it keeps fitting the viewport as its content grows (Google Doc hint, file chips, textarea growth) instead of only clamping on the first paint.
…ntext-issues-f1bf03 # Conflicts: # templates/slides/app/pages/DeckEditor.tsx
|
great, thanks @SajalChaplot - let's go with google slides-like |
Increases the deck-title/Share/Present bar height and moves New Slide back into the row with the text/style controls and zoom in/out, always rendered as its own line below the title bar at every viewport size instead of merging into it on wide screens. Lifts the describe-slide popover trigger from EditorSidebar up to DeckEditor so the button works from its new toolbar location.
…ntext-issues-f1bf03 # Conflicts: # templates/slides/app/components/editor/EditorActionCluster.tsx # templates/slides/app/components/editor/EditorSidebar.tsx # templates/slides/app/components/editor/EditorToolbar.tsx # templates/slides/app/pages/DeckEditor.tsx
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable) |
|
We've now completed roughly 20+ minutes of continuous monitoring with no new review feedback since the last round (all replied at 2026-08-12 17:35 UTC). Final status: PR #2816 status:
The PR is ready to merge pending a maintainer's approval. |
3 participants
Summary
templates/slidesfrom the top toolbar into the left slide rail, as an outline button above the thumbnail list.update-slide) rather than inserting a duplicate.duplicateSlidenow returns the new slide's id so callers can select it.newSlide,closeAddSlides,describeThisSlide) to all 11 locale files.EditorToolbar/EditorActionCluster(which now only owns the text-box toggle).Screen.Recording.2026-08-14.at.12.23.14.AM.mov
Test plan
tsc --noEmitontemplates/slides— no new type errors introduced (only pre-existing environment-level@agent-native/core/*module-resolution noise unrelated to this change).