Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
40 changes: 36 additions & 4 deletions apps/desktop/src/components/ProjectDeleteDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,40 @@ import { useTranslation } from "react-i18next";
import { ErrorCodes } from "@pi-desktop/shared";
import { useAppStore } from "../stores/app-store";
import { Button, TooltipButton } from "./ui";
import { IconCircleAlert, IconClose, IconTrash } from "./icons";
import { IconCircleAlert, IconClose, IconStop, IconTrash } from "./icons";

/**
* Second confirmation for deleting a project. The store action removes the
* project record and its stored sessions; the folder on disk is never touched.
*/
export function ProjectDeleteDialog({
project,
runningSessionIds,
onClose,
onDeleted,
onError,
}: {
project: { name: string; path: string; sessionCount: number };
/**
* Sessions of this project whose turn is still live. The host refuses the
* bulk delete (and the single session delete) while one exists, so the dialog
* names them and stops them as the explicit step its confirm label promises.
*/
runningSessionIds: string[];
onClose: () => void;
onDeleted: () => void | Promise<void>;
onError: (error: unknown) => void;
}) {
const { t } = useTranslation();
const deleteProject = useAppStore((s) => s.deleteProject);
const abortSession = useAppStore((s) => s.abortSession);
const [busy, setBusy] = useState(false);
const busyRef = useRef(false);
const dialogRef = useRef<HTMLDivElement | null>(null);
// The surfaces that own this dialog subscribe to `runningSessions`, so a turn
// that starts or finishes while the dialog is open is reflected here before
// the user confirms.
const runningCount = runningSessionIds.length;

useEffect(() => {
const previouslyFocused = document.activeElement as HTMLElement | null;
Expand Down Expand Up @@ -68,11 +80,19 @@ export function ProjectDeleteDialog({
busyRef.current = true;
setBusy(true);
try {
// A live turn still owns its session's tools and transcript, so the host
// refuses the bulk delete until every attached session is idle. Stopping
// the listed sessions is the step the confirm button names; a turn that
// starts after this loop still makes the host refuse with CONFLICT below,
// which is why the refusal path stays reachable.
for (const sessionId of runningSessionIds) {
await abortSession(sessionId);
}
await deleteProject(project.path);
Comment on lines +88 to 91
await onDeleted();
} catch (error) {
// The host refuses the delete while a task of this project is running;
// show the same localized explanation the menu guard uses.
// show the same localized explanation the menu guard used to show.
if ((error as { errorCode?: unknown } | null)?.errorCode === ErrorCodes.CONFLICT) {
onError(new Error(t("project.deleteRunningBlocked")));
return;
Expand All @@ -98,7 +118,9 @@ export function ProjectDeleteDialog({
role="dialog"
aria-modal="true"
aria-labelledby="project-delete-dialog-title"
aria-describedby="project-delete-dialog-description project-delete-dialog-sessions project-delete-dialog-folder-kept"
aria-describedby={`project-delete-dialog-description project-delete-dialog-sessions project-delete-dialog-folder-kept${
runningCount > 0 ? " project-delete-dialog-running" : ""
}`}
tabIndex={-1}
onClick={(event) => event.stopPropagation()}
>
Expand Down Expand Up @@ -129,6 +151,12 @@ export function ProjectDeleteDialog({
<IconTrash size={14} aria-hidden />
<span>{t("project.deleteSessions", { count: project.sessionCount })}</span>
</p>
{runningCount > 0 ? (
<p id="project-delete-dialog-running" className="project-delete-dialog-running">
<IconStop size={14} aria-hidden />
<span>{t("project.deleteRunning", { count: runningCount })}</span>
</p>
) : null}
<p id="project-delete-dialog-folder-kept" className="project-memory-dialog-hint">
{t("project.deleteFolderKept")}
</p>
Expand All @@ -144,7 +172,11 @@ export function ProjectDeleteDialog({
disabled={busy}
onClick={() => void confirm()}
>
{busy ? t("project.deleting") : t("project.deleteConfirm")}
{busy
? t("project.deleting")
: runningCount > 0
? t("project.deleteRunningConfirm")
: t("project.deleteConfirm")}
</Button>
</div>
</div>
Expand Down
12 changes: 5 additions & 7 deletions apps/desktop/src/components/Sidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1976,14 +1976,9 @@ export function Sidebar({
className="danger"
data-action="delete-project"
onClick={() => {
// Never refuse silently: the dialog names the running sessions
// and asks for an explicit confirmation before it stops them.
closeMenus(false);
const runningCount = entry.sessions.filter(
(session) => runningSessions[session.id] === true,
).length;
if (runningCount > 0) {
showToast(t("project.deleteRunningBlocked"), { variant: "warning" });
return;
}
setDeleteProjectFor(entry);
}}
>
Expand Down Expand Up @@ -2290,6 +2285,9 @@ export function Sidebar({
path: deleteProjectFor.path,
sessionCount: deleteProjectFor.sessions.length,
}}
runningSessionIds={deleteProjectFor.sessions
.filter((session) => runningSessions[session.id] === true)
.map((session) => session.id)}
onClose={() => setDeleteProjectFor(null)}
onDeleted={() => {
setDeleteProjectFor(null);
Expand Down
25 changes: 14 additions & 11 deletions apps/desktop/src/pages/ProjectsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,13 @@ export function ProjectsPage() {
roots?: ProjectGroupRecord["roots"];
legacy?: boolean;
} | null>(null);
// `roots` rides along so the dialog can keep deriving the project's live
// running sessions while it is open, instead of a snapshot taken on click.
const [deleteFor, setDeleteFor] = useState<{
name: string;
path: string;
sessionCount: number;
roots: ProjectGroupRecord["roots"];
} | null>(null);
const searchRef = useRef<HTMLInputElement | null>(null);
const [instructionsFor, setInstructionsFor] = useState<{
Expand Down Expand Up @@ -814,21 +817,14 @@ export function ProjectsPage() {
data-action="delete-project"
onClick={() => {
setMenuFor(null);
const runningCount = sessions.filter(
(session) =>
sessionMatchesIndexProject(session, project) &&
runningSessions[session.id] === true,
).length;
if (runningCount > 0) {
showToast(t("project.deleteRunningBlocked"), {
variant: "warning",
});
return;
}
// Never refuse silently: the dialog names the
// running sessions and asks for an explicit
// confirmation before it stops them.
setDeleteFor({
name: project.name,
path: project.path,
sessionCount: totalSessions,
roots: project.roots,
});
}}
>
Expand Down Expand Up @@ -1021,6 +1017,13 @@ export function ProjectsPage() {
{deleteFor ? (
<ProjectDeleteDialog
project={deleteFor}
runningSessionIds={sessions
.filter(
(session) =>
sessionMatchesIndexProject(session, deleteFor) &&
runningSessions[session.id] === true,
)
.map((session) => session.id)}
onClose={() => setDeleteFor(null)}
onDeleted={() => {
setDeleteFor(null);
Expand Down
19 changes: 19 additions & 0 deletions apps/desktop/src/styles/projects.css
Original file line number Diff line number Diff line change
Expand Up @@ -777,6 +777,25 @@
margin-top: 2px;
}

/*
The running-session line is a "these will be stopped" notice, not the
destructive warning above it, so it carries the warning tone.
*/
.project-delete-dialog-running {
display: flex;
align-items: flex-start;
gap: 8px;
margin: 0;
color: var(--ds-warning);
font-size: var(--text-sm);
line-height: var(--leading-body);
}

.project-delete-dialog-running svg {
flex: 0 0 auto;
margin-top: 2px;
}

.project-delete-dialog-confirm {
background: var(--ds-error);
border-color: var(--ds-error);
Expand Down
116 changes: 93 additions & 23 deletions apps/desktop/test/project-delete.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ const DELETE_KEYS = [
"deleteSessions_other",
"deleteFolderKept",
"deleteRunningBlocked",
"deleteRunning_one",
"deleteRunning_other",
"deleteRunningConfirm",
"deleteConfirm",
"deleteCancel",
"deleting",
Expand Down Expand Up @@ -69,6 +72,16 @@ function deleteHandler(source) {
return rest.slice(0, end);
}

/** The props `<ProjectDeleteDialog>` receives on a surface, up to `onClose`. */
function dialogProps(source) {
const start = source.indexOf("<ProjectDeleteDialog");
assert.ok(start >= 0, "the dialog is rendered");
const rest = source.slice(start);
const end = rest.indexOf("onClose=");
assert.ok(end > 0, "the dialog receives props");
return rest.slice(0, end);
}

test("the delete dialog is a real confirmation backed by the store action", () => {
assert.match(dialogSource, /export function ProjectDeleteDialog/);
assert.match(dialogSource, /const deleteProject = useAppStore\(\(s\) => s\.deleteProject\)/);
Expand All @@ -88,7 +101,8 @@ test("the delete dialog is a labelled modal that blocks cancel while busy", () =
assert.match(dialogSource, /role="dialog"/);
assert.match(dialogSource, /aria-labelledby="project-delete-dialog-title"/);
assert.match(dialogSource, /id="project-delete-dialog-title"/);
assert.match(dialogSource, /aria-describedby="project-delete-dialog-description/);
// The running-session line joins the description by id only while rendered.
assert.match(dialogSource, /aria-describedby=\{`project-delete-dialog-description/);
for (const id of [
"project-delete-dialog-description",
"project-delete-dialog-sessions",
Expand All @@ -98,7 +112,10 @@ test("the delete dialog is a labelled modal that blocks cancel while busy", () =
}
assert.match(dialogSource, /event\.key === "Escape"/);
assert.match(dialogSource, /disabled=\{busy\}/);
assert.match(dialogSource, /busy \? t\("project\.deleting"\) : t\("project\.deleteConfirm"\)/);
assert.match(
dialogSource,
/busy\s*\?\s*t\("project\.deleting"\)\s*:\s*runningCount > 0\s*\?\s*t\("project\.deleteRunningConfirm"\)\s*:\s*t\("project\.deleteConfirm"\)/,
);
});

test("both project menus expose a destructive delete action", () => {
Expand Down Expand Up @@ -136,35 +153,65 @@ test("both project menus expose a destructive delete action", () => {
assert.match(sidebarSource, /onError=\{reportError\}/);
});

test("deleting a project is blocked while its tasks are running", () => {
for (const [surface, source, dialogSetter] of [
["ProjectsPage", projectsSource, "setDeleteFor("],
["Sidebar", sidebarSource, "setDeleteProjectFor("],
test("both project menus reach the confirmation dialog while tasks run", () => {
for (const [surface, source] of [
["ProjectsPage", projectsSource],
["Sidebar", sidebarSource],
]) {
const handler = deleteHandler(source);
const runningCheck = handler.indexOf("runningSessions[session.id]");
assert.ok(runningCheck >= 0, `${surface} consults runningSessions`);
assert.match(handler, /\.filter\(/, `${surface} filters the project sessions`);
assert.match(
handler,
/showToast\(\s*t\("project\.deleteRunningBlocked"\),\s*\{\s*variant: "warning",?\s*\},?\s*\)/,
`${surface} warns instead of deleting`,
);
// The previous behaviour warned through a transient toast and returned
// before the dialog state was touched, so a project with a live task could
// never be deleted and the refusal disappeared with the toast.
assert.doesNotMatch(handler, /deleteRunningBlocked/, `${surface} drops the toast refusal`);
assert.doesNotMatch(handler, /showToast/, `${surface} does not toast instead of confirming`);
assert.doesNotMatch(handler, /runningSessions/, `${surface} keeps the guard out of the menu`);
assert.match(handler, /setDelete(?:Project)?For\(/, `${surface} opens the dialog`);

// The guard must short-circuit before the dialog state is touched.
const dialogState = handler.indexOf(dialogSetter);
assert.ok(dialogState > runningCheck, `${surface} checks running tasks first`);
// The dialog still has to learn which of the project's sessions are live.
const props = dialogProps(source);
assert.match(props, /runningSessionIds=\{/, `${surface} passes the running sessions`);
assert.match(
handler.slice(runningCheck, dialogState),
/return;/,
`${surface} returns before opening the dialog`,
props,
/runningSessions\[session\.id\] === true/,
`${surface} derives them from the live store`,
);
assert.match(props, /\.map\(\(session\) => session\.id\)/, `${surface} passes session ids`);
}

// Each surface counts the sessions that belong to the project row itself.
assert.match(deleteHandler(projectsSource), /sessionMatchesIndexProject\(session, project\)/);
assert.match(deleteHandler(sidebarSource), /entry\.sessions\.filter\(/);
// Each surface counts the sessions that belong to its own row.
assert.match(dialogProps(projectsSource), /sessionMatchesIndexProject\(session, deleteFor\)/);
assert.match(dialogProps(sidebarSource), /deleteProjectFor\.sessions\s*\.filter\(/);
});

test("the delete dialog names the running sessions and asks for an explicit confirmation", () => {
assert.match(dialogSource, /runningSessionIds: string\[\]/);
assert.match(dialogSource, /const runningCount = runningSessionIds\.length/);
assert.match(dialogSource, /id="project-delete-dialog-running"/);
assert.match(dialogSource, /t\("project\.deleteRunning", \{ count: runningCount \}\)/);
// The description lists the line only while the line is actually rendered.
assert.match(dialogSource, /runningCount > 0 \? " project-delete-dialog-running" : ""/);
assert.match(dialogSource, /t\("project\.deleteRunningConfirm"\)/);
assert.ok(
dialogSource.indexOf('t("project.deleteRunningConfirm")') <
dialogSource.indexOf('t("project.deleteConfirm")'),
"the running label is offered instead of the plain confirm label",
);
});

test("the delete dialog stops the running sessions before it deletes the project", () => {
assert.match(dialogSource, /const abortSession = useAppStore\(\(s\) => s\.abortSession\)/);
const confirmBlock =
dialogSource.match(/const confirm = async \(\) => \{[\s\S]*?\n \};/)?.[0] ?? "";
assert.ok(confirmBlock, "confirm handler exists");
const loop = confirmBlock.indexOf("for (const sessionId of runningSessionIds)");
const abortCall = confirmBlock.indexOf("await abortSession(sessionId)");
const deleteCall = confirmBlock.indexOf("await deleteProject(project.path)");
assert.ok(loop >= 0, "the confirm handler stops the running sessions");
assert.ok(abortCall > loop, "each running session is aborted");
assert.ok(deleteCall > abortCall, "the delete is awaited only after every stop");
});

test("deleting a project still explains a host-side running-task refusal", () => {
const englishBlock = projectBlock(catalogs.get("en"));
assert.equal(
projectValue(englishBlock, "deleteRunningBlocked"),
Expand All @@ -191,6 +238,29 @@ test("deleting a project is blocked while its tasks are running", () => {
);
}
}

// The host still refuses a turn that starts after the dialog opened, so the
// localized explanation has to stay reachable from the dialog.
assert.match(dialogSource, /onError\(new Error\(t\("project\.deleteRunningBlocked"\)\)\)/);
});

test("the running-session copy is translated in every shipped catalog", () => {
const englishBlock = projectBlock(catalogs.get("en"));
assert.equal(
projectValue(englishBlock, "deleteRunning_one"),
"{{count}} session in this project is still running. Deleting the project stops it.",
);
assert.equal(projectValue(englishBlock, "deleteRunningConfirm"), "Stop tasks and delete");
assert.deepEqual(placeholders(projectValue(englishBlock, "deleteRunning_other")), ["count"]);
assert.deepEqual(placeholders(projectValue(englishBlock, "deleteRunningConfirm")), []);
assert.equal(
projectValue(projectBlock(catalogs.get("zh-CN")), "deleteRunningConfirm"),
"停止任务并删除",
);
assert.deepEqual(
placeholders(projectValue(projectBlock(catalogs.get("zh-TW")), "deleteRunning_one")),
["count"],
);
});

test("every shipped catalog defines the delete keys with matching placeholders", () => {
Expand Down
Loading