Skip to content

Bugfix/tab close bug#552

Merged
shantur merged 3 commits into
NeuralNomadsAI:devfrom
aayurt:bugfix/tabCloseBug
Jun 15, 2026
Merged

Bugfix/tab close bug#552
shantur merged 3 commits into
NeuralNomadsAI:devfrom
aayurt:bugfix/tabCloseBug

Conversation

@aayurt

@aayurt aayurt commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown

PR builds are available as GitHub Actions artifacts:

https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27473802868

Artifacts expire in 7 days.
Artifacts:

  • pr-552-bbfc2ace22e8672a76e4f77e4e47e16e39663ac0-tauri-macos
  • pr-552-bbfc2ace22e8672a76e4f77e4e47e16e39663ac0-tauri-linux
  • pr-552-bbfc2ace22e8672a76e4f77e4e47e16e39663ac0-tauri-windows
  • pr-552-bbfc2ace22e8672a76e4f77e4e47e16e39663ac0-electron-macos
  • pr-552-bbfc2ace22e8672a76e4f77e4e47e16e39663ac0-tauri-macos-arm64
  • pr-552-bbfc2ace22e8672a76e4f77e4e47e16e39663ac0-electron-windows
  • pr-552-bbfc2ace22e8672a76e4f77e4e47e16e39663ac0-electron-linux

@shantur

shantur commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

@CodeNomadBot

I want you to perform the gatekeeper checks for latest PR state

check for regressions
check if PR can be implemented better in a different way
Be the PR gatekeeper
Be ruthless about code quality

@codenomadbot

codenomadbot Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Gatekeeper review for latest PR state.

Verdict: changes requested. The intent is right, but the current implementation trades the stuck tab for a real consistency race.

Findings:

  1. Blocking: closing a tab now creates a duplicate-workspace race.
    packages/ui/src/stores/instances.ts:722-725 removes the instance from local state before the backend DELETE /api/workspaces/:id has completed, and the delete is now fire-and-forget. During that backend stop window, selecting/opening the same folder again will not hit getExistingInstanceForFolder() because the local instance is gone. The server WorkspaceManager.create() path creates a fresh workspace id without de-duping by folder (packages/server/src/workspaces/manager.ts:117-143), so this can launch a second OpenCode process for the same directory while the old one is still stopping. Before this PR, the local instance stayed present until the delete request returned, which largely prevented that user-level race. A safer fix is to keep explicit stopping/pending-close state client-side until the server acknowledges deletion, or move the latency fix server-side so DELETE accepts the stop request quickly while the server owns the pending-stop state and prevents duplicate creates for the same folder.

  2. High risk: stopInstance() no longer means “stop completed” despite being async and awaited.
    Callers still do await stopInstance(instanceId) (packages/ui/src/App.tsx:392, packages/ui/src/stores/instances.ts:1351), but after this PR that await resolves immediately after local cleanup, not after the backend stop/delete attempt. That is a semantic change hidden behind the same async API. If the desired UX is optimistic close, split the operations explicitly, e.g. closeInstanceTabOptimistically() plus a tracked backend stop promise, or keep stopInstance() resolving only when backend stop is known while a separate UI action handles immediate tab removal.

  3. Regression coverage is missing for the exact behavior being changed.
    There are no focused tests around stopInstance, workspace deletion, duplicate open prevention, or the new optimistic close behavior. Given this bug is a user-visible lifecycle race, this needs at least a small store-level regression test or an integration test that covers: close removes/marks the tab promptly, backend delete is invoked, delete failure is handled intentionally, and reopening the same folder while deletion is pending cannot create a duplicate workspace.

Implementation direction I would accept:

  • Prefer a server-owned lifecycle fix: make workspace deletion acknowledge quickly, mark workspace as stopping/deleting, reject or return the existing pending workspace for duplicate same-folder creates, and publish workspace.stopped when teardown finishes.
  • If keeping the UI-only fix, add a pending deletion registry keyed by workspace id/folder so getExistingInstanceForFolder() and createInstance() cannot create a second workspace for a folder whose old workspace is still being deleted.
  • Preserve a clear API contract: either stopInstance() waits for backend stop or it is renamed/refactored so callers do not mistake optimistic UI cleanup for completed teardown.

Validation performed:

  • git diff --check origin/dev...HEAD: passed.
  • npm run typecheck --workspace @codenomad/ui: could not validate due missing dependencies in this workspace (solid-js, @opencode-ai/sdk, Node types, etc.).
  • npm run build --workspace @codenomad/ui: could not validate due missing vite dependency in this workspace.

Additional note: packages/ui/src/stores/instances.ts is about 1,408 lines. Since this PR touches it, it is worth flagging for future refactor planning, but I would not bundle that refactor into this bugfix.

--
Yours,
CodeNomadBot

Keep the tab-close fix optimistic by removing the instance from local state immediately, but make the backend workspace deletion explicitly fire-and-forget instead of exposing a misleading async stop contract.

Remove stale awaits from call sites so callers no longer imply that stopInstance waits for backend teardown. If backend deletion fails after the tab is closed, surface the failure through the existing localized toast notification system while still logging the underlying error.

Validation: npm run typecheck --workspace @codenomad/ui; npm run build --workspace @codenomad/ui; git diff --check.
@shantur

shantur commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

@CodeNomadBot

And now?

@codenomadbot

codenomadbot Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Follow-up gatekeeper review after 86ac87a3.

Verdict: still changes requested.

What improved:

  • The hidden async contract problem is mostly clarified: stopInstance() is now synchronous, and callers no longer await it in App.tsx, session-picker.tsx, or acknowledgeDisconnectedInstance().
  • Backend stop failures now surface a localized error toast instead of only logging.
  • The new i18n key was added across the existing locale files.

Remaining blocker:

  1. The duplicate-workspace race is still present.
    packages/ui/src/stores/instances.ts:724-727 still calls removeInstance(id) before serverApi.deleteWorkspace(id) completes. That means the UI no longer sees the workspace as existing while the backend may still be stopping it. During that window, reopening/selecting the same folder can still bypass getExistingInstanceForFolder() and create a second workspace for the same directory. The server create path still does not de-dupe by folder or track pending deletion/stopping state, so the race from the previous review remains materially unchanged.

Why this matters:

  • The PR fixes the immediate symptom of a stuck tab by making close optimistic.
  • But it does not preserve the invariant that a folder maps to one active/stopping workspace.
  • A failure toast does not recover the removed local state if deletion fails, and it does not prevent duplicate launches while deletion is pending.

What I would accept:

  • Add a pending-stop/pending-delete registry keyed by folder, and make getExistingInstanceForFolder() / createInstance() treat pending deletion as an existing workspace until the backend delete settles.
  • Or, preferably, move this lifecycle handling server-side: make delete acknowledge quickly, mark the workspace as stopping/deleting, prevent duplicate creates for the same folder, and publish the final stopped event when teardown completes.
  • Add a focused regression test for closing then immediately reopening the same folder while delete is still pending.

Validation performed:

  • git diff --check origin/dev...HEAD: passed.
  • npm run typecheck --workspace @codenomad/ui: still blocked by missing installed dependencies/types in this workspace, starting with solid-js, @opencode-ai/sdk, and Node test types.
  • npm run build --workspace @codenomad/ui: still blocked by missing vite in this workspace.

Additional note: packages/ui/src/stores/instances.ts is now about 1,411 lines. This remains an oversized touched file worth refactor planning later, not as part of this bugfix.

--
Yours,
CodeNomadBot

@shantur shantur merged commit 9c8baf8 into NeuralNomadsAI:dev Jun 15, 2026
11 checks passed
@shantur

shantur commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Thanks @aayurt

@github-actions

Copy link
Copy Markdown

PR builds are available as GitHub Actions artifacts:

https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27534282506

Artifacts expire in 7 days.
Artifacts:

  • pr-552-86ac87a314a64918393123c82f9db0f26fc6a8ee-tauri-linux
  • pr-552-86ac87a314a64918393123c82f9db0f26fc6a8ee-tauri-macos
  • pr-552-86ac87a314a64918393123c82f9db0f26fc6a8ee-tauri-windows
  • pr-552-86ac87a314a64918393123c82f9db0f26fc6a8ee-electron-macos
  • pr-552-86ac87a314a64918393123c82f9db0f26fc6a8ee-tauri-macos-arm64
  • pr-552-86ac87a314a64918393123c82f9db0f26fc6a8ee-electron-windows
  • pr-552-86ac87a314a64918393123c82f9db0f26fc6a8ee-electron-linux

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