From 4b86fe82928aa5335828f68a0754153f084bb151 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 19 May 2026 14:01:04 -0400 Subject: [PATCH] Use app-styled confirmations for PR actions (#2324) Co-authored-by: Orca --- src/renderer/src/App.tsx | 375 +++++++++--------- .../src/components/GitHubItemDialog.tsx | 20 +- src/renderer/src/components/TaskPage.tsx | 14 +- .../src/components/confirmation-dialog.tsx | 114 ++++++ .../right-sidebar/useFileDeletion.ts | 21 +- .../src/components/settings/Settings.tsx | 54 ++- 6 files changed, 391 insertions(+), 207 deletions(-) create mode 100644 src/renderer/src/components/confirmation-dialog.tsx diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index eee045cc5..d6a2698a3 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -58,6 +58,7 @@ import { import { TOGGLE_FLOATING_TERMINAL_EVENT } from '@/lib/floating-terminal' import { DictationController } from './components/dictation/DictationController' import { CrashReportDialog } from './components/crash-report/CrashReportDialog' +import { ConfirmationDialogProvider } from './components/confirmation-dialog' import RecentTabSwitcher from './components/tab-bar/RecentTabSwitcher' import { useGitStatusPolling } from './components/right-sidebar/useGitStatusPolling' import { useEditorExternalWatch } from './hooks/useEditorExternalWatch' @@ -1351,66 +1352,67 @@ function App(): React.JSX.Element { } > - {/* Why: leaf-mounted retention sync keeps agent-status retention + + {/* Why: leaf-mounted retention sync keeps agent-status retention subscriptions from re-rendering the App tree. */} - -
- {/* Why: the non-workspace titlebar lives inside this left+center + +
+ {/* Why: the non-workspace titlebar lives inside this left+center wrapper so it does not span over the right-sidebar column — when the right sidebar is open, its own header anchors at the top alongside the titlebar instead of being pushed below it. */} -
- {/* Why: in workspace view (split groups always enabled), the +
+ {/* Why: in workspace view (split groups always enabled), the full-width titlebar is removed so tab groups + terminal extend to the top of the window. Left titlebar controls move to a header above the sidebar. Settings, landing, and the tasks page keep the titlebar. */} - {!workspaceActive ? ( -
-
- {titlebarLeftControls} -
- {activeView === 'activity' ? ( - - ) : ( + {!workspaceActive ? ( +
- )} - {showTitlebarExpandButton && ( - - - - - - Collapse pane - - - )} - {/* Why: when the right sidebar is open, its own header renders + className={`flex items-center${showSidebar && sidebarOpen ? ' overflow-hidden shrink-0' : ' shrink-0 mr-2'}`} + style={{ width: showSidebar && sidebarOpen ? sidebarWidth : undefined }} + > + {titlebarLeftControls} +
+ {activeView === 'activity' ? ( + + ) : ( +
+ )} + {showTitlebarExpandButton && ( + + + + + + Collapse pane + + + )} + {/* Why: when the right sidebar is open, its own header renders an identical close button — hide this copy so only one is visible at a time. */} - {!rightSidebarOpen && rightSidebarToggle} - {/* Why: reserve space so content is not obscured by the + {!rightSidebarOpen && rightSidebarToggle} + {/* Why: reserve space so content is not obscured by the fixed-position window-controls overlay on Windows. */} - {isWindows &&
} -
- ) : null} -
- {showSidebar ? ( - workspaceActive ? ( - /* Why: left column wraps the sidebar with a titlebar-height + {isWindows &&
} +
+ ) : null} +
+ {showSidebar ? ( + workspaceActive ? ( + /* Why: left column wraps the sidebar with a titlebar-height header above it. The header holds the same controls (traffic lights, sidebar toggle, "Orca" title, agent badge) that the full-width titlebar held while the center and right @@ -1418,173 +1420,176 @@ function App(): React.JSX.Element { When the sidebar is collapsed, take this header out of flex layout so the terminal/editor reclaim the left edge instead of leaving behind a content-width blank strip. */ -
- {titlebarLeftControls} -
-
- {/* Why: the workspace-view wrapper adds a fixed 36px header +
+ {titlebarLeftControls} +
+
+ {/* Why: the workspace-view wrapper adds a fixed 36px header above the sidebar. Without a flex-1/min-h-0 slot here, the sidebar falls back to its content height, so the worktree list loses its scroll viewport and the fixed bottom toolbar (including Add Project) gets pushed offscreen. */} - + +
-
- ) : ( - - ) - ) : null} -
- {/* Why: right sidebar toggle floats at the top-right of the center + ) : ( + + ) + ) : null} +
+ {/* Why: right sidebar toggle floats at the top-right of the center column so it's always accessible whether the right sidebar is open or closed. Match the RightSidebar header's 36px height and top-0 anchor so the icon's vertical center is identical between open and closed states — otherwise toggling makes the icon jump a few pixels, which reads as layout jitter. */} - {workspaceActive && !rightSidebarOpen && ( -
- {rightSidebarToggle} + {workspaceActive && !rightSidebarOpen && ( +
+ {rightSidebarToggle} +
+ )} +
+
+ +
+ + {activeView === 'settings' ? : null} + {activeView === 'skills' ? : null} + {activeView === 'tasks' ? : null} + {activeView === 'automations' ? : null} + {activeView === 'activity' ? : null} + {activeView === 'space' ? : null} + {activeView === 'terminal' && !activeWorktreeId ? : null} +
- )} -
-
- -
- - {activeView === 'settings' ? : null} - {activeView === 'skills' ? : null} - {activeView === 'tasks' ? : null} - {activeView === 'automations' ? : null} - {activeView === 'activity' ? : null} - {activeView === 'space' ? : null} - {activeView === 'terminal' && !activeWorktreeId ? : null} - + {showFloatingTerminalButton ? ( + setFloatingTerminalOpenWithFocus((open) => !open)} + /> + ) : null}
- {showFloatingTerminalButton ? ( - setFloatingTerminalOpenWithFocus((open) => !open)} - /> - ) : null}
-
- {/* Why: keep RightSidebar mounted even when closed so that its + {/* Why: keep RightSidebar mounted even when closed so that its child components (FileExplorer, SourceControl, etc.) and their filesystem watchers + cached directory trees survive across open/close toggles. Unmount on the tasks view since that surface is intentionally distraction-free. */} - {showRightSidebarControls ? : null} -
- {floatingTerminalEnabled ? ( - - ) : null} - - {/* Why: root overlays can render Radix s; keep them inside + {showRightSidebarControls ? : null} +
+ {floatingTerminalEnabled ? ( + + ) : null} + + {/* Why: root overlays can render Radix s; keep them inside the shared provider so lazy surfaces mount safely from any entry point. */} - - {mountedLazyModalIds.has('new-workspace-composer') ? : null} - {mountedLazyModalIds.has('workspace-cleanup') ? : null} - - - {mountedLazyModalIds.has('quick-open') ? : null} - {mountedLazyModalIds.has('worktree-palette') ? : null} - {mountedLazyModalIds.has('feature-wall') ? : null} - {mountedLazyModalIds.has('feature-tips') ? : null} - - {/* Why: mount PetOverlay only when the experimental flag is on AND + + {mountedLazyModalIds.has('new-workspace-composer') ? ( + + ) : null} + {mountedLazyModalIds.has('workspace-cleanup') ? : null} + + + {mountedLazyModalIds.has('quick-open') ? : null} + {mountedLazyModalIds.has('worktree-palette') ? : null} + {mountedLazyModalIds.has('feature-wall') ? : null} + {mountedLazyModalIds.has('feature-tips') ? : null} + + {/* Why: mount PetOverlay only when the experimental flag is on AND the user hasn't hit "Hide pet" in the status-bar menu. Both conditions must be true — see design doc (pet-overlay.md) on why the two toggles are kept independent. */} - {petEnabled && petVisible ? ( - - - - ) : null} - - - - {/* Why: the existing-user opt-in banner mounts at App root so it + {petEnabled && petVisible ? ( + + + + ) : null} + + + + {/* Why: the existing-user opt-in banner mounts at App root so it renders once per renderer session, not per view. It gates internally on the cohort markers populated by the migration, so it only shows for users who installed before the telemetry release and have not yet resolved consent. New users get no first-launch surface — see telemetry-plan.md §First-launch experience. */} - - - - - - {onboarding && shouldShowOnboarding(onboarding) && !onboardingSettingsDetour ? ( - - - - ) : null} - - + + + + + + {onboarding && shouldShowOnboarding(onboarding) && !onboardingSettingsDetour ? ( + + + + ) : null} + + + {/* Why: rendered last so it sits after all -webkit-app-region:drag elements diff --git a/src/renderer/src/components/GitHubItemDialog.tsx b/src/renderer/src/components/GitHubItemDialog.tsx index a0635b7fa..9221e82b5 100644 --- a/src/renderer/src/components/GitHubItemDialog.tsx +++ b/src/renderer/src/components/GitHubItemDialog.tsx @@ -41,6 +41,7 @@ import { toast } from 'sonner' import { Button } from '@/components/ui/button' import { ButtonGroup } from '@/components/ui/button-group' import { Input } from '@/components/ui/input' +import { useConfirmationDialog } from '@/components/confirmation-dialog' import { Sheet, SheetContent, SheetDescription, SheetTitle } from '@/components/ui/sheet' import { VisuallyHidden } from 'radix-ui' import { @@ -2101,6 +2102,7 @@ function PRActionsPanel({ const [mergePending, setMergePending] = useState(false) const patchWorkItem = useAppStore((s) => s.patchWorkItem) const patchProjectRowContent = useAppStore((s) => s.patchProjectRowContent) + const confirm = useConfirmationDialog() const actionItem = { ...item, state: localState } const canMutateState = localState !== 'merged' && (!!repoPath || !!projectOrigin) const nextState: 'open' | 'closed' = localState === 'closed' ? 'open' : 'closed' @@ -2135,7 +2137,16 @@ function PRActionsPanel({ return } const label = nextState === 'closed' ? 'Close' : 'Reopen' - if (!window.confirm(`${label} PR #${item.number}?`)) { + const confirmed = await confirm({ + title: `${label} PR #${item.number}?`, + description: + nextState === 'closed' + ? 'This will close the pull request on GitHub.' + : 'This will reopen the pull request on GitHub.', + confirmLabel: label, + confirmVariant: nextState === 'closed' ? 'destructive' : 'default' + }) + if (!confirmed) { return } const previousState = localState @@ -2165,7 +2176,12 @@ function PRActionsPanel({ } const label = method === 'squash' ? 'Squash and merge' : method === 'rebase' ? 'Rebase and merge' : 'Merge' - if (!window.confirm(`${label} PR #${item.number}?`)) { + const confirmed = await confirm({ + title: `${label} PR #${item.number}?`, + description: 'This will update the pull request on GitHub.', + confirmLabel: label + }) + if (!confirmed) { return } setMergePending(true) diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index 5008dcb7c..1ade83fb1 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -76,6 +76,7 @@ import RepoDotLabel from '@/components/repo/RepoDotLabel' import IssueSourceIndicator, { sameGitHubOwnerRepo } from '@/components/github/IssueSourceIndicator' import IssueSourceSelector, { issueSourceChipClass } from '@/components/github/IssueSourceSelector' import { reconcileLinearTeamSelection } from '@/components/task-page-linear-team-selection' +import { useConfirmationDialog } from '@/components/confirmation-dialog' import { getGitHubPRPrimaryReviewer, getGitHubPRReviewLabel, @@ -1411,6 +1412,7 @@ function PRMergeCell({ onRefresh: () => void }): React.JSX.Element { const [merging, setMerging] = useState(false) + const confirm = useConfirmationDialog() if (item.type !== 'pr') { return Issue } @@ -1425,11 +1427,13 @@ function PRMergeCell({ if (!repo || mergeDisabled) { return } - const confirmed = window.confirm( - method === 'squash' - ? `Squash and merge PR #${item.number}?` - : `${method === 'rebase' ? 'Rebase and merge' : 'Merge'} PR #${item.number}?` - ) + const label = + method === 'squash' ? 'Squash and merge' : method === 'rebase' ? 'Rebase and merge' : 'Merge' + const confirmed = await confirm({ + title: `${label} PR #${item.number}?`, + description: 'This will update the pull request on GitHub.', + confirmLabel: label + }) if (!confirmed) { return } diff --git a/src/renderer/src/components/confirmation-dialog.tsx b/src/renderer/src/components/confirmation-dialog.tsx new file mode 100644 index 000000000..833ad7e76 --- /dev/null +++ b/src/renderer/src/components/confirmation-dialog.tsx @@ -0,0 +1,114 @@ +import React, { createContext, useCallback, useContext, useEffect, useRef, useState } from 'react' + +import { Button } from '@/components/ui/button' +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle +} from '@/components/ui/dialog' + +type ConfirmationDialogOptions = { + title: string + description?: string + confirmLabel?: string + cancelLabel?: string + confirmVariant?: 'default' | 'destructive' +} + +type ConfirmationDialogRequest = { + id: number + options: ConfirmationDialogOptions + resolve: (confirmed: boolean) => void +} + +type ConfirmationDialogContextValue = (options: ConfirmationDialogOptions) => Promise + +const ConfirmationDialogContext = createContext(null) + +export function ConfirmationDialogProvider({ + children +}: { + children: React.ReactNode +}): React.JSX.Element { + const nextIdRef = useRef(0) + const [queue, setQueue] = useState([]) + const activeRequest = queue[0] ?? null + const [renderedRequest, setRenderedRequest] = useState(null) + const activeRequestRef = useRef(activeRequest) + const displayedRequest = activeRequest ?? renderedRequest + + useEffect(() => { + activeRequestRef.current = activeRequest + if (activeRequest) { + setRenderedRequest(activeRequest) + } + }, [activeRequest]) + + const confirm = useCallback((options) => { + return new Promise((resolve) => { + const request: ConfirmationDialogRequest = { + id: nextIdRef.current, + options, + resolve + } + nextIdRef.current += 1 + setQueue((currentQueue) => [...currentQueue, request]) + }) + }, []) + + const settleActiveRequest = useCallback((confirmed: boolean) => { + const request = activeRequestRef.current + if (!request) { + return + } + request.resolve(confirmed) + setQueue((currentQueue) => { + if (currentQueue[0]?.id === request.id) { + return currentQueue.slice(1) + } + return currentQueue.filter((queuedRequest) => queuedRequest.id !== request.id) + }) + }, []) + + return ( + + {children} + !open && settleActiveRequest(false)} + > + + + {displayedRequest?.options.title} + {displayedRequest?.options.description ? ( + {displayedRequest.options.description} + ) : null} + + + + + + + + + ) +} + +export function useConfirmationDialog(): ConfirmationDialogContextValue { + const confirm = useContext(ConfirmationDialogContext) + if (!confirm) { + throw new Error('useConfirmationDialog must be used inside ConfirmationDialogProvider') + } + return confirm +} diff --git a/src/renderer/src/components/right-sidebar/useFileDeletion.ts b/src/renderer/src/components/right-sidebar/useFileDeletion.ts index 6aeb95dd2..49c15f462 100644 --- a/src/renderer/src/components/right-sidebar/useFileDeletion.ts +++ b/src/renderer/src/components/right-sidebar/useFileDeletion.ts @@ -2,6 +2,7 @@ import { useCallback, useMemo, useRef } from 'react' import type { Dispatch, SetStateAction } from 'react' import { toast } from 'sonner' import { useAppStore } from '@/store' +import { useConfirmationDialog } from '@/components/confirmation-dialog' import { dirname } from '@/lib/path' import { getConnectionId } from '@/lib/connection-context' import { findWorktreeById } from '@/store/slices/worktree-helpers' @@ -49,6 +50,7 @@ export function useFileDeletion({ isMac, isWindows }: UseFileDeletionParams): UseFileDeletionResult { + const confirm = useConfirmationDialog() // Why: track in-flight deletes per-path so repeated Del presses on the same // node don't issue duplicate IPC calls; the map is a ref to avoid re-renders. const inFlightRef = useRef>(new Set()) @@ -82,7 +84,13 @@ export function useFileDeletion({ const message = node.isDirectory ? `Permanently delete '${node.name}' and all its contents? This cannot be undone.` : `Permanently delete '${node.name}'? This cannot be undone.` - if (!window.confirm(message)) { + const confirmed = await confirm({ + title: `Permanently delete '${node.name}'?`, + description: message, + confirmLabel: 'Delete', + confirmVariant: 'destructive' + }) + if (!confirmed) { inFlightRef.current.delete(node.path) return } @@ -193,7 +201,16 @@ export function useFileDeletion({ inFlightRef.current.delete(node.path) } }, - [activeWorktreeId, closeFile, isWindows, openFiles, refreshDir, selectedPath, setSelectedPath] + [ + activeWorktreeId, + closeFile, + confirm, + isWindows, + openFiles, + refreshDir, + selectedPath, + setSelectedPath + ] ) const requestDelete = useCallback( diff --git a/src/renderer/src/components/settings/Settings.tsx b/src/renderer/src/components/settings/Settings.tsx index 5f66a1cab..6a9830ca9 100644 --- a/src/renderer/src/components/settings/Settings.tsx +++ b/src/renderer/src/components/settings/Settings.tsx @@ -31,6 +31,7 @@ import { useAppStore } from '../../store' import { useSystemPrefersDark } from '@/components/terminal-pane/use-system-prefers-dark' import { isMacUserAgent, isWindowsUserAgent } from '@/components/terminal-pane/pane-helpers' import { applyDocumentTheme } from '@/lib/document-theme' +import { useConfirmationDialog } from '@/components/confirmation-dialog' import { SCROLLBACK_PRESETS_MB, getFallbackTerminalFonts } from './SettingsConstants' import { DEFAULT_APP_FONT_FAMILY } from '../../../../shared/constants' import { GeneralPane, GENERAL_PANE_SEARCH_ENTRIES } from './GeneralPane' @@ -277,6 +278,7 @@ function Settings(): React.JSX.Element { const [pendingNavRequestTick, setPendingNavRequestTick] = useState(0) const [hasUnsavedCommitPromptChanges, setHasUnsavedCommitPromptChanges] = useState(false) const [commitPromptDiscardSignal, setCommitPromptDiscardSignal] = useState(0) + const confirm = useConfirmationDialog() // Why: the hidden-experimental group is an unlock — Shift-clicking the // Experimental sidebar entry reveals it for the remainder of the session. // Not persisted on purpose: it's a power-user affordance we don't want to @@ -291,22 +293,25 @@ function Settings(): React.JSX.Element { const repoHooksRequestSeqRef = useRef(0) const repoHooksRuntimeIdentityRef = useRef('local') - const confirmDiscardCommitPromptChanges = useCallback((): boolean => { + const confirmDiscardCommitPromptChanges = useCallback(async (): Promise => { if (!hasUnsavedCommitPromptChanges) { return true } - const shouldDiscard = window.confirm( - 'You have unsaved AI commit prompt changes. Leave without saving?' - ) + const shouldDiscard = await confirm({ + title: 'Discard unsaved commit prompt changes?', + description: 'You have unsaved AI commit prompt changes. Leaving will discard them.', + confirmLabel: 'Discard', + confirmVariant: 'destructive' + }) if (shouldDiscard) { setCommitPromptDiscardSignal((signal) => signal + 1) setHasUnsavedCommitPromptChanges(false) } return shouldDiscard - }, [hasUnsavedCommitPromptChanges]) + }, [confirm, hasUnsavedCommitPromptChanges]) - const closeSettingsPageWithPromptGuard = useCallback((): void => { - if (!confirmDiscardCommitPromptChanges()) { + const closeSettingsPageWithPromptGuard = useCallback(async (): Promise => { + if (!(await confirmDiscardCommitPromptChanges())) { return } closeSettingsPage() @@ -319,10 +324,33 @@ function Settings(): React.JSX.Element { const runtimeTargetIdentity = getRuntimeTargetIdentity(settings) useEffect(() => { + const hasVisibleOverlay = (): boolean => + Array.from( + document.querySelectorAll('[role="dialog"], [role="listbox"], [role="menu"]') + ).some((element) => { + if (!(element instanceof HTMLElement)) { + return false + } + if (element.closest('[aria-hidden="true"]')) { + return false + } + const style = window.getComputedStyle(element) + return ( + style.display !== 'none' && + style.visibility !== 'hidden' && + element.getClientRects().length > 0 + ) + }) + const handleKeyDown = (event: KeyboardEvent): void => { if (event.key !== 'Escape' || event.defaultPrevented) { return } + // Why: nested dialogs and menus own Escape before Settings page-level + // navigation, including the unsaved commit prompt confirmation dialog. + if (hasVisibleOverlay()) { + return + } // Why: Escape in an editable control usually means "cancel this edit", // not "close Settings". Closing the entire page would discard the user's // in-progress typing. Defer to the field's own handler when focus is on @@ -331,7 +359,7 @@ function Settings(): React.JSX.Element { if (isEditableTarget(event.target)) { return } - closeSettingsPageWithPromptGuard() + void closeSettingsPageWithPromptGuard() } document.addEventListener('keydown', handleKeyDown) @@ -949,11 +977,11 @@ function Settings(): React.JSX.Element { }, [visibleNavSections]) const scrollToSection = useCallback( - ( + async ( sectionId: string, modifiers?: { metaKey: boolean; ctrlKey: boolean; shiftKey: boolean; altKey: boolean } - ) => { - if (sectionId !== activeSectionId && !confirmDiscardCommitPromptChanges()) { + ): Promise => { + if (sectionId !== activeSectionId && !(await confirmDiscardCommitPromptChanges())) { return } // Why: Shift-clicking the Experimental sidebar entry unlocks a hidden @@ -971,8 +999,8 @@ function Settings(): React.JSX.Element { [activeSectionId, confirmDiscardCommitPromptChanges] ) - const openComputerUseFromBrowser = useCallback(() => { - if (!confirmDiscardCommitPromptChanges()) { + const openComputerUseFromBrowser = useCallback(async () => { + if (!(await confirmDiscardCommitPromptChanges())) { return } pendingNavSectionRef.current = 'computer-use'