chore(upstream): PR3 v2 workspace delete 統合 (#3443)#178
Merged
Conversation
* Delete
* plans(workspace-delete): refine design per review
- Drop unsupported claim about v2 branch ephemerality; deleteBranch
defaults off, no persisted preference
- Add Host ownership section: v2Workspaces.hostId is 1:1 so destroy
is host-local; FORBIDDEN from other hosts
- Replace "remote-host cleanup" with "abandoned-host cleanup" as the
real follow-up (cross-device delete isn't a thing under the schema)
* feat(host-service): workspaceCleanup.destroy
Adds a single unified delete path for v2 workspaces. Sequences:
terminals → teardown → worktree → branch → cloud → host sqlite.
- runTeardown silently executes .superset/teardown.sh with a 60s
timeout, SIGKILL on timeout, 4KB output tail capture; returns
status + exitCode + tail.
- disposeSessionsByWorkspaceId kills all active PTYs for a workspace
(releases file locks before worktree remove / teardown).
- Typed errors: CONFLICT for dirty worktrees (prompt force: true),
INTERNAL_SERVER_ERROR with cause.kind=TEARDOWN_FAILED surfaced to
the client via errorFormatter so the renderer can show outputTail.
- deleteBranch opts in (default false); force also upgrades
`git branch -d` to `-D`.
- Cloud failure is swallowed as a warning — disk is already clean,
cloud self-heals on next sync.
* feat(desktop): wire v2 sidebar delete through workspaceCleanup.destroy
- useDestroyWorkspace hook resolves the workspace's host-service URL
and calls workspaceCleanup.destroy, normalizing TRPC errors into a
typed union (conflict / teardown-failed / unknown) for the dialog.
- DashboardSidebarDeleteDialog becomes self-contained: owns the
mutation, the "delete local branch" checkbox (off by default), and
renders the force-retry UI for dirty worktrees and teardown
failures (with outputTail preview).
- useDashboardSidebarWorkspaceItemActions drops the direct
apiTrpcClient.v2Workspace.delete call; post-success cleanup moves
to an onDeleted callback (sidebar removal + focus navigation).
Path B (renderer → cloud v2Workspace.delete) is now unused in desktop.
* fix(workspace-delete): review follow-ups
- Run teardown script via createTerminalSessionInternal (same PTY
primitive v2 uses for interactive terminals) so the script inherits
the user's shell environment — login rcfiles, PATH, nvm/rbenv, etc.
Silent: session is transient and never surfaced as a visible pane.
Output captured via pty.onData; timeout via pty.kill() so behavior
is cross-platform (no process-group SIGKILL on Windows).
- Guard the timeout callback with `if (settled) return;` to prevent
the event-loop race where a successful 60s exit is misreported as
a timeout.
- disposeSession now always marks the DB row disposed even when the
in-memory session is missing, so zombie `active` rows left from
crashes get reconciled during bulk workspace cleanup.
- Export TEARDOWN_TIMEOUT_MS from host-service; renderer uses it in
the dialog copy instead of a literal `60`. Renderer also runs the
output tail through strip-ansi (host-service returns raw PTY bytes;
sanitizing is a presentation concern).
- TeardownFailureCause.signal is now `number | null` (Unix signal
number from node-pty) rather than a bogus NodeJS.Signals cast.
- JSDoc: INTERNAL_ERROR → INTERNAL_SERVER_ERROR on the workspace-
cleanup error contract.
- Tag the audit doc's ASCII-diagram fence as `text` for markdownlint.
- biome lint:fix.
* Refactor
* fix(workspace-delete): optimistic-close UX (no in-dialog wait)
Mirrors v1's pattern: the destroy runs in the background under a
toast.loading → success/error, the dialog closes immediately on
confirm, and only re-opens when the user has a decision to make.
- Confirm/force-retry close the dialog optimistically; mutation
continues with a toast for feedback. No more frozen "Deleting..."
for the up-to-60s teardown duration.
- CONFLICT and TEARDOWN_FAILED reopen the dialog in the matching
error pane so the user can force-retry with full context. The
branch opt-in is preserved across the reopen.
- Unknown errors fall through to toast.error — no reopen.
- isPending state and "Deleting..." button copy dropped from all
panes (never visible since the dialog closes optimistically).
* fix(workspace-delete): move TEARDOWN_TIMEOUT_MS to @superset/shared
The renderer used to value-import TEARDOWN_TIMEOUT_MS from
@superset/host-service, which dragged node-pty (and transitively
@parcel/watcher's native .node binary) into the renderer bundle
— esbuild had no loader for .node and failed the build.
@superset/shared is renderer-safe. Put the constant there so both
host-service and the renderer import the same single source of
truth without crossing the bundler boundary.
* fix(workspace-delete): auth + tolerate missing host-sqlite row
Two bugs unblocking the v2 delete flow:
1. v2Workspace.delete cloud procedure used protectedProcedure
(session), so host-service's JWT auth returned 401. Switch to
jwtProcedure (mirrors v2Workspace.create) with an org-membership
check derived from the workspace's organizationId. Returns
alreadyGone: true idempotently when the row is missing.
2. workspaceCleanup.destroy threw NOT_FOUND when the host-sqlite
row was missing, even though the workspace exists in the cloud
and belongs to this host. That state happens when the workspace
was created via a flow that didn't register it locally or the
host DB was reset — the user has no way to delete without this
fix. Now each disk step is gated on having the local row + a
resolvable project; missing rows surface as warnings and cloud
cleanup still proceeds.
Also makes ctx.api absence a warning instead of an upfront
PRECONDITION_FAILED, so local-only cleanup still completes.
* plans(workspace-delete): cloud-as-commit-point redesign
Reshape destroy as a linear preflight → teardown → cloud → local-
cleanup saga. No tombstones, no reconciler, no persistent state.
- Any failure before the cloud step leaves the workspace untouched.
- Any failure after is a warning (local orphans are cheap).
- Force skips preflight + teardown; cleanup is always --force past
the commit point.
- Three phases cleanly separated so future changes (auto-retry,
cross-device reconcile) land at a single seam.
Also updates the teardown contract to reflect the PTY-via-
createTerminalSessionInternal approach (env parity w/ v2 setup;
cross-platform timeout via pty.kill) and pins TEARDOWN_TIMEOUT_MS
to @superset/shared so the renderer doesn't drag node-pty into
its bundle.
* fix(workspace-delete): cloud is the commit point
Reorders workspaceCleanup.destroy into three phases so the failure
semantics match the plan doc:
0. Preflight (dirty worktree) — throws CONFLICT if !force
1. Teardown script — throws TEARDOWN_FAILED if !force
2. Cloud delete ← commit point; throws passthrough on failure
3. Local cleanup (PTYs, worktree, branch, host sqlite) — warnings only
Any failure in phases 0–2 leaves the workspace fully intact. The
user retries when they've addressed the cause (committed work, fixed
teardown, reconnected cloud, re-authed). Phase 3 is best-effort; local
orphans are cheap and surface as warnings.
No tombstones, no reconciler, no persistent state. Step 3b always
uses --force since we're past the commit point regardless of the
input flag.
* fix(workspace-delete): preserve TeardownFailureCause fields over wire
tRPC's `new TRPCError({ cause })` runs non-Error causes through
getCauseFromUnknown() which wraps them in a synthetic
UnknownCauseError (Error subclass) while copying fields as own
properties. Superjson's transformer recognises it as Error and
serialises only { name, message, stack } — our { kind, exitCode,
signal, timedOut, outputTail } fields were being dropped on the wire,
so the renderer saw an Error with no teardown metadata and
TeardownFailedPane crashed on stripAnsi(undefined).
The errorFormatter now rebuilds a plain object from the cause fields
so superjson serialises it as an object and the renderer gets the
full TeardownFailureCause.
Also defensive outputTail ?? "" in the pane so stripAnsi never
sees undefined.
* feat(ui/alert-dialog): make content text selectable
Desktop globals set user-select: none on html/body for a native feel.
Alert dialogs carry user-facing messages (error details, teardown
output tails, descriptions) that users need to copy — e.g. paste an
error into chat or grep a log snippet.
Applied at the component level so every AlertDialog in the app
benefits, not just the workspace-delete error panes.
* chore(workspace-delete): tighten stale comments + copy
- TeardownFailedPane: drop the defensive-round-trip note now that the
errorFormatter reliably serializes the cause as a plain object;
keep just the `?? ""` coalesce and the WHY of stripAnsi.
- ConflictPane: was phrased as "uncommitted or unlocked work" when
CONFLICT is now only thrown by the preflight dirty check (locked-
worktree cases fall to warnings). Rewrites copy + JSDoc to match.
- teardown.ts: timeout message said SIGKILL, but `pty.kill()` with no
args sends SIGHUP on Unix. Drop the false specificity.
* chore(workspace-delete): dev-only hook to simulate cloud failure
SUPERSET_DEBUG_FAIL_CLOUD_DELETE makes workspaceCleanup.destroy throw
at phase 2 (cloud delete) without needing to stop the cloud API dev
server. Used to verify the destroy saga's passthrough-on-cloud-fail
behavior end-to-end.
No-op when unset; guarded solely by env-var presence (truthy).
* Revert "chore(workspace-delete): dev-only hook to simulate cloud failure"
This reverts commit 5c88d88.
* fix(workspace-delete): review sweep
Addresses open PR comments:
- branchDeleted (P1, cubic): track via local flag inside the try
block instead of computing from preconditions, so the field reports
whether git branch actually succeeded.
- Teardown timeout can hang (coderabbit major): if pty.kill() doesn't
cause onExit to fire (zombie PTY), settle the promise directly after
a 2s grace window so workspaceCleanup.destroy never blocks forever.
- formatTeardownReason signal-terminated (coderabbit minor): handle
`exitCode === null && signal !== null && !timedOut` with a dedicated
"terminated by signal N" branch instead of falling through to
"failed to start".
- Duplicate run calls (coderabbit major): in-flight ref guard in
useDestroyDialogState.run so a rapid second click (same pane or
re-opened error pane) can't fire the mutation twice before the
first resolves.
- Global checkbox id (coderabbit minor): DestroyConfirmPane uses
useId() so the "Also delete local branch" label targeting doesn't
collide across dialog instances.
- Audit doc tense (cubic + coderabbit): v1-v2-delete-patterns-audit.md
now explicitly labeled as a pre-unification snapshot.
- Plan doc delivered-vs-follow-up split (coderabbit): the UI section
and work order clearly mark what landed in this PR vs. what's
follow-up (hotkey, EmptyTabView, V2WorkspaceRow affordance).
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (30)
✨ 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 |
MocA-Love
added a commit
that referenced
this pull request
Apr 15, 2026
-s ours merge to record that upstream commits a3e34bf through de70163 (13 commits) are semantically already present on origin/main via the PR1-6 cherry-pick series (PRs #176, #177, #178, #179, #180, #182), plus fork-adaptation fixes layered on top. This merge target is de70163 specifically (not upstream/main) so newer upstream commits (9fff075 and later) remain visible in future behind counts. Upstream commits covered by this audit merge: - a3e34bf fix(desktop): restore cmd+click requirement for v1 terminal file links (superset-sh#3457) [PR1/#176] - 57557f8 fix(desktop): gate v2 workspace children on collection readiness (superset-sh#3464) [PR1/#176] - 4ee2e61 fix(desktop): use native clipboard for copy path in v2 sidebar (superset-sh#3462) [PR1/#176] - 87d6e93 feat(desktop): close settings with Escape key (superset-sh#3466) [PR1/#176] - 9c7f5f4 chore(desktop): auto-restart host-service on bundle change in dev (superset-sh#3461) [PR1/#176] - 93140d9 fix(mcp): accept MCP resource URL as valid OAuth audience (superset-sh#3459) [PR2/#177] - be9e000 fix(desktop): drive tray menu off events, fetch real org name (superset-sh#3458) [PR2/#177] - c5f791e feat(v2): unify workspace delete through host-service (superset-sh#3443) [PR3/#178] - 2c24d93 feat(desktop): paginated branch picker with checkout + open actions (superset-sh#3397) [PR4/#179] - 2bf1049 feat(desktop/hotkeys): v1 directional pane focus + best-effort v1 override migrator (superset-sh#3460) [PR5/#180] - 1294a7d feat(desktop/hotkeys): restore Cmd+Alt+Arrow for tab/workspace nav (superset-sh#3472) [PR5/#180] - de70163 feat(desktop): v2 review tab first pass — PR info, checks, comments (superset-sh#3463) [PR6/#182] Intentionally skipped (version bump, fork has independent versioning): - 1e23353 chore(desktop): bump version to 1.5.5 (superset-sh#3473) Fork-adaptation fixes layered on top of the cherry-picks: - PR1: host-service-coordinator alias import fix, settings Escape selector narrowing (role-based + popper wrapper), Escape close uses replace navigation - PR2: dual quit mode preservation (requestQuit "release"/"stop"), trayUpdateToken guard for stale async fetchHostInfo results - PR4: ChangesHeader.normalizeBranchName regex rewrite (lint false positive), worktree add uses fullRef for remote-tracking refs, syncTimedOut reset on pendingId change, GIT_REFS.md barrel example fix - PR5: migrate.ts re-sanitize of existing localStorage overrides (v2 marker bump intent), FOCUS_PANE_* enabled:isActive for KeepAliveWorkspaces, CATEGORY_ORDER merges Navigation (upstream) and Browser (fork) - PR6: normalizeThreadsToComments flattens all thread.comments (not just first), CommentPane overrides <a> (openUrl) and <img> (SafeImage), zero-badge suppression, pr-null comments gate Fork features verified intact (Explore agent audit of combined 36d4de4..35d95f3 range): - BROWSER_RELOAD / BROWSER_HARD_RELOAD hotkeys - dual quit mode menu in tray - v1 terminal cold-restore + retry reconnect (out of range but unaffected) - KeepAliveWorkspaces (FOCUS_PANE_* gated on isActive) - useCommandPalette + addMemoTab in v2 workspace - host-service-coordinator rename alias pattern
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
2 participants
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.
概要
upstream superset-sh#3443
feat(v2): unify workspace delete through host-service(commitc5f791e9a) を取り込みます。30ファイル +1457/-107 の大型リファクタで、v2 ワークスペース削除フローを host-service 経由の統合パイプラインに置き換えます。主な変更
packages/host-service/src/trpc/router/workspace-cleanup/+packages/host-service/src/runtime/teardown/— ワークスペース teardown saga と error taxonomyuseDestroyWorkspacehook + 4つの新Pane (ConflictPane,DestroyConfirmPane,TeardownFailedPane,UnknownErrorPane) +useDestroyDialogStatehookprotectedProcedure→jwtProcedureに変更し host-service から cloud delete を呼べるように。org 外 delete は引き続きFORBIDDENalert-dialog.tsxにselect-text追加、Dashboard sidebar delete dialog の API をonConfirm/title/description→workspaceId/workspaceName/onDeletedにplans/v1-v2-delete-patterns-audit.md,plans/workspace-delete-unification.mdAuto-merge された3ファイル (fork差分との衝突なし)
DashboardSidebarWorkspaceItem.tsx— collapsed/expanded 分岐、context menu、row レイアウトは維持useDashboardSidebarWorkspaceItemActions.ts— direct な cloud delete 呼び出しが新フローに差し替わるだけ。rename/open-in-finder/copy-path 系の fork 既存挙動は温存packages/host-service/src/terminal/terminal.ts—disposeSessionexport とdisposeSessionsByWorkspaceId追加のみ。relay/tunnel/env 周りの fork 独自実装には未接触Codex 事前レビュー結果
軽微な注意点 (blocker ではない)
Codex が指摘した通り、
useDestroyDialogState.ts:72で unknown error のときに dialog を reopen せず toast のみにしているため、UnknownErrorPaneは現状ほぼ到達しません。これはコメントで記載された UX 判断なので、今回は触らず upstream 通りとします。テスト
bun installbun run lintbun run typecheck:/tmpworktree の既知問題 (@types/node 解決) により CI 側に委ねます次のステップ
このPRマージ後、PR4 (v2 paginated branch picker, +3013/-796) に進みます。