fix(orchestration): allow deleting a project whose only threads are archived (#2866)#3098
fix(orchestration): allow deleting a project whose only threads are archived (#2866)#3098leorivastech wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Approved Straightforward bug fix allowing deletion of projects with only archived threads without requiring force=true. The change is limited in scope, well-documented with comments referencing issue #2866, and includes comprehensive test coverage. You can customize Macroscope's approvability policy. Learn more. |
…rchived A project whose threads are all archived shows as empty in the UI, so the client sends project.delete without force=true. The server counted archived threads as blocking and rejected the command, leaving 'Remove project' broken. Align the server's notion of 'empty' with the client: only non-archived, non-deleted threads block a plain delete, while archived threads are still cascade-deleted along with the project so none are orphaned. Fixes pingdotgg#2866.
d85f38b to
ff560d9
Compare
|
Additional repro for the same invariant, in case it helps scope this fix:
Actual: the client sends plain Expected: either the delete succeeds when the server can safely cascade, or the client falls back to the existing force-confirm flow when This PR may cover the case when the hidden remaining threads are archived-only. If not, the complementary client-side fallback is small: catch this specific invariant and show the existing "Delete anyway" confirmation. |
|
Thanks, that's a great repro 🙏 I think it's a slightly different case from this PR. Yours sounds like real (non-archived) threads still hanging around after the external delete, while this change only handles the archived-only case on the server. I'd like to keep this PR small and focused so it's easy to review and hopefully get it merged, then open a separate issue to dig into your scenario. The client-side "Delete anyway" fallback you described sounds like the right approach. Appreciate you flagging it! |
Fixes #2866.
Right now, if you archive every thread in a project and then try to remove the
project, "Remove project" just doesn't work. You get a confusing error:
"Project '...' is not empty and cannot be deleted without force=true" — even
though the project looks empty in the UI.
The reason is that the client and the server don't agree on what "empty" means.
The shell snapshot the client receives leaves archived threads out (it filters
on archived_at IS NULL), so the client sees zero threads and sends a plain
project.delete without force=true. The server, on the other hand, treats any
thread that isn't deleted as still counting — archived ones included — so it
rejects the command. The button ends up dead.
This change teaches the server to count the same way the client already does.
In decideOrchestrationCommand's project.delete branch, I split apart two things
that were previously lumped into one list. One question is "what should block a
plain delete?", and the answer is now only the threads that are neither deleted
nor archived. The other question is "what gets cleaned up along with the
project?", and that still includes every non-deleted thread, archived ones too,
so nothing is left orphaned when the project goes away.
Projects that actually have active (non-archived) threads behave exactly as
before: they still require force=true. The only thing that changes is that a
project whose remaining threads are all archived can now be removed normally,
and those archived threads are deleted with it.
I also considered fixing this on the client side instead (detect the archived
threads and show the "Delete anyway" force dialog), but matching the server's
definition of "empty" to the snapshot the client already uses is the smaller,
single-place fix.
For tests, I added a regression case in decider.delete.test.ts: a project whose
only thread is archived. It fails on main with the exact invariant error, and
passes with this change, deleting the project and emitting thread.deleted
followed by project.deleted. The existing delete tests are untouched and still
pass, the full orchestration suite is green (152/152), and typecheck, lint and
formatting are clean.
Note
Medium Risk
Changes orchestration invariants for project deletion; behavior for active threads is preserved, but archived-thread edge cases now delete data without force.
Overview
Fixes #2866 by aligning server
project.deletewith the client’s notion of an “empty” project.In
decideOrchestrationCommand, non-forced deletion is blocked only by undeleted, non-archived threads (blockingThreads), not every undeleted thread. Projects whose remaining threads are all archived can be removed withoutforce=true. Cascade cleanup is unchanged: all undeleted threads (including archived) still getthread.deletebeforeproject.deleted, so nothing is orphaned.A regression test seeds a project with a single archived thread and expects
thread.deletedthenproject.deletedon a plainproject.delete.Reviewed by Cursor Bugbot for commit 4c66c55. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix project deletion to allow deleting projects with only archived threads
project.deletecase ofdecideOrchestrationCommand, renamesactiveThreadstoundeletedThreadsand introducesblockingThreadsas the subset of undeleted threads that are not archived.blockingThreads.length > 0instead of all undeleted threads, so archived-only projects can be deleted without theforceflag.project.deleted.Macroscope summarized 4c66c55.