From 393b750a4e874dbd4fa2631f8b1859d6c3fca4d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Tue, 30 Jun 2026 16:40:52 +0100 Subject: [PATCH] fix: sync archived task websocket removals (#1556) * fix: sync archived task websocket removals * fix: address archived task review cleanup * fix: handle archived office task routes * fix: share task route pathname normalization * fix: refresh office task removal views --------- Co-authored-by: root --- apps/web/lib/links.test.ts | 16 +- apps/web/lib/links.ts | 7 +- .../web/lib/ws/handlers/tasks-archive.test.ts | 234 ++++++++++++++++++ apps/web/lib/ws/handlers/tasks.test.ts | 17 +- apps/web/lib/ws/handlers/tasks.ts | 202 +++++++++------ 5 files changed, 387 insertions(+), 89 deletions(-) create mode 100644 apps/web/lib/ws/handlers/tasks-archive.test.ts diff --git a/apps/web/lib/links.test.ts b/apps/web/lib/links.test.ts index 69e6e684bb..e1d65ab5cc 100644 --- a/apps/web/lib/links.test.ts +++ b/apps/web/lib/links.test.ts @@ -1,5 +1,11 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { isTaskDetailPath, linkToTask, linkToTasks, replaceTaskUrl } from "./links"; +import { + isTaskDetailPath, + linkToTask, + linkToTasks, + normalizePathname, + replaceTaskUrl, +} from "./links"; describe("task links", () => { afterEach(() => { @@ -47,3 +53,11 @@ describe("isTaskDetailPath", () => { expect(isTaskDetailPath("/t/task-123/extra", "task-123")).toBe(false); }); }); + +describe("normalizePathname", () => { + it("removes one trailing slash except from the root path", () => { + expect(normalizePathname("/")).toBe("/"); + expect(normalizePathname("/office/tasks/task-123/")).toBe("/office/tasks/task-123"); + expect(normalizePathname("/office/tasks/task-123")).toBe("/office/tasks/task-123"); + }); +}); diff --git a/apps/web/lib/links.ts b/apps/web/lib/links.ts index c04863d6e4..7646a5092e 100644 --- a/apps/web/lib/links.ts +++ b/apps/web/lib/links.ts @@ -6,13 +6,16 @@ export function linkToTask(taskId: string, layout?: string): string { /** Task-detail route prefixes the SPA serves: canonical and compatibility. */ const TASK_DETAIL_PREFIXES = ["/t/", "/tasks/"]; +export function normalizePathname(pathname: string): string { + return pathname.length > 1 && pathname.endsWith("/") ? pathname.slice(0, -1) : pathname; +} + /** * True when `pathname` is a task-detail route for `taskId`. Matches both the * canonical `/t/:id` and the compatibility `/tasks/:id` paths. */ export function isTaskDetailPath(pathname: string, taskId: string): boolean { - const normalized = - pathname.length > 1 && pathname.endsWith("/") ? pathname.slice(0, -1) : pathname; + const normalized = normalizePathname(pathname); return TASK_DETAIL_PREFIXES.some((prefix) => normalized === `${prefix}${taskId}`); } diff --git a/apps/web/lib/ws/handlers/tasks-archive.test.ts b/apps/web/lib/ws/handlers/tasks-archive.test.ts new file mode 100644 index 0000000000..9826d594e4 --- /dev/null +++ b/apps/web/lib/ws/handlers/tasks-archive.test.ts @@ -0,0 +1,234 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { StoreApi } from "zustand"; +import { removeRecentTask } from "@/lib/recent-tasks"; +import type { AppState } from "@/lib/state/store"; +import { registerTasksHandlers } from "./tasks"; + +vi.mock("@/lib/recent-tasks", () => ({ + removeRecentTask: vi.fn(), +})); + +type Listener = (state: AppState) => void; +type KanbanTask = { id: string; title: string; workflowId: string; workflowStepId: string }; +const TASK_ID = "t1"; +const SESSION_ID = "sess-old"; +const ARCHIVED_AT = "2026-06-30T12:00:00Z"; + +function makeStore(initial: Partial = {}) { + let state = { + kanban: { workflowId: "wf1", steps: [], tasks: [] }, + kanbanMulti: { snapshots: {}, isLoading: false }, + tasks: { + activeTaskId: null, + activeSessionId: null, + pinnedSessionId: null, + lastSessionByTaskId: {}, + }, + taskSessionsByTask: { itemsByTaskId: {}, loadedByTaskId: {}, loadingByTaskId: {} }, + environmentIdBySessionId: {}, + setActiveSessionAuto: vi.fn(), + removeTaskFromSidebarPrefs: vi.fn(), + setTaskDeletedNotification: vi.fn(), + setOfficeRefetchTrigger: vi.fn(), + ...initial, + } as unknown as AppState; + + const listeners = new Set(); + return { + getState: () => state, + setState: (updater: AppState | ((s: AppState) => AppState)) => { + const next = + typeof updater === "function" ? (updater as (s: AppState) => AppState)(state) : updater; + state = { ...state, ...next }; + for (const listener of listeners) listener(state); + }, + subscribe: (listener: Listener) => { + listeners.add(listener); + return () => listeners.delete(listener); + }, + destroy: vi.fn(), + getInitialState: vi.fn(), + } as unknown as StoreApi & { getState: () => AppState }; +} + +function makeUpdatedMessage(payload: Record) { + return { + id: "msg-1", + type: "notification" as const, + action: "task.updated" as const, + payload, + } as Parameters["task.updated"]>>[0]; +} + +function makeDeletedMessage(payload: Record) { + return { + id: "msg-1", + type: "notification" as const, + action: "task.deleted" as const, + payload, + } as Parameters["task.deleted"]>>[0]; +} + +function taskPayload(id: string, workflowId = "wf1") { + return { + task_id: id, + workflow_id: workflowId, + workflow_step_id: "step1", + title: "Test", + state: "IN_PROGRESS", + is_ephemeral: false, + }; +} + +function archiveTask(store: StoreApi, taskId = TASK_ID) { + const handlers = registerTasksHandlers(store); + handlers["task.updated"]!( + makeUpdatedMessage({ + ...taskPayload(taskId), + archived_at: ARCHIVED_AT, + }), + ); +} + +function makeStoreWithTask(initial: Partial = {}) { + return makeStore({ + kanban: { + workflowId: "wf1", + steps: [], + tasks: [{ id: TASK_ID, primarySessionId: SESSION_ID, workflowId: "wf1" }], + } as unknown as AppState["kanban"], + ...initial, + }); +} + +describe("task.updated archive cleanup", () => { + beforeEach(() => { + vi.mocked(removeRecentTask).mockClear(); + window.history.replaceState({}, "", "/"); + }); + + it("removes archived tasks from the active kanban cache even when workflow focus changed", () => { + const staleTask: KanbanTask = { + id: "t1", + title: "Test", + workflowId: "wf1", + workflowStepId: "step1", + }; + const store = makeStore({ + kanban: { + workflowId: "wf-active", + steps: [], + tasks: [staleTask], + } as unknown as AppState["kanban"], + kanbanMulti: { + isLoading: false, + snapshots: { + wf1: { workflowId: "wf1", workflowName: "WF1", steps: [], tasks: [staleTask] }, + }, + } as unknown as AppState["kanbanMulti"], + }); + + const handlers = registerTasksHandlers(store); + handlers["task.updated"]!( + makeUpdatedMessage({ + ...taskPayload(TASK_ID, "wf1"), + archived_at: ARCHIVED_AT, + }), + ); + + const state = store.getState(); + expect(state.kanban.tasks).toEqual([]); + expect(state.kanbanMulti.snapshots.wf1.tasks).toEqual([]); + }); + + it("clears active task state, pin, recent history, and sidebar prefs for archived task events", () => { + const store = makeStoreWithTask({ + tasks: { + activeTaskId: TASK_ID, + activeSessionId: SESSION_ID, + pinnedSessionId: SESSION_ID, + lastSessionByTaskId: { [TASK_ID]: SESSION_ID, t2: "sess-other" }, + }, + environmentIdBySessionId: {}, + } as unknown as Partial); + + archiveTask(store); + + const state = store.getState(); + expect(state.tasks.activeTaskId).toBeNull(); + expect(state.tasks.activeSessionId).toBeNull(); + expect(state.tasks.pinnedSessionId).toBeNull(); + expect(state.tasks.lastSessionByTaskId).not.toHaveProperty(TASK_ID); + expect(state.tasks.lastSessionByTaskId).toHaveProperty("t2", "sess-other"); + expect(removeRecentTask).toHaveBeenCalledWith(TASK_ID); + expect(state.removeTaskFromSidebarPrefs).toHaveBeenCalledWith(TASK_ID); + expect(state.setOfficeRefetchTrigger).toHaveBeenCalledWith("tasks"); + }); + + it.each(["/t/t1", "/tasks/t1"])("redirects away when archived on %s", (path) => { + window.history.replaceState({}, "", path); + const store = makeStoreWithTask({ + tasks: { + activeTaskId: TASK_ID, + activeSessionId: SESSION_ID, + pinnedSessionId: null, + lastSessionByTaskId: {}, + }, + environmentIdBySessionId: {}, + } as unknown as Partial); + + archiveTask(store); + + expect(window.location.pathname).toBe("/"); + }); + + it("does not redirect when a different task is archived", () => { + window.history.replaceState({}, "", "/t/other"); + const store = makeStoreWithTask(); + + archiveTask(store); + + expect(window.location.pathname).toBe("/t/other"); + }); +}); + +describe("office task removal routes", () => { + beforeEach(() => { + vi.mocked(removeRecentTask).mockClear(); + window.history.replaceState({}, "", "/"); + }); + + it.each(["/office/tasks/t1", "/office/tasks/t1/"])( + "redirects office task detail route %s to the office task list when archived", + (path) => { + window.history.replaceState({}, "", path); + const store = makeStoreWithTask(); + + archiveTask(store); + + expect(window.location.pathname).toBe("/office/tasks"); + }, + ); + + it("redirects office task detail routes to the office task list when auto-deleted", () => { + window.history.replaceState({}, "", "/office/tasks/t1"); + const store = makeStoreWithTask(); + const handlers = registerTasksHandlers(store); + + handlers["task.deleted"]!( + makeDeletedMessage({ + task_id: TASK_ID, + workflow_id: "wf1", + title: "Deleted task", + reason: "pr_approved_by_user", + }), + ); + + expect(window.location.pathname).toBe("/office/tasks"); + expect(store.getState().setTaskDeletedNotification).toHaveBeenCalledWith({ + taskId: TASK_ID, + title: "Deleted task", + reason: "pr_approved_by_user", + }); + }); +}); diff --git a/apps/web/lib/ws/handlers/tasks.test.ts b/apps/web/lib/ws/handlers/tasks.test.ts index 69c4a231f9..545a078285 100644 --- a/apps/web/lib/ws/handlers/tasks.test.ts +++ b/apps/web/lib/ws/handlers/tasks.test.ts @@ -536,6 +536,10 @@ describe("task.updated repository clearing", () => { }); describe("task.deleted cleanup", () => { + beforeEach(() => { + vi.mocked(removeRecentTask).mockClear(); + }); + it("removes the deleted task from recent task history", () => { const store = makeStore({ kanban: { @@ -558,18 +562,18 @@ describe("task.deleted cleanup", () => { expect(removeRecentTask).toHaveBeenCalledWith("t1"); }); - it("removes the deleted task from lastSessionByTaskId", () => { + it("clears deleted task session state", () => { const store = makeStore({ kanban: { workflowId: "wf1", steps: [], - tasks: [{ id: "t1", primarySessionId: "sess-old", workflowId: "wf1" }], + tasks: [{ id: "t1", primarySessionId: SESS_PINNED, workflowId: "wf1" }], } as unknown as AppState["kanban"], tasks: { - activeTaskId: null, - activeSessionId: null, - pinnedSessionId: null, - lastSessionByTaskId: { t1: "sess-old", t2: SESS_OTHER }, + activeTaskId: "t1", + activeSessionId: SESS_PINNED, + pinnedSessionId: SESS_PINNED, + lastSessionByTaskId: { t1: SESS_PINNED, t2: SESS_OTHER }, }, environmentIdBySessionId: {}, }); @@ -578,6 +582,7 @@ describe("task.deleted cleanup", () => { handlers["task.deleted"]!(makeDeletedMessage({ task_id: "t1", workflow_id: "wf1" })); const state = store.getState(); + expect(state.tasks.pinnedSessionId).toBeNull(); expect(state.tasks.lastSessionByTaskId).not.toHaveProperty("t1"); expect(state.tasks.lastSessionByTaskId).toHaveProperty("t2", SESS_OTHER); }); diff --git a/apps/web/lib/ws/handlers/tasks.ts b/apps/web/lib/ws/handlers/tasks.ts index ee5871bd4a..e5bd9dd46e 100644 --- a/apps/web/lib/ws/handlers/tasks.ts +++ b/apps/web/lib/ws/handlers/tasks.ts @@ -9,7 +9,7 @@ import { toKanbanTask, type TaskLike } from "@/lib/kanban/map-task"; import { sessionId as toSessionId } from "@/lib/types/http"; import { mergeTaskRepositoryFields } from "@/lib/ws/handlers/task-repositories"; import { softNavigate } from "@/lib/routing/client-router"; -import { isTaskDetailPath } from "@/lib/links"; +import { isTaskDetailPath, normalizePathname } from "@/lib/links"; import { clearPinnedSessionIfOverridden, shouldPreservePinnedSessionForTask, @@ -95,40 +95,137 @@ function findTaskInState(state: AppState, taskId: string): KanbanTask | undefine } /** Remove a task from both single-kanban and multi-kanban snapshots. */ -function removeTaskFromBothKanbans(state: AppState, wfId: string, taskId: string): AppState { +function removeTaskFromBothKanbans(state: AppState, taskId: string): AppState { let next = state; - if (state.kanban.workflowId === wfId) { + if (state.kanban.tasks.some((t) => t.id === taskId)) { next = { ...next, kanban: { ...next.kanban, tasks: next.kanban.tasks.filter((t) => t.id !== taskId) }, }; } - const snapshot = state.kanbanMulti.snapshots[wfId]; - if (snapshot) { + + const snapshots = Object.entries(next.kanbanMulti.snapshots); + const changedSnapshots = snapshots.filter(([, snapshot]) => + snapshot.tasks.some((t) => t.id === taskId), + ); + if (changedSnapshots.length > 0) { + const nextSnapshots = { ...next.kanbanMulti.snapshots }; + for (const [workflowId, snapshot] of changedSnapshots) { + nextSnapshots[workflowId] = { + ...snapshot, + tasks: snapshot.tasks.filter((t) => t.id !== taskId), + }; + } next = { ...next, kanbanMulti: { ...next.kanbanMulti, - snapshots: { - ...next.kanbanMulti.snapshots, - [wfId]: { ...snapshot, tasks: snapshot.tasks.filter((t) => t.id !== taskId) }, - }, + snapshots: nextSnapshots, }, }; } return next; } +function clearRemovedTaskSelection(state: AppState, taskId: string): AppState { + let next = state; + if (next.tasks.activeTaskId === taskId) { + next = { + ...next, + tasks: { + ...next.tasks, + activeTaskId: null, + activeSessionId: null, + pinnedSessionId: null, + }, + }; + } + if (next.tasks.lastSessionByTaskId[taskId]) { + const { [taskId]: _, ...rest } = next.tasks.lastSessionByTaskId; + next = { ...next, tasks: { ...next.tasks, lastSessionByTaskId: rest } }; + } + return next; +} + +function removedTaskRedirectHref(pathname: string, taskId: string): string | null { + if (isTaskDetailPath(pathname, taskId)) return "/"; + const normalized = normalizePathname(pathname); + return normalized === `/office/tasks/${taskId}` ? "/office/tasks" : null; +} + /** - * Soft-redirect away from a deleted task's page. Only fires when the user is - * currently parked on that task's route (`/t/` or the compatibility - * `/tasks/`), so a background deletion of some other task never yanks the - * user elsewhere. + * Soft-redirect away from a removed task's page. Only fires when the user is + * currently parked on that task's route, so a background removal of some other + * task never yanks the user elsewhere. */ -function redirectAwayFromDeletedTask(deletedId: string): void { +function redirectAwayFromRemovedTask(taskId: string): void { if (typeof window === "undefined") return; - if (!isTaskDetailPath(window.location.pathname, deletedId)) return; - softNavigate("/", "replace"); + const href = removedTaskRedirectHref(window.location.pathname, taskId); + if (!href) return; + softNavigate(href, "replace"); +} + +type TaskUpdatedMessage = Parameters>[0]; + +function handleTaskUpdated(store: StoreApi, message: TaskUpdatedMessage): void { + // Skip ephemeral tasks (e.g., quick chat) - they shouldn't appear on the Kanban board + if (message.payload.is_ephemeral) return; + + // Capture the previous primary session id BEFORE the upsert so we can + // detect a primary-session swap (e.g. workflow profile switch reusing a + // different session) and follow focus to the new primary. + const beforeState = store.getState(); + const taskId = message.payload.task_id; + const previousPrimary = findTaskInState(beforeState, taskId)?.primarySessionId ?? null; + const archivedAt = message.payload.archived_at; + + if (archivedAt) { + removeRecentTask(taskId); + const state = store.getState(); + state.removeTaskFromSidebarPrefs(taskId); + state.setOfficeRefetchTrigger("tasks"); + } + + store.setState((state) => { + const wfId = message.payload.workflow_id; + const oldWfId = message.payload.old_workflow_id; + let next = state; + + if (archivedAt || (oldWfId && oldWfId !== wfId)) { + next = removeTaskFromBothKanbans(next, taskId); + } + + if (archivedAt) { + return clearRemovedTaskSelection(next, taskId); + } + + return upsertTaskInBothKanbans(next, wfId, message.payload); + }); + + if (archivedAt) { + redirectAwayFromRemovedTask(taskId); + return; + } + + // Follow focus to the new primary when: + // - the user is currently viewing this task, + // - the user was sitting on the previous primary, + // - they do NOT have a non-terminal pinned session for this task, and + // - the primary actually changed. + // This makes workflow profile switches transparent for unpinned users + // without yanking users off a live session they deliberately selected. + const afterState = store.getState(); + const newPrimary = findTaskInState(afterState, taskId)?.primarySessionId ?? null; + if ( + newPrimary && + newPrimary !== previousPrimary && + afterState.tasks.activeTaskId === taskId && + afterState.tasks.activeSessionId === previousPrimary && + !shouldPreservePinnedSessionForTask(afterState, taskId) + ) { + clearPinnedSessionIfOverridden(store, newPrimary); + afterState.setActiveSessionAuto(taskId, newPrimary); + } } export function registerTasksHandlers(store: StoreApi): WsHandlers { @@ -140,56 +237,9 @@ export function registerTasksHandlers(store: StoreApi): WsHandlers { upsertTaskInBothKanbans(state, message.payload.workflow_id, message.payload), ); }, - "task.updated": (message) => { - // Skip ephemeral tasks (e.g., quick chat) - they shouldn't appear on the Kanban board - if (message.payload.is_ephemeral) return; - - // Capture the previous primary session id BEFORE the upsert so we can - // detect a primary-session swap (e.g. workflow profile switch reusing a - // different session) and follow focus to the new primary. - const beforeState = store.getState(); - const taskId = message.payload.task_id; - const previousPrimary = findTaskInState(beforeState, taskId)?.primarySessionId ?? null; - - store.setState((state) => { - const wfId = message.payload.workflow_id; - const oldWfId = message.payload.old_workflow_id; - let next = state; - - if (oldWfId && oldWfId !== wfId) { - next = removeTaskFromBothKanbans(next, oldWfId, taskId); - } - - if (message.payload.archived_at) { - return removeTaskFromBothKanbans(next, wfId, taskId); - } - - return upsertTaskInBothKanbans(next, wfId, message.payload); - }); - - // Follow focus to the new primary when: - // - the user is currently viewing this task, - // - the user was sitting on the previous primary, - // - they do NOT have a non-terminal pinned session for this task, and - // - the primary actually changed. - // This makes workflow profile switches transparent for unpinned users - // without yanking users off a live session they deliberately selected. - const afterState = store.getState(); - const newPrimary = findTaskInState(afterState, taskId)?.primarySessionId ?? null; - if ( - newPrimary && - newPrimary !== previousPrimary && - afterState.tasks.activeTaskId === taskId && - afterState.tasks.activeSessionId === previousPrimary && - !shouldPreservePinnedSessionForTask(afterState, taskId) - ) { - clearPinnedSessionIfOverridden(store, newPrimary); - afterState.setActiveSessionAuto(taskId, newPrimary); - } - }, + "task.updated": (message) => handleTaskUpdated(store, message), "task.deleted": (message) => { const deletedId = message.payload.task_id; - const wfId = message.payload.workflow_id; removeRecentTask(deletedId); const currentState = store.getState(); @@ -221,25 +271,17 @@ export function registerTasksHandlers(store: StoreApi): WsHandlers { const wasActive = currentState.tasks.activeTaskId === deletedId; - store.setState((state) => { - const isActive = state.tasks.activeTaskId === deletedId; - let next = removeTaskFromBothKanbans(state, wfId, deletedId); - if (isActive) { - next = { ...next, tasks: { ...next.tasks, activeTaskId: null, activeSessionId: null } }; - } - if (next.tasks.lastSessionByTaskId[deletedId]) { - const { [deletedId]: _, ...rest } = next.tasks.lastSessionByTaskId; - next = { ...next, tasks: { ...next.tasks, lastSessionByTaskId: rest } }; - } - return next; - }); + store.setState((state) => + clearRemovedTaskSelection(removeTaskFromBothKanbans(state, deletedId), deletedId), + ); // Capture the route match before any redirect mutates the pathname. This // covers a fresh load where the browser is parked on the task's route - // (`/t/` or `/tasks/`) but TaskPageContent hasn't hydrated - // `activeTaskId` yet, so `wasActive` is still false. + // but TaskPageContent hasn't hydrated `activeTaskId` yet, so `wasActive` + // is still false. const onDeletedRoute = - typeof window !== "undefined" && isTaskDetailPath(window.location.pathname, deletedId); + typeof window !== "undefined" && + removedTaskRedirectHref(window.location.pathname, deletedId) !== null; // Only react to genuine auto-deletions, which the backend tags with a // reason (e.g. a review task whose PR was approved). User-initiated deletes @@ -248,7 +290,7 @@ export function registerTasksHandlers(store: StoreApi): WsHandlers { // preempt it and strand the user on the home route. For auto-deletions we // move off the now-dead route (helper is route-guarded) and explain why. if (message.payload.reason && (wasActive || onDeletedRoute)) { - redirectAwayFromDeletedTask(deletedId); + redirectAwayFromRemovedTask(deletedId); store.getState().setTaskDeletedNotification({ taskId: deletedId, title: message.payload.title,