fix(studio): sidebar/panels UX — asset delete confirm, rename, search trap, undo

[vanceingalls]

Summary

Left-sidebar and slideshow-panel fixes from the studio UX review — four criticals lived here: an unconfirmed permanent asset delete, a Rename menu item that did nothing, a search box that unmounted itself while its filter stayed applied, and slideshow edits whose persist failures were silently swallowed.

Changes

Asset context menu (2 criticals):

• Delete now shows an inline DeleteConfirm before calling the API (was: one click → permanent DELETE, failure console-only).
• Dead "Rename" item replaced with a working inline rename (validates /, \, ..; preserves directory + extension).
• Menu gets role="menu"/menuitem, Escape, arrow-key nav, focus-into-menu, viewport clamping.

Assets tab (critical):

• Search trap fixed: header controls gate on the unfiltered asset count; a query matching nothing shows "No assets match" + Clear search (was: the search box unmounted with the stale query still applied, tab looked permanently empty).
• Cards keyboard-operable; visible "Copy path"/"Copied"/"Copy failed" chip (click-to-copy was invisible and clipboard failures silent); import button shows a spinner; broken thumbnails render an extension-label fallback.

Slideshow panel (critical):

• Persist failures surface a red "Changes not saved — Retry" banner (role="alert") with a working retry (was: console.error while the user kept editing).
• In-panel undo stack (50 snapshots, scoped ⌘Z that never collides with file undo) — branch deletes and reorders were previously irreversible.
• Branch delete uses an inline styled confirm (was window.confirm); reorder buttons truly disable at boundaries; HotspotTool explains its prerequisites instead of dead-ending on a disabled button.

Blocks / compositions tabs:

• "Added!"/"Copied!" are promise-truthful with failure states (was: optimistic success regardless of outcome). Hover-only action overlays also reveal on keyboard focus. PromptPreviewModal gets the dialog contract + dirty-draft guard. Lint dot → labeled count badge. CompCards keyboard-operable; render button explains "A render is already in progress".
• Sidebar tabs are a real APG tablist (roving tabIndex, arrow keys, aria-selected, tabpanels). AudioRow: labeled play/pause, single-preview coordination (two previews can no longer play simultaneously).
• Shared SearchInput adopted in both tabs.

Verification

• SlideshowPanel + CompositionsTab tests 52/52; oxlint/oxfmt/tsc clean

Stack

PR 5/7 of the studio UX-review fixes.

🤖 Generated with Claude Code

[vanceingalls]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• fix(studio): captions UX — mode exit, undo, autosave surfacing, honest gating #1968
• fix(studio): player UX — keyboard timeline, honest waveform, keyframe menu actions #1967
• fix(studio): sidebar/panels UX — asset delete confirm, rename, search trap, undo #1966 👈 (View in Graphite)
• fix(studio): editor panel UX — commit safety, keyboard a11y, wired BlockParamsPanel #1965
• fix(studio): shell UX — data-loss guards, error surfacing, dialog contracts, toasts #1964
• feat(studio,studio-server,cli): render cancel end-to-end + renders/nle/storyboard UX #1963
• feat(studio): harden ui primitives — focus rings, keyboard tooltips, dialog hook #1962
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miga-heygen]

PR Review — fix(studio): sidebar/panels UX — asset delete confirm, rename, search trap, undo

VERDICT: Approve (with nits)

This is a well-executed UX remediation PR that closes four genuine severity holes (unconfirmed permanent delete, dead rename, search trap, silent persist failures) and layers on proper a11y and keyboard support throughout. The code is clean, the patterns are consistent, and the attention to detail (dirty-draft guard on the modal, single-preview coordination for audio, extension-label fallback for broken thumbnails) is excellent. Two real issues worth addressing, and a few nits below.

---

Findings

[medium] Undo pushes the pre-edit snapshot but pops and persists it without catching async errors meaningfully

SlideshowPanel.tsx — handleUndo does applyManifest(prev, { skipUndo: true }).catch(() => {}). That swallowed .catch(() => {}) means a failed undo-persist will set persistError = true inside applyManifest (good), but the caller has no awareness. This is borderline acceptable because the persist-error banner does surface the failure to the user. However, the real concern is the interaction between handleUndo and handleRetryPersist: if undo fails to persist but the user hits "Retry", handleRetryPersist re-persists manifestRef.current — which IS the undone manifest, so it does the right thing. Just wanted to flag the .catch(() => {}) as an explicit design choice that's been thought through vs. accidental swallowing. A // persist error already surfaced by applyManifest comment would document the intent.

[medium] Module-level mutable singleton for audio coordination

AudioRow.tsx — let stopCurrentPreview: (() => void) | null = null; is a module-scoped mutable binding shared across all AudioRow instances. This works correctly for the single-preview-at-a-time requirement, but it's a subtle pattern that could cause issues if AudioRow is ever rendered in multiple independent panels or React roots (the singleton is per-module, not per-tree). A React context or a Zustand slice would be more idiomatic for the studio codebase. Low risk today — just a future-proofing note.

[low / nit] Asset name validation regex rejects only / and \, allows other problematic chars

AssetContextMenu.tsx — isValidAssetName rejects /, \, and .., which covers path traversal. But it allows names with leading/trailing dots, spaces, control characters, or characters like ?, *, : that are invalid on Windows (if assets are ever downloaded or synced to a local filesystem). Might be worth a tighter allowlist or at minimum stripping leading/trailing whitespace before the check (the trim() in commitRename handles whitespace, so this is covered — just noting the validation is OS-portable-minimal rather than strict).

[low / nit] Rename drops the original extension

AssetContextMenu.tsx — commitRename constructs the new path as ${dir}${trimmed} where trimmed comes from the input seeded with filename(asset) (which includes the extension). But if a user edits the name and accidentally deletes the extension (e.g., renames hero.png to hero), the rename silently drops it. The initial renameDraft state includes the extension, so this requires active user error, but a safeguard like warning when the extension changes might be a nice UX touch. Not a blocker.

[low / nit] CompCard uses aria-pressed on a non-toggle button

CompositionsTab.tsx — The CompCard div has role="button" with aria-pressed={isActive}. aria-pressed is for toggle buttons (switches); for selection among a list of cards, aria-selected (with a role="option" inside a role="listbox") or aria-current would be more semantically accurate. Minor a11y pedantry.

[low / nit] fallow-ignore-next-line — looks like a custom lint suppression comment

AssetContextMenu.tsx line 7: // fallow-ignore-next-line complexity. Is "fallow" an internal lint tool, or is this a typo for something else? Just want to make sure it's actually suppressing something and not a dead comment.

---

Ponytail Lens

From the "ponytail engineer staring at the screen for five minutes" perspective: the search trap fix is the kind of bug that makes a user think their data is gone — the tab looks permanently empty because the search input that caused it has also vanished. The fix (gating header controls on allMediaAssets instead of the filtered mediaAssets) is simple and correct. The "No assets match" + "Clear search" empty state is the right UX for this. The undo stack capping at 50 entries with a shift() is fine for an in-memory stack; the scoped Cmd+Z that skips inputs/textareas is well-considered. The useDialogBehavior hook (not in this diff but depended on) is solid — focus trap, restore, dirty-draft guard. Overall, this PR fixes real user-facing pain points and the fixes are proportionate.

---

DIFF STATS: +805 / -371 across 10 files (1 new: PromptPreviewModal.tsx extracted from BlocksTab.tsx).

Review by Miga

[miguel-heygen]

Stack review for PR 5/7.

Audited: packages/studio/src/components/sidebar/AssetContextMenu.tsx, AssetsTab.tsx, AudioRow.tsx, BlocksTab.tsx, CompositionsTab.tsx, PromptPreviewModal.tsx, and packages/studio/src/components/panels/SlideshowPanel.tsx at head 97e5054.

The asset search trap fix is correct: header controls are gated on the unfiltered media pool, so a zero-result query cannot unmount the search box that would clear it. Asset delete now has an explicit confirmation step, rename stays within the current directory with traversal guards, and copy failures become visible. The slideshow undo/persist-error flow keeps the in-memory edited manifest while surfacing failed writes and retrying the current manifest, which is the right failure mode.

Nits remain non-blocking: rename validation is intentionally minimal, and the context menu submodes could be tightened semantically/keyboard-wise later.

Verdict: APPROVE
Reasoning: I did not find a blocker in the sidebar/panels delta; the critical search-trap, delete-confirm, rename, and persist-error paths are addressed coherently.

— Magi

[james-russo-rames-d-jusso]

Reviewed at 61c893b.

Layering on Miga's Approve-with-nits and Magi's APPROVE. Both correctly credit the four criticals (search trap, delete confirm, dead rename, silent persist). Miga's five findings all reproduce for me; Magi's structural read (search trap gates on allMediaAssets, persist-error retries current manifest) is right. Adding a few second-pass findings on top.

---

🟠 DeleteConfirm misses the keyboard contract that Rename mode gets

packages/studio/src/components/sidebar/AssetContextMenu.tsx:143-152, 194-222 — when the user activates Delete from the context menu, mode flips to "confirm-delete" and the DeleteConfirm component renders. But:

• No autoFocus on either button — focus stays on the Delete menuitem the user just clicked (which no longer exists in the tree). Focus falls to document.body.
• No Enter handler — a keyboard user has to Tab to Delete and press Space/Enter. Escape works (the outer keydown listener at line 47-71 backs mode out to "menu"), so that half of the contract is there.

Compare with mode === "rename" (line 153-188): autoFocus on the input, Enter commits, Escape backs out. Same-file inconsistency between modes. Per my feedback_same_file_protection_list_disagreement_severity rubric: same-file divergence on the same conceptual set (submode keyboard contract) is 🟠 not 🟡. Suggest autoFocus on the Delete button + a keydown handler for Enter inside DeleteConfirm (or move the two buttons into role="menuitem" so the existing arrow-key nav in the outer effect picks them up).

For a "permanent delete" operation this matters more than a normal button — the whole point of the confirm modal is to make the destructive action deliberate, and having it hard-to-reach by keyboard undermines that.

---

🟠 isValidAssetName is portable-minimal — bumping Miga's nit to 🟠

AssetContextMenu.tsx:5-7 currently rejects only /, \, and ... Miga flagged this as "low / nit" because it covers path traversal. Bumping severity because:

• HeyGen studio users sync compositions via git, Dropbox, and (per team memory) sometimes to Windows-mounted volumes. Names like hero:v2.png, test<beta>.mp4, or report?.mp4 would be accepted here and break the moment anyone touches a Windows filesystem or a strict server-side path check.
• Reserved DOS device names (CON, PRN, AUX, NUL, COM1-9, LPT1-9) are also blocking on Windows — CON.png fails to create.
• Leading/trailing dots + control chars would fool the API path handler in various ways.

Suggested allowlist: /^[^<>:"/\\|?*\x00-\x1F]+$/ + case-insensitive reserved-name check + strip leading/trailing dots. Not gating this PR, but worth doing before the next studio-on-Windows report comes in — because "silent rename that then can't be persisted or renamed back" is exactly the failure mode this PR set out to eliminate elsewhere.

---

🟠 Slideshow persist failures surfaced to user, not to ops

Same finding as #1964: SlideshowPanel.tsx:236-245 (setPersistError(true) on write failure) + handleRetryPersist (SlideshowPanel.tsx:255-266) fire no telemetry. If an oncall wants to know how often slideshow-manifest writes are actually failing across the user base, there's no metric. packages/studio/src/telemetry/events.ts is right there. Same guidance as #1964: trackSlideshowPersistOutcome({ result, retry }) inside the try/catch + retry handler. Optional but load-bearing for the "we surfaced silent failures" thesis.

---

🟡 Nits already covered by Miga (all correct)

• handleUndo .catch(() => {}) is intentional (persistError banner already surfaces) — deserves a comment. Miga said the same.
• AudioRow.tsx module-scoped stopCurrentPreview singleton — works today, would break under multiple React roots.
• Rename can silently drop the extension.
• CompCard uses aria-pressed on a non-toggle.
• fallow-ignore-next-line complexity — real (internal lint pragma per repo conventions), not dead comment.

---

🟢 Confirmed good

• Search trap fix is complete. AssetsTab.tsx:423, 434 gate on allMediaAssets (unfiltered) — no unmount-under-zero-results. Verified other tabs: BlocksTab.tsx:51 always renders SearchInput unconditionally regardless of filteredBlocks.length (no trap); CompositionsTab.tsx has no search input at all. No other search-trap sites in the sidebar.
• Undo memory is not a concern. SlideshowManifest (packages/parsers/src/slideshow/slideshow.types.ts:8-13) is refs-only: sceneId strings, notes, hotspots (label + region rect). No blobs, no media. 50 snapshots × small refs = negligible retention. Push-by-reference is safe because every applyManifest caller uses immutable spreads (verified across the 12 call sites).
• History cleared on comp switch at line 220 (useEffect on compHtml) — no cross-composition stack leaks.
• shift() on overflow at line 233 keeps the newest 50, not the oldest — correct semantics.

---

Verdict: 🟢 with three 🟠 follow-ups (DeleteConfirm keyboard contract, cross-platform rename validation, persist-failure telemetry). Ship-safe; the follow-ups are additive.

— Review by Rames D Jusso