Guard the active project from deletion and fix the post-delete flow - #2555
Conversation
There was a problem hiding this comment.
Overall this looks solid: the UI guard is consistent across list + detail, the post-delete flow avoids the empty shell, and the 410/not-found handling + cross-project resolve skip make the error states much more honest.
Found 2 issues (0 critical, 1 improvement, 1 nit).
The projects list let you delete the project you were working in — the one scoping every request. EntityCard grows a deleteDisabledReason prop that renders the trash can disabled, dimmed and not-allowed with the reason as its tooltip, instead of the all-or-nothing hide it had before. The list page passes it for the active project and re-checks in confirmDelete, since the active project can resolve after the card rendered. Signed-off-by: Arman Beykmohammadi <arman.beykmohammadi@rhesis.ai>
Deleting a project left you on its detail page, an empty shell with no title. The delete now unmounts the detail body straight away, refreshes ActiveProjectProvider so the sidebar and switcher drop the project without a reload, and uses router.replace so Back can't return to the deleted URL. That URL now renders the not-found state: projects come back as 410, not 404, so this checks both — the repo's 404-only helper would miss it, and the 410 path offers a Restore that produces a project nobody can see, because deletion hard-drops every membership row. The Delete FAB is disabled on the active project for the same reason as the list page. Closes #2415 Signed-off-by: Arman Beykmohammadi <arman.beykmohammadi@rhesis.ai>
projectCreated was set once and never unset, so a project removed by another member or the API left the checklist claiming a step with no project behind it. It's now derived from the live list rather than the stored bit — deliberately not written back, because mergeProgress ORs local with remote and an unset would be resurrected on the next load. DetailNotFoundState also fired a cross-project resolve for entities the backend won't resolve. A project isn't scoped to a project, so GET /resolve rejects it and the page said "we could not check other projects right now, try again in a moment" — a transient-failure message for a permanent condition. Mirror the backend's resolvable-table set and skip the resolve when it doesn't apply. Signed-off-by: Arman Beykmohammadi <arman.beykmohammadi@rhesis.ai>
The projects list had no test file at all. Asserts the trash can is disabled with its reason on the active project and enabled elsewhere, that confirmDelete refuses a target that became active while the dialog was open, and that a failed delete surfaces. The detail-page test moves off getAllByRole index lookups onto the new aria-labels, and covers the FAB guard plus the detail body unmounting before the navigation lands. Signed-off-by: Arman Beykmohammadi <arman.beykmohammadi@rhesis.ai>
a0c2466 to
cd1e999
Compare
There was a problem hiding this comment.
Overall this looks solid and the tests/readability improvements are great.
Improvement (blocking): The detail page still needs the same mid-flight guard you added on the list page. If activeProject resolves after the delete modal is opened, handleDeleteConfirm can still delete the now-active project. Suggest re-checking isActiveProject (or comparing against latest activeProject?.id) inside handleDeleteConfirm before calling deleteProject, and ideally show ACTIVE_PROJECT_DELETE_BLOCKED so the modal doesn’t just disappear.
Nit/UX: On the list page, when the mid-flight guard triggers (deleteTarget became active), we currently just close the modal; a toast explaining why would avoid confusion.
The Delete FAB was guarded at render time only. With no active-project cookie and a single project, ActiveProjectContext auto-selects it client-side after mount — so the FAB could be enabled when the dialog opened and confirming would delete the project scoping the app. Re-check before the DELETE, as the list page does. Both guard paths now say why nothing happened instead of letting the dialog vanish silently. Signed-off-by: Arman Beykmohammadi <arman.beykmohammadi@rhesis.ai>
Purpose
The projects list showed a trash can on every card, including the card for the project you were currently working in — the one named in the sidebar, whose id goes out as the
X-Project-Idheader on every request. Nothing signalled that deleting it was a bad idea, and doing it left the app scoped to a project that no longer existed.Issue #2415 reports the downstream half of the same problem: after deleting a project you stayed on its detail page, which rendered an empty shell with no title, and only manual navigation got you out.
What Changed
cursor: not-allowed, with a tooltip explaining why: "Active project — cannot be deleted. Switch to another project first."EntityCardgained adeleteDisabledReasonprop for this; previously the delete button was all-or-nothing (rendered or absent), which is whyModelCardhas to hide it entirely for protected models. The four otherEntityCardcall sites are unchanged. The trash can and the detail-page Delete FAB also gained accessible names, which they were missing.confirmDeletere-checks before the DELETE. The active project can resolve after the card rendered (no cookie yet, or a single project being auto-selected mid-flight), so a click can slip through a not-yet-disabled trash can.router.replaceis used instead ofpushso Back can't return to the deleted URL.useActiveProject().refresh(), so a deleted project stayed in the switcher until a full page reload.router.refresh()would not have fixed this — that state is seeded by the root layout, whichrouter.refresh()does not re-run.projectCreatedonboarding progress is derived from the live project list rather than a stored bit that was set once and never unset.console.error, so the dialog closed, the card stayed, and the user got no signal.Additional Context
Closes #2415.
Three things worth flagging for review:
Projects return 410, not 404.
crud/project.py→get_item_detail→_check_and_raise_if_deletedraisesItemDeletedException, whichapp/main.py:597turns into 410 withcan_restore: true(confirmed against a running backend). So this deliberately does not use the repo'snotFoundIfEntityMissinghelper, which is 404-only — that would miss the exact case the issue is about. The 410 path leads toerror.tsx, which offers Restore, and that is a trap:unenroll_all_project_membershard-deletes everyproject_membershiprow and restore does not re-enroll anyone, so a restored project would be invisible to everyone including whoever restored it. "Gone" and "missing" get the same honest state.DetailNotFoundStatewas firing a cross-project resolve for the project itself. A project isn't scoped to a project, soprojectisn't in the backend'sRESOLVABLE_ENTITY_TABLESandGET /resolverejects it — which made the page say "We could not check other projects right now. Try again in a moment.", a transient-failure message for a permanent condition. This adds a frontend mirror of that set and skips the resolve when it doesn't apply. It touches a shared component, so it fixes the same misleading copy for any other non-resolvable entity landing on a detail not-found page.Two acceptance criteria from the issue are not implemented, on purpose:
/projectscannot reach zero through the UI:ActiveProjectContextauto-selects when there is exactly one project, the switcher never clears the selection, so the last project is always the active one and always guarded. A "you just deleted your last project" state would be dead code from day one. The guard tests are the evidence.NoProjectAccess), but/projectscurrently shows the first-run "No project yet" card instead. Changing that copy costs an E2E update (mocked-states.spec.ts:38asserts it) for a case unrelated to deletion.Also note the guard is UI-only.
Capability.Projecthas noDELETE— both delete paths gate onUPDATE— androuters/project.pyonly 404s on missing, so the backend does not reject deleting the active project. A second tab, the API, the SDK, or another member still can; the recovery path (refresh()clearing the active-project cookie) is what covers that.Testing
npm run format,npm run type-checkandnpm run lintare clean (0 errors; the 61 pre-existing warnings are all in files this PR does not touch).npx jestpasses 212/212 suites, 1970 tests.New and updated tests:
components/common/__tests__/EntityCard.test.tsx— the enabled trash can has an accessible name and firesonDelete; with a reason it is disabled, carries the reason as its tooltip, and swallows clicks; the title still clears the top-right slot.projects/components/__tests__/ProjectsClientWrapper.test.tsx— new file, the list page had no test at all. The active project's trash can is disabled with its reason while the others are enabled;confirmDeleterefuses a target that became active while the dialog was open; a failed delete surfaces.projects/[identifier]/__tests__/client-wrapper.test.tsx— moved offgetAllByRoleindex lookups onto the new aria-labels; covers the FAB guard, the Edit FAB staying enabled, and the detail body unmounting before the navigation lands.utils/__tests__/entity-error-handler.test.ts— the resolvable-table set, including thatprojectis not in it.Verified end-to-end against a real backend and frontend (Playwright driving the app, not mocks):
disabled,rgba(0, 0, 0, 0.26)vsrgb(0, 95, 130)for the others,cursor: not-allowed,pointer-events: none/projects; gone from the list; gone from the sidebar switcher without a reloadGET /projects/<deleted>isNotFoundApiErrorcheck)To try it manually: with two or more projects, open
/projectsand hover the trash can on the project named in the sidebar, then delete a different one from its detail page and watch the sidebar switcher.