From bb1a93ef22181f915201018f84793395f058e09c Mon Sep 17 00:00:00 2001 From: Cristian Date: Sun, 23 Aug 2026 07:27:34 -0500 Subject: [PATCH 1/2] feat: let a conversation be deleted, and its Intelligence thread with it Nothing removed a channel, so the roster only ever grew (#196). Adds DELETE /api/channels/:channelId: the channel row, its memberships, agent links, and thread mapping go through the FK cascades that already existed for them, and the deployment asks Intelligence to permanently delete the thread itself. The local delete commits first. A rejected or unreachable upstream thread delete is non-fatal and leaves the channel gone from the roster either way, with an audit row (channel.deleted) naming the thread and whether Intelligence actually forgot it - a channel gone locally with an orphaned thread is a smaller, more honest failure than one still sitting in the roster with its history silently wiped. Removal fans out over the existing channel_activity NOTIFY topic so other open tabs see it live. The sidebar row gets an options menu with a two-button confirm dialog (new alert-dialog.tsx, wrapping @base-ui/react/alert-dialog). --- CHANGELOG.md | 22 +++ app/src/components/app-sidebar/channel.tsx | 150 ++++++++++++++++---- app/src/components/ui/alert-dialog.tsx | 150 ++++++++++++++++++++ app/src/lib/channels/mutations.ts | 19 +++ app/src/lib/channels/use-channel-events.ts | 20 +++ server/src/app.ts | 21 +-- server/src/audit.ts | 6 + server/src/channels/events.ts | 2 + server/src/channels/routes.ts | 156 +++++++++++++++++++++ server/tests/channel-routes.test.ts | 137 ++++++++++++++++++ 10 files changed, 644 insertions(+), 39 deletions(-) create mode 100644 app/src/components/ui/alert-dialog.tsx diff --git a/CHANGELOG.md b/CHANGELOG.md index fb11af23..35bf09e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -235,6 +235,28 @@ one they are, so those match too. No configuration changes and nothing is stored differently; a deployment that was already on the light theme sees no difference at all. +### A conversation can be deleted + +Nothing removed a channel. Starting one was the only lever the product gave a person, and every +conversation with every coworker sat in the roster forever, growing on every message the way +`DEFAULT_CHANNEL_PAGE`'s own note already described: a page that was instant in a demo returns +thousands of rows for anybody who has actually been using the product a while, one that never shrinks +again. + +Deleting a channel now removes it for good. The channel row goes, and its memberships, its linked +coworkers, and its Intelligence thread mapping go with it through the same foreign-key cascades that +already existed for them — no migration needed, only a query that finally uses them. The deployment +also asks Intelligence to permanently delete the thread itself, so the message history is not just +unlisted, it is gone from the platform too. + +A thread the platform refuses to delete does not hold the channel hostage. The local removal already +committed by the time that call runs, so a rejected or unreachable upstream delete leaves the channel +gone from the roster regardless, with an audit row (`channel.deleted`) naming the thread and whether +Intelligence actually forgot it. A channel that is gone locally with an orphaned thread still on the +platform is a smaller, more honest failure than a channel sitting in the roster with its history +silently wiped out from under it — and the audit trail is where an administrator finds the one that +did not clean up completely. + ## 0.0.4 ### A click citing a ref this deployment cannot resolve is refused diff --git a/app/src/components/app-sidebar/channel.tsx b/app/src/components/app-sidebar/channel.tsx index 74314781..28c1e40d 100644 --- a/app/src/components/app-sidebar/channel.tsx +++ b/app/src/components/app-sidebar/channel.tsx @@ -1,12 +1,31 @@ -import { Link } from "@tanstack/react-router"; -import { memo } from "react"; +import { IconDots } from "@tabler/icons-react"; +import { useMutation, useQueryClient } from "@tanstack/react-query"; +import { Link, useNavigate, useParams } from "@tanstack/react-router"; +import { useState } from "react"; +import { deleteChannelMutationOptions } from "@/lib/channels/mutations"; import { ChannelAvatar } from "../channels/avatar"; +import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from "../ui/alert-dialog"; +import { Button } from "../ui/button"; +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuGroup, + DropdownMenuItem, + DropdownMenuTrigger, +} from "../ui/dropdown-menu"; -/** - * Memoized roster row. `use-channel-events` preserves unchanged row identity, and - * `content-visibility` keeps off-screen rows cheap without virtualization. - */ -export const Channel = memo(function Channel({ +// No longer `memo`: the delete dialog needs its own open state and the current route's channel id, +// both independent of the props `use-channel-events` keeps stable. +export function Channel({ channelId, participantIds, name, @@ -19,32 +38,101 @@ export const Channel = memo(function Channel({ lastMessage?: string; lastMessageAt?: string; }) { + const queryClient = useQueryClient(); + const navigate = useNavigate(); + // `strict: false`: this row renders in the sidebar on every screen, not only while its own + // channel is open, so there may be no `channelId` route param to read at all. + const { channelId: openChannelId } = useParams({ strict: false }); + const [deleteDialogOpen, setDeleteDialogOpen] = useState(false); + const deleteChannel = useMutation(deleteChannelMutationOptions(queryClient)); + + const handleDelete = async () => { + // Navigate away first: the row this menu lives on unmounts the moment the list invalidates, + // and a screen still pointed at a channel id that no longer resolves is worse than a screen + // that moved on a beat early. + if (openChannelId === channelId) { + await navigate({ to: "/" }); + } + deleteChannel.mutate(channelId); + }; + return ( - -
- -
-
-
- {name} -
- {lastMessageAt} -
+
+ +
+
-
- - {lastMessage} - +
+
+ {name} +
+ {lastMessageAt} +
+
+
+ + {lastMessage} + +
+ +
+ + + + + } + /> + + + {/* Only opens the dialog below; the menu closes on click, too early to confirm anything. */} + setDeleteDialogOpen(true)} + variant="destructive" + > + Delete + + + +
- + + + + Delete this conversation? + + This deletes your conversation with{" "} + {name}, + including its message history. This cannot be undone. + + + + + Cancel + + void handleDelete()} + > + {deleteChannel.isPending ? "Deleting…" : "Delete"} + + + + +
); -}); +} diff --git a/app/src/components/ui/alert-dialog.tsx b/app/src/components/ui/alert-dialog.tsx new file mode 100644 index 00000000..b30e42a2 --- /dev/null +++ b/app/src/components/ui/alert-dialog.tsx @@ -0,0 +1,150 @@ +import { AlertDialog as AlertDialogPrimitive } from "@base-ui/react/alert-dialog"; +import type * as React from "react"; + +import { Button } from "@/components/ui/button"; +import { cn } from "@/lib/utils"; + +/** + * A modal for an action a click cannot undo. Built on `AlertDialogRoot` rather than `Dialog`'s + * `DialogRoot`: it carries `role="alertdialog"` and is announced immediately, and has no corner + * close button — the only way out is one of the footer's own buttons. + */ +function AlertDialog({ ...props }: AlertDialogPrimitive.Root.Props) { + return ; +} + +function AlertDialogTrigger({ ...props }: AlertDialogPrimitive.Trigger.Props) { + return ( + + ); +} + +function AlertDialogPortal({ ...props }: AlertDialogPrimitive.Portal.Props) { + return ( + + ); +} + +function AlertDialogOverlay({ + className, + ...props +}: AlertDialogPrimitive.Backdrop.Props) { + return ( + + ); +} + +function AlertDialogContent({ + className, + ...props +}: AlertDialogPrimitive.Popup.Props) { + return ( + + + + + ); +} + +function AlertDialogHeader({ className, ...props }: React.ComponentProps<"div">) { + return ( +
+ ); +} + +function AlertDialogFooter({ className, ...props }: React.ComponentProps<"div">) { + return ( +
+ ); +} + +function AlertDialogTitle({ + className, + ...props +}: AlertDialogPrimitive.Title.Props) { + return ( + + ); +} + +function AlertDialogDescription({ + className, + ...props +}: AlertDialogPrimitive.Description.Props) { + return ( + + ); +} + +/** The button that answers "no" or "not now." Closes without running anything else. */ +function AlertDialogCancel({ + className, + ...props +}: React.ComponentProps) { + return ( + } + /> + ); +} + +/** + * The button that carries out the action, styled destructive by default since that is the only + * reason this component exists rather than the ordinary `Dialog`. Pass `onClick` to run the action; + * closing is automatic, the same as `AlertDialogCancel`. + */ +function AlertDialogAction({ + className, + variant = "destructive", + ...props +}: React.ComponentProps) { + return ( + } + /> + ); +} + +export { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogOverlay, + AlertDialogPortal, + AlertDialogTitle, + AlertDialogTrigger, +}; diff --git a/app/src/lib/channels/mutations.ts b/app/src/lib/channels/mutations.ts index 95f7185d..efe30203 100644 --- a/app/src/lib/channels/mutations.ts +++ b/app/src/lib/channels/mutations.ts @@ -30,6 +30,25 @@ export function createChannelMutationOptions(queryClient: QueryClient) { * * Fire-and-forget on purpose: a failed preview update is a stale roster line, not a lost message. */ +/** + * Other tabs learn a channel is gone from the socket event in `use-channel-events.ts`; this tab + * issued the delete itself and never receives its own event, so it clears the roster and detail + * cache directly on success. + */ +export function deleteChannelMutationOptions(queryClient: QueryClient) { + return mutationOptions({ + mutationFn: (channelId: string) => + client(`/api/channels/${channelId}`, { + method: "DELETE", + fallback: "Could not delete this conversation", + }), + onSuccess: (_data, channelId) => { + queryClient.invalidateQueries({ queryKey: channelKeys.all }); + queryClient.removeQueries({ queryKey: channelKeys.detail(channelId) }); + }, + }); +} + export function recordChannelActivityMutationOptions() { return mutationOptions({ mutationFn: async (variables: { diff --git a/app/src/lib/channels/use-channel-events.ts b/app/src/lib/channels/use-channel-events.ts index d8737b86..23f5eb3a 100644 --- a/app/src/lib/channels/use-channel-events.ts +++ b/app/src/lib/channels/use-channel-events.ts @@ -14,6 +14,8 @@ type ChannelActivityEvent = { lastMessage: string | null; lastMessageAt: string | null; lastMessageAgentId: string | null; + /** The channel is gone. Absent on an ordinary activity event. */ + deleted?: true; }; const FIRST_RETRY_MS = 500; @@ -72,6 +74,24 @@ export function useChannelEvents() { (channel) => channel.id === activity.channelId, ), ); + + // Must run before the patch below, which spreads the event onto the existing row — + // reaching that first would stamp `deleted: true` on the row instead of removing it. + // An unknown channel here is already gone from this cache, so there is nothing to patch + // or invalidate for, unlike the "unknown channel" case below for an ordinary event. + if (activity.deleted) { + if (holdingPage === -1) return data; + const page = data.pages[holdingPage] as ChannelPage; + const pages = data.pages.slice(); + pages[holdingPage] = { + ...page, + channels: page.channels.filter( + (channel) => channel.id !== activity.channelId, + ), + }; + return { ...data, pages }; + } + // An unknown channel id means the roster is stale; refetch rather than patch. if (holdingPage === -1) { void queryClient.invalidateQueries({ diff --git a/server/src/app.ts b/server/src/app.ts index fcf65c87..edd47dff 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -686,10 +686,19 @@ export function createApp( } } + // Shared by the thread-existence check below and a deleted channel's request to forget its thread. + const intelligence = createIntelligenceClient(config.runtime.intelligence); + if (channelStore) { app.route( "/api/channels", - createChannelRoutes(channelStore, requireUser, channelEvents), + createChannelRoutes( + channelStore, + requireUser, + channelEvents, + auditStore, + (params) => intelligence.deleteThread(params), + ), ); } @@ -828,13 +837,9 @@ export function createApp( threadIdentity, requireUser, // config.ts refuses to boot without the full Intelligence contract (see copilot.ts's - // header comment), so `config.runtime.intelligence` is never missing here. Built from it - // rather than assumed, though: this is the one place besides the runtime mount itself that - // needs to reach Intelligence, and it should keep working unmodified if that guarantee ever - // loosens and a deployment can legitimately have no reader to build. - createThreadReader( - createIntelligenceClient(config.runtime.intelligence), - ), + // header comment), so `config.runtime.intelligence` is never missing here, and the shared + // `intelligence` client built above is never missing either. + createThreadReader(intelligence), ), ); } diff --git a/server/src/audit.ts b/server/src/audit.ts index 24ff40f8..d504024e 100644 --- a/server/src/audit.ts +++ b/server/src/audit.ts @@ -50,6 +50,12 @@ export const auditEventTypes = [ * routing decision is a fact about where a conversation went, not a copy of what was said. */ "channel.routed", + /** + * A channel was deleted, taking its memberships, agent links, and thread mapping with it. The + * channel row is already gone by the time this is written, so this is the only place left that + * says it ever existed. `payload.threadForgotten: false` marks a thread that outlived it. + */ + "channel.deleted", "agent.invoked", /** * An address this deployment declined to dial for a Bot, and why. diff --git a/server/src/channels/events.ts b/server/src/channels/events.ts index 3bd6cd12..b0682a41 100644 --- a/server/src/channels/events.ts +++ b/server/src/channels/events.ts @@ -22,6 +22,8 @@ export type ChannelActivityEvent = { lastMessage: string | null; lastMessageAt: string | null; lastMessageAgentId: string | null; + /** The channel is gone. Absent on an ordinary activity event. */ + deleted?: true; }; type Send = (payload: string) => void; diff --git a/server/src/channels/routes.ts b/server/src/channels/routes.ts index 9dfa8065..31883d82 100644 --- a/server/src/channels/routes.ts +++ b/server/src/channels/routes.ts @@ -6,6 +6,7 @@ import { type AgentProfileStore, } from "../agents/profile-store"; import type { AgentActor, AgentProfile } from "../agents/profile-types"; +import { type AuditStore, recordAuditEvent } from "../audit"; import type { AppVariables } from "../auth/guards"; import type { Database } from "../db/client"; import { @@ -102,8 +103,19 @@ export type ChannelStore = { channelId: string, activity: ChannelActivity, ): Promise; + /** Deletes the channel for everyone in it. Returns the thread it owned, so the caller can forget it upstream. */ + remove(actor: AgentActor, channelId: string): Promise; }; +/** + * The local development actor, which is not a row in `users`. + * + * The audit table has a foreign key to that table, so writing this id would fail the constraint and + * lose the row entirely. Who it was is in the payload's target either way. Same reasoning as + * agents/routes.ts's constant of the same name. + */ +const DEV_ACTOR_EMAIL = "dev@openbot.local"; + const PRIVATE_AGENT_CHANNEL_DESCRIPTION = "Private agent channel."; const MAX_CHANNEL_NAME_CODE_POINTS = 120; const MAX_ACTIVITY_CODE_POINTS = 200; @@ -438,6 +450,64 @@ export function createChannelStore( { isolationLevel: "read committed" }, ); }, + + // `create` inserts exactly one membership row, so "delete" and "leave" are the same act while a + // channel has exactly one member. Named `remove` for the multi-member split this becomes later. + remove(actor, channelId) { + return database.transaction( + async (transaction) => { + const [membership] = await transaction + .select({ channelId: channelMemberships.channelId }) + .from(channelMemberships) + .where( + and( + eq(channelMemberships.channelId, channelId), + eq(channelMemberships.userId, actor.id), + ), + ); + // Not a member, or no such channel: the same answer either way, so belonging to a channel + // is not something an outsider can probe for. Same reasoning as `recordActivity` above. + if (!membership) throw new ChannelNotFoundError(channelId); + + // Read before the delete, not after: the cascade below wipes both of these tables, and the + // notify payload needs the members it is telling, while the caller needs the thread id to + // ask Intelligence to forget it. + const members = await transaction + .select({ userId: channelMemberships.userId }) + .from(channelMemberships) + .where(eq(channelMemberships.channelId, channelId)); + const [mapping] = await transaction + .select({ threadId: intelligenceChannelMappings.threadId }) + .from(intelligenceChannelMappings) + .where( + and( + eq(intelligenceChannelMappings.channelId, channelId), + eq(intelligenceChannelMappings.userId, actor.id), + ), + ); + + // Cascades `channel_memberships`, `channel_agents`, and `intelligence_channel_mappings`: + // see the `onDelete: "cascade"` on each in db/schema/core.ts. Nothing else references a + // channel, so this one delete is the whole local removal. + await transaction.delete(channels).where(eq(channels.id, channelId)); + + const event: ChannelActivityEvent = { + channelId, + memberIds: members.map((member) => member.userId), + lastMessage: null, + lastMessageAt: null, + lastMessageAgentId: null, + deleted: true, + }; + await transaction.execute( + sql`select pg_notify(${CHANNEL_ACTIVITY_TOPIC}, ${JSON.stringify(event)})`, + ); + + return mapping?.threadId ?? null; + }, + { isolationLevel: "read committed" }, + ); + }, }; } @@ -528,9 +598,54 @@ export function createChannelRoutes( requireUser: MiddlewareHandler<{ Variables: AppVariables }>, /** Absent in tests and wherever live updates are not wanted; the routes still work without it. */ events?: ChannelEventHub, + /** Where a channel's deletion is written. Absent in tests that do not care about the trail. */ + auditStore?: AuditStore, + /** + * Ask Intelligence to permanently delete a thread. Absent leaves a channel deletable and its + * thread left behind on the platform: the local removal below does not depend on this existing. + */ + forgetThread?: (params: { + threadId: string; + userId: string; + agentId: string; + }) => Promise, ) { const routes = new Hono<{ Variables: AppVariables }>(); + /** + * Write the one audit row this file ever writes, tolerantly. + * + * Mirrors `record` in agents/routes.ts: never fatal, because the channel is already gone and the + * caller has already been told so by the time this runs. A trail that is briefly unavailable is + * not a reason to report a failure that did not happen. + */ + const recordDeleted = async ( + context: Context<{ Variables: AppVariables }>, + channelId: string, + payload: { threadId: string | null; threadForgotten: boolean }, + ): Promise => { + if (!auditStore) return; + const actor = context.var.actor; + try { + await recordAuditEvent(auditStore, { + eventType: "channel.deleted", + targetType: "channel", + targetId: channelId, + ...(actor.email !== DEV_ACTOR_EMAIL ? { actorUserId: actor.id } : {}), + payload, + }); + } catch (error) { + console.error( + JSON.stringify({ + type: "channel-audit-write-failed", + eventType: "channel.deleted", + channelId, + error: String(error), + }), + ); + } + }; + // Before `/:channelId`, or "events" is read as a channel id. if (events) { routes.get( @@ -622,6 +737,47 @@ export function createChannelRoutes( } }); + // Registered unconditionally, unlike `GET /:threadId` in thread-routes.ts: removing your own + // channel from your own roster can always succeed locally, whether or not Intelligence is reachable. + routes.delete("/:channelId", requireUser, async (context) => { + const channelId = context.req.param("channelId"); + try { + const threadId = await store.remove(context.var.actor, channelId); + + // The local delete already committed above, so a failed thread deletion is non-fatal: a + // channel gone locally with an orphaned thread beats one still on screen with its history wiped. + let threadForgotten = false; + if (threadId && forgetThread) { + try { + await forgetThread({ + threadId, + userId: context.var.actor.id, + // Derived here, never accepted from the caller: this is the same string + // channel-chat.tsx builds for the same channel, and trusting a client-supplied agentId + // would let a request name any thread it likes and ask the platform to delete it. + agentId: `channel:${channelId}`, + }); + threadForgotten = true; + } catch { + console.error( + JSON.stringify({ + type: "channel-thread-forget-failed", + note: "Could not delete the Intelligence thread for a removed channel.", + channelId, + threadId, + }), + ); + } + } + + await recordDeleted(context, channelId, { threadId, threadForgotten }); + + return context.body(null, 204); + } catch (error) { + return mapStoreError(context, error); + } + }); + return routes; } diff --git a/server/tests/channel-routes.test.ts b/server/tests/channel-routes.test.ts index af81fc33..90256274 100644 --- a/server/tests/channel-routes.test.ts +++ b/server/tests/channel-routes.test.ts @@ -65,6 +65,10 @@ function fakeStore( calls.push(["get", receivedActor, id]); return channel({ id }); }, + async remove(receivedActor, id) { + calls.push(["remove", receivedActor, id]); + return "thread-1"; + }, }; return Object.assign(base, overrides, { calls }); @@ -292,6 +296,100 @@ describe("channel routes", () => { }); }); +describe("channel delete route", () => { + function appWithForget( + store: ChannelStore, + forgetThread?: (params: { + threadId: string; + userId: string; + agentId: string; + }) => Promise, + ) { + const app = new Hono<{ Variables: AppVariables }>(); + app.route( + "/", + createChannelRoutes( + store, + requireUser, + undefined, + undefined, + forgetThread, + ), + ); + return app; + } + + test("returns 204 and calls store.remove with the actor and channel id", async () => { + const store = fakeStore(); + const response = await appWithForget(store).request( + "http://openbot.test/channel-1", + { method: "DELETE" }, + ); + + expect(response.status).toBe(204); + expect(store.calls).toEqual([["remove", actor, "channel-1"]]); + }); + + test("maps ChannelNotFoundError to 404", async () => { + const store = fakeStore({ + remove: async () => { + throw new ChannelNotFoundError("channel-1"); + }, + }); + const response = await appWithForget(store).request( + "http://openbot.test/channel-1", + { method: "DELETE" }, + ); + + expect(response.status).toBe(404); + expect(await json(response)).toEqual({ error: "Channel not found." }); + }); + + test("calls forgetThread with the derived channel-scoped agent id", async () => { + const store = fakeStore(); + const calls: unknown[] = []; + const response = await appWithForget(store, async (params) => { + calls.push(params); + }).request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(response.status).toBe(204); + expect(calls).toEqual([ + { threadId: "thread-1", userId: actor.id, agentId: "channel:channel-1" }, + ]); + }); + + /* + * The critical ordering-decision regression test: a rejected upstream delete must not become a + * failure response. The local removal already committed by the time `forgetThread` runs, so this + * MUST fail if a later change propagates that rejection as an error status instead of swallowing + * it and still answering 204. + */ + test("still returns 204 when forgetThread rejects", async () => { + const store = fakeStore(); + const response = await appWithForget(store, async () => { + throw new Error("Intelligence is unreachable"); + }).request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(response.status).toBe(204); + }); + + test("does not call forgetThread when remove found no thread to forget", async () => { + const store = fakeStore({ + remove: async (receivedActor, id) => { + store.calls.push(["remove", receivedActor, id]); + return null; + }, + }); + let called = false; + const response = await appWithForget(store, async () => { + called = true; + }).request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(response.status).toBe(204); + expect(called).toBe(false); + }); +}); + describe("channel route composition", () => { test("mounts the store behind createApp authentication with the derived actor", async () => { const store = fakeStore(); @@ -739,6 +837,45 @@ describe("channel store integration", () => { expect(await channelTableSnapshot()).toEqual(before); }, ); + + test("deletes the channel row and cascades memberships, agents, and the thread mapping", async () => { + const actor = await createPersistentUser(); + const agentId = await createPersistentAgent({ + name: "Removable agent", + owner: actor, + }); + const created = await persistentStore.create(actor, [agentId]); + // Not pushed to createdChannelIds: `remove` is the thing under test, and afterEach's cleanup + // deleting an already-deleted row is a no-op either way. + + const threadId = await persistentStore.remove(actor, created.id); + + expect(threadId).toBe(created.threadId); + const persisted = await persistedChannel(created.id); + expect(persisted.channelRow).toBeUndefined(); + expect(persisted.memberships).toEqual([]); + expect(persisted.linkedAgents).toEqual([]); + expect(persisted.mappings).toEqual([]); + }); + + test("refuses a non-member's remove and leaves the channel row untouched", async () => { + const owner = await createPersistentUser(); + const outsider = await createPersistentUser(); + const agentId = await createPersistentAgent({ + name: "Guarded agent", + owner, + }); + const created = await persistentStore.create(owner, [agentId]); + createdChannelIds.push(created.id); + + await expect( + persistentStore.remove(outsider, created.id), + ).rejects.toBeInstanceOf(ChannelNotFoundError); + + expect((await persistedChannel(created.id)).channelRow).toMatchObject({ + id: created.id, + }); + }); }); /** From ec92a341f42892d49b2d9ad7eb912d475261c4f5 Mon Sep 17 00:00:00 2001 From: David McKay Date: Mon, 24 Aug 2026 10:02:13 -0700 Subject: [PATCH 2/2] Keep the roster memoized, attribute the row, and stop a failed delete vanishing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The confirm button was AlertDialogAction, which renders the primitive's Close: it shut the dialog the instant it was pressed, before the request it started was answered. A delete that failed then reported nothing at all, leaving the conversation in the roster with no explanation, and "Deleting…" could never appear. A plain button waits for the answer, closes on success, and shows the server's message otherwise. DELETE answers 200 with historyLeftBehind rather than a bare 204. The thread deletion is the half that can fail on its own, and 204 said the whole act succeeded whichever way it went, so a screen had no way to avoid claiming a message history was gone while it was still on the platform. The roster row lost its memo for a stated reason that does not hold: memo compares props and says nothing about a hook, and use-channel-events preserves row identity precisely so rows do not re-render. The audit row dropped the actor in single-user mode, believing audit_events.actor_user_id has a foreign key into users. It has none, and initializeDevActorUser writes that row at start-up anyway. Single-user is the mode .env.example ships switched on, so that was the default row, and it recorded that a conversation was deleted but not by whom. Also move a tab that is looking at the channel another tab just deleted, which was left on a route that no longer resolves, and cover the audit row, which nothing tested. --- CHANGELOG.md | 5 +- app/src/components/app-sidebar/channel.tsx | 54 +++++++-- app/src/lib/channels/mutations.ts | 30 +++-- app/src/lib/channels/use-channel-events.ts | 22 +++- server/src/channels/routes.ts | 38 ++++-- server/tests/channel-routes.test.ts | 129 +++++++++++++++++++-- 6 files changed, 235 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35bf09e7..ecd611c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -254,8 +254,9 @@ committed by the time that call runs, so a rejected or unreachable upstream dele gone from the roster regardless, with an audit row (`channel.deleted`) naming the thread and whether Intelligence actually forgot it. A channel that is gone locally with an orphaned thread still on the platform is a smaller, more honest failure than a channel sitting in the roster with its history -silently wiped out from under it — and the audit trail is where an administrator finds the one that -did not clean up completely. +silently wiped out from under it, and the audit trail is where an administrator finds the one that +did not clean up completely. `DELETE /api/channels/:channelId` answers with `historyLeftBehind`, so a +screen showing the outcome does not have to guess which of the two happened. ## 0.0.4 diff --git a/app/src/components/app-sidebar/channel.tsx b/app/src/components/app-sidebar/channel.tsx index 28c1e40d..9b15b799 100644 --- a/app/src/components/app-sidebar/channel.tsx +++ b/app/src/components/app-sidebar/channel.tsx @@ -1,12 +1,11 @@ import { IconDots } from "@tabler/icons-react"; import { useMutation, useQueryClient } from "@tanstack/react-query"; import { Link, useNavigate, useParams } from "@tanstack/react-router"; -import { useState } from "react"; +import { memo, useState } from "react"; import { deleteChannelMutationOptions } from "@/lib/channels/mutations"; import { ChannelAvatar } from "../channels/avatar"; import { AlertDialog, - AlertDialogAction, AlertDialogCancel, AlertDialogContent, AlertDialogDescription, @@ -23,9 +22,15 @@ import { DropdownMenuTrigger, } from "../ui/dropdown-menu"; -// No longer `memo`: the delete dialog needs its own open state and the current route's channel id, -// both independent of the props `use-channel-events` keeps stable. -export function Channel({ +/** + * Memoized roster row. `use-channel-events` preserves unchanged row identity, and + * `content-visibility` keeps off-screen rows cheap without virtualization. + * + * State inside a row is no reason to drop the memo: `memo` compares the props it is handed and has + * nothing to say about a hook. Dropping it re-renders every row in the roster whenever the sidebar + * renders, which is the cost the identity-preserving patch in `use-channel-events` exists to avoid. + */ +export const Channel = memo(function Channel({ channelId, participantIds, name, @@ -53,7 +58,23 @@ export function Channel({ if (openChannelId === channelId) { await navigate({ to: "/" }); } - deleteChannel.mutate(channelId); + try { + await deleteChannel.mutateAsync(channelId); + /* + * Closed on success rather than left to the unmount. + * + * The row does go away when the roster invalidates, taking this dialog with it, but that is a + * side effect of a cache write and not something this component controls. + */ + setDeleteDialogOpen(false); + } catch { + /* + * Left open, deliberately. A delete that failed leaves the row exactly where it was, so + * closing would return the person to a roster that still lists the conversation they just + * asked to be rid of, with nothing anywhere saying why. The message is rendered below; + * `mutateAsync` rejects rather than swallowing, which is why this catch exists at all. + */ + } }; return ( @@ -120,19 +141,34 @@ export function Channel({ including its message history. This cannot be undone. + {deleteChannel.isError ? ( +

+ {deleteChannel.error.message} +

+ ) : null} Cancel - void handleDelete()} + variant="destructive" > {deleteChannel.isPending ? "Deleting…" : "Delete"} - +
); -} +}); diff --git a/app/src/lib/channels/mutations.ts b/app/src/lib/channels/mutations.ts index efe30203..1fb3f76c 100644 --- a/app/src/lib/channels/mutations.ts +++ b/app/src/lib/channels/mutations.ts @@ -23,25 +23,27 @@ export function createChannelMutationOptions(queryClient: QueryClient) { } /** - * Report the last thing said in a channel. + * Delete a channel, and ask the platform to forget the thread behind it. * - * The client that ran the agent already has the message before platform replay can return it; the - * runtime exposes no run-completion hook and its run endpoint returns before the reply exists. - * - * Fire-and-forget on purpose: a failed preview update is a stale roster line, not a lost message. - */ -/** * Other tabs learn a channel is gone from the socket event in `use-channel-events.ts`; this tab * issued the delete itself and never receives its own event, so it clears the roster and detail * cache directly on success. + * + * Resolves to whether the message history outlived the channel. The local delete commits first and + * the thread deletion can fail on its own, so this is not a failure to throw on: the conversation + * is gone either way, and the caller shows the residue rather than reporting an error that did not + * happen. */ export function deleteChannelMutationOptions(queryClient: QueryClient) { return mutationOptions({ - mutationFn: (channelId: string) => - client(`/api/channels/${channelId}`, { + mutationFn: async (channelId: string): Promise => { + const response = await client(`/api/channels/${channelId}`, { method: "DELETE", fallback: "Could not delete this conversation", - }), + }); + const body = (await response.json()) as { historyLeftBehind?: boolean }; + return body.historyLeftBehind === true; + }, onSuccess: (_data, channelId) => { queryClient.invalidateQueries({ queryKey: channelKeys.all }); queryClient.removeQueries({ queryKey: channelKeys.detail(channelId) }); @@ -49,6 +51,14 @@ export function deleteChannelMutationOptions(queryClient: QueryClient) { }); } +/** + * Report the last thing said in a channel. + * + * The client that ran the agent already has the message before platform replay can return it; the + * runtime exposes no run-completion hook and its run endpoint returns before the reply exists. + * + * Fire-and-forget on purpose: a failed preview update is a stale roster line, not a lost message. + */ export function recordChannelActivityMutationOptions() { return mutationOptions({ mutationFn: async (variables: { diff --git a/app/src/lib/channels/use-channel-events.ts b/app/src/lib/channels/use-channel-events.ts index 23f5eb3a..63459dd4 100644 --- a/app/src/lib/channels/use-channel-events.ts +++ b/app/src/lib/channels/use-channel-events.ts @@ -1,4 +1,5 @@ import { useQueryClient } from "@tanstack/react-query"; +import { useRouter } from "@tanstack/react-router"; import { useEffect } from "react"; import { type ChannelPage, type ChannelSummary, channelKeys } from "./queries"; @@ -29,6 +30,7 @@ function socketUrl() { export function useChannelEvents() { const queryClient = useQueryClient(); + const router = useRouter(); useEffect(() => { let socket: WebSocket | undefined; @@ -123,6 +125,24 @@ export function useChannelEvents() { return { ...data, pages }; }, ); + + /* + * A tab looking at the channel somebody just deleted in another tab. + * + * The tab that issued the delete moves itself before it fires the request. Every other tab + * only ever hears about it here, and dropping the row without moving leaves that tab on a + * route whose channel no longer resolves: an error, or an empty conversation, depending on + * which query answers first. + * + * Read off the router at event time rather than through `useParams`, so the effect does not + * have to be torn down and reconnected on every navigation just to keep this value fresh. + */ + if (activity.deleted) { + const { pathname } = router.state.location; + if (pathname === `/channel/${activity.channelId}`) { + void router.navigate({ to: "/" }); + } + } }; // WebSocket needs explicit reconnect handling. @@ -142,7 +162,7 @@ export function useChannelEvents() { if (socket) socket.onclose = null; socket?.close(); }; - }, [queryClient]); + }, [queryClient, router]); } /** diff --git a/server/src/channels/routes.ts b/server/src/channels/routes.ts index 31883d82..a912aa68 100644 --- a/server/src/channels/routes.ts +++ b/server/src/channels/routes.ts @@ -107,15 +107,6 @@ export type ChannelStore = { remove(actor: AgentActor, channelId: string): Promise; }; -/** - * The local development actor, which is not a row in `users`. - * - * The audit table has a foreign key to that table, so writing this id would fail the constraint and - * lose the row entirely. Who it was is in the payload's target either way. Same reasoning as - * agents/routes.ts's constant of the same name. - */ -const DEV_ACTOR_EMAIL = "dev@openbot.local"; - const PRIVATE_AGENT_CHANNEL_DESCRIPTION = "Private agent channel."; const MAX_CHANNEL_NAME_CODE_POINTS = 120; const MAX_ACTIVITY_CODE_POINTS = 200; @@ -631,7 +622,17 @@ export function createChannelRoutes( eventType: "channel.deleted", targetType: "channel", targetId: channelId, - ...(actor.email !== DEV_ACTOR_EMAIL ? { actorUserId: actor.id } : {}), + /* + * Attributed, including in single-user mode. + * + * The other audited surfaces drop this id when the actor is the local development one, on + * the grounds that `audit_events.actor_user_id` has a foreign key into `users` that it would + * violate. It has no foreign key, and `initializeDevActorUser` writes that row at start-up + * anyway, so neither half of the reason holds. It matters here more than most: single-user + * is the mode `.env.example` ships switched on, so an unattributed row is what a fork sees + * by default, and "somebody deleted this conversation" is the whole point of the row. + */ + actorUserId: actor.id, payload, }); } catch (error) { @@ -772,7 +773,22 @@ export function createChannelRoutes( await recordDeleted(context, channelId, { threadId, threadForgotten }); - return context.body(null, 204); + /* + * 200 with the outcome, not a bare 204. + * + * The thread deletion is the half that can fail on its own, and 204 says the whole act + * succeeded whichever way it went. The screen then tells somebody their message history is + * gone while it is still sitting on the platform, which is the one thing a person deleting a + * conversation is asking about. + * + * Reported as the question the caller has rather than the two facts it is derived from: no + * thread and a forgotten thread both mean nothing was left behind, and only a thread that + * survived is worth putting on a screen. + */ + return context.json( + { historyLeftBehind: threadId !== null && !threadForgotten }, + 200, + ); } catch (error) { return mapStoreError(context, error); } diff --git a/server/tests/channel-routes.test.ts b/server/tests/channel-routes.test.ts index 90256274..17906cc7 100644 --- a/server/tests/channel-routes.test.ts +++ b/server/tests/channel-routes.test.ts @@ -1,4 +1,11 @@ -import { afterAll, afterEach, describe, expect, test } from "bun:test"; +import { + afterAll, + afterEach, + beforeEach, + describe, + expect, + test, +} from "bun:test"; import { randomUUID } from "node:crypto"; import { eq } from "drizzle-orm"; import type { MiddlewareHandler } from "hono"; @@ -9,6 +16,8 @@ import { } from "../src/agents/profile-store"; import type { AgentActor } from "../src/agents/profile-types"; import { createApp } from "../src/app"; +import type { AuditEventInput, AuditStore } from "../src/audit"; +import { DEV_ACTOR } from "../src/auth/dev-actor"; import type { AppVariables } from "../src/auth/guards"; import { type AgentChannel, @@ -21,7 +30,6 @@ import { import { createThreadIdentity } from "../src/channels/thread-identity"; import { loadConfig } from "../src/config"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { agentProfiles, agents, @@ -31,6 +39,7 @@ import { intelligenceChannelMappings, users, } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; import { testEnvironment } from "./support/environment"; const actor = { @@ -297,6 +306,13 @@ describe("channel routes", () => { }); describe("channel delete route", () => { + /** Rows written by the route under test, in order. */ + let audited: AuditEventInput[] = []; + + beforeEach(() => { + audited = []; + }); + function appWithForget( store: ChannelStore, forgetThread?: (params: { @@ -304,6 +320,9 @@ describe("channel delete route", () => { userId: string; agentId: string; }) => Promise, + auditStore: AuditStore = { + insert: async (event) => void audited.push(event), + }, ) { const app = new Hono<{ Variables: AppVariables }>(); app.route( @@ -312,21 +331,22 @@ describe("channel delete route", () => { store, requireUser, undefined, - undefined, + auditStore, forgetThread, ), ); return app; } - test("returns 204 and calls store.remove with the actor and channel id", async () => { + test("returns 200 and calls store.remove with the actor and channel id", async () => { const store = fakeStore(); - const response = await appWithForget(store).request( + const response = await appWithForget(store, async () => {}).request( "http://openbot.test/channel-1", { method: "DELETE" }, ); - expect(response.status).toBe(204); + expect(response.status).toBe(200); + expect(await json(response)).toEqual({ historyLeftBehind: false }); expect(store.calls).toEqual([["remove", actor, "channel-1"]]); }); @@ -352,7 +372,7 @@ describe("channel delete route", () => { calls.push(params); }).request("http://openbot.test/channel-1", { method: "DELETE" }); - expect(response.status).toBe(204); + expect(response.status).toBe(200); expect(calls).toEqual([ { threadId: "thread-1", userId: actor.id, agentId: "channel:channel-1" }, ]); @@ -364,13 +384,16 @@ describe("channel delete route", () => { * MUST fail if a later change propagates that rejection as an error status instead of swallowing * it and still answering 204. */ - test("still returns 204 when forgetThread rejects", async () => { + test("still succeeds when forgetThread rejects, and says the history survived", async () => { const store = fakeStore(); const response = await appWithForget(store, async () => { throw new Error("Intelligence is unreachable"); }).request("http://openbot.test/channel-1", { method: "DELETE" }); - expect(response.status).toBe(204); + expect(response.status).toBe(200); + // The half that failed is the half a person deleting a conversation is asking about, so it has + // to reach them rather than being swallowed into an indistinguishable success. + expect(await json(response)).toEqual({ historyLeftBehind: true }); }); test("does not call forgetThread when remove found no thread to forget", async () => { @@ -385,9 +408,95 @@ describe("channel delete route", () => { called = true; }).request("http://openbot.test/channel-1", { method: "DELETE" }); - expect(response.status).toBe(204); + expect(response.status).toBe(200); + // Nothing to forget is not something left behind. + expect(await json(response)).toEqual({ historyLeftBehind: false }); expect(called).toBe(false); }); + + /* + * The channel row is gone by the time this runs, so this row is the only thing left that says the + * conversation ever existed or who ended it. Untested, it is also the easiest thing to drop in a + * later refactor without anything going red. + */ + test("writes an attributed audit row naming the thread it forgot", async () => { + const store = fakeStore(); + await appWithForget(store, async () => {}).request( + "http://openbot.test/channel-1", + { method: "DELETE" }, + ); + + expect(audited).toEqual([ + { + eventType: "channel.deleted", + targetType: "channel", + targetId: "channel-1", + actorUserId: actor.id, + payload: { threadId: "thread-1", threadForgotten: true }, + }, + ]); + }); + + test("records a thread that outlived the channel", async () => { + const store = fakeStore(); + await appWithForget(store, async () => { + throw new Error("Intelligence is unreachable"); + }).request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(audited).toEqual([ + { + eventType: "channel.deleted", + targetType: "channel", + targetId: "channel-1", + actorUserId: actor.id, + payload: { threadId: "thread-1", threadForgotten: false }, + }, + ]); + }); + + /* + * Single-user is the mode `.env.example` ships switched on, so this is the row a fork sees by + * default. The other audited surfaces drop the id here, believing `audit_events.actor_user_id` + * has a foreign key into `users`; it has none, and `initializeDevActorUser` writes that row at + * start-up regardless. An unattributed row would answer "was this conversation deleted" and not + * "by whom", which is the half worth keeping. + */ + test("attributes the local development actor rather than dropping it", async () => { + const store = fakeStore(); + const app = new Hono<{ Variables: AppVariables }>(); + app.route( + "/", + createChannelRoutes( + store, + async (context, next) => { + context.set("actor", DEV_ACTOR); + await next(); + }, + undefined, + { insert: async (event) => void audited.push(event) }, + async () => {}, + ), + ); + + await app.request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(audited[0]?.actorUserId).toBe(DEV_ACTOR.id); + }); + + /* + * The channel is already gone and the caller has already been told so. A trail that is briefly + * unavailable is not a reason to report a failure that did not happen. + */ + test("still answers when the audit write throws", async () => { + const store = fakeStore(); + const response = await appWithForget(store, async () => {}, { + insert: async () => { + throw new Error("audit table is unreachable"); + }, + }).request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(response.status).toBe(200); + }); }); describe("channel route composition", () => {