From 6551d457757a3a671a8712245062927253892f5a Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Thu, 2 Jul 2026 06:54:16 -0700 Subject: [PATCH 1/2] fix: confirm before quitting when review comments/notes exist --- src/ui/App.tsx | 37 ++++ src/ui/AppHost.interactions.test.tsx | 197 +++++++++++++++++- .../components/chrome/QuitConfirmDialog.tsx | 74 +++++++ src/ui/hooks/useAppKeyboardShortcuts.ts | 32 +++ 4 files changed, 337 insertions(+), 3 deletions(-) create mode 100644 src/ui/components/chrome/QuitConfirmDialog.tsx diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 8b5b0985..0b9833bf 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -49,6 +49,9 @@ const LazyHelpDialog = lazy(async () => ({ const LazyMenuDropdown = lazy(async () => ({ default: (await import("./components/chrome/MenuDropdown")).MenuDropdown, })); +const LazyQuitConfirmDialog = lazy(async () => ({ + default: (await import("./components/chrome/QuitConfirmDialog")).QuitConfirmDialog, +})); const LazyThemeSelectorDialog = lazy(async () => ({ default: (await import("./components/chrome/ThemeSelectorDialog")).ThemeSelectorDialog, })); @@ -146,6 +149,7 @@ export function App({ const [forceSidebarOpen, setForceSidebarOpen] = useState(false); const [showHelp, setShowHelp] = useState(false); const [showAgentSkill, setShowAgentSkill] = useState(false); + const [quitConfirmOpen, setQuitConfirmOpen] = useState(false); const [focusArea, setFocusArea] = useState("files"); const [activeAddNoteTarget, setActiveAddNoteTarget] = useState(null); const [sidebarWidth, setSidebarWidth] = useState(34); @@ -182,6 +186,8 @@ export function App({ [activeTheme.id, themeOptions], ); const review = useReviewController({ files: bootstrap.changeset.files }); + const hasReviewWorkToLose = + review.reviewNoteCount > 0 || review.liveCommentCount > 0 || review.draftNote !== null; const filteredFiles = review.visibleFiles; const selectedFile = review.selectedFile; const selectedHunkIndex = review.selectedHunkIndex; @@ -646,9 +652,25 @@ export function App({ /** Leave the app through the shared shutdown path. */ const requestQuit = useCallback(() => { + if (hasReviewWorkToLose) { + setQuitConfirmOpen(true); + return; + } + + onQuit(); + }, [hasReviewWorkToLose, onQuit]); + + /** Confirm quitting after the user acknowledges review notes will be discarded. */ + const confirmQuit = useCallback(() => { + setQuitConfirmOpen(false); onQuit(); }, [onQuit]); + /** Keep the current review open after an accidental quit shortcut. */ + const cancelQuit = useCallback(() => { + setQuitConfirmOpen(false); + }, []); + /** Close the modal keyboard help overlay. */ const closeHelp = useCallback(() => { setShowHelp(false); @@ -824,7 +846,9 @@ export function App({ closeAgentSkill, closeHelp, closeMenu, + confirmQuit, acceptThemeSelector, + cancelQuit, cancelDraftNote, closeThemeSelector, focusArea, @@ -837,6 +861,7 @@ export function App({ openMenu, openThemeSelector, pagerMode, + quitConfirmOpen, requestQuit, scrollCodeHorizontally, saveDraftNote, @@ -1128,6 +1153,18 @@ export function App({ /> ) : null} + + {quitConfirmOpen ? ( + + + + ) : null} ); } diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index 8dba4a6e..922e1fef 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -148,6 +148,28 @@ function createBootstrap(initialMode: LayoutMode = "split", pager = false): AppB }); } +function createNoReviewBootstrap(initialMode: LayoutMode = "split", pager = false): AppBootstrap { + return createTestVcsAppBootstrap({ + changesetId: "changeset:app-no-review-work", + files: [ + createTestDiffFile( + "alpha", + "alpha.ts", + "export const alpha = 1;\n", + "export const alpha = 2;\nexport const add = true;\n", + ), + createTestDiffFile( + "beta", + "beta.ts", + "export const beta = 1;\n", + "export const betaValue = 1;\n", + ), + ], + initialMode, + pager, + }); +} + function createSingleFileBootstrap(): AppBootstrap { return createTestVcsAppBootstrap({ changesetId: "changeset:app-single-file", @@ -3356,10 +3378,10 @@ describe("App interactions", () => { } }); - test("quit shortcuts route through the provided onQuit handler in regular and pager modes", async () => { + test("quit shortcuts route through the provided onQuit handler without review notes", async () => { const regularQuit = mock(() => undefined); const regularSetup = await testRender( - , + , { width: 220, height: 24 }, ); @@ -3379,7 +3401,7 @@ describe("App interactions", () => { const pagerQuit = mock(() => undefined); const pagerSetup = await testRender( - , + , { width: 180, height: 20 }, ); @@ -3397,4 +3419,173 @@ describe("App interactions", () => { }); } }); + + test("quit confirmation protects saved review notes until confirmed", async () => { + const onQuit = mock(() => undefined); + const setup = await testRender( + , + { width: 220, height: 24, useKittyKeyboard: null }, + ); + + try { + await flush(setup); + + await act(async () => { + await setup.mockInput.typeText("c"); + }); + await flush(setup); + + await act(async () => { + await setup.mockInput.typeText("Keep this note."); + }); + await flush(setup); + + await act(async () => { + await setup.mockInput.pressKeys(["\u001b[115;5u"]); + }); + await flush(setup); + + await act(async () => { + await setup.mockInput.typeText("q"); + }); + let frame = await waitForFrame(setup, (nextFrame) => nextFrame.includes("Quit review?")); + + expect(onQuit).toHaveBeenCalledTimes(0); + expect(frame).toContain("comments or notes"); + + await act(async () => { + await setup.mockInput.typeText("t"); + }); + await flush(setup); + + frame = setup.captureCharFrame(); + expect(frame).toContain("Quit review?"); + expect(frame).not.toContain("Theme selector"); + + await act(async () => { + await setup.mockInput.typeText("n"); + }); + frame = await waitForFrame(setup, (nextFrame) => !nextFrame.includes("Quit review?")); + + expect(frame).not.toContain("Quit review?"); + expect(onQuit).toHaveBeenCalledTimes(0); + + await act(async () => { + await setup.mockInput.typeText("q"); + }); + await waitForFrame(setup, (nextFrame) => nextFrame.includes("Quit review?")); + + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flush(setup); + + expect(onQuit).toHaveBeenCalledTimes(1); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("quit confirmation protects live comments until confirmed", async () => { + const onQuit = mock(() => undefined); + const { dispatchCommand, hostClient } = createMockHostClient(); + const setup = await testRender( + , + { width: 220, height: 24 }, + ); + + try { + await flush(setup); + + await act(async () => { + await dispatchCommand({ + type: "command", + requestId: "quit-comment-1", + command: "comment", + input: { + sessionId: "session-1", + filePath: "alpha.ts", + side: "new", + line: 1, + summary: "Do not lose this live comment.", + reveal: true, + }, + }); + }); + + await waitForFrame(setup, (frame) => frame.includes("Do not lose this live comment.")); + + await act(async () => { + await setup.mockInput.typeText("q"); + }); + const frame = await waitForFrame(setup, (nextFrame) => nextFrame.includes("Quit review?")); + + expect(frame).toContain("Quit review?"); + expect(onQuit).toHaveBeenCalledTimes(0); + + await act(async () => { + await setup.mockInput.pressEnter(); + }); + await flush(setup); + + expect(onQuit).toHaveBeenCalledTimes(1); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("quit confirmation protects draft notes and treats Escape as gated quit when closed", async () => { + const onQuit = mock(() => undefined); + const setup = await testRender( + , + { width: 220, height: 24 }, + ); + + try { + await flush(setup); + + await act(async () => { + await setup.mockInput.typeText("c"); + }); + await flush(setup); + + await act(async () => { + await setup.mockMouse.click(6, 4); + }); + await flush(setup); + + await act(async () => { + await setup.mockInput.typeText("q"); + }); + let frame = await waitForFrame(setup, (nextFrame) => nextFrame.includes("Quit review?")); + + expect(frame).toContain("Quit review?"); + expect(onQuit).toHaveBeenCalledTimes(0); + + await act(async () => { + await setup.mockInput.pressEscape(); + }); + frame = await waitForFrame(setup, (nextFrame) => !nextFrame.includes("Quit review?")); + + expect(frame).not.toContain("Quit review?"); + expect(frame).toContain("Draft note"); + expect(onQuit).toHaveBeenCalledTimes(0); + + await act(async () => { + await setup.mockInput.pressEscape(); + }); + frame = await waitForFrame(setup, (nextFrame) => nextFrame.includes("Quit review?")); + + expect(frame).toContain("Quit review?"); + expect(onQuit).toHaveBeenCalledTimes(0); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); }); diff --git a/src/ui/components/chrome/QuitConfirmDialog.tsx b/src/ui/components/chrome/QuitConfirmDialog.tsx new file mode 100644 index 00000000..2965477b --- /dev/null +++ b/src/ui/components/chrome/QuitConfirmDialog.tsx @@ -0,0 +1,74 @@ +import type { MouseEvent as TuiMouseEvent } from "@opentui/core"; +import { fitText, padText } from "../../lib/text"; +import type { AppTheme } from "../../themes"; +import { ModalFrame } from "./ModalFrame"; + +/** Confirm before leaving a review that has comments or notes in memory. */ +export function QuitConfirmDialog({ + terminalHeight, + terminalWidth, + theme, + onCancel, + onConfirm, +}: { + terminalHeight: number; + terminalWidth: number; + theme: AppTheme; + onCancel: () => void; + onConfirm: () => void; +}) { + const width = Math.min(68, Math.max(48, terminalWidth - 8)); + const bodyWidth = Math.max(1, width - 4); + const modalHeight = Math.min(10, Math.max(8, terminalHeight - 2)); + const stayLabel = " Stay "; + const quitLabel = " Quit "; + + return ( + + + + + {fitText("This review has comments or notes that will be lost.", bodyWidth)} + + + + + {fitText("Press Enter or y to quit, Esc or n to stay.", bodyWidth)} + + + + + { + event.stopPropagation(); + onCancel(); + }} + > + {stayLabel} + + + {padText("", Math.max(1, bodyWidth - stayLabel.length - quitLabel.length - 1))} + + { + event.stopPropagation(); + onConfirm(); + }} + > + {quitLabel} + + + + + ); +} diff --git a/src/ui/hooks/useAppKeyboardShortcuts.ts b/src/ui/hooks/useAppKeyboardShortcuts.ts index 1f09de60..8ef8dce3 100644 --- a/src/ui/hooks/useAppKeyboardShortcuts.ts +++ b/src/ui/hooks/useAppKeyboardShortcuts.ts @@ -57,7 +57,9 @@ export interface UseAppKeyboardShortcutsOptions { closeAgentSkill: () => void; closeHelp: () => void; closeMenu: () => void; + confirmQuit: () => void; acceptThemeSelector: () => void; + cancelQuit: () => void; cancelDraftNote: () => void; closeThemeSelector: () => void; focusArea: FocusArea; @@ -70,6 +72,7 @@ export interface UseAppKeyboardShortcutsOptions { openMenu: (menuId: MenuId) => void; openThemeSelector: () => void; pagerMode: boolean; + quitConfirmOpen: boolean; requestQuit: () => void; scrollCodeHorizontally: (delta: number) => void; scrollDiff: (delta: number, unit: ScrollUnit) => void; @@ -101,7 +104,9 @@ export function useAppKeyboardShortcuts({ closeAgentSkill, closeHelp, closeMenu, + confirmQuit, acceptThemeSelector, + cancelQuit, cancelDraftNote, closeThemeSelector, focusArea, @@ -114,6 +119,7 @@ export function useAppKeyboardShortcuts({ openMenu, openThemeSelector, pagerMode, + quitConfirmOpen, requestQuit, scrollCodeHorizontally, saveDraftNote, @@ -139,6 +145,7 @@ export function useAppKeyboardShortcuts({ const activeMenuIdRef = useRef(activeMenuId); const focusAreaRef = useRef(focusArea); const pagerModeRef = useRef(pagerMode); + const quitConfirmOpenRef = useRef(quitConfirmOpen); const showAgentSkillRef = useRef(showAgentSkill); const showHelpRef = useRef(showHelp); const themeSelectorOpenRef = useRef(themeSelectorOpen); @@ -146,6 +153,7 @@ export function useAppKeyboardShortcuts({ activeMenuIdRef.current = activeMenuId; focusAreaRef.current = focusArea; pagerModeRef.current = pagerMode; + quitConfirmOpenRef.current = quitConfirmOpen; showAgentSkillRef.current = showAgentSkill; showHelpRef.current = showHelp; themeSelectorOpenRef.current = themeSelectorOpen; @@ -285,6 +293,26 @@ export function useAppKeyboardShortcuts({ return false; }; + const handleQuitConfirmShortcut = (key: KeyEvent) => { + if (!quitConfirmOpenRef.current) { + return false; + } + + consumeKey(key); + + if (key.name === "return" || key.name === "enter" || key.name === "y") { + confirmQuit(); + return true; + } + + if (isEscapeKey(key) || key.name === "n") { + cancelQuit(); + return true; + } + + return true; + }; + const handleThemeSelectorShortcut = (key: KeyEvent) => { if (!themeSelectorOpenRef.current) { return false; @@ -581,6 +609,10 @@ export function useAppKeyboardShortcuts({ }; useKeyboard((key: KeyEvent) => { + if (handleQuitConfirmShortcut(key)) { + return; + } + if (handleMenuToggleShortcut(key)) { return; } From 901cb58136684167c5de291eca6ce43267690275 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Thu, 2 Jul 2026 07:05:58 -0700 Subject: [PATCH 2/2] fix: gate quit confirmation on in-memory review work only --- src/ui/App.tsx | 2 +- src/ui/AppHost.interactions.test.tsx | 41 +++++++++++++++++++++++++++- src/ui/hooks/useReviewController.ts | 8 ++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 0b9833bf..5e1b6fbb 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -187,7 +187,7 @@ export function App({ ); const review = useReviewController({ files: bootstrap.changeset.files }); const hasReviewWorkToLose = - review.reviewNoteCount > 0 || review.liveCommentCount > 0 || review.draftNote !== null; + review.userNoteCount > 0 || review.liveCommentCount > 0 || review.draftNote !== null; const filteredFiles = review.visibleFiles; const selectedFile = review.selectedFile; const selectedHunkIndex = review.selectedHunkIndex; diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index 922e1fef..b56ad844 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -170,6 +170,21 @@ function createNoReviewBootstrap(initialMode: LayoutMode = "split", pager = fals }); } +function createAgentSidecarOnlyBootstrap(): AppBootstrap { + return createTestVcsAppBootstrap({ + changesetId: "changeset:app-agent-sidecar-only", + files: [ + createTestDiffFile( + "alpha", + "alpha.ts", + "export const alpha = 1;\n", + "export const alpha = 2;\nexport const add = true;\n", + true, + ), + ], + }); +} + function createSingleFileBootstrap(): AppBootstrap { return createTestVcsAppBootstrap({ changesetId: "changeset:app-single-file", @@ -3420,7 +3435,31 @@ describe("App interactions", () => { } }); - test("quit confirmation protects saved review notes until confirmed", async () => { + test("quit shortcut ignores persisted agent sidecar annotations", async () => { + const qQuit = mock(() => undefined); + const qSetup = await testRender( + , + { width: 220, height: 24 }, + ); + + try { + await flush(qSetup); + + await act(async () => { + await qSetup.mockInput.typeText("q"); + }); + await flush(qSetup); + + expect(qQuit).toHaveBeenCalledTimes(1); + expect(qSetup.captureCharFrame()).not.toContain("Quit review?"); + } finally { + await act(async () => { + qSetup.renderer.destroy(); + }); + } + }); + + test("quit confirmation protects saved user notes until confirmed", async () => { const onQuit = mock(() => undefined); const setup = await testRender( , diff --git a/src/ui/hooks/useReviewController.ts b/src/ui/hooks/useReviewController.ts index 6e14f203..3e9d5eda 100644 --- a/src/ui/hooks/useReviewController.ts +++ b/src/ui/hooks/useReviewController.ts @@ -135,6 +135,7 @@ export interface ReviewController { liveCommentsByFileId: Record; reviewNoteCount: number; reviewNoteSummaries: SessionReviewNoteSummary[]; + userNoteCount: number; userNotesByFileId: Record; moveToAnnotatedFile: (delta: number) => void; moveToAnnotatedHunk: (delta: number) => void; @@ -908,6 +909,12 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon [liveCommentsByFileId], ); + /** Count only human-authored notes created during this in-memory review session. */ + const userNoteCount = useMemo( + () => Object.values(userNotesByFileId).reduce((sum, notes) => sum + notes.length, 0), + [userNotesByFileId], + ); + /** Format current inline notes for daemon snapshots without exposing UI-only objects. */ const reviewNoteSummaries = useMemo(() => { const noteSummaries: SessionReviewNoteSummary[] = []; @@ -996,6 +1003,7 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon liveCommentsByFileId, reviewNoteCount, reviewNoteSummaries, + userNoteCount, userNotesByFileId, scrollToNote, selectedFile,