feat(pages): add page deletion with safe subpage reparenting - #49
Pioneer113 wants to merge 10 commits into
Conversation
…cover cleanup in tests
…document deletion
NathanTarbert
left a comment
There was a problem hiding this comment.
Thanks @Pioneer113, this is a really thorough PR. The description explains each decision: keeping review receipts, leaving the Intelligence thread alone, owner-only, not exposed to agents. It's also upfront about what's still open, which makes it much easier to review. The tombstones are a nice fix for the old assumption in mergePageSnapshot that a missing row means a deleted page. And the test that a retried review can't bring a deleted page back covers the guarantee that matters most.
The server side holds up well. Deleting a middle page moves its children to the grandparent and leaves grandchildren alone. Deleting a root page lifts its children to the top level. Deleting twice, deleting through another Space's id, and saving from a stale tab all end in a clean 404 rather than a partial change.
One thing has changed underneath it: #48 merged today and reworked the same review-card code. The receipt GET now returns the page with its reviewDraft, and PageReviewCard.tsx asks the server for every card, including ones that recorded an approval. So this needs a rebase, mostly to fold the deleted response into #48's ReviewedPage flow. The good news is that because every card now checks the server, the second known limitation in your description, an approved card still offering "Open page" for a deleted page, should go away once it's on top of #48.
The other thing is the confirmation text. Deletion is permanent, and any unsaved edits in the open editor go with it, but the prompt only mentions where subpages will move. Adding "This can't be undone." would make sure nobody is surprised.
|
Thanks for the review, @NathanTarbert. Both points are addressed in the latest push.
|
|
Thanks @Pioneer113, this looks great. "Saved, then deleted" with no "Open page" is a nice way to show it, and good to see #48's draft check covering deleted reviews too. The live check from review card to delete to reload is reassuring. Looks good to me. |
jerelvelarde
left a comment
There was a problem hiding this comment.
Page deletion is useful and the server transaction, Space scope, reparenting, and retained draft-bound receipts look sound. Hold merge for a client race that can lose another page's draft. Both findings were reproduced with React renderer tests. Validation: 204 tests, typecheck, lint, formatting, and the rerun production build passed.
| // Drop pending autosave so navigation is not blocked by the | ||
| // unsaved-draft prompt for a page that no longer exists. | ||
| controller.dispose(); | ||
| onDirty(false); |
There was a problem hiding this comment.
[P1] Ignore stale deletion completion after leaving this document. Delay DELETE for page A, navigate to page B, edit B, then let A's deletion finish. The old onDirty(false) clears B's navigation protection, and onHome() unmounts B, silently discarding its unsaved draft without another confirmation. Guard navigation and dirty cleanup by the active document lifecycle; retain the tombstone update for A.
| return; | ||
| } | ||
| if (isDeletedReview(page)) { | ||
| setDeletedReview(page); |
There was a problem hiding this comment.
[P2] Clear savedPage when a refreshed receipt reports deletion. If this card already has a saved receipt and Continue conversation returns deleted:true, setDeletedReview leaves savedPage populated. The card then says Saved, then deleted while still offering Open page with a dead URL. Clear the saved-page state when accepting the tombstone.
|
Thanks for the careful review, @jerelvelarde. Both findings are fixed in the next push.
|
User workflow
Users can create pages, save AI drafts, organize subpages, and move pages within Spaces, but there was previously no way to delete unwanted or temporary pages.
This PR adds page deletion to the document workspace. When a user deletes a page, any nested subpages are reparented to the deleted page's parent (or moved to the root level if the deleted page was at the top level), so no child documents are lost.
Changes
src/server/pages.ts):Pages.delete(spaceId, id)runs in oneBEGIN IMMEDIATEtransaction. It reparents children (bumping theirrevisionso clients resync the new parent), removes the page'spage_threadsbinding, deletes the page, and validates the Space. It returnsfalsefor a missing page. NewPages.exists(spaceId, id)helper.src/server/page-routes.ts):DELETE /spaces/:spaceId/pages/:idreturns{ ok: true }or 404 for a missing page or Space. The review receipt endpoint (GET /conversations/:id/reviewed-page/:toolCallId) now returns{ deleted: true, pageId, spaceId, reviewDraft }when the reviewed page was deleted, instead of a 404.SpaceWorkspaceremoves it, re-parents its children locally and keeps a tombstone so a stale poll cannot bring it back (mergePageSnapshototherwise never removes pages). It also drops the pending autosave and the dirty flag before navigating home, so the "Leave your unsaved page draft?" prompt does not appear for a page that no longer exists.PageReviewCard(rebased onto fix: bind reviewed-page retries to saved draft #48'sReviewedPageflow) shows "Saved, then deleted" for such a review, with no "Open page" link, and lets the conversation continue (the agent is told not to link the page). The draft-binding check from fix: bind reviewed-page retries to saved draft #48 also applies to deleted reviews. The delete confirmation now says "This can't be undone."docs/SETUP.mdlists page deletion.Design decisions
page_reviewsrows are kept on delete.createRevieweduses them for idempotent retries ("retries recover the same saved page"). Deleting them would let a retried approval silently recreate a deleted page. A retry after deletion now fails with 404 instead.page_threadsbinding is removed. The page's conversation thread stays in the configured Intelligence project; no Intelligence deletion call is made.Known limitations
PageService.conversation()call racing with a delete could leave an orphanedpage_threadsrow (no foreign keys in the schema). The window is small and the row is unreachable because lookups join againstpages.PageDocument's deletion lifecycle and the review card's deleted-receipt handling also havereact-test-renderertests.Verification
npm test— 243/243 passed on top of currentmain(also under Node 24.21).npm run typecheck,npm run lint,npm run check-format— passed.npm run build— succeeded.tests/pages.test.ts); DELETE route and deleted-review receipt (tests/page-routes.test.ts); deleted-review recognition (tests/page-review.test.tsx). The new regression tests fail on the code before the fix.Manual UI check (dev server, local)
review_space_pagecall): the review card is ready → Approve & save → the saved page opens → Delete page (the confirmation reads "This can't be undone…") → reload the chat. The card now shows "Saved, then deleted", with no "Open page" link and no restore error.