Skip to content

fix: confirm before quitting when unsaved review work would be lost #504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mvanhorn wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: confirm before quitting when unsaved review work would be lost #504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6551d45
fix: confirm before quitting when review comments/notes exist
mvanhorn Jul 2, 2026
901cb58
fix: gate quit confirmation on in-memory review work only
mvanhorn Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/ui/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}));
Expand Down Expand Up @@ -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<FocusArea>("files");
const [activeAddNoteTarget, setActiveAddNoteTarget] = useState<ActiveAddNoteTarget | null>(null);
const [sidebarWidth, setSidebarWidth] = useState(34);
Expand Down Expand Up @@ -182,6 +186,8 @@ export function App({
[activeTheme.id, themeOptions],
);
const review = useReviewController({ files: bootstrap.changeset.files });
const hasReviewWorkToLose =
review.userNoteCount > 0 || review.liveCommentCount > 0 || review.draftNote !== null;
const filteredFiles = review.visibleFiles;
const selectedFile = review.selectedFile;
const selectedHunkIndex = review.selectedHunkIndex;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -824,7 +846,9 @@ export function App({
closeAgentSkill,
closeHelp,
closeMenu,
confirmQuit,
acceptThemeSelector,
cancelQuit,
cancelDraftNote,
closeThemeSelector,
focusArea,
Expand All @@ -837,6 +861,7 @@ export function App({
openMenu,
openThemeSelector,
pagerMode,
quitConfirmOpen,
requestQuit,
scrollCodeHorizontally,
saveDraftNote,
Expand Down Expand Up @@ -1128,6 +1153,18 @@ export function App({
/>
</Suspense>
) : null}

{quitConfirmOpen ? (
<Suspense fallback={null}>
<LazyQuitConfirmDialog
terminalHeight={terminal.height}
terminalWidth={terminal.width}
theme={baseTheme}
onCancel={cancelQuit}
onConfirm={confirmQuit}
/>
</Suspense>
) : null}
</box>
);
}
236 changes: 233 additions & 3 deletions src/ui/AppHost.interactions.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,43 @@ 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 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",
Expand Down Expand Up @@ -3356,10 +3393,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(
<AppHost bootstrap={createBootstrap()} onQuit={regularQuit} />,
<AppHost bootstrap={createNoReviewBootstrap()} onQuit={regularQuit} />,
{ width: 220, height: 24 },
);

Expand All @@ -3379,7 +3416,7 @@ describe("App interactions", () => {

const pagerQuit = mock(() => undefined);
const pagerSetup = await testRender(
<AppHost bootstrap={createBootstrap("auto", true)} onQuit={pagerQuit} />,
<AppHost bootstrap={createNoReviewBootstrap("auto", true)} onQuit={pagerQuit} />,
{ width: 180, height: 20 },
);

Expand All @@ -3397,4 +3434,197 @@ describe("App interactions", () => {
});
}
});

test("quit shortcut ignores persisted agent sidecar annotations", async () => {
const qQuit = mock(() => undefined);
const qSetup = await testRender(
<AppHost bootstrap={createAgentSidecarOnlyBootstrap()} onQuit={qQuit} />,
{ 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(
<AppHost bootstrap={createNoReviewBootstrap()} onQuit={onQuit} />,
{ 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(
<AppHost bootstrap={createNoReviewBootstrap()} hostClient={hostClient} onQuit={onQuit} />,
{ 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(
<AppHost bootstrap={createNoReviewBootstrap()} onQuit={onQuit} />,
{ 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();
});
}
});
});
Loading
Loading