Skip to content

Fix Space deletion lifecycle and feedback - #369

Merged
cxxxxxn (cxxxxxn) merged 6 commits into
mainfrom
fix/space-deletion-lifecycle
Nov 3, 2026
Merged

cxxxxxn (cxxxxxn) merged 6 commits into
mainfrom
fix/space-deletion-lifecycle

Conversation

@cxxxxxn

@cxxxxxn cxxxxxn (cxxxxxn) commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Route requester and child deletion preparation through one prepareSpaceDeletion(signal) entry backed by pageLifecycle. This covers editor settlement, retained title retry, pending imports/mutations, and registered Canvas/Note, title, and draft persistence. Aborting a stalled retry releases feedback ownership and cannot acknowledge or clear a replacement attempt; requester unmount also aborts local preparation.
  • Bound deletion preparation without adding a new cancellation flow: the local save drain and subsequent parallel child-renderer drains each have a fixed 15-second deadline. Heartbeats cannot extend deletion preparation; timeouts retain edits/tabs, resume pages, skip unnecessary metadata refresh, and cannot be revived by late save replies.
  • Restrict duplicate Refresh cleanup to a current duplicate-file failure, preserving newer content-conflict notifications and their recovery actions.
  • Limit UI deletion to Home and a parent Space's direct child entries. Remove "Delete current Space"; validate container ownership again against the deletion preview and reject desktop requests that would delete their own requesting Space.
  • Save and freeze affected open tabs before deletion, keep the container alive, and close only server-confirmed deletions. Users do not need to close child tabs manually.
  • Remove self-deletion-only return navigation, ancestor destination mapping, error-handoff IPC, queued destination errors, and special deletion-modal freeze handling.
  • Send Workspace-scoped confirmed deletion IDs to Home before resuming tabs. Reuse Home's list reconciliation to prune deleted cards/counts and invalidate stale list requests, even if a later refresh fails.
  • Retain localized confirmation, accessible pending states, inline failure recovery, partial-deletion reconciliation, and fresh-preview confirmation before retrying. Ordinary Note-save failures stay in the deletion dialog; successful deletion followed by failed refresh gets one explicit deletion-success/refresh-failure notification.
  • Update architecture documentation, bridge mocks, and browser/desktop regression coverage for the narrowed design.
  • Defer special Note conflict and duplicate-file recovery notifications while the requesting renderer's deletion dialog is open. Preserve frozen revisions, duplicate banners, and Keep mine / Load latest actions; release recovery after the dialog closes and cancel obsolete notifications when their failure is resolved or removed.

Validation

  • Unified-preparation follow-up: 158 tests across lifecycle, desktop hook, deletion dialog, title store, sign-out, command builders, and Space organization passed. Four browser deletion scenarios and the real Electron test:tabs smoke passed again. Repository typecheck, format, and lint passed (357 existing warnings, no errors).
  • Latest bounded-preparation and notification-scope follow-up: 198 web tests across 11 files, 171 desktop tests (one existing todo), and four browser deletion scenarios passed. Repository typecheck, format, and lint passed with 357 existing warnings and no errors.
  • Repository pnpm typecheck, pnpm format, and pnpm lint:fix completed; lint reports 357 existing warnings and no errors.
  • 149 targeted web tests and 99 desktop lifecycle tests passed.
  • The subsequent special-save-feedback fix passed 195 web tests across 11 files, including all save-queue suites, deferred recovery actions, stale-notification cancellation, and deletion feedback owners. Repository typecheck, format, and lint were rerun for this follow-up.
  • Four browser scenarios passed: cancelling child deletion, Home partial cleanup, absence of current-Space deletion plus parent-owned child deletion, and localized keyboard-accessible recovery.
  • Real Electron test:tabs smoke passed, including parent-owned deletion, child-tab freezing/closing, and continued parent interaction. An earlier run timed out in the later tab-drag cancellation scenario; a standalone rerun passed without changing drag code.
  • git diff --check passed.

Scope and limitations

Preparation deadlines apply only before a deletion request is sent. This follow-up does not add cancellation, forced deletion, automatic retry, or reconciliation of uncertain outcomes after an issued request loses its response; explicit partial-deletion responses retain the existing recovery behavior.
This PR excludes the unrelated changes in the original worktree. Deletion remains permanent; no recycle bin or undo is added. Running/busy Agent safeguards remain intact. Removing self-deletion also removes the reviewed return-tab placement and destination-error paths.

The special Note conflict/duplicate-file notification finding is addressed without removing recovery actions or automatically overwriting conflicting content. Background child tabs retain their recovery notifications for when the user returns; the active container's deletion dialog remains its only new error surface until closed.

Prepare and freeze affected tabs before deletion, close only confirmed deletions, and return deleted active Spaces to their nearest surviving parent. Keep failures on one surface and add localized confirmation, recovery, pending feedback, and modal focus containment.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 2, 2026 10:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Deletion still makes the pending dialog inaccessible to assistive technology, produces duplicate error notifications, and misplaces the return Tab during multi-Tab removal.

4 open issues
What changed in this PR

This PR extends Space deletion across desktop content tabs, coordinating saves, tab cleanup, parent navigation, and failure feedback across the desktop, web, and server layers.

Changes:

  • Prepare affected tabs before deletion and close only server-confirmed deletions, returning deleted active Spaces to a surviving parent.
  • Add localized pending and recovery states, single-surface error handling, and modal focus containment.
  • Expand regression coverage and update architecture documentation.
File Description
packages/​shared/​src/​types/​desktop-tabs.ts Extend deletion commands and notification contracts.
packages/​shared/​src/​types/​api/​space-hierarchy.ts Include parent identities in deletion previews.
docs/​architecture/​web-architecture.md Document modal focus and scrolling behavior.
docs/​architecture/​space-preview.md Describe deletion navigation and recovery states.
docs/​architecture/​desktop-tabs.md Document tab preparation, cleanup, and error ownership.
apps/​web/​src/​utils/​desktopStorage.test.ts Update desktop bridge mocks.
apps/​web/​src/​utils/​applicationPreferenceStore.test.ts Update desktop bridge mocks.
apps/​web/​src/​pages/​CanvasListPage.tsx Allow open-Space deletion and adjust failure refreshes.
apps/​web/​src/​pages/​CanvasListPage.test.tsx Cover desktop deletion preparation and feedback.
apps/​web/​src/​i18n/​resources/​zh-CN/​common.json Add Chinese deletion and recovery messages.
apps/​web/​src/​i18n/​resources/​en/​common.json Add English deletion and recovery messages.
apps/​web/​src/​hooks/​useDesktopContent.ts Handle deletion preparation, save retries, and destination errors.
apps/​web/​src/​hooks/​useDesktopContent.test.tsx Test deletion-specific save and notification behavior.
apps/​web/​src/​hooks/​useDesktopContent.preferences.test.tsx Update desktop bridge mocks.
apps/​web/​src/​components/​Space/​SpaceDeletionDialog.tsx Coordinate deletion, reconciliation, and recovery UI.
apps/​web/​src/​components/​Space/​SpaceDeletionDialog.test.tsx Cover pending, partial-failure, and retry states.
apps/​web/​src/​components/​Space/​SpaceActions.tsx Navigate deleted browser Spaces to surviving parents.
apps/​web/​src/​components/​Space/​SpaceActions.test.tsx Test navigation and error presentation ownership.
apps/​web/​src/​components/​Common/​Modal.tsx Add focus containment and bounded scrolling.
apps/​web/​src/​components/​Common/​Modal.test.tsx Test initial focus, restoration, and viewport bounds.
apps/​web/​e2e/​space-hierarchy.spec.ts Cover localized recovery, focus, and responsive layouts.
apps/​web/​e2e/​desktop-tab-drag.spec.ts Update the desktop bridge mock.
apps/​server/​src/​modules/​canvas/​space-hierarchy.ts Return canonical parents in deletion previews.
apps/​server/​src/​modules/​canvas/​space-hierarchy.test.ts Verify preview parent metadata.
apps/​desktop/​src/​preload.ts Expose deletion-error notifications to renderers.
apps/​desktop/​src/​desktop-tabs.ts Manage deletion ownership, preparation, cleanup, and return tabs.
apps/​desktop/​src/​desktop-tabs.test.ts Cover deletion lifecycle and notification handoff.
apps/​desktop/​scripts/​test-tabs.mjs Extend Electron smoke coverage for self-deletion and parent return.

Review effort: Balanced

(tab) =>
tab.state.canvasId && canvasIds.includes(tab.state.canvasId),
)
.map((tab) => this.drain(tab, 'space-deletion')),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in dd6a3e7. Deletion preparation freezes background body children (including later portals), but leaves the marked Common Modal and native preparation controls accessible. The requesting page keeps its deletion status instead of opening a second saving overlay. Added inert/event-guard coverage and verified the pending dialog remains in the accessibility tree in the real Electron tab smoke test.

Comment thread apps/desktop/src/desktop-tabs.ts Resolved
Comment thread apps/web/src/hooks/useDesktopContent.ts Outdated
Comment on lines +200 to +202
if (deleting && error instanceof ConversationTitleSaveError)
deletionSaveFailure = error;
const message = reportSaveError(error, !deleting);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in dd6a3e7. Both the dialog-local drain and desktop preparation scope save-error feedback to the caller. The actual Note queue retains/logs failures and rejects preparation without its duplicate toast. Added real queue integration coverage for both entry points, retained edits and retry, and restoration of ordinary autosave/close notifications.

Comment thread apps/web/src/pages/CanvasListPage.tsx Resolved
Keep the pending deletion dialog accessible while freezing background edits, preserve parent return tab order, and make save and refresh errors use their owning surface.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 3, 2026 02:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unresolved deletion-flow issues can leave deleted cards and counts visible after a failed Home refresh and produce duplicate or persistent error notifications.

2 open issues
3 previously missed issues

In code that hasn't changed since last review

Medium severity Reconcile confirmed Space deletions in Home before refreshing

apps/​desktop/​src/​desktop-tabs.ts:1255

When deletion starts in a Space Tab, Home never receives the confirmed deleted IDs and does not run its local onDeleted reconciliation. If self-deletion returns to Home and its list refresh fails, the deleted cards and count remain cached and visible. Send workspace-scoped confirmed deletions to Home before releasing the operation, then invalidate older list requests and prune those IDs before refreshing.

Medium severity Avoid duplicate errors when Home's deletion refresh fails

apps/​web/​src/​pages/​CanvasListPage.tsx:140

When a top-level Space deletes its own desktop Tab, Home has no deletion dialog, so deletionOpen.current is false. If both the requester's metadata refresh and Home's activation-triggered list refresh fail, Home shows the forwarded deletion error plus a second list-error toast. Coordinate feedback ownership for that destination refresh with the deletion handoff so only one notification appears.

Medium severity Suppress persistent duplicate and conflict toasts during deletion preparation

apps/​web/​src/​store/​canvasStore/​save/​nodeContentQueue.ts:859

During deletion preparation, NodeDuplicateFilesError and NODE_CONTENT_CONFLICT bypass this guard through notifyDuplicate() and handleContentConflict(), which still create persistent danger toasts. Their retained failures also reject flushAllForSubmission(), leaving a toast alongside the dialog's inline error. Apply caller-owned feedback to both paths while preserving duplicate flags, conflict freezing, and access to conflict recovery after the dialog closes.

2 resolved since last review

Review effort: Balanced

Remove self-deletion navigation, error handoff, and special modal freezing. Reuse list reconciliation to prune confirmed desktop deletions from Home and retain save coordination and partial-failure recovery.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 3, 2026 03:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Deletion preparation can indefinitely block the window when a hidden child’s save stalls, because the visible dialog cannot cancel and heartbeats keep extending the timeout.

3 open issues

Review effort: Balanced

Comment thread apps/desktop/src/desktop-tabs.ts Resolved
Reuse save feedback coordination to preserve conflict and duplicate-file recovery actions without competing with the deletion dialog. Cancel deferred notifications when their failure is resolved or removed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 3, 2026 03:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The duplicate banner’s Refresh action can dismiss a newer content-conflict notification while leaving saves blocked, removing the user’s recovery actions.

4 open issues

Review effort: Balanced

Reuse fixed save deadlines to retain tabs and release preparation after stalled saves. Skip metadata refresh when no deletion was requested, ignore late acknowledgements, and only dismiss duplicate feedback when it remains the current failure.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 3, 2026 05:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A stalled title-save retry can outlive the preparation timeout, suppressing ordinary Note-save errors and delaying conflict/duplicate recovery until the obsolete retry completes.

3 open issues
2 resolved since last review

Review effort: Balanced

Comment thread apps/web/src/hooks/useDesktopContent.ts Outdated
Comment on lines +185 to +188
if (deleting && deletionSaveFailure) {
await deletionSaveFailure.retry();
deletionSaveFailure = undefined;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed by routing both requester and child preparation through prepareSpaceDeletion(signal), backed by pageLifecycle. Retained title retry now runs inside the lifecycle lock and abortable wait, so resume/timeout releases feedback without waiting for the underlying retry. Late completion cannot proceed to registered saves, acknowledge a replacement, or clear its retry state. Also scanned registered owners: editor settlement, imports/mutations, Canvas/Note/preprocessing, titles, and drafts now participate in the requester path too. Added stalled retry/import/save, immediate replacement, feedback release, late completion, and requester-unmount coverage. Browser deletion scenarios and real Electron smoke passed.

Move retained title retry inside the abortable preparation boundary and share the complete save-owner drain between requesting pages and desktop children. Release feedback ownership when preparation is retired and fence late completions from replacement attempts.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 3, 2026 05:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues block the unified Space deletion lifecycle changes.

3 open issues

Review effort: Balanced

@cxxxxxn
cxxxxxn (cxxxxxn) merged commit 772b3b6 into main Nov 3, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants