diff --git a/apps/backend/internal/task/service/service_tasks_content_editing_test.go b/apps/backend/internal/task/service/service_tasks_content_editing_test.go new file mode 100644 index 0000000000..3528d5cfce --- /dev/null +++ b/apps/backend/internal/task/service/service_tasks_content_editing_test.go @@ -0,0 +1,132 @@ +package service + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/task/models" +) + +// AC-29: committing a title on a task carrying a pending agent-generated +// title persists the user's title and clears the pending-agent-title +// metadata. +func TestUpdateTaskTitle_ClearsPendingAgentTitleMetadata(t *testing.T) { + svc, bus, repo := newTitleTestService(t) + ctx := context.Background() + seedPendingTitleTask(t, repo, "task-ac29", "sess-owner", true) + + newTitle := "Fix the login bug" + task, err := svc.UpdateTask(ctx, "task-ac29", &UpdateTaskRequest{Title: &newTitle}) + if err != nil { + t.Fatalf("UpdateTask: %v", err) + } + if task.Title != newTitle { + t.Fatalf("title = %q, want %q", task.Title, newTitle) + } + if models.IsAgentTitlePending(task.Metadata) { + t.Fatalf("metadata = %v, want the pending marker cleared", task.Metadata) + } + if models.AgentTitleOwnerSessionID(task.Metadata) != "" { + t.Fatalf("metadata = %v, want the owner claim cleared", task.Metadata) + } + + stored, err := repo.GetTask(ctx, "task-ac29") + if err != nil { + t.Fatalf("GetTask: %v", err) + } + if stored.Title != newTitle { + t.Fatalf("persisted title = %q, want %q", stored.Title, newTitle) + } + if models.IsAgentTitlePending(stored.Metadata) { + t.Fatalf("persisted metadata = %v, want the pending marker cleared", stored.Metadata) + } + + if types := eventTypes(bus.GetPublishedEvents()); len(types) != 1 || types[0] != events.TaskUpdated { + t.Fatalf("published %v, want exactly one %s", types, events.TaskUpdated) + } +} + +// AC-48: committing a description-only save on a task carrying a pending +// agent-generated title persists the new description, leaves the +// pending-agent-title metadata intact, and leaves the stored title +// unchanged. This exercises the conditional title statement in +// updateTaskTx. +func TestUpdateTaskDescriptionOnly_LeavesPendingAgentTitleAndStoredTitleIntact(t *testing.T) { + svc, bus, repo := newTitleTestService(t) + ctx := context.Background() + seedPendingTitleTask(t, repo, "task-ac48", "sess-owner", true) + + newDescription := "Investigate the checkout flow regression" + task, err := svc.UpdateTask(ctx, "task-ac48", &UpdateTaskRequest{Description: &newDescription}) + if err != nil { + t.Fatalf("UpdateTask: %v", err) + } + if task.Description != newDescription { + t.Fatalf("description = %q, want %q", task.Description, newDescription) + } + if task.Title != "Untitled" { + t.Fatalf("title = %q, want the pending agent title left untouched", task.Title) + } + if !models.IsAgentTitlePending(task.Metadata) { + t.Fatalf("metadata = %v, want the pending marker to survive a description-only save", task.Metadata) + } + if models.AgentTitleOwnerSessionID(task.Metadata) != "sess-owner" { + t.Fatalf("metadata = %v, want the owner claim to survive a description-only save", task.Metadata) + } + + stored, err := repo.GetTask(ctx, "task-ac48") + if err != nil { + t.Fatalf("GetTask: %v", err) + } + if stored.Description != newDescription { + t.Fatalf("persisted description = %q, want %q", stored.Description, newDescription) + } + if stored.Title != "Untitled" { + t.Fatalf("persisted title = %q, want it unchanged", stored.Title) + } + if !models.IsAgentTitlePending(stored.Metadata) { + t.Fatalf("persisted metadata = %v, want the pending marker intact", stored.Metadata) + } + + if types := eventTypes(bus.GetPublishedEvents()); len(types) != 1 || types[0] != events.TaskUpdated { + t.Fatalf("published %v, want exactly one %s", types, events.TaskUpdated) + } +} + +// AC-31: a title or description update publishes task.updated without +// assignee_agent_profile_id on the payload, so queueTaskAssignedRun cannot +// queue a run as a result of the edit. +func TestUpdateTaskTitleOrDescription_OmitsAssigneeAgentProfileIDFromPayload(t *testing.T) { + svc, bus, repo := newTitleTestService(t) + ctx := context.Background() + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-ac31", WorkspaceID: "ws-title", WorkflowID: "wf-title", WorkflowStepID: "step-1", + Title: "Original title", Description: "Original description", Priority: "medium", + AssigneeAgentProfileID: "agent-x", + }); err != nil { + t.Fatalf("create task: %v", err) + } + bus.ClearEvents() + + newTitle := "Edited title" + if _, err := svc.UpdateTask(ctx, "task-ac31", &UpdateTaskRequest{Title: &newTitle}); err != nil { + t.Fatalf("UpdateTask (title): %v", err) + } + titleData := singlePublishedEventData(t, bus) + if _, ok := titleData["assignee_agent_profile_id"]; ok { + t.Fatalf("title-only update published assignee_agent_profile_id = %#v, want the key absent", + titleData["assignee_agent_profile_id"]) + } + + bus.ClearEvents() + newDescription := "Edited description" + if _, err := svc.UpdateTask(ctx, "task-ac31", &UpdateTaskRequest{Description: &newDescription}); err != nil { + t.Fatalf("UpdateTask (description): %v", err) + } + descData := singlePublishedEventData(t, bus) + if _, ok := descData["assignee_agent_profile_id"]; ok { + t.Fatalf("description-only update published assignee_agent_profile_id = %#v, want the key absent", + descData["assignee_agent_profile_id"]) + } +} diff --git a/apps/web/app/office/tasks/[id]/page.test.tsx b/apps/web/app/office/tasks/[id]/page.test.tsx new file mode 100644 index 0000000000..d2f4b2af9f --- /dev/null +++ b/apps/web/app/office/tasks/[id]/page.test.tsx @@ -0,0 +1,468 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { act, renderHook, waitFor } from "@testing-library/react"; +import type { ReactNode } from "react"; +import { StateProvider, useAppStoreApi } from "@/components/state-provider"; +import { + __resetOfficeTaskContentSyncForTests, + beginWrite, + closeFieldEditor, + endWrite, + getCanonicalValue, + isFieldGuarded, + nextTaskSequence, + openFieldEditor, + recordRefetchCandidate, + recordWriteSuccess, + seedInitialCanonical, +} from "@/lib/state/office-task-content-sync"; +import type { OfficeTask } from "@/lib/state/slices/office/types"; +import type { Task } from "./types"; + +const mockGetTask = vi.fn(); +const mockListActivityForTarget = vi.fn(); +const mockListComments = vi.fn(); +const mockListTaskSessions = vi.fn(); + +vi.mock("@/lib/api/domains/office-api", () => ({ + getTask: (...args: unknown[]) => mockGetTask(...args), + listActivityForTarget: (...args: unknown[]) => mockListActivityForTarget(...args), + listComments: (...args: unknown[]) => mockListComments(...args), +})); + +vi.mock("@/lib/api/domains/session-api", () => ({ + listTaskSessions: (...args: unknown[]) => mockListTaskSessions(...args), +})); + +import { useIssueData, useTaskOptimisticHelpers } from "./page"; + +afterEach(() => { + vi.clearAllMocks(); + __resetOfficeTaskContentSyncForTests(); +}); + +const TS = "2026-05-01T00:00:00Z"; +const TASK_ID = "t-1"; +const TITLE_FIELD = "title"; +const EDITED_TITLE = "Edited While Loading"; +const ORIGINAL_TITLE = "Original Title"; +const ORIGINAL_DESCRIPTION = "Original description"; + +const officeTaskFixture: OfficeTask = { + id: TASK_ID, + workspaceId: "ws-1", + identifier: "TASK-1", + title: ORIGINAL_TITLE, + description: ORIGINAL_DESCRIPTION, + status: "todo", + priority: "medium", + createdAt: TS, + updatedAt: TS, +}; + +function makeStoreWrapper(initialTasks: OfficeTask[]) { + function StoreSeed({ children }: { children: ReactNode }) { + const api = useAppStoreApi(); + if (initialTasks.length > 0) { + api.getState().setTasks(initialTasks); + } + return <>{children}; + } + function Wrapper({ children }: { children: ReactNode }) { + return ( + + {children} + + ); + } + return Wrapper; +} + +function taskFixture(overrides: Partial = {}): Task { + return { + id: TASK_ID, + workspaceId: "ws-1", + identifier: "TASK-1", + title: "Live Title", + status: "todo", + priority: "medium", + labels: [], + blockedBy: [], + blocking: [], + children: [], + reviewers: [], + approvers: [], + decisions: [], + createdBy: "user", + createdAt: TS, + updatedAt: TS, + ...overrides, + }; +} + +function makeTaskRefSetter(initialTask: Task = taskFixture()) { + const taskRef: { current: Task | null } = { current: initialTask }; + const setTask: React.Dispatch> = (updater) => { + taskRef.current = + typeof updater === "function" + ? (updater as (prev: Task | null) => Task | null)(taskRef.current) + : updater; + }; + return { taskRef, setTask }; +} + +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; +} + +describe("useIssueData — initial load guard preservation (AC-27/28/38/39)", () => { + it("does not overwrite title/description with a stale GET response that resolves after a mid-flight commit", async () => { + mockListActivityForTarget.mockResolvedValue({ activity: [] }); + mockListComments.mockResolvedValue({ comments: [] }); + mockListTaskSessions.mockResolvedValue({ sessions: [] }); + const { promise: getTaskPromise, resolve: resolveGetTask } = deferred<{ + task: OfficeTask; + timeline: []; + }>(); + mockGetTask.mockReturnValue(getTaskPromise); + + const Wrapper = makeStoreWrapper([officeTaskFixture]); + const { result } = renderHook(() => useIssueData(TASK_ID), { wrapper: Wrapper }); + + // The from-store fast path seeds the displayed task synchronously. + await waitFor(() => expect(result.current.task?.title).toBe(ORIGINAL_TITLE)); + + // Simulate a title commit landing in full (useCommitTaskTitle's actual + // sequence: guard the field, apply optimistically, then record the + // write's own success) while the initial GET is still in flight. + const writeSequence = nextTaskSequence(TASK_ID); + act(() => { + beginWrite(TASK_ID, TITLE_FIELD, writeSequence); + result.current.applyTaskPatch({ title: EDITED_TITLE }); + recordWriteSuccess(TASK_ID, TITLE_FIELD, EDITED_TITLE, "2026-05-01T00:00:05Z", writeSequence); + endWrite(TASK_ID, TITLE_FIELD, writeSequence); + }); + expect(result.current.task?.title).toBe(EDITED_TITLE); + + // The GET resolves with the DTO as it looked before that commit landed + // (an older title, a different status — the merge must only protect + // title/description, not block the whole update). + await act(async () => { + resolveGetTask({ + task: { ...officeTaskFixture, title: "Server Title", status: "in_progress" }, + timeline: [], + }); + await getTaskPromise; + await Promise.resolve(); + await Promise.resolve(); + }); + + await waitFor(() => expect(result.current.task?.status).toBe("in_progress")); + expect(result.current.task?.title).toBe(EDITED_TITLE); + expect(result.current.task?.description).toBe(ORIGINAL_DESCRIPTION); + }); +}); + +describe("useIssueData — canonical seeded synchronously from the store cache (COR-001)", () => { + it("seeds canonical title/description as soon as the store-cache fast path renders, before the authoritative GET resolves", async () => { + mockListActivityForTarget.mockResolvedValue({ activity: [] }); + mockListComments.mockResolvedValue({ comments: [] }); + mockListTaskSessions.mockResolvedValue({ sessions: [] }); + // The GET is left permanently pending for this test: the render gate + // makes the page interactive from the store cache alone, and canonical + // must already be seeded in that window, not only once this resolves. + const { promise: getTaskPromise } = deferred<{ task: OfficeTask; timeline: [] }>(); + mockGetTask.mockReturnValue(getTaskPromise); + + const Wrapper = makeStoreWrapper([officeTaskFixture]); + const { result } = renderHook(() => useIssueData(TASK_ID), { wrapper: Wrapper }); + + await waitFor(() => expect(result.current.task?.title).toBe(ORIGINAL_TITLE)); + + // AC-9/AC-64: a title commit that fails during this window restores the + // field's recorded canonical value. Without a canonical value seeded + // here, that restore silently no-ops (getCanonicalValue returns + // undefined) and the failed edit is left standing. + expect(getCanonicalValue(TASK_ID, "title")).toBe(ORIGINAL_TITLE); + expect(getCanonicalValue(TASK_ID, "description")).toBe(ORIGINAL_DESCRIPTION); + }); +}); + +describe("useIssueData — store-cache seed does not clobber an existing canonical value (P1 review fix)", () => { + it("does not let the store-cache fast path's seed win an equal-timestamp tiebreak against an already-recorded canonical value", async () => { + mockListActivityForTarget.mockResolvedValue({ activity: [] }); + mockListComments.mockResolvedValue({ comments: [] }); + mockListTaskSessions.mockResolvedValue({ sessions: [] }); + // Left pending: the seed-vs-clobber window under test is between the + // store-cache fast path rendering and this GET resolving. + const { promise: getTaskPromise } = deferred<{ task: OfficeTask; timeline: [] }>(); + mockGetTask.mockReturnValue(getTaskPromise); + + // Simulates a canonical value already recorded on an earlier visit (e.g. + // a write still pending when the user last navigated away), at the same + // instant as the store-cached task seeded below. + seedInitialCanonical(TASK_ID, "title", "Pending Write Title", TS); + + const Wrapper = makeStoreWrapper([officeTaskFixture]); + const { result } = renderHook(() => useIssueData(TASK_ID), { wrapper: Wrapper }); + + await waitFor(() => expect(result.current.task?.title).toBe(ORIGINAL_TITLE)); + + // Without the if-absent guard, the store-cache fast path's own seed call + // (issued at a higher sequence, same instant as the pre-existing + // canonical) would win the AC-63 tiebreak and clobber it. + expect(getCanonicalValue(TASK_ID, "title")).toBe("Pending Write Title"); + }); +}); + +describe("useIssueData — initial load failure still seeds canonical (AC-59/AC-72)", () => { + it("seeds canonical title/description from the store cache when the authoritative GET throws", async () => { + mockListActivityForTarget.mockResolvedValue({ activity: [] }); + mockListComments.mockResolvedValue({ comments: [] }); + mockListTaskSessions.mockResolvedValue({ sessions: [] }); + mockGetTask.mockRejectedValue(new Error("network down")); + + const Wrapper = makeStoreWrapper([officeTaskFixture]); + const { result } = renderHook(() => useIssueData(TASK_ID), { wrapper: Wrapper }); + + // The from-store fast path renders the task interactively; the + // authoritative GET then fails outright. + await waitFor(() => expect(result.current.task?.title).toBe(ORIGINAL_TITLE)); + + // AC-9/AC-64: a later failed title commit restores the recorded + // canonical value. If the authoritative load never completes, the page + // still renders (and lets the user edit) the store-cached title, so a + // canonical value must exist for that restore to have a target. + await waitFor(() => expect(getCanonicalValue(TASK_ID, "title")).toBe(ORIGINAL_TITLE)); + expect(getCanonicalValue(TASK_ID, "description")).toBe(ORIGINAL_DESCRIPTION); + }); +}); + +describe("useIssueData — guard release does not spuriously patch an unset description (AC-71)", () => { + it("leaves a no-description task's description undefined after a no-op description-editor open/close", async () => { + mockListActivityForTarget.mockResolvedValue({ activity: [] }); + mockListComments.mockResolvedValue({ comments: [] }); + mockListTaskSessions.mockResolvedValue({ sessions: [] }); + const taskNoDescription: OfficeTask = { ...officeTaskFixture, description: undefined }; + mockGetTask.mockResolvedValue({ task: taskNoDescription, timeline: [] }); + + const Wrapper = makeStoreWrapper([taskNoDescription]); + const { result } = renderHook(() => ({ issue: useIssueData(TASK_ID), api: useAppStoreApi() }), { + wrapper: Wrapper, + }); + + await waitFor(() => expect(result.current.issue.task?.title).toBe(ORIGINAL_TITLE)); + await waitFor(() => expect(result.current.issue.loading).toBe(false)); + + // No-op: a user opens the description editor to check whether there's a + // description, then cancels without typing anything. The canonical value + // is normalised to "" (AC-71), but the store/display never held a + // description in the first place, so nothing should be written. + act(() => { + openFieldEditor(TASK_ID, "description"); + closeFieldEditor(TASK_ID, "description"); + }); + + expect(result.current.issue.task?.description).toBeUndefined(); + expect( + result.current.api.getState().office.tasks.items.find((t) => t.id === TASK_ID)?.description, + ).toBeUndefined(); + }); +}); + +describe("useTaskOptimisticHelpers — unmount cleanup order (AC-68)", () => { + it("flushes a canonical value recorded while guarded to the store on unmount", () => { + openFieldEditor(TASK_ID, TITLE_FIELD); + const sequence = nextTaskSequence(TASK_ID); + recordRefetchCandidate(TASK_ID, TITLE_FIELD, "Flushed On Release", TS, sequence); + + const Wrapper = makeStoreWrapper([officeTaskFixture]); + // Mirrors the real invariant: by the time a listener could fire, the + // page has already finished its initial load, so `task` is non-null. + const { taskRef, setTask } = makeTaskRefSetter(); + const setTimeline = vi.fn(); + + const { result, unmount } = renderHook( + () => ({ + helpers: useTaskOptimisticHelpers(TASK_ID, setTask, setTimeline), + api: useAppStoreApi(), + }), + { wrapper: Wrapper }, + ); + const storeApi = result.current.api; + + unmount(); + + expect(storeApi.getState().office.tasks.items.find((t) => t.id === TASK_ID)?.title).toBe( + "Flushed On Release", + ); + expect(taskRef.current?.title).toBe("Flushed On Release"); + }); + + it("reconciles the store once a write still pending at unmount time resolves (P1 review fix)", () => { + const sequence = nextTaskSequence(TASK_ID); + beginWrite(TASK_ID, TITLE_FIELD, sequence); + + const Wrapper = makeStoreWrapper([officeTaskFixture]); + const { taskRef, setTask } = makeTaskRefSetter(); + const setTimeline = vi.fn(); + + const { result, unmount } = renderHook( + () => ({ + helpers: useTaskOptimisticHelpers(TASK_ID, setTask, setTimeline), + api: useAppStoreApi(), + }), + { wrapper: Wrapper }, + ); + const storeApi = result.current.api; + + unmount(); + + // The field is still guarded by the pending write, so the listener must + // stay registered past unmount instead of unsubscribing unconditionally. + expect(isFieldGuarded(TASK_ID, TITLE_FIELD)).toBe(true); + + act(() => { + recordWriteSuccess( + TASK_ID, + TITLE_FIELD, + "Reconciled After Unmount", + "2026-05-01T00:00:05Z", + sequence, + ); + endWrite(TASK_ID, TITLE_FIELD, sequence); + }); + + expect(isFieldGuarded(TASK_ID, TITLE_FIELD)).toBe(false); + expect(taskRef.current?.title).toBe("Reconciled After Unmount"); + expect(storeApi.getState().office.tasks.items.find((t) => t.id === TASK_ID)?.title).toBe( + "Reconciled After Unmount", + ); + }); + + it("never restores title/description when rolling back an unrelated picker mutation (AC-61)", () => { + const Wrapper = makeStoreWrapper([officeTaskFixture]); + const { taskRef, setTask } = makeTaskRefSetter(); + const setTimeline = vi.fn(); + + const { result } = renderHook(() => useTaskOptimisticHelpers(TASK_ID, setTask, setTimeline), { + wrapper: Wrapper, + }); + + // A stale snapshot from before a since-confirmed title edit landed. + const staleSnapshot: Task = { ...taskRef.current!, title: "Stale Title", priority: "high" }; + act(() => { + result.current.restoreTask(staleSnapshot); + }); + + expect(taskRef.current?.title).toBe("Live Title"); + expect(taskRef.current?.priority).toBe("high"); + }); +}); + +describe("useTaskOptimisticHelpers — task id changes with pending writes", () => { + function renderTaskHelpers() { + const oldTask = taskFixture({ id: "t-1", title: "Old title", description: "Old description" }); + const newTask = taskFixture({ id: "t-2", title: "New title", description: "New description" }); + const { taskRef, setTask } = makeTaskRefSetter(oldTask); + const setTimeline = vi.fn(); + const Wrapper = makeStoreWrapper([]); + const rendered = renderHook( + ({ taskId }: { taskId: string }) => useTaskOptimisticHelpers(taskId, setTask, setTimeline), + { initialProps: { taskId: "t-1" }, wrapper: Wrapper }, + ); + return { ...rendered, oldTask, newTask, taskRef }; + } + + it("does not apply an old task's deferred title to the replacement task", () => { + const sequence = nextTaskSequence("t-1"); + beginWrite("t-1", TITLE_FIELD, sequence); + const { rerender, unmount, newTask, taskRef } = renderTaskHelpers(); + + taskRef.current = newTask; + rerender({ taskId: "t-2" }); + + act(() => { + recordWriteSuccess( + "t-1", + TITLE_FIELD, + "Old title from server", + "2026-05-01T00:00:05Z", + sequence, + ); + endWrite("t-1", TITLE_FIELD, sequence); + }); + + expect(taskRef.current).toEqual(newTask); + unmount(); + }); + + it("does not apply an old task's captured patch or restore to the replacement task", () => { + const { result, rerender, unmount, newTask, oldTask, taskRef } = renderTaskHelpers(); + const oldApplyPatch = result.current.applyTaskPatch; + const oldRestoreTask = result.current.restoreTask; + + taskRef.current = newTask; + rerender({ taskId: "t-2" }); + + act(() => { + oldApplyPatch({ title: "Old optimistic title" }); + oldRestoreTask({ ...oldTask, title: "Old restored title", priority: "high" }); + }); + + expect(taskRef.current).toEqual(newTask); + unmount(); + }); +}); + +describe("useIssueData — initial load task identity", () => { + it("does not preserve the previous task's guarded fields after a route change", async () => { + mockListActivityForTarget.mockResolvedValue({ activity: [] }); + mockListComments.mockResolvedValue({ comments: [] }); + mockListTaskSessions.mockResolvedValue({ sessions: [] }); + const firstLoad = deferred<{ task: OfficeTask; timeline: [] }>(); + const secondLoad = deferred<{ task: OfficeTask; timeline: [] }>(); + mockGetTask.mockImplementation((taskId: string) => + taskId === "t-1" ? firstLoad.promise : secondLoad.promise, + ); + + const Wrapper = makeStoreWrapper([ + { ...officeTaskFixture, title: "First task", description: "First description" }, + ]); + const { result, rerender } = renderHook( + ({ taskId }: { taskId: string }) => useIssueData(taskId), + { initialProps: { taskId: "t-1" }, wrapper: Wrapper }, + ); + + act(() => { + firstLoad.resolve({ + task: { ...officeTaskFixture, title: "First task", description: "First description" }, + timeline: [], + }); + }); + await waitFor(() => expect(result.current.task?.id).toBe("t-1")); + rerender({ taskId: "t-2" }); + openFieldEditor("t-2", TITLE_FIELD); + await act(async () => { + secondLoad.resolve({ + task: { + ...officeTaskFixture, + id: "t-2", + identifier: "TASK-2", + title: "Second task", + description: "Second description", + }, + timeline: [], + }); + await secondLoad.promise; + }); + await waitFor(() => expect(result.current.task?.id).toBe("t-2")); + + expect(result.current.task?.title).toBe("Second task"); + expect(result.current.task?.description).toBe("Second description"); + closeFieldEditor("t-2", TITLE_FIELD); + }); +}); diff --git a/apps/web/app/office/tasks/[id]/page.tsx b/apps/web/app/office/tasks/[id]/page.tsx index 4c07b4dfd2..51e4385d64 100644 --- a/apps/web/app/office/tasks/[id]/page.tsx +++ b/apps/web/app/office/tasks/[id]/page.tsx @@ -12,7 +12,7 @@ import { type SetStateAction, } from "react"; import { useRouter, useSearchParams } from "@/lib/routing/client-router"; -import { useAppStore } from "@/components/state-provider"; +import { useAppStore, useAppStoreApi } from "@/components/state-provider"; import { useOfficeRefetch } from "@/hooks/use-office-refetch"; import { useLatestOnly } from "@/hooks/use-latest-only"; import { TaskOptimisticContextProvider } from "@/hooks/use-optimistic-task-mutation"; @@ -23,6 +23,15 @@ import { type TaskCommentResponse, } from "@/lib/api/domains/office-api"; import { fetchTask } from "@/lib/api/domains/kanban-api"; +import { + endAllEditorsForTask, + getCanonicalValue, + isFieldGuarded, + nextTaskSequence, + recordRefetchCandidate, + seedInitialCanonical, + subscribeField, +} from "@/lib/state/office-task-content-sync"; import { listTaskSessions } from "@/lib/api/domains/session-api"; import { liveSessionMetadataFromStore, @@ -237,9 +246,24 @@ function useSessionLiveSync({ const taskId = task?.id; useEffect(() => { if (!taskId || !sessionStatesKey) return; + // AC-52: the sequence marks when this refetch was *issued*, so a slow + // response can still be recognized as stale against a write issued + // after it but resolved before it. + const sequence = nextTaskSequence(taskId); getTask(taskId) .then((res) => { - if (res.task) onTaskRefetch(mapOfficeTaskToTask(res.task), res.timeline ?? []); + if (res.task) { + const mapped = mapOfficeTaskToTask(res.task); + onTaskRefetch(mapped, res.timeline ?? []); + recordRefetchCandidate(taskId, "title", mapped.title, mapped.updatedAt, sequence); + recordRefetchCandidate( + taskId, + "description", + mapped.description, + mapped.updatedAt, + sequence, + ); + } }) .catch(() => {}); void onCommentsRefetch(); @@ -252,15 +276,79 @@ function useSessionLiveSync({ // Optimistic update helpers // --------------------------------------------------------------------------- -function useTaskOptimisticHelpers( +// Exported (in addition to being used internally by `useIssueData`) so unit +// tests can drive the real guard-cleanup and restore behavior directly, +// rather than only through a fully-rendered page. +export function useTaskOptimisticHelpers( id: string, setTask: React.Dispatch>, setTimeline: React.Dispatch>, ) { + const storeApi = useAppStoreApi(); + + const applyTaskPatch = useCallback( + (patch: Partial) => { + setTask((prev) => (prev && prev.id === id ? { ...prev, ...patch } : prev)); + }, + [id, setTask], + ); + + // AC-27/49/50/71: apply a field's canonical value to the displayed task + // (and the Office task store entry) the instant it becomes unguarded — + // either because a candidate was recorded while already unguarded, or + // because the last open editor / in-flight write for that field just + // cleared. `notifyField` fires unconditionally on every guard release + // (office-task-content-sync.ts), so these listeners own the "if the two + // differ" comparison against what's currently displayed/stored (AC-71): + // without it, a no-op editor open/close on a task with no description + // would write "" over an `undefined` description, since the canonical + // value is always normalised to "" but the store never held one. + useEffect(() => { + let unmounted = false; + const unsubTitle = subscribeField(id, "title", (value) => { + setTask((prev) => + prev && prev.id === id && prev.title !== value ? { ...prev, title: value } : prev, + ); + const current = storeApi.getState().office.tasks.items.find((t) => t.id === id); + if (current && current.title !== value) { + storeApi.getState().patchTaskInStore(id, { title: value }); + } + if (unmounted && !isFieldGuarded(id, "title")) unsubTitle(); + }); + const unsubDescription = subscribeField(id, "description", (value) => { + setTask((prev) => + prev && prev.id === id && (prev.description ?? "") !== value + ? { ...prev, description: value } + : prev, + ); + const current = storeApi.getState().office.tasks.items.find((t) => t.id === id); + if (current && (current.description ?? "") !== value) { + storeApi.getState().patchTaskInStore(id, { description: value }); + } + if (unmounted && !isFieldGuarded(id, "description")) unsubDescription(); + }); + return () => { + // AC-68/69: ending the open-editor contribution can synchronously flush + // a canonical value that was recorded while guarded (deferred-apply), + // and that flush needs the listeners above still registered to reach + // the Office task store. A write issued before unmount is deliberately + // left pending by endAllEditorsForTask and must still be able to reach + // the store once it resolves, so a field with a write still in flight + // keeps its listener alive past this cleanup; each listener retires + // itself once that field's guard finally clears. + unmounted = true; + endAllEditorsForTask(id); + if (!isFieldGuarded(id, "title")) unsubTitle(); + if (!isFieldGuarded(id, "description")) unsubDescription(); + }; + }, [id, setTask, storeApi]); + // Refetch the canonical task DTO when the backend broadcasts an update // (priority / project / parent / blockers / participants / assignee). // The optimistic patch we applied locally gets reconciled with server - // state. + // state. Title/description are excluded from this merge — the + // subscribeField effect above governs them so a guarded field's + // optimistic/in-flight value survives an unrelated refetch (AC-38/39). // // A WS-driven trigger can fire again before a prior GET resolves (two // status changes in quick succession, ordinary network jitter). Without @@ -271,12 +359,19 @@ function useTaskOptimisticHelpers( const { begin, isCurrent } = useLatestOnly(); const refetchTask = useCallback(async () => { const token = begin(); + // AC-52: the sequence marks when this refetch was *issued*. + const sequence = nextTaskSequence(id); try { const [res, genericTask] = await Promise.all([getTask(id), fetchTask(id).catch(() => null)]); if (!isCurrent(token)) return; if (res.task) { - setTask(mapOfficeTaskToTask(res.task, genericTask ?? undefined)); + const mapped = mapOfficeTaskToTask(res.task, genericTask ?? undefined); + setTask((prev) => + prev ? { ...mapped, title: prev.title, description: prev.description } : mapped, + ); if (res.timeline) setTimeline(res.timeline); + recordRefetchCandidate(id, "title", mapped.title, mapped.updatedAt, sequence); + recordRefetchCandidate(id, "description", mapped.description, mapped.updatedAt, sequence); } } catch { /* swallow — next user action will retry */ @@ -286,18 +381,22 @@ function useTaskOptimisticHelpers( void refetchTask(); }); - const applyTaskPatch = useCallback( - (patch: Partial) => { - setTask((prev) => (prev ? { ...prev, ...patch } : prev)); - }, - [setTask], - ); - + // `restoreTask` backs `TaskOptimisticContextValue.restore`, the failure + // rollback for the generic picker mutation hook (status/priority/project/ + // parent/labels/blockedBy/assignee). Title/description are excluded from + // the restored snapshot and always kept at their live value — those two + // fields are governed exclusively by the guard above, and this generic + // rollback never patches them optimistically in the first place (AC-61: + // only two writers may touch a guarded field, and this isn't one of them). const restoreTask = useCallback( (snapshot: Task) => { - setTask(snapshot); + setTask((prev) => + prev && prev.id === id + ? { ...snapshot, title: prev.title, description: prev.description } + : prev, + ); }, - [setTask], + [id, setTask], ); return { applyTaskPatch, restoreTask }; @@ -318,6 +417,14 @@ function useTaskDetailRefetch( // loaded by the initial detail request until the next full load. statusSummary: updated.statusSummary ?? previous.statusSummary, repositories: updated.repositories ?? previous.repositories, + // AC-27/28/38/39/49/50: title/description must not be blindly + // overwritten by this whole-task refetch merge — they're + // governed exclusively by the subscribeField-driven + // deferred-apply effect in useTaskOptimisticHelpers, which + // respects the open-editor/in-flight-write guard. Every other + // field applies immediately. + title: previous.title, + description: previous.description, } : updated, ); @@ -337,7 +444,48 @@ function useTaskDetailRefetch( // driven refetch in useTaskOptimisticHelpers handles canonical refresh after // a property mutation commits). // `errorKey` remains a catalog key so locale changes do not re-issue the load. -function useIssueData(id: string) { + +// AC-59: seeds each field's canonical value from a loaded task. +function seedCanonicalTitleAndDescription(taskId: string, task: Task): void { + seedInitialCanonical(taskId, "title", task.title, task.updatedAt); + seedInitialCanonical(taskId, "description", task.description ?? "", task.updatedAt); +} + +// P1 review fix: the store-cache fast path can observe an unpersisted +// optimistic value (a title/description write still in flight when the user +// last navigated away). Seeding from it unconditionally would let that value +// win an equal-timestamp tiebreak against the already-recorded canonical +// value on a revisit before the authoritative GET resolves. Only seed a +// field from the cache when nothing is recorded for it yet; once a canonical +// value exists, only the authoritative GET response may supersede it. +function seedCanonicalTitleAndDescriptionIfAbsent(taskId: string, task: Task): void { + if (getCanonicalValue(taskId, "title") === undefined) { + seedInitialCanonical(taskId, "title", task.title, task.updatedAt); + } + if (getCanonicalValue(taskId, "description") === undefined) { + seedInitialCanonical(taskId, "description", task.description ?? "", task.updatedAt); + } +} + +// AC-59/AC-72: if the authoritative GET fails but the page is already +// rendering this task interactively from the store cache, seed canonical +// from it — otherwise a commit issued before the load ever succeeds (or if +// it never does) would have no restore target on failure (AC-9/AC-64). In +// practice this is now redundant with the synchronous seed in `load()` +// below, but is kept as a defensive fallback for the store-miss case. +function handleInitialLoadFailure( + taskId: string, + fromStoreTask: Task | null, + setErrorKey: (key: string | null) => void, +): void { + if (fromStoreTask) { + seedCanonicalTitleAndDescription(taskId, fromStoreTask); + } else { + setErrorKey("office:failedToLoadTask"); + } +} + +export function useIssueData(id: string) { const storeIssues = useAppStore((s) => s.office.tasks.items); const setTaskSessionsForTask = useAppStore((s) => s.setTaskSessionsForTask); const storeIssuesRef = useRef(storeIssues); @@ -370,7 +518,16 @@ function useIssueData(id: string) { setLoading(true); setErrorKey(null); const fromStore = storeIssuesRef.current.find((i) => i.id === id); - if (fromStore && !cancelled) setTask(mapOfficeTaskToTask(fromStore)); + const fromStoreTask = fromStore ? mapOfficeTaskToTask(fromStore) : null; + if (fromStoreTask && !cancelled) { + setTask(fromStoreTask); + // AC-59/AC-9/AC-64: the page is interactive from here, so a commit + // issued before the GET below resolves needs a canonical value to + // restore to on failure. Seeded only if absent (see the function's + // doc comment): the later GET-success seed below is what's allowed + // to supersede an existing canonical value. + seedCanonicalTitleAndDescriptionIfAbsent(id, fromStoreTask); + } try { const [res, genericTask] = await Promise.all([ @@ -382,13 +539,24 @@ function useIssueData(id: string) { if (!fromStore) setErrorKey("office:taskNotFound"); } else { const freshTask = mapOfficeTaskToTask(res.task, genericTask ?? undefined); - setTask(freshTask); + // AC-27/28/38/39: this GET can resolve after a title/description + // commit has already landed (the page renders interactively from + // `fromStore` before this request settles), so it must not blindly + // overwrite those two fields — same merge pattern as `refetchTask` + // and `onTaskRefetch` below. + setTask((prev) => + prev && prev.id === id + ? { ...freshTask, title: prev.title, description: prev.description } + : freshTask, + ); if (res.timeline) setTimeline(res.timeline); + // AC-59: the initial load seeds each field's canonical value. + seedCanonicalTitleAndDescription(id, freshTask); const detail = await fetchIssueDetailData(freshTask.workspaceId, id); if (!cancelled) applyDetail(detail); } } catch { - if (!cancelled && !fromStore) setErrorKey("office:failedToLoadTask"); + if (!cancelled) handleInitialLoadFailure(id, fromStoreTask, setErrorKey); } if (!cancelled) setLoading(false); diff --git a/apps/web/components/task/simple/OfficeSimplePane.test.tsx b/apps/web/components/task/simple/OfficeSimplePane.test.tsx index 3623c019f8..7ded6d2bf7 100644 --- a/apps/web/components/task/simple/OfficeSimplePane.test.tsx +++ b/apps/web/components/task/simple/OfficeSimplePane.test.tsx @@ -1,13 +1,30 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { cleanup, render, screen } from "@testing-library/react"; +import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; import type { ReactNode } from "react"; import { StateProvider } from "@/components/state-provider"; +import { TaskOptimisticContextProvider } from "@/hooks/use-optimistic-task-mutation"; import type { Task, TaskSession } from "@/app/office/tasks/[id]/types"; import { OfficeTopbarChromeProvider, useOfficeTopbarChrome, } from "@/app/office/components/office-topbar-context"; +vi.mock("@/lib/api/domains/kanban-api", async () => { + const actual = await vi.importActual( + "@/lib/api/domains/kanban-api", + ); + return { + ...actual, + updateTask: vi.fn(), + }; +}); + +const TITLE_HEADING_TESTID = "office-task-title"; +const TITLE_INPUT_TESTID = "office-task-title-input"; +const DESCRIPTION_READ_TESTID = "office-task-description"; +const DESCRIPTION_TEXTAREA_TESTID = "office-task-description-textarea"; +const SOME_DESCRIPTION = "Some description"; + const { CHAT_EDITABLE, CHAT_READONLY, CHAT_READONLY_TEST_ID } = vi.hoisted(() => ({ CHAT_EDITABLE: "editable", CHAT_READONLY: "readonly", @@ -85,8 +102,14 @@ vi.mock("./chat-activity-tabs", () => ({ })); import { OfficeSimplePane } from "./OfficeSimplePane"; +import { updateTask } from "@/lib/api/domains/kanban-api"; -afterEach(() => cleanup()); +const mockedUpdateTask = vi.mocked(updateTask); + +afterEach(() => { + cleanup(); + vi.clearAllMocks(); +}); const baseTask: Task = { id: "task-1", @@ -136,6 +159,10 @@ const runningSession: TaskSession = { updatedAt: "2026-05-01T10:07:00Z", }; +function optimisticContextFor(task: Task) { + return { task, applyPatch: vi.fn(), restore: vi.fn() }; +} + function TopbarActions() { const chrome = useOfficeTopbarChrome(); return <>{chrome?.actions}; @@ -145,8 +172,10 @@ function PaneHarness({ task, sessions }: { task: Task; sessions: TaskSession[] } return ( - - + + + + ); @@ -208,3 +237,73 @@ describe("OfficeSimplePane ExecutionIndicator wiring", () => { expect(screen.getByTestId("execution-indicator").textContent).toBe("in_progress"); }); }); + +describe("OfficeSimplePane editor composition (AC-35, AC-57)", () => { + it("commits the title while a dirty description draft stays open with its draft intact (AC-35)", async () => { + mockedUpdateTask.mockResolvedValueOnce({ + title: "New title", + updated_at: "2026-05-02T00:00:00Z", + } as never); + const task = { ...baseTask, description: SOME_DESCRIPTION }; + renderPane(task, []); + + fireEvent.click(screen.getByTestId(DESCRIPTION_READ_TESTID)); + const textarea = screen.getByTestId(DESCRIPTION_TEXTAREA_TESTID) as HTMLTextAreaElement; + fireEvent.change(textarea, { target: { value: "Dirty draft" } }); + + fireEvent.doubleClick(screen.getByTestId(TITLE_HEADING_TESTID)); + const titleInput = screen.getByTestId(TITLE_INPUT_TESTID); + fireEvent.change(titleInput, { target: { value: "New title" } }); + await act(async () => { + fireEvent.keyDown(titleInput, { key: "Enter" }); + }); + + expect(mockedUpdateTask).toHaveBeenCalledWith(task.id, { title: "New title" }); + expect(screen.queryByTestId(TITLE_INPUT_TESTID)).toBeNull(); + expect((screen.getByTestId(DESCRIPTION_TEXTAREA_TESTID) as HTMLTextAreaElement).value).toBe( + "Dirty draft", + ); + }); + + it("closes the title editor and discards its draft when the description editor opens (AC-57)", () => { + const task = { ...baseTask, description: SOME_DESCRIPTION }; + renderPane(task, []); + + fireEvent.doubleClick(screen.getByTestId(TITLE_HEADING_TESTID)); + const titleInput = screen.getByTestId(TITLE_INPUT_TESTID); + fireEvent.change(titleInput, { target: { value: "Uncommitted draft" } }); + + // Opening the description editor moves focus off the title input. + fireEvent.blur(titleInput); + fireEvent.click(screen.getByTestId(DESCRIPTION_READ_TESTID)); + + expect(screen.queryByTestId(TITLE_INPUT_TESTID)).toBeNull(); + expect(screen.getByTestId(TITLE_HEADING_TESTID).textContent).toBe(task.title); + expect(screen.getByTestId(DESCRIPTION_TEXTAREA_TESTID)).toBeTruthy(); + expect(mockedUpdateTask).not.toHaveBeenCalled(); + }); +}); + +describe("OfficeSimplePane editors have no status/lifecycle/archived gate (AC-30, AC-47)", () => { + it("still opens the title and description editors when the task is cancelled with a failed session", () => { + const task = { ...baseTask, status: "cancelled" as const, description: SOME_DESCRIPTION }; + renderPane(task, [completedSession, failedSession]); + + fireEvent.doubleClick(screen.getByTestId(TITLE_HEADING_TESTID)); + expect(screen.getByTestId(TITLE_INPUT_TESTID)).toBeTruthy(); + + fireEvent.click(screen.getByTestId(DESCRIPTION_READ_TESTID)); + expect(screen.getByTestId(DESCRIPTION_TEXTAREA_TESTID)).toBeTruthy(); + }); + + it("still opens the title and description editors while a session is RUNNING on a done task", () => { + const task = { ...baseTask, status: "done" as const, description: SOME_DESCRIPTION }; + renderPane(task, [runningSession]); + + fireEvent.doubleClick(screen.getByTestId(TITLE_HEADING_TESTID)); + expect(screen.getByTestId(TITLE_INPUT_TESTID)).toBeTruthy(); + + fireEvent.click(screen.getByTestId(DESCRIPTION_READ_TESTID)); + expect(screen.getByTestId(DESCRIPTION_TEXTAREA_TESTID)).toBeTruthy(); + }); +}); diff --git a/apps/web/components/task/simple/OfficeSimplePane.tsx b/apps/web/components/task/simple/OfficeSimplePane.tsx index 19b47cfe8e..55bf046515 100644 --- a/apps/web/components/task/simple/OfficeSimplePane.tsx +++ b/apps/web/components/task/simple/OfficeSimplePane.tsx @@ -33,6 +33,8 @@ import { copyToClipboard } from "@/lib/utils/copy-to-clipboard"; import { NewTaskDialog } from "@/app/office/components/new-task-dialog"; import { ActiveSessionRefProvider } from "./components/active-session-ref-context"; import { TopbarWorkingIndicator } from "./components/topbar-working-indicator"; +import { TaskTitleEditable } from "./components/task-title-editable"; +import { TaskDescriptionEditable } from "./components/task-description-editable"; import { TreeCancelDialog } from "@/components/task/TreeCancelDialog"; import { cancelTaskTree, @@ -507,12 +509,12 @@ export function OfficeSimplePane({ executionState={task.executionState} /> )} -

{task.title}

- {task.description && ( -
- {task.description} -
- )} + + { + const actual = await vi.importActual( + "@/lib/api/domains/kanban-api", + ); + return { + ...actual, + updateTask: vi.fn(), + }; +}); + +import { updateTask } from "@/lib/api/domains/kanban-api"; + +const mockedUpdateTask = vi.mocked(updateTask); + +const ORIGINAL_DESCRIPTION = "Original description"; +const CHANGED_DESCRIPTION = "Changed description"; +const READ_TESTID = "office-task-description"; +const EMPTY_TESTID = "office-task-description-empty"; +const TEXTAREA_TESTID = "office-task-description-textarea"; +const SAVE_TESTID = "office-task-description-save"; +const CANCEL_TESTID = "office-task-description-cancel"; +const DISCARD_CONFIRM_TESTID = "office-task-description-discard-confirm"; +const RUNNING_NOTE_TESTID = "office-task-description-running-note"; +const UPDATED_AT = "2026-05-02T00:00:00Z"; + +afterEach(() => { + cleanup(); + vi.clearAllMocks(); + __resetOfficeTaskContentSyncForTests(); +}); + +const task: Task = { + id: "t-1", + workspaceId: "ws-1", + identifier: "TASK-1", + title: "Task", + description: ORIGINAL_DESCRIPTION, + status: "todo", + priority: "medium", + labels: [], + blockedBy: [], + blocking: [], + children: [], + reviewers: [], + approvers: [], + decisions: [], + createdBy: "user", + createdAt: "2026-05-01T00:00:00Z", + updatedAt: "2026-05-01T00:00:00Z", +}; + +function Wrapper({ children }: { children: ReactNode }) { + const ctx = { task, applyPatch: vi.fn(), restore: vi.fn() }; + return ( + + {children} + + ); +} + +function renderEditable(description: string | undefined, sessions: TaskSession[] = []) { + return render( + + + , + ); +} + +function openEditor(description = ORIGINAL_DESCRIPTION) { + renderEditable(description); + fireEvent.click(screen.getByTestId(READ_TESTID)); + return screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement; +} + +function changeDraft(textarea: HTMLTextAreaElement, value: string) { + fireEvent.change(textarea, { target: { value } }); +} + +describe("TaskDescriptionEditable read view", () => { + it("renders the empty-state placeholder when there is no description", () => { + renderEditable(undefined); + expect(screen.getByTestId(EMPTY_TESTID)).toBeTruthy(); + expect(screen.queryByTestId(READ_TESTID)).toBeNull(); + }); + + it("opens the editor with an empty draft when the empty-state placeholder is clicked (AC-13)", () => { + renderEditable(undefined); + fireEvent.click(screen.getByTestId(EMPTY_TESTID)); + const textarea = screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement; + expect(textarea.value).toBe(""); + }); + + it("renders the description text and opens the editor on click, focused (AC-12)", () => { + const textarea = openEditor(); + expect(textarea.value).toBe(ORIGINAL_DESCRIPTION); + expect(document.activeElement).toBe(textarea); + }); + + it("opens the editor when the description element has keyboard focus and Enter is pressed (AC-33)", () => { + renderEditable(ORIGINAL_DESCRIPTION); + fireEvent.keyDown(screen.getByTestId(READ_TESTID), { key: "Enter" }); + const textarea = screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement; + expect(textarea.value).toBe(ORIGINAL_DESCRIPTION); + }); + + it("renders Markdown syntax as literal plain text rather than formatted output (AC-46)", () => { + const markdown = "# Heading\n\n**bold** and _italic_ and [a link](https://example.com)"; + renderEditable(markdown); + const view = screen.getByTestId(READ_TESTID); + expect(view.textContent).toBe(markdown); + expect(view.querySelector("strong")).toBeNull(); + expect(view.querySelector("em")).toBeNull(); + expect(view.querySelector("a")).toBeNull(); + expect(view.querySelector("h1")).toBeNull(); + }); + + it("does not open the editor when clicking to select text", () => { + renderEditable(ORIGINAL_DESCRIPTION); + const view = screen.getByTestId(READ_TESTID); + const getSelectionSpy = vi.spyOn(window, "getSelection").mockReturnValue({ + toString: () => "some selected text", + } as Selection); + fireEvent.click(view); + expect(screen.queryByTestId(TEXTAREA_TESTID)).toBeNull(); + getSelectionSpy.mockRestore(); + }); +}); + +describe("TaskDescriptionEditable save", () => { + it("Save is disabled until the draft differs from the baseline", () => { + const textarea = openEditor(); + const save = screen.getByTestId(SAVE_TESTID) as HTMLButtonElement; + expect(save.disabled).toBe(true); + changeDraft(textarea, CHANGED_DESCRIPTION); + expect(save.disabled).toBe(false); + }); + + it("commits a trimmed draft on Save and closes the editor", async () => { + mockedUpdateTask.mockResolvedValue({ + description: CHANGED_DESCRIPTION, + updated_at: UPDATED_AT, + } as never); + const textarea = openEditor(); + changeDraft(textarea, ` ${CHANGED_DESCRIPTION} `); + await act(async () => { + fireEvent.click(screen.getByTestId(SAVE_TESTID)); + }); + expect(mockedUpdateTask).toHaveBeenCalledWith("t-1", { description: CHANGED_DESCRIPTION }); + expect(screen.queryByTestId(TEXTAREA_TESTID)).toBeNull(); + }); + + it("sends and persists a long description unmodified apart from whitespace trimming (AC-45)", async () => { + const longDescription = "Lorem ipsum dolor sit amet. ".repeat(400).trim(); + mockedUpdateTask.mockResolvedValue({ + description: longDescription, + updated_at: UPDATED_AT, + } as never); + const textarea = openEditor(); + changeDraft(textarea, ` ${longDescription} `); + await act(async () => { + fireEvent.click(screen.getByTestId(SAVE_TESTID)); + }); + expect(mockedUpdateTask).toHaveBeenCalledWith("t-1", { description: longDescription }); + expect(screen.queryByTestId(TEXTAREA_TESTID)).toBeNull(); + }); +}); + +describe("TaskDescriptionEditable clearing to empty (AC-24)", () => { + it("clears the stored description and renders the empty-state placeholder after saving an empty draft", async () => { + mockedUpdateTask.mockResolvedValue({ + description: "", + updated_at: UPDATED_AT, + } as never); + const { rerender } = render( + + + , + ); + fireEvent.click(screen.getByTestId(READ_TESTID)); + const textarea = screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement; + changeDraft(textarea, " "); + await act(async () => { + fireEvent.click(screen.getByTestId(SAVE_TESTID)); + }); + expect(mockedUpdateTask).toHaveBeenCalledWith("t-1", { description: "" }); + expect(screen.queryByTestId(TEXTAREA_TESTID)).toBeNull(); + + // The parent re-renders with the newly cleared description once the + // store/canonical value converges (simulated here via rerender). + rerender( + + + , + ); + expect(screen.getByTestId(EMPTY_TESTID)).toBeTruthy(); + }); +}); + +describe("TaskDescriptionEditable whitespace-only draft on an empty description (AC-43)", () => { + it("treats a whitespace-only draft as equal to an empty baseline, keeping Save disabled and sending no request", () => { + renderEditable(""); + fireEvent.click(screen.getByTestId(EMPTY_TESTID)); + const textarea = screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement; + changeDraft(textarea, " "); + expect((screen.getByTestId(SAVE_TESTID) as HTMLButtonElement).disabled).toBe(true); + fireEvent.click(screen.getByTestId(SAVE_TESTID)); + expect(mockedUpdateTask).not.toHaveBeenCalled(); + }); +}); + +describe("TaskDescriptionEditable in-flight save state (AC-41)", () => { + it("keeps the textarea editable, disables Save and Cancel, and takes no action on Escape while saving", async () => { + let resolveUpdate: (value: never) => void = () => {}; + mockedUpdateTask.mockImplementation( + () => + new Promise((resolve) => { + resolveUpdate = resolve; + }), + ); + const textarea = openEditor(); + changeDraft(textarea, CHANGED_DESCRIPTION); + + await act(async () => { + fireEvent.click(screen.getByTestId(SAVE_TESTID)); + // Yield one microtask so the `isSaving` state commits without waiting + // for the still-pending mocked update to resolve. + await Promise.resolve(); + }); + + expect(textarea.disabled).toBe(false); + expect((screen.getByTestId(SAVE_TESTID) as HTMLButtonElement).disabled).toBe(true); + expect((screen.getByTestId(CANCEL_TESTID) as HTMLButtonElement).disabled).toBe(true); + + fireEvent.keyDown(textarea, { key: "Escape" }); + expect(screen.getByTestId(TEXTAREA_TESTID)).toBeTruthy(); + + await act(async () => { + resolveUpdate({ + description: CHANGED_DESCRIPTION, + updated_at: UPDATED_AT, + } as never); + }); + }); +}); + +describe("TaskDescriptionEditable draft changed mid-save (AC-42)", () => { + it("keeps the editor open with the newer draft, rebases the baseline to the saved value, and re-enables Save", async () => { + let resolveUpdate: (value: never) => void = () => {}; + mockedUpdateTask.mockImplementation( + () => + new Promise((resolve) => { + resolveUpdate = resolve; + }), + ); + const textarea = openEditor(); + changeDraft(textarea, "First draft"); + + await act(async () => { + fireEvent.click(screen.getByTestId(SAVE_TESTID)); + await Promise.resolve(); + }); + + // The user keeps typing while the save for "First draft" is in flight. + changeDraft(textarea, "Second draft"); + + await act(async () => { + resolveUpdate({ description: "First draft", updated_at: UPDATED_AT } as never); + }); + + expect(screen.getByTestId(TEXTAREA_TESTID)).toBeTruthy(); + expect((screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement).value).toBe("Second draft"); + expect((screen.getByTestId(SAVE_TESTID) as HTMLButtonElement).disabled).toBe(false); + }); +}); + +describe("TaskDescriptionEditable blur (AC-21)", () => { + it("keeps the editor open, keeps the draft, and sends no request when the textarea loses focus", () => { + const textarea = openEditor(); + changeDraft(textarea, "Changed but not saved"); + fireEvent.blur(textarea); + expect(screen.getByTestId(TEXTAREA_TESTID)).toBeTruthy(); + expect((screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement).value).toBe( + "Changed but not saved", + ); + expect(mockedUpdateTask).not.toHaveBeenCalled(); + }); +}); + +describe("TaskDescriptionEditable escape and cancel", () => { + it("closes the editor on Escape when the draft is clean", () => { + const textarea = openEditor(); + fireEvent.keyDown(textarea, { key: "Escape" }); + expect(screen.queryByTestId(TEXTAREA_TESTID)).toBeNull(); + }); + + it("does nothing on Escape when the draft is dirty", () => { + const textarea = openEditor(); + changeDraft(textarea, "Changed"); + fireEvent.keyDown(textarea, { key: "Escape" }); + expect(screen.getByTestId(TEXTAREA_TESTID)).toBeTruthy(); + }); + + it("closes immediately on Cancel when the draft is clean", () => { + openEditor(); + fireEvent.click(screen.getByTestId(CANCEL_TESTID)); + expect(screen.queryByTestId(TEXTAREA_TESTID)).toBeNull(); + }); +}); + +describe("TaskDescriptionEditable discard confirmation", () => { + it("shows a discard-confirm dialog on Cancel when dirty, and discards on confirm", () => { + const textarea = openEditor(); + changeDraft(textarea, "Changed"); + fireEvent.click(screen.getByTestId(CANCEL_TESTID)); + expect(screen.getByTestId(DISCARD_CONFIRM_TESTID)).toBeTruthy(); + fireEvent.click(screen.getByRole("button", { name: "Discard" })); + expect(screen.queryByTestId(TEXTAREA_TESTID)).toBeNull(); + }); + + it("keeps the draft when discard is dismissed via 'keep editing'", () => { + const textarea = openEditor(); + changeDraft(textarea, "Changed"); + fireEvent.click(screen.getByTestId(CANCEL_TESTID)); + fireEvent.click(screen.getByRole("button", { name: "Keep editing" })); + expect((screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement).value).toBe("Changed"); + }); + + it("keeps the draft when discard is dismissed via Escape rather than the explicit discard choice (AC-36)", () => { + const textarea = openEditor(); + changeDraft(textarea, "Changed"); + fireEvent.click(screen.getByTestId(CANCEL_TESTID)); + expect(screen.getByTestId(DISCARD_CONFIRM_TESTID)).toBeTruthy(); + fireEvent.keyDown(screen.getByTestId(DISCARD_CONFIRM_TESTID), { key: "Escape" }); + expect(screen.queryByTestId(DISCARD_CONFIRM_TESTID)).toBeNull(); + expect((screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement).value).toBe("Changed"); + }); +}); + +describe("TaskDescriptionEditable frozen baseline (AC-40)", () => { + it("keeps Save disabled and Escape closing when a refetch changes the stored description while open", () => { + const { rerender } = render( + + + , + ); + fireEvent.click(screen.getByTestId(READ_TESTID)); + const textarea = screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement; + + // A refetch changes the stored description while the editor is open. + rerender( + + + , + ); + + // The draft still equals the frozen (original) baseline, not the + // refetched value, so Save stays disabled... + const save = screen.getByTestId(SAVE_TESTID) as HTMLButtonElement; + expect(save.disabled).toBe(true); + + // ...and Escape closes immediately rather than being treated as dirty. + fireEvent.keyDown(textarea, { key: "Escape" }); + expect(screen.queryByTestId(TEXTAREA_TESTID)).toBeNull(); + }); +}); + +describe("TaskDescriptionEditable unmount cleanup (AC-68, COR-002)", () => { + it("releases the field's guard when the component unmounts while the editor is still open", () => { + const { unmount } = renderEditable(ORIGINAL_DESCRIPTION); + fireEvent.click(screen.getByTestId(READ_TESTID)); + expect(isFieldGuarded("t-1", "description")).toBe(true); + + unmount(); + + expect(isFieldGuarded("t-1", "description")).toBe(false); + }); + + it("is a no-op on unmount when the editor is already closed", () => { + const { unmount } = renderEditable(ORIGINAL_DESCRIPTION); + expect(isFieldGuarded("t-1", "description")).toBe(false); + + unmount(); + + expect(isFieldGuarded("t-1", "description")).toBe(false); + }); +}); + +describe("TaskDescriptionEditable taskId change while editing (P1 review fix, COR-002)", () => { + it("closes the editor and releases the old task's guard when taskId changes while open", () => { + const { rerender } = render( + + + , + ); + fireEvent.click(screen.getByTestId(READ_TESTID)); + const textarea = screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement; + changeDraft(textarea, "Changed"); + expect(isFieldGuarded("t-1", "description")).toBe(true); + + // TaskBody swaps simple/advanced panes on a URL search param, which can + // reuse this component instance for a different taskId without + // unmounting it (no key={taskId} upstream). Description never closes on + // blur (AC-21), so this is the only safety net for a still-open editor. + rerender( + + + , + ); + + expect(isFieldGuarded("t-1", "description")).toBe(false); + expect(isFieldGuarded("t-2", "description")).toBe(false); + expect(screen.queryByTestId(TEXTAREA_TESTID)).toBeNull(); + expect(screen.getByTestId(READ_TESTID).textContent).toBe("Other task description"); + }); + + it("ignores an old save result after the replacement task editor opens", async () => { + let resolveUpdate: (value: never) => void = () => {}; + mockedUpdateTask.mockImplementation( + () => + new Promise((resolve) => { + resolveUpdate = resolve; + }), + ); + const { rerender } = render( + + + , + ); + fireEvent.click(screen.getByTestId(READ_TESTID)); + const textarea = screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement; + changeDraft(textarea, CHANGED_DESCRIPTION); + + await act(async () => { + fireEvent.click(screen.getByTestId(SAVE_TESTID)); + await Promise.resolve(); + }); + + rerender( + + + , + ); + fireEvent.click(screen.getByTestId(READ_TESTID)); + expect((screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement).value).toBe( + "Other task description", + ); + + await act(async () => { + resolveUpdate({ description: CHANGED_DESCRIPTION, updated_at: UPDATED_AT } as never); + }); + + expect((screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement).value).toBe( + "Other task description", + ); + expect((screen.getByTestId(SAVE_TESTID) as HTMLButtonElement).disabled).toBe(true); + }); +}); + +describe("TaskDescriptionEditable running-session note", () => { + it("shows the note only while a session is RUNNING", () => { + const sessions: TaskSession[] = [ + { + id: "s-1", + agentName: "agent", + agentRole: "engineer", + state: "RUNNING", + isPrimary: true, + }, + ]; + renderEditable(ORIGINAL_DESCRIPTION, sessions); + fireEvent.click(screen.getByTestId(READ_TESTID)); + expect(screen.getByTestId(RUNNING_NOTE_TESTID)).toBeTruthy(); + }); + + it("does not show the note when no session is RUNNING", () => { + renderEditable(ORIGINAL_DESCRIPTION, []); + fireEvent.click(screen.getByTestId(READ_TESTID)); + expect(screen.queryByTestId(RUNNING_NOTE_TESTID)).toBeNull(); + }); + + it("keeps Save enabled while a session is RUNNING and the draft is dirty (AC-25)", () => { + const sessions: TaskSession[] = [ + { + id: "s-1", + agentName: "agent", + agentRole: "engineer", + state: "RUNNING", + isPrimary: true, + }, + ]; + renderEditable(ORIGINAL_DESCRIPTION, sessions); + fireEvent.click(screen.getByTestId(READ_TESTID)); + const textarea = screen.getByTestId(TEXTAREA_TESTID) as HTMLTextAreaElement; + changeDraft(textarea, CHANGED_DESCRIPTION); + expect(screen.getByTestId(RUNNING_NOTE_TESTID)).toBeTruthy(); + expect((screen.getByTestId(SAVE_TESTID) as HTMLButtonElement).disabled).toBe(false); + }); +}); diff --git a/apps/web/components/task/simple/components/task-description-editable.tsx b/apps/web/components/task/simple/components/task-description-editable.tsx new file mode 100644 index 0000000000..3f928a3c0d --- /dev/null +++ b/apps/web/components/task/simple/components/task-description-editable.tsx @@ -0,0 +1,321 @@ +"use client"; + +import { useCallback, useEffect, useRef, useState } from "react"; +import type { KeyboardEvent, MouseEvent } from "react"; +import { useTranslation } from "react-i18next"; +import { Button } from "@kandev/ui/button"; +import { Textarea } from "@kandev/ui/textarea"; +import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from "@kandev/ui/alert-dialog"; +import { useCommitTaskDescription } from "@/hooks/use-optimistic-task-mutation"; +import { closeFieldEditor, openFieldEditor } from "@/lib/state/office-task-content-sync"; +import type { TaskSession } from "@/app/office/tasks/[id]/types"; + +type TaskDescriptionEditableProps = { + taskId: string; + description: string | undefined; + sessions: TaskSession[]; +}; + +type DescriptionReadViewProps = { + description: string | undefined; + onActivate: () => void; +}; + +function DescriptionReadView({ description, onActivate }: DescriptionReadViewProps) { + const { t } = useTranslation(); + const trimmedDescription = (description ?? "").trim(); + + const handleKeyDown = useCallback( + (e: KeyboardEvent) => { + if (e.key === "Enter") { + e.preventDefault(); + onActivate(); + } + }, + [onActivate], + ); + + const handleClick = useCallback(() => { + const selection = window.getSelection(); + if (selection && selection.toString().length > 0) return; // AC-34 + onActivate(); + }, [onActivate]); + + const stopSelectionMouseDown = useCallback((e: MouseEvent) => { + // Allow text selection inside the read view without triggering a click + // activation once the mouseup fires with a non-empty selection. + e.stopPropagation(); + }, []); + + if (!trimmedDescription) { + return ( +
+ {t("task:addDescription")} +
+ ); + } + + return ( +
+ {description} +
+ ); +} + +type DescriptionDiscardDialogProps = { + open: boolean; + onOpenChange: (open: boolean) => void; + onConfirm: () => void; +}; + +function DescriptionDiscardDialog({ + open, + onOpenChange, + onConfirm, +}: DescriptionDiscardDialogProps) { + const { t } = useTranslation(); + return ( + + + + {t("task:discardDescriptionTitle")} + {t("task:discardDescriptionBody")} + + + {t("task:keepEditing")} + + {t("task:discard")} + + + + + ); +} + +type DescriptionEditViewProps = { + draft: string; + isDirty: boolean; + isSaving: boolean; + hasRunningSession: boolean; + showDiscardConfirm: boolean; + onChange: (value: string) => void; + onKeyDown: (e: KeyboardEvent) => void; + onSave: () => void; + onCancel: () => void; + onDiscardOpenChange: (open: boolean) => void; + onDiscardConfirm: () => void; +}; + +function DescriptionEditView({ + draft, + isDirty, + isSaving, + hasRunningSession, + showDiscardConfirm, + onChange, + onKeyDown, + onSave, + onCancel, + onDiscardOpenChange, + onDiscardConfirm, +}: DescriptionEditViewProps) { + const { t } = useTranslation(); + return ( +
+