Skip to content

Close the active thread when it is deleted from another client #4091

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alfredmouelle wants to merge 1 commit into
base: main
Choose a base branch
from
+59 −14
Open

Close the active thread when it is deleted from another client #4091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions apps/server/src/server.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5665,6 +5665,58 @@ it.layer(NodeServices.layer)("server router seam", (it) => {
}).pipe(Effect.provide(NodeHttpServer.layerTest), TestClock.withLive),
);

it.effect("delivers thread.deleted to thread detail subscribers", () =>
Effect.gen(function* () {
const thread = makeDefaultOrchestrationReadModel().threads[0]!;
const liveEvents = yield* PubSub.unbounded<OrchestrationEvent>();
const deletedEvent = {
sequence: 2,
eventId: EventId.make("event-deleted"),
aggregateKind: "thread",
aggregateId: defaultThreadId,
occurredAt: "2026-01-01T00:00:01.000Z",
commandId: null,
causationEventId: null,
correlationId: null,
metadata: {},
type: "thread.deleted",
payload: {
threadId: defaultThreadId,
deletedAt: "2026-01-01T00:00:01.000Z",
},
} satisfies Extract<OrchestrationEvent, { type: "thread.deleted" }>;

yield* buildAppUnderTest({
layers: {
orchestrationEngine: {
streamDomainEvents: Stream.fromPubSub(liveEvents),
},
projectionSnapshotQuery: {
getThreadDetailSnapshot: () =>
Effect.gen(function* () {
yield* Effect.sleep("25 millis");
yield* PubSub.publish(liveEvents, deletedEvent);
return Option.some({ snapshotSequence: 1, thread });
}),
},
},
});

const wsUrl = yield* getWsServerUrl("/ws");
const items = yield* Effect.scoped(
withWsRpcClient(wsUrl, (client) =>
client[ORCHESTRATION_WS_METHODS.subscribeThread]({
threadId: defaultThreadId,
}).pipe(Stream.take(2), Stream.runCollect),
),
).pipe(Effect.timeout("2 seconds"));

assert.equal(items[0]?.kind, "snapshot");
assert.equal(items[1]?.kind, "event");
assert.equal(items[1]?.kind === "event" ? items[1].event.type : null, "thread.deleted");
}).pipe(Effect.provide(NodeHttpServer.layerTest), TestClock.withLive),
);

it.effect("enriches replayed project events with repository identity metadata", () =>
Effect.gen(function* () {
const repositoryIdentity = {
Expand Down
6 changes: 4 additions & 2 deletions apps/server/src/ws.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,8 @@ function isThreadDetailEvent(event: OrchestrationEvent): event is Extract<
| "thread.activity-appended"
| "thread.turn-diff-completed"
| "thread.reverted"
| "thread.session-set";
| "thread.session-set"
| "thread.deleted";
}
> {
return (
Expand All @@ -270,7 +271,8 @@ function isThreadDetailEvent(event: OrchestrationEvent): event is Extract<
event.type === "thread.activity-appended" ||
event.type === "thread.turn-diff-completed" ||
event.type === "thread.reverted" ||
event.type === "thread.session-set"
event.type === "thread.session-set" ||
event.type === "thread.deleted"
);
}

Expand Down
15 changes: 3 additions & 12 deletions apps/web/src/routes/_chat.$environmentId.$threadId.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { threadHasStarted } from "../components/ChatView.logic";
import { finalizePromotedDraftThreadByRef, useComposerDraftStore } from "../composerDraftStore";
import { resolveThreadRouteRef } from "../threadRoutes";
import { SidebarInset } from "~/components/ui/sidebar";
import { useEnvironmentThreadRefs, useThreadDetail, useThreadShell } from "../state/entities";
import { useThreadDetail, useThreadShell } from "../state/entities";
import { useEnvironmentQuery } from "../state/query";
import { environmentShell } from "../state/shell";

Expand All @@ -20,35 +20,26 @@ function ChatThreadRouteView() {
);
const serverThreadShell = useThreadShell(threadRef);
const serverThreadDetail = useThreadDetail(threadRef);
const environmentThreadRefs = useEnvironmentThreadRefs(threadRef?.environmentId ?? null);
const bootstrapComplete = shell.data?.snapshot._tag === "Some";
const threadExists = serverThreadShell !== null || serverThreadDetail !== null;
const environmentHasServerThreads = environmentThreadRefs.length > 0;
const draftThreadExists = useComposerDraftStore((store) =>
threadRef ? store.getDraftThreadByRef(threadRef) !== null : false,
);
const draftThread = useComposerDraftStore((store) =>
threadRef ? store.getDraftThreadByRef(threadRef) : null,
);
const environmentHasDraftThreads = useComposerDraftStore((store) => {
if (!threadRef) {
return false;
}
return store.hasDraftThreadsInEnvironment(threadRef.environmentId);
});
const routeThreadExists = threadExists || draftThreadExists;
const serverThreadStarted = threadHasStarted(serverThreadDetail);
const environmentHasAnyThreads = environmentHasServerThreads || environmentHasDraftThreads;

useEffect(() => {
if (!threadRef || !bootstrapComplete) {
return;
}

if (!routeThreadExists && environmentHasAnyThreads) {
if (!routeThreadExists) {

@cursor cursor Bot Jul 17, 2026

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.

Delete redirect races fallback navigation

Medium Severity

Forwarding thread.deleted clears detail state so routeThreadExists can become false while useThreadActions.deleteThread is still navigating to a fallback thread. This effect then fires an unawaited navigate to /, which can finish after the fallback navigation and leave same-client deletes on the empty state instead of the next thread.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 47b4ed3. Configure here.

@alfredmouelle alfredmouelle Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I looked into this specific scenario (same-client delete of the active thread while other threads remain, expecting to land on the fallback thread). I could not reproduce the bad outcome, and I believe the ordering makes it safe. Details below.

Empirical: with two threads present, deleting the active one from the same client consistently lands on the fallback thread and stays there (verified stable over ~1.5s, no transient flash to /). Local dev has near-zero WS latency, which is the worst case for thread.deleted arriving ahead of the mutation ack, and it still resolved correctly.

Why the ordering holds. In useThreadActions.deleteThread, everything between the mutation resolving and the fallback navigation is synchronous:

await deleteThreadMutation(...)      // thread.deleted may arrive around here
// clearComposerDraftForThread / clearProjectDraftThreadById / clearTerminalUiState are all sync, no await
if (shouldNavigateToFallback) { await router.navigate({ to: fallback, replace: true }) }

Two possible orderings:

  1. Ack before event: React can't re-render mid-synchronous-JS, so the thread.deleted handler (a separate task) can't fire the route guard before navigate(fallback) is issued. Fallback wins.
  2. Event before ack: the guard issues navigate({ to: "/" }), then deleteThread issues navigate(fallback) second. TanStack Router supersedes the pending navigation with the latest one, so the fallback still wins.

The only way to land on / would be case 2 plus the two replace navigations applying out of order, which the router's supersede behavior prevents. Production network latency delays the event relative to the ack, pushing things further toward case 1.

Worth noting the two navigations target different destinations by design: the guard is the cross-client safety net (goes to the empty state), while deleteThread picks a same-project fallback for local deletes. Even in the improbable event this raced, the worst case would be landing on the empty state instead of the next thread on a same-client delete, still a strict improvement over the current behavior, where the deleted thread stays fully rendered.

Happy to add a defensive guard (e.g. having deleteThread navigate before dispatch, or skipping the route redirect for in-flight local deletes) if you'd prefer belt-and-suspenders here.

void navigate({ to: "/", replace: true });
}
}, [bootstrapComplete, environmentHasAnyThreads, navigate, routeThreadExists, threadRef]);
}, [bootstrapComplete, navigate, routeThreadExists, threadRef]);

useEffect(() => {
if (!threadRef || !serverThreadStarted || !draftThread) {
Expand Down
Loading