fix(projects): make project deletion reachable while sessions are running - #439
Merged
Merged
Conversation
…ning Both project menus refused Delete project with a transient `project.deleteRunningBlocked` toast whenever one of the project's sessions was running, and returned before `ProjectDeleteDialog` was mounted. The refusal disappeared with the toast and left no path forward, so a project with one live task could not be deleted at all (#360). The menus now always open the dialog and hand it the project's live running session ids. The dialog names how many sessions are still running, swaps its confirm label to `project.deleteRunningConfirm` ("Stop tasks and delete"), and aborts exactly those sessions before deleting the project. Removing a running session therefore stays an explicit, second confirmation of a stated consequence, and cancelling removes nothing. The host guard is unchanged: `projects.remove` still refuses with 1008 / CONFLICT while an attached session has a running turn, and the dialog still reports that refusal with `project.deleteRunningBlocked` for a turn that starts after the abort loop. Refs #360 item 5 only. Items 1 and 2 are visual claims with no code-level inconsistency (every choice/interaction panel already carries a `--ds-*` surface, and the sidebar and window backgrounds differ in both themes); items 3 and 4 are feature requests that change a default shortcut or add UI and belong in their own issues. Renderer only: no protocol, storage, host, permission, or migration change, and no new default. `project.deleteRunningBlocked` keeps its meaning, copy, and every translation. See D429 and E2E-PROJECT-delete-running-sessions-are-named-and-stopped.
…-delete-and-surfaces # Conflicts: # docs/spec/08-meta/decisions-log.md
Closed
There was a problem hiding this comment.
🟡 Changes recommended
The abort flow can race durable turn settlement, and required rendered coverage and component-spec alignment are still missing.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Makes project deletion reachable while sessions are running by stopping active sessions through the confirmation dialog before deletion.
Changes:
- Pass live running-session IDs from both project menus.
- Add warning copy, stop-and-delete confirmation, styling, and translations.
- Update regression coverage, E2E plans, and decision logs.
File summaries
| File | Summary | Findings |
|---|---|---|
packages/i18n/src/locales/zh-TW/index.ts |
Traditional Chinese strings | — |
packages/i18n/src/locales/zh-CN/index.ts |
Simplified Chinese strings | — |
packages/i18n/src/locales/tr/index.ts |
Turkish strings | — |
packages/i18n/src/locales/ko/index.ts |
Korean strings | — |
packages/i18n/src/locales/fr/index.ts |
French strings | — |
packages/i18n/src/locales/es/index.ts |
Spanish strings | — |
packages/i18n/src/locales/en/index.ts |
English strings | — |
packages/i18n/src/locales/de/index.ts |
German strings | — |
docs/zh-CN/spec/08-meta/decisions-log.md |
Chinese D431 decision record | — |
docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md |
Chinese E2E scenario | — |
docs/spec/08-meta/decisions-log.md |
English D431 decision record | — |
docs/spec/06-delivery/04-e2e-test-plan.md |
English E2E scenario | Nit (1 vote): align both component specs with the changed behavior. |
apps/desktop/test/project-delete.test.mjs |
Static deletion-flow coverage | Moderate (1 vote): add executed renderer/integration coverage for both entry points, cancellation, and ordering. |
apps/desktop/src/styles/projects.css |
Running-session warning styling | — |
apps/desktop/src/pages/ProjectsPage.tsx |
Projects-page dialog and live-session wiring | — |
apps/desktop/src/components/Sidebar.tsx |
Sidebar dialog and live-session wiring | — |
apps/desktop/src/components/ProjectDeleteDialog.tsx |
Confirmation, abort, and deletion flow | Critical (1 vote): wait for durable turn settlement before deletion. Moderate (1 vote): add executed rendered/integration coverage. |
Review details
Suppressed comments (4)
apps/desktop/src/components/ProjectDeleteDialog.tsx:97
api.invokeattaches the IPC error code toError.code, notError.errorCode. Consequently a turn that starts after the abort loop reaches the hostCONFLICTpath but this condition is false, so users see the generic host error instead of the promised localizedproject.deleteRunningBlockedfallback. Checkerror.codehere and add a regression assertion for the late-conflict path.
// 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")));
apps/desktop/src/components/ProjectDeleteDialog.tsx:91
- The new abort-then-delete behavior is only checked by source-text regexes in
project-delete.test.mjs;pnpm test:e2eis protocol-level host-core coverage, and the updated scenario explicitly says the rendered journey remains Draft. Please add an executed rendered/integration test for cancel, abort-before-delete, and the post-open running-turn refusal so regressions in this destructive wiring cannot pass the listed checks.
for (const sessionId of runningSessionIds) {
await abortSession(sessionId);
}
await deleteProject(project.path);
apps/desktop/test/project-delete.test.mjs:211
- This test only searches source text; it never renders either menu/dialog or invokes
abortSession, so it cannot catch runtime wiring failures or the realCONFLICTerror-display path. The existing project E2E exercises the host RPC directly and remains refusal-only, while this new renderer journey is still marked Draft in the plan. Add a renderer E2E/component test covering both entry points, cancellation, and abort-before-delete ordering.
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");
docs/spec/06-delivery/04-e2e-test-plan.md:8300
- The new behavior no longer refuses deletion while a session runs, but the linked UI contract is still contradictory:
docs/spec/04-ux/08-component-spec.md:640(and its Chinese mirror atdocs/zh-CN/spec/04-ux/08-component-spec.md:499) says the action is refused with a message. Because this is an observable behavior change, update both component specs in the same change rather than leaving the test plan and component contract inconsistent.
error. Deleting D while its task runs opens the confirmation dialog instead
of a warning that disappears with its toast; the dialog names the running
sessions and its confirm button reads as stopping them, cancelling removes
nothing, and confirming stops exactly those turns and then deletes D (see
E2E-PROJECT-delete-running-sessions-are-named-and-stopped).
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+88
to
91
| for (const sessionId of runningSessionIds) { | ||
| await abortSession(sessionId); | ||
| } | ||
| await deleteProject(project.path); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #360 is a mixed bundle (5 items, 6 screenshots). This PR fixes one of them — the only one that is a provable defect — and the rest are classified below, because three of them are features and two are visual claims that cannot be verified from the code.
The defect: item 5, "项目管理中无法真正删除项目"
The project context menu did have a delete action, but it refused whenever any session in the project was running:
setDeleteProjectFor(entry)was never reached, soProjectDeleteDialognever mounted. The refusal was a transient toast with no path forward — a project with one live task simply could not be deleted. That is exactly how the report reads.Fix
runningSessions[session.id], over the rows it already matched), and the dialog derives its copy from that prop on every render, so a turn that starts or finishes while the dialog is open is reflected before the user confirms.{{count}}running sessions and swaps its confirm label toproject.deleteRunningConfirm— "Stop tasks and delete". Confirming aborts exactly those sessions and only then callsdeleteProject. Cancelling removes nothing.Deleting a live turn therefore stays an explicit second confirmation of a stated consequence. The host guard is untouched:
projects.removestill refuses withCONFLICTwhile an attached session has a running turn, and the dialog still maps that refusal toproject.deleteRunningBlocked— which is what covers a turn that starts after the abort loop. No permission, confirmation, or safety check was weakened.New scenario
E2E-PROJECT-delete-running-sessions-are-named-and-stopped;project.deleteRunningBlockedkeeps its meaning, copy and every translation.The other four items — classified, not implemented
--ds-*surfaces. The claim is about rendered appearance and the screenshots are not available to us. Needs a current-version screenshot.--ds-*surface tier and are deliberately close by design; whether they are "insufficiently differentiated" is a visual judgement, not a code fact. Needs a current-version screenshot.pluginLauncherAccelerator,summonWindowAcceleratorinbootstrap/launcher.ts, registered/unregistered there and inbootstrap/shutdown.ts). Merging them removes or repurposes a shortcut users may rely on, which is a user-visible default change. Needs its own issue stating whether it is a merge, an alias, or a removal.Validation
On the integrated local
main(merge commit9b1f3631, which carries this branch on top of the currentorigin/main):pnpm build:jspnpm --filter @pi-desktop/desktop typecheckpnpm lint:biomepnpm -r --if-present testpnpm docs:checkpnpm test:e2eE2E-PROJECT-delete-...-refuses-while-runningpnpm test:e2e:bootpnpm test:e2e:layoutpnpm test:e2e:transcript,:transcript-disclosure,:theme-surfaces,:skill-market,:subagentsproject-delete.test.mjswas extended (116 lines changed) so the running-session path is covered; it fails without the change.Notes
mainand claimed D429, so this one is D431 (the sibling disclosure fix is D430).zh-CNmirrors of the E2E plan and the decisions log are updated.Refs #360