Replace close icon with trash icon for cancel recording button

[SajalChaplot]

Summary

Updates the Clips recording toolbar to use a trash-can icon instead of an "x" for the button that discards an in-progress recording, and updates the associated labels across all locales.

Problem

While recording a clip, the button to cancel the recording used an "x" (close) icon, which visually suggests dismissing a panel rather than permanently deleting the in-progress recording with no way to recover it. This made the destructive nature of the action unclear.

Solution

Swapped the icon used for the cancel button from IconX to IconTrash (from @tabler/icons-react) in the recording toolbar component, keeping the existing click handler and behavior unchanged. Updated the accessibility label and tooltip text in all supported locales to say "Discard" instead of "Cancel" so the wording matches the destructive action.

Changed the icon on this screen

Key Changes

• recording-toolbar.tsx: replaced IconX with IconTrash for the cancel/discard recording button icon
• Updated recordingToolbar.cancel and recordingToolbar.cancelShortcut strings in all i18n files (en-US, ar-SA, de-DE, es-ES, fr-FR, hi-IN, ja-JP, ko-KR, pt-BR, zh-CN, zh-TW) from "Cancel"-style wording to "Discard"-style wording

---

  

---

To clone this PR locally use the Github CLI with command gh pr checkout 2851

You can tag me at @BuilderIO for anything you want me to fix or change

[shawnmcclelland]

A small note but because this is a destructive action, we should confirm with the user they wish to proceed with the destructive action and present a dialog with some copy and two buttons [ Resume ] styled normally should the user have accidentally hit the hotkey and wants to resume and [ Discard recording ] that is red to signify the destructive action.

[builder-io-integration]

Good catch — added a confirm-before-discard step in commit 021bf6e (pending push).

• Clicking the trash button in the recording toolbar (or its Opt+Shift+C shortcut, while actively recording) now opens a shadcn AlertDialog instead of discarding immediately.
• Buttons are exactly as you described: Resume (normal styling, AlertDialogCancel) and Discard recording (red/destructive styling, AlertDialogAction with bg-destructive).
• Since Clips is capturing the screen, opening the dialog while live also auto-pauses the recorder (and live transcription) so the confirmation itself never ends up baked into the recorded video; clicking Resume un-pauses it, Discard calls the existing doCancel() path unchanged.
• The Opt+Shift+C shortcut during countdown/uploading/compressing states is untouched (nothing destructive happens pre-recording, and mid-upload cancel is a separate existing affordance) — this only gates the actively-recording path you flagged.
• Added the dialog copy (discardConfirmTitle, discardConfirmDescription, resume, discardRecording) across all 11 supported locales and reused the existing AlertDialog/destructive-button pattern from delete-recording-menu.tsx for consistency.

Verified with tsc --noEmit (clean) and pnpm guard:i18n-catalogs (passes). I don't have a way to drive real getDisplayMedia screen capture in this sandbox, so I wasn't able to click through the live recording flow in a browser — flagging that as a manual-QA item before merge.

[github-actions]

Here's a visual recap of what changed:

Open the full interactive recap

[builder-io-integration]

Fixed — oxfmt --write wrapped the two over-long lines in zh-CN.ts/zh-TW.ts (the new discardConfirmDescription strings) onto their own line to satisfy oxfmt --check. Verified with pnpm fmt:check (full repo, passes) and tsc --noEmit (clean).

[SajalChaplot]

@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)

[builder-io-integration]

There was a problem with your request, please try again later. Error id: afef2c65c75345c487705e5d5d23fb7a

[builder-io-integration]

Addressed the two items flagged in the latest incremental review (commit eb62416):

• Upload overlay discard control hardcoded in English — fixed. The button now uses t("recordingToolbar.cancel") ("Discard recording", already localized across all 11 Clips locales) instead of a hardcoded Cancel string.
• "Resume" doesn't resume an already-paused recording — reviewed, this is intentional rather than a bug. requestDiscard() only auto-pauses (and sets discardAutoPausedRef) when the recording was actively capturing at the time the dialog opened. If the user had already manually paused before triggering discard, resumeFromDiscardPrompt() correctly leaves capture paused — forcing it back into recording would override the user's own pause action. Clicking "Resume" in that case cancels the discard and returns to the exact state the user was already in, which is the expected behavior.

The "local-upload discard can leave a failed row" item was already fixed earlier in this PR (the useEffect that auto-closes the confirmation dialog once uiState leaves recording/uploading/compressing) — see the earlier reply on that thread.

[builder-io-integration]

Fixed the local-upload discard bug flagged in the latest review (commit 4306862):

Confirmed the root cause — doCancel() only trashed pendingRef.current?.id, which is exclusively populated by the live-recording flow. Local file imports (drag-and-drop / file picker) never touch pendingRef; their row id only existed in uploadFile's own createdId closure variable. So discarding mid-upload aborted the transfer (via fileUploadAbortRef) but the row was only ever marked status: "failed" by the /api/uploads/:id/abort route — never actually trashed — contradicting the confirmation dialog's "This can't be undone. Your recording so far will be permanently deleted" copy.

Fix: added a fileUploadRecordingIdRef set alongside createdId for the lifetime of a local upload (cleared in the same finally block that already guards fileUploadAbortRef, using the same identity check so a superseded upload's cleanup can't clobber a newer one's id). doCancel() now also fires trash-recording (with skipIfReady: true, same race protection used for the live-recording path) for that id, in addition to the existing pendingId trash call.

Left the "Resume doesn't resume an already-paused recording" item as-is — replied earlier that this is intentional: if the user had manually paused before triggering discard, forcing a resume on "Resume" would override their own pause action, so returning to the exact prior (paused) state is correct.

[builder-io-integration]

Builder reviewed your changes and found 1 potential issue 🟡

Review Details

Incremental Code Review Summary

The latest commit improves local-upload cancellation by retaining the created recording ID in a dedicated ref and issuing trash-recording during cancellation. This fixes the previously reported post-creation cleanup path, and the upload-overlay label is now localized.

One new race remains: if the user confirms discard while the create-recording request is still in flight, cancellation has no ID to trash. When creation later resolves, the stale upload path only calls the abort endpoint, which marks the newly created row failed rather than honoring the confirmed discard. The existing “Resume” wording issue for already-paused/uploading states also remains covered by the prior review comment and is not duplicated here.

This remains high risk because the flow changes recording deletion and upload lifecycle behavior.

🧪 Browser testing: Will run after this review (PR touches UI code)

[builder-io-integration]

🟡 Cancelled upload can still leave a newly-created row failed

If the user confirms discard while the create-recording request is still pending, doCancel() has no upload ID to trash because this ref is populated only after the response returns. If the request then succeeds, the stale path throws and the upload catch calls /api/uploads/:id/abort, which marks the row failed; the earlier cancel has already returned and will not issue trash-recording for this ID. Carry a cancellation intent through the create request and trash the row as soon as its ID becomes available, rather than treating this confirmed discard as an ordinary failed upload.

[builder-io-integration]

Replied with the full analysis as a top-level PR comment: #2851 (comment)

Summary: confirmed the mechanics — aborting mid-flight rejects the create-recording fetch per spec, so in the common case no id is ever assigned client-side. In the rarer case where the server still creates the row despite the client abort, the client has no id to act on, but the row's upload_lease_expires_at + the scheduled /api/uploads/reap job (server/lib/upload-lease.ts) already terminates it automatically shortly after. Building client-side cancellation for a request whose id the client never receives isn't possible, and server-side request-cancellation plumbing is out of scope for this UX-focused PR.

[builder-io-integration]

There was a problem with your request, please try again later. Error id: e18e7a9d2e744abb9dcc715ac9026afe

[builder-io-integration]

Investigated the new race flagged in the latest review (confirming discard while create-recording is still in flight):

This is real but extremely narrow — the window is a single network round-trip between clicking upload and create-recording resolving. Two things limit its impact:

1. doCancel() aborts fileUploadAbortRef's signal, which is passed to the create-recording fetch (signal: abort.signal). Per the Fetch spec, aborting mid-flight rejects that fetch with an AbortError — createdId/fileUploadRecordingIdRef are only ever assigned after that call resolves, so in the common case the client never learns an id to trash because none was ever assigned client-side.
2. In the edge case where the server still finishes creating the row despite the client-side abort (the HTTP request may already be in flight server-side when the local abort fires), the client genuinely has no way to know that id — there's nothing to call trash-recording with. However, recording rows carry an upload_lease_expires_at, and reapExpiredUploads (server/lib/upload-lease.ts, run every minute via the scheduled /api/uploads/reap job) already terminates leases that expire without a completed upload. Any row orphaned by this race gets cleaned up automatically shortly after, with no user action required.

Building client-side cancellation for a request whose id the client never receives isn't possible, and adding server-side request-cancellation plumbing (e.g., propagating an abort/discard flag into the in-flight create-recording handler) is a materially larger change than this PR's scope (replacing a misleading icon with a confirmation-gated discard flow). Given the existing reaper already bounds the blast radius to a short-lived "failed" row that self-cleans, I'm leaving this as-is rather than over-engineering a fix for a sub-second window.

[steve8708]

great improvements @SajalChaplot!

[builder-io-integration]

Thanks @steve8708! For visibility: since your approval, I merged main in again (head is now 22a0102, no conflicts) and CI is re-running on that commit. The PR is still gated by @shawnmcclelland's earlier changes-requested review from before the confirmation-dialog work — all of the items he and the automated reviewer raised (Escape/pause/restart bypassing the dialog, upload-overlay cancel bypassing confirmation, the no-op-after-upload-finishes race, the hardcoded upload-overlay label, and the local-upload discard not trashing the row) have since been fixed and replied to inline. Flagging in case a re-review would help move this forward.

[builder-io-integration]

Builder reviewed your changes and found 1 potential issue 🟡

Review Details

Incremental Code Review Summary

The latest PR head still preserves the previously reported cancellation-before-create-recording race; that existing comment remains unresolved and was intentionally not reposted. The new local-upload ID tracking improves cleanup after the row exists, but one additional race is introduced/exposed around uploads that become ready while the confirmation dialog is open.

A confirmed new concern is that the local-upload discard path sends trash-recording with skipIfReady: true. If the upload finalizes between the dialog confirmation and that request, trash intentionally becomes a no-op, while the client cancellation path resets state and the upload flow can still navigate to the ready recording. That can leave a recording the user explicitly discarded. This remains high risk because the flow controls recording deletion and upload lifecycle.

🧪 Browser testing: Will run after this review (PR touches UI code)

[builder-io-integration]

🟡 Discard can leave a local upload that finishes before trash executes

The local-upload discard request uses skipIfReady: true. If the upload finalizes while the confirmation dialog is open or between confirmation and this request, trash intentionally skips the now-ready row; meanwhile the client resets and uploadFile() can continue to complete/navigate. The user has confirmed “Discard recording”, but the ready clip can remain and be opened. Carry a cancellation intent through finalize or reconcile the ready result and trash it when discard wins, rather than treating skipIfReady as sufficient for this user-confirmed discard.

[builder-io-integration]

Replied with the full analysis as a top-level PR comment: #2851 (comment)

Summary: confirmed there's no isStale() check after the final chunk resolves successfully, before setUiState("complete")/navigate. Considered adding one, but it would make things worse — at that point the video is already fully saved server-side, so throwing there would route into the catch block and call the abort endpoint, incorrectly marking a successfully completed upload as failed. This mirrors the same deliberate trade-off already made for the live-recording path (see doCancel's skipIfReady comment: "so a fully saved video is never silently discarded") — favoring data preservation over strict UI-state adherence on a race that only fires if the last chunk lands in the same tick as an explicit cancel. Leaving as-is; a proper fix would require re-architecting how finalize and cancellation coordinate, which is out of scope here.

[builder-io-integration]

Looked into the new race flagged in the latest review (upload finalizing between discard confirmation and the trash-recording call):

Confirmed the mechanics: there's an isStale() guard right before the final chunk is sent (line ~1694), but none after it resolves successfully — so if doCancel() fires while that last chunk request is already in flight and it happens to succeed anyway, the finalize continuation proceeds to setUiState("complete") and navigates to /r/:id, overwriting the "idle" state doCancel() had just set.

I looked at adding an isStale() check after the final chunk succeeds to throw instead of finalizing, but that would make things worse: at that point the video is already fully saved server-side (the chunk was accepted), so throwing would route into the catch block and call /api/uploads/:id/abort, incorrectly marking a successfully completed upload as "failed" — destroying a real, fully-saved recording to satisfy a UI-state race. That's a worse outcome than the current behavior.

This is the same tradeoff already made deliberately for the live-recording path — see the existing comment on doCancel's pendingId trash call: "skipIfReady makes the trash a conditional no-op if the row is already 'ready' ... so a fully saved video is never silently discarded." The local-upload path (via trash-recording's own skipIfReady: true) follows the same principle: if the upload wins the race, we intentionally keep the video rather than delete completed work. The remaining rough edge is cosmetic — the UI can briefly show "complete"/navigate after an idle reset — not data loss.

Given a proper fix here would mean re-architecting how finalize and cancellation coordinate (e.g. checking the server-confirmed status before deciding UI state, rather than local staleness), which is a much larger change than this icon/confirmation-dialog PR, I'm leaving this as-is. The trade-off it exposes (favoring data preservation over exact UI-state adherence on a race that can only occur if the last chunk lands in the same tick as an explicit cancel) already exists in the pre-PR code for live recordings.

[builder-io-integration]

There was a problem with your request, please try again later. Error id: 1c7cb909776a4060afdc2f18f680ed07

[shawnmcclelland]

Solid improvements and the new discard dialog is clear and actionable.

[builder-io-integration]

There was a problem with your request, please try again later. Error id: 0ea85370d87e40769d6e2f21f1697f7c