diff --git a/packages/core/issues/cache-helpers.test.ts b/packages/core/issues/cache-helpers.test.ts new file mode 100644 index 000000000..17b8a123e --- /dev/null +++ b/packages/core/issues/cache-helpers.test.ts @@ -0,0 +1,130 @@ +import { describe, expect, it } from "vitest"; +import type { Issue, ListIssuesCache } from "../types"; +import { insertByPosition, patchIssueInBuckets } from "./cache-helpers"; + +const WS_ID = "ws-1"; + +function mk(id: string, status: Issue["status"], position: number): Issue { + return { + id, + workspace_id: WS_ID, + number: 1, + identifier: `MUL-${id}`, + title: id, + description: null, + status, + priority: "none", + assignee_type: null, + assignee_id: null, + creator_type: "member", + creator_id: "user-1", + parent_issue_id: null, + project_id: null, + position, + start_date: null, + due_date: null, + metadata: {}, + labels: [], + created_at: "2025-01-01T00:00:00Z", + updated_at: "2025-01-01T00:00:00Z", + }; +} + +function cache(byStatus: ListIssuesCache["byStatus"]): ListIssuesCache { + return { byStatus }; +} + +function ids(c: ListIssuesCache, status: Issue["status"]): string[] { + return (c.byStatus[status]?.issues ?? []).map((i) => i.id); +} + +describe("insertByPosition", () => { + it("inserts at the position-sorted slot", () => { + const a = mk("a", "todo", 1); + const c = mk("c", "todo", 3); + const b = mk("b", "todo", 2); + expect(insertByPosition([a, c], b).map((i) => i.id)).toEqual([ + "a", + "b", + "c", + ]); + }); + + it("appends when the new position is the largest", () => { + const a = mk("a", "todo", 1); + const z = mk("z", "todo", 9); + expect(insertByPosition([a], z).map((i) => i.id)).toEqual(["a", "z"]); + }); + + it("prepends when the new position is the smallest", () => { + const b = mk("b", "todo", 2); + const a = mk("a", "todo", 1); + expect(insertByPosition([b], a).map((i) => i.id)).toEqual(["a", "b"]); + }); +}); + +describe("patchIssueInBuckets — cross-status move", () => { + it("inserts the moved card at its position slot, not the end", () => { + const c0 = cache({ + todo: { issues: [mk("moved", "todo", 5)], total: 1 }, + in_progress: { + issues: [mk("x", "in_progress", 1), mk("y", "in_progress", 3)], + total: 2, + }, + }); + // Move "moved" into in_progress at position 2 (between x and y). + const next = patchIssueInBuckets(c0, "moved", { + status: "in_progress", + position: 2, + }); + expect(ids(next, "in_progress")).toEqual(["x", "moved", "y"]); + expect(ids(next, "todo")).toEqual([]); + }); + + it("adjusts both bucket totals", () => { + const c0 = cache({ + todo: { issues: [mk("moved", "todo", 5)], total: 1 }, + in_progress: { issues: [mk("x", "in_progress", 1)], total: 1 }, + }); + const next = patchIssueInBuckets(c0, "moved", { + status: "in_progress", + position: 2, + }); + expect(next.byStatus.todo?.total).toBe(0); + expect(next.byStatus.in_progress?.total).toBe(2); + }); +}); + +describe("patchIssueInBuckets — same status", () => { + it("keeps the slot for a plain field update (no reorder)", () => { + const c0 = cache({ + todo: { + issues: [mk("a", "todo", 1), mk("b", "todo", 2), mk("c", "todo", 3)], + total: 3, + }, + }); + // A remote label/title edit must not move the card. + const next = patchIssueInBuckets(c0, "b", { title: "renamed" }); + expect(ids(next, "todo")).toEqual(["a", "b", "c"]); + expect(next.byStatus.todo?.issues[1]?.title).toBe("renamed"); + }); + + it("re-sorts within the column when position changes", () => { + const c0 = cache({ + todo: { + issues: [mk("a", "todo", 1), mk("b", "todo", 2), mk("c", "todo", 3)], + total: 3, + }, + }); + // Drag "a" below "b" (new position 2.5). + const next = patchIssueInBuckets(c0, "a", { position: 2.5 }); + expect(ids(next, "todo")).toEqual(["b", "a", "c"]); + }); +}); + +describe("patchIssueInBuckets — unknown issue", () => { + it("returns the cache unchanged when the id is absent", () => { + const c0 = cache({ todo: { issues: [mk("a", "todo", 1)], total: 1 } }); + expect(patchIssueInBuckets(c0, "ghost", { position: 9 })).toBe(c0); + }); +}); diff --git a/packages/core/issues/cache-helpers.ts b/packages/core/issues/cache-helpers.ts index f47e15827..9709c584d 100644 --- a/packages/core/issues/cache-helpers.ts +++ b/packages/core/issues/cache-helpers.ts @@ -63,10 +63,28 @@ export function removeIssueFromBuckets( }); } +/** + * Insert `issue` into `issues` at the slot implied by `position ASC` — the same + * ordering the board renders (server `ORDER BY position ASC`). Returns a new + * array; the input is not mutated. + * + * Inserting at the right slot (instead of appending to the end) is what keeps an + * optimistic move from snapping: the card lands where it will be after the + * server confirms, so no later cache refresh teleports it to the column tail. + */ +export function insertByPosition(issues: Issue[], issue: Issue): Issue[] { + const idx = issues.findIndex((i) => i.position > issue.position); + if (idx === -1) return [...issues, issue]; + return [...issues.slice(0, idx), issue, ...issues.slice(idx)]; +} + /** * Merge `patch` into the issue with `id`. If `patch.status` differs from the * current bucket, the issue moves to the new bucket and both buckets' totals - * are adjusted. + * are adjusted. The moved card — and a same-column card whose `position` + * changed — is re-inserted at its `position`-sorted slot rather than appended, + * so the cache order stays consistent with what the board renders. A plain + * field update (no status/position change) keeps the card in place. */ export function patchIssueInBuckets( resp: ListIssuesCache, @@ -80,9 +98,23 @@ export function patchIssueInBuckets( if (nextStatus === loc.status) { const bucket = getBucket(resp, loc.status); + const positionChanged = + patch.position !== undefined && patch.position !== loc.issue.position; + if (!positionChanged) { + // Plain field update (labels, metadata, title, …): keep the slot so a + // remote edit never reorders an otherwise-untouched column. + return setBucket(resp, loc.status, { + ...bucket, + issues: bucket.issues.map((i) => (i.id === id ? merged : i)), + }); + } + // Same-column reorder: lift the card out and re-insert at its new slot. return setBucket(resp, loc.status, { ...bucket, - issues: bucket.issues.map((i) => (i.id === id ? merged : i)), + issues: insertByPosition( + bucket.issues.filter((i) => i.id !== id), + merged, + ), }); } @@ -93,7 +125,7 @@ export function patchIssueInBuckets( total: Math.max(0, fromBucket.total - 1), }); next = setBucket(next, nextStatus, { - issues: [...toBucket.issues, merged], + issues: insertByPosition(toBucket.issues, merged), total: toBucket.total + 1, }); return next; diff --git a/packages/core/issues/mutations.test.tsx b/packages/core/issues/mutations.test.tsx index f11c23a0b..1982ad6ba 100644 --- a/packages/core/issues/mutations.test.tsx +++ b/packages/core/issues/mutations.test.tsx @@ -2,13 +2,19 @@ * @vitest-environment jsdom */ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { act, renderHook } from "@testing-library/react"; +import { act, renderHook, waitFor } from "@testing-library/react"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import type { ReactNode } from "react"; import { setApiInstance } from "../api"; import type { ApiClient } from "../api/client"; -import { useLoadMoreByAssigneeGroup, useLoadMoreByStatus, useResolveComment } from "./mutations"; +import { + useBatchUpdateIssues, + useLoadMoreByAssigneeGroup, + useLoadMoreByStatus, + useResolveComment, + useUpdateIssue, +} from "./mutations"; import { issueKeys, type IssueSortParam, @@ -315,6 +321,228 @@ describe("useLoadMoreByAssigneeGroup", () => { }); }); +describe("useUpdateIssue — optimistic move keeps every bucketed board in sync", () => { + const sort: IssueSortParam = { sort_by: "position", sort_direction: undefined }; + const myScope = "assigned"; + const myFilter = { assignee_id: "user-1" }; + const wsKey = issueKeys.listSorted(WS_ID, sort); + // My-Issues AND the Project board both ride this myList cache; a move that + // only patched the workspace cache snaps back on those boards. + const myKey = issueKeys.myListSorted(WS_ID, myScope, myFilter, sort); + + let qc: QueryClient; + let updateIssue: ReturnType Promise>>; + + function makeBucketed(): ListIssuesCache { + return { + byStatus: { + todo: { issues: [makeIssue(1)], total: 1 }, + in_progress: { issues: [], total: 0 }, + }, + }; + } + + function bucketIds( + key: readonly unknown[], + status: "todo" | "in_progress", + ): string[] { + const c = qc.getQueryData(key); + return (c?.byStatus[status]?.issues ?? []).map((i) => i.id); + } + + beforeEach(() => { + qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + updateIssue = vi.fn(); + setApiInstance({ updateIssue } as unknown as ApiClient); + qc.setQueryData(wsKey, makeBucketed()); + qc.setQueryData(myKey, makeBucketed()); + }); + + afterEach(() => { + qc.clear(); + vi.restoreAllMocks(); + }); + + it("optimistically moves the card in both the workspace and myList caches", async () => { + let resolve!: (issue: Issue) => void; + updateIssue.mockReturnValue( + new Promise((r) => { + resolve = r; + }), + ); + + const { result } = renderHook(() => useUpdateIssue(), { + wrapper: createWrapper(qc), + }); + + act(() => { + result.current.mutate({ id: "issue-1", status: "in_progress", position: 5 }); + }); + + // Optimistic state — the regression: myList must move too, not just ws. + for (const key of [wsKey, myKey]) { + expect(bucketIds(key, "todo")).toEqual([]); + expect(bucketIds(key, "in_progress")).toEqual(["issue-1"]); + } + + await act(async () => { + resolve(makeIssue(1, { status: "in_progress", position: 5 })); + }); + + // Authoritative settle keeps the card in place in both caches. + for (const key of [wsKey, myKey]) { + expect(bucketIds(key, "in_progress")).toEqual(["issue-1"]); + } + }); + + it("rolls both caches back when the request fails", async () => { + updateIssue.mockRejectedValue(new Error("boom")); + + const { result } = renderHook(() => useUpdateIssue(), { + wrapper: createWrapper(qc), + }); + + await act(async () => { + await result.current + .mutateAsync({ id: "issue-1", status: "in_progress", position: 5 }) + .catch(() => {}); + }); + + for (const key of [wsKey, myKey]) { + expect(bucketIds(key, "todo")).toEqual(["issue-1"]); + expect(bucketIds(key, "in_progress")).toEqual([]); + } + }); + + it("does not invalidate the board list on settle (no refetch flicker)", async () => { + updateIssue.mockResolvedValue(makeIssue(1, { status: "in_progress", position: 5 })); + const invalidateSpy = vi.spyOn(qc, "invalidateQueries"); + + const { result } = renderHook(() => useUpdateIssue(), { + wrapper: createWrapper(qc), + }); + + await act(async () => { + await result.current.mutateAsync({ id: "issue-1", status: "in_progress", position: 5 }); + }); + + const invalidatedKeys = invalidateSpy.mock.calls.map((c) => c[0]?.queryKey); + // The board list + myList are reconciled surgically, never refetched. + expect(invalidatedKeys).not.toContainEqual(issueKeys.list(WS_ID)); + expect(invalidatedKeys).not.toContainEqual(issueKeys.myAll(WS_ID)); + }); +}); + +describe("useBatchUpdateIssues — optimistic patch covers filtered boards too", () => { + const sort: IssueSortParam = { sort_by: "position", sort_direction: undefined }; + const myScope = "assigned"; + const myFilter = { assignee_id: "user-1" }; + const wsKey = issueKeys.listSorted(WS_ID, sort); + const myKey = issueKeys.myListSorted(WS_ID, myScope, myFilter, sort); + + let qc: QueryClient; + let batchUpdateIssues: ReturnType< + typeof vi.fn<(ids: string[], updates: unknown) => Promise<{ updated: number }>> + >; + + function makeBucketed(): ListIssuesCache { + return { + byStatus: { + todo: { issues: [makeIssue(1)], total: 1 }, + in_progress: { issues: [], total: 0 }, + }, + }; + } + + function bucketIds(key: readonly unknown[], status: "todo" | "in_progress"): string[] { + const c = qc.getQueryData(key); + return (c?.byStatus[status]?.issues ?? []).map((i) => i.id); + } + + beforeEach(() => { + qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + batchUpdateIssues = vi.fn(); + setApiInstance({ batchUpdateIssues } as unknown as ApiClient); + qc.setQueryData(wsKey, makeBucketed()); + qc.setQueryData(myKey, makeBucketed()); + }); + + afterEach(() => { + qc.clear(); + vi.restoreAllMocks(); + }); + + it("optimistically patches BOTH the workspace and myList caches (not just ws)", async () => { + let resolve!: (r: { updated: number }) => void; + batchUpdateIssues.mockReturnValue( + new Promise<{ updated: number }>((r) => { + resolve = r; + }), + ); + + const { result } = renderHook(() => useBatchUpdateIssues(), { + wrapper: createWrapper(qc), + }); + + act(() => { + result.current.mutate({ ids: ["issue-1"], updates: { status: "in_progress" } }); + }); + + // The regression Howard flagged: batch must move the card on the myList + // board too, not only the workspace board. onMutate awaits cancelQueries, + // so the optimistic patch lands a microtask later — wait for it. + await waitFor(() => { + for (const key of [wsKey, myKey]) { + expect(bucketIds(key, "todo")).toEqual([]); + expect(bucketIds(key, "in_progress")).toEqual(["issue-1"]); + } + }); + + await act(async () => { + resolve({ updated: 1 }); + }); + + for (const key of [wsKey, myKey]) { + expect(bucketIds(key, "in_progress")).toEqual(["issue-1"]); + } + }); + + it("rolls both caches back when the request fails", async () => { + batchUpdateIssues.mockRejectedValue(new Error("boom")); + + const { result } = renderHook(() => useBatchUpdateIssues(), { + wrapper: createWrapper(qc), + }); + + await act(async () => { + await result.current + .mutateAsync({ ids: ["issue-1"], updates: { status: "in_progress" } }) + .catch(() => {}); + }); + + for (const key of [wsKey, myKey]) { + expect(bucketIds(key, "todo")).toEqual(["issue-1"]); + expect(bucketIds(key, "in_progress")).toEqual([]); + } + }); + + it("does not invalidate the board list on settle (no refetch flicker)", async () => { + batchUpdateIssues.mockResolvedValue({ updated: 1 }); + const invalidateSpy = vi.spyOn(qc, "invalidateQueries"); + + const { result } = renderHook(() => useBatchUpdateIssues(), { + wrapper: createWrapper(qc), + }); + + await act(async () => { + await result.current.mutateAsync({ ids: ["issue-1"], updates: { status: "in_progress" } }); + }); + + const invalidatedKeys = invalidateSpy.mock.calls.map((c) => c[0]?.queryKey); + expect(invalidatedKeys).not.toContainEqual(issueKeys.list(WS_ID)); + }); +}); + describe("useResolveComment", () => { const ISSUE_ID = "issue-1"; diff --git a/packages/core/issues/mutations.ts b/packages/core/issues/mutations.ts index e072ce94a..129991108 100644 --- a/packages/core/issues/mutations.ts +++ b/packages/core/issues/mutations.ts @@ -211,6 +211,21 @@ export function useCreateIssue() { export function useUpdateIssue() { const qc = useQueryClient(); const wsId = useWorkspaceId(); + // Every bucketed board cache an optimistic move must keep in sync: the + // workspace board (issueKeys.list*) AND the My-Issues / Project board + // (issueKeys.myList* under `my`), which share the ListIssuesCache shape. + // Filtering by `byStatus` skips the grouped (assignee) and flat + // (gantt/detail/children) caches that also live under those prefixes. The + // board reconciles local columns from its own feeding cache on settle, so a + // move that only patched the workspace cache would snap back on My-Issues / + // Project boards. + const readBucketedLists = () => + [ + ...qc.getQueriesData({ queryKey: issueKeys.list(wsId) }), + ...qc.getQueriesData({ queryKey: issueKeys.myAll(wsId) }), + ].filter( + (entry): entry is [QueryKey, ListIssuesCache] => !!entry[1]?.byStatus, + ); return useMutation({ mutationFn: ({ id, ...data }: { id: string } & UpdateIssueRequest) => api.updateIssue(id, data), @@ -220,7 +235,8 @@ export function useUpdateIssue() { // yield to the event loop, letting @dnd-kit reset its visual state // before the optimistic update lands. qc.cancelQueries({ queryKey: issueKeys.list(wsId) }); - const prevLists = qc.getQueriesData({ queryKey: issueKeys.list(wsId) }); + qc.cancelQueries({ queryKey: issueKeys.myAll(wsId) }); + const prevLists = readBucketedLists(); const firstListData = prevLists[0]?.[1]; const prevDetail = qc.getQueryData(issueKeys.detail(wsId, id)); @@ -280,9 +296,29 @@ export function useUpdateIssue() { ); } }, + onSuccess: (serverIssue) => { + // Reconcile with the authoritative server entity by patching the one card + // in place — NOT by invalidating + refetching the list. The list refetch + // is what made a successful move flicker: the optimistic card was already + // in the right place, then the refetch replaced the whole column and the + // card re-landed. updateIssue returns the full issue and a position update + // touches only that row, so a surgical patch is the authoritative + // reconcile and is a visual no-op when the optimistic value matched. + for (const [key, cached] of readBucketedLists()) { + qc.setQueryData( + key, + patchIssueInBuckets(cached, serverIssue.id, serverIssue), + ); + } + qc.setQueryData(issueKeys.detail(wsId, serverIssue.id), (old) => + old ? { ...old, ...serverIssue } : old, + ); + }, onSettled: (_data, _err, vars, ctx) => { - qc.invalidateQueries({ queryKey: issueKeys.detail(wsId, vars.id) }); - qc.invalidateQueries({ queryKey: issueKeys.list(wsId) }); + // The issue's own list + detail caches are reconciled surgically in + // onSuccess / onError, so they are deliberately NOT invalidated here — a + // full-list refetch on settle is what made drags flicker. Only aggregate + // caches that cannot be patched from a single issue are refreshed below. qc.invalidateQueries({ queryKey: issueKeys.assigneeGroupsAll(wsId) }); qc.invalidateQueries({ queryKey: issueKeys.myAssigneeGroupsAll(wsId) }); qc.invalidateQueries({ queryKey: issueKeys.projectGanttAll(wsId) }); @@ -408,10 +444,21 @@ export function useBatchUpdateIssues() { updates: UpdateIssueRequest; }) => api.batchUpdateIssues(ids, updates), onMutate: async ({ ids, updates }) => { + // Patch BOTH the workspace board (issueKeys.list) and the filtered + // My-Issues / Project / actor lists (issueKeys.myAll). The single-issue + // update already patches both; batch only touched issueKeys.list, so a + // batch edit on a My-Issues board had no optimistic effect and relied + // entirely on the settle refetch. Filter to bucketed (byStatus) caches so + // grouped/flat caches under the same prefix are skipped. await qc.cancelQueries({ queryKey: issueKeys.list(wsId) }); - const prevLists = qc.getQueriesData({ queryKey: issueKeys.list(wsId) }); + await qc.cancelQueries({ queryKey: issueKeys.myAll(wsId) }); + const prevLists = [ + ...qc.getQueriesData({ queryKey: issueKeys.list(wsId) }), + ...qc.getQueriesData({ queryKey: issueKeys.myAll(wsId) }), + ].filter( + (entry): entry is [QueryKey, ListIssuesCache] => !!entry[1]?.byStatus, + ); for (const [key, cached] of prevLists) { - if (!cached) continue; let next = cached; for (const id of ids) next = patchIssueInBuckets(next, id, updates); qc.setQueryData(key, next); @@ -451,7 +498,13 @@ export function useBatchUpdateIssues() { } }, onSettled: (_data, _err, _vars, ctx) => { - qc.invalidateQueries({ queryKey: issueKeys.list(wsId) }); + // Deliberately NOT invalidating issueKeys.list / myAll here: the onMutate + // patch above is a complete surgical reconcile for these bucketed boards + // (batch changes status / priority / project — never a server-computed + // value), so a full-board refetch on settle would only re-introduce the + // flicker the single-issue update already removed. Aggregate / grouped + // caches that cannot be recomputed from a single-issue patch are still + // refreshed below. qc.invalidateQueries({ queryKey: issueKeys.assigneeGroupsAll(wsId) }); qc.invalidateQueries({ queryKey: issueKeys.myAssigneeGroupsAll(wsId) }); qc.invalidateQueries({ queryKey: issueKeys.projectGanttAll(wsId) }); diff --git a/packages/core/issues/ws-updaters.test.ts b/packages/core/issues/ws-updaters.test.ts index aad2ad365..043dad29b 100644 --- a/packages/core/issues/ws-updaters.test.ts +++ b/packages/core/issues/ws-updaters.test.ts @@ -263,6 +263,67 @@ describe("project progress invalidation", () => { }); }); +describe("onIssueUpdated — position move is surgical, not a list refetch", () => { + let qc: QueryClient; + + beforeEach(() => { + qc = new QueryClient(); + }); + + const issueA: Issue = { ...baseIssue, id: "issue-1", position: 0 }; + const issueB: Issue = { ...baseIssue, id: "issue-2", position: 10 }; + + it("reorders the moved card in place and does NOT invalidate the workspace list", () => { + qc.setQueryData(issueKeys.list(WS_ID), makeListCache(issueA, issueB)); + + // issue-1 moves below issue-2 (position 0 -> 20) — a remote/echoed drag. + onIssueUpdated(qc, WS_ID, { ...issueA, position: 20 }); + + const list = qc.getQueryData(issueKeys.list(WS_ID)); + // Surgically reordered into its new slot: proof the patch alone suffices. + expect(list?.byStatus.todo?.issues.map((i) => i.id)).toEqual(["issue-2", "issue-1"]); + // The old redundant `position -> invalidate(list)` is gone — no full-board + // refetch on top of the surgical patch (that was the flicker source). + expect(qc.getQueryState(issueKeys.list(WS_ID))?.isInvalidated).toBe(false); + }); + + it("surgically patches the filtered myAll lists on a non-membership change (no refetch)", () => { + qc.setQueryData(issueKeys.list(WS_ID), makeListCache(issueA, issueB)); + qc.setQueryData(issueKeys.myAll(WS_ID), makeListCache(issueA, issueB)); + + // Pure position move: membership cannot change, so myAll is patched in place. + onIssueUpdated(qc, WS_ID, { ...issueA, position: 20 }); + + const my = qc.getQueryData(issueKeys.myAll(WS_ID)); + expect(my?.byStatus.todo?.issues.map((i) => i.id)).toEqual(["issue-2", "issue-1"]); + // Reconciled in place — no full-list refetch on My Issues (that was the + // remaining drag flicker on filtered boards). + expect(qc.getQueryState(issueKeys.myAll(WS_ID))?.isInvalidated).toBe(false); + }); + + it("invalidates myAll when the assignee changes (membership may shift)", () => { + qc.setQueryData(issueKeys.myAll(WS_ID), makeListCache(issueA)); + + onIssueUpdated( + qc, + WS_ID, + { ...issueA, assignee_type: "member", assignee_id: "user-2" }, + { assigneeChanged: true }, + ); + + expectInvalidated(qc, issueKeys.myAll(WS_ID)); + }); + + it("invalidates myAll when the project changes (Project board membership)", () => { + qc.setQueryData(issueKeys.myAll(WS_ID), makeListCache(issueA)); + + // issueA.project_id is null; moving it into a project shifts Project-board membership. + onIssueUpdated(qc, WS_ID, { ...issueA, project_id: "project-9" }); + + expectInvalidated(qc, issueKeys.myAll(WS_ID)); + }); +}); + describe("onIssueDeleted", () => { let qc: QueryClient; diff --git a/packages/core/issues/ws-updaters.ts b/packages/core/issues/ws-updaters.ts index 6ad090008..42db0fc59 100644 --- a/packages/core/issues/ws-updaters.ts +++ b/packages/core/issues/ws-updaters.ts @@ -40,6 +40,10 @@ export function onIssueUpdated( qc: QueryClient, wsId: string, issue: Partial & { id: string }, + // assigneeChanged comes from the server's issue:updated flags. It gates the + // filtered-list (myAll) invalidate so a non-membership change keeps those + // lists in place instead of refetching. + meta: { assigneeChanged?: boolean } = {}, ) { // Look up the OLD parent before mutating list state, so we can keep // the parent's children cache in sync (powers the sub-issues list @@ -56,13 +60,45 @@ export function onIssueUpdated( const parentChanged = issue.parent_issue_id !== undefined && newParentId !== oldParentId; + // Project-board membership keys on project_id. There is no project_changed + // flag on the wire, so diff the incoming project_id against the cached one. + const oldProjectId = + detailData?.project_id ?? + (firstListData ? findIssueLocation(firstListData, issue.id)?.issue.project_id : null) ?? + null; + const projectChanged = + issue.project_id !== undefined && (issue.project_id ?? null) !== oldProjectId; + for (const [key, data] of listQueries) { if (data) qc.setQueryData(key, patchIssueInBuckets(data, issue.id, issue)); } - if (issue.position !== undefined) { - qc.invalidateQueries({ queryKey: issueKeys.list(wsId) }); + // The workspace board (issueKeys.list) is NOT filtered: an issue is always a + // member, so patchIssueInBuckets above is a complete surgical reconcile — + // cross-status move, same-column reorder, and field updates all land in the + // right bucket/slot. The old `if (position) invalidateQueries(list)` re-pulled + // the entire board on top of that, which is the full-list refetch that made a + // drag (local or echoed back over WS) flicker. It is pure redundancy here. + // + // myAll (My Issues / Project / actor lists) IS filtered. Surgically patch the + // cards that already live in those caches too, so a non-membership change + // (pure status / position / priority / label) reconciles in place — no + // refetch, no flicker — exactly like the workspace board above. + const myListQueries = qc.getQueriesData({ queryKey: issueKeys.myAll(wsId) }); + for (const [key, data] of myListQueries) { + if (data?.byStatus) { + qc.setQueryData(key, patchIssueInBuckets(data, issue.id, issue)); + } + } + // Only refetch the filtered lists when the change can actually move an issue + // in/out of one. My-Issues / actor-panel membership keys on the assignee (the + // "involves" leg — my agents / my squads — is assignee-based too), so the + // server's assignee_changed flag covers it; the Project board keys on + // project_id. A pure status / position / priority / label change cannot change + // membership, so the surgical patch above is the complete reconcile and we + // skip the invalidate that used to make a My-Issues drag refetch + flicker. + if (meta.assigneeChanged || projectChanged) { + qc.invalidateQueries({ queryKey: issueKeys.myAll(wsId) }); } - qc.invalidateQueries({ queryKey: issueKeys.myAll(wsId) }); qc.invalidateQueries({ queryKey: issueKeys.assigneeGroupsAll(wsId) }); qc.invalidateQueries({ queryKey: issueKeys.myAssigneeGroupsAll(wsId) }); if (issue.status !== undefined || issue.project_id !== undefined) { diff --git a/packages/core/realtime/use-realtime-sync.ts b/packages/core/realtime/use-realtime-sync.ts index 4ffef07f7..14378a206 100644 --- a/packages/core/realtime/use-realtime-sync.ts +++ b/packages/core/realtime/use-realtime-sync.ts @@ -572,11 +572,14 @@ export function useRealtimeSync( // Instead, both mutations and WS handlers use dedup checks to be idempotent. const unsubIssueUpdated = ws.on("issue:updated", (p) => { - const { issue } = p as IssueUpdatedPayload; + const payload = p as IssueUpdatedPayload; + const { issue } = payload; if (!issue?.id) return; const wsId = getCurrentWsId(); if (wsId) { - onIssueUpdated(qc, wsId, issue); + onIssueUpdated(qc, wsId, issue, { + assigneeChanged: payload.assignee_changed, + }); if (issue.status) { onInboxIssueStatusChanged(qc, wsId, issue.id, issue.status); } diff --git a/packages/core/types/events.ts b/packages/core/types/events.ts index 8fe1f954b..473b42267 100644 --- a/packages/core/types/events.ts +++ b/packages/core/types/events.ts @@ -94,6 +94,12 @@ export interface IssueCreatedPayload { export interface IssueUpdatedPayload { issue: Issue; + // The server stamps issue:updated with which fields actually changed + // (server/internal/handler/issue.go publish). Only assignee_changed is read + // today: it lets the realtime layer keep filtered myList caches in place on a + // non-membership change instead of refetching. Other change flags are present + // on the wire too and can be surfaced here when needed. + assignee_changed?: boolean; } export interface IssueDeletedPayload { diff --git a/packages/views/issues/components/board-view.tsx b/packages/views/issues/components/board-view.tsx index c9584e931..3748f9676 100644 --- a/packages/views/issues/components/board-view.tsx +++ b/packages/views/issues/components/board-view.tsx @@ -24,6 +24,7 @@ import { BoardCardContent } from "./board-card"; import { HiddenColumnsPanel, HiddenColumnRow } from "./hidden-columns-panel"; import { InfiniteScrollSentinel } from "./infinite-scroll-sentinel"; import type { ChildProgress } from "./list-row"; +import { useDragSettle } from "./use-drag-settle"; import { useT } from "../../i18n"; import { type DragMoveUpdates, @@ -33,6 +34,7 @@ import { buildColumns, computePosition, findColumn, + insertIdByPosition, issueMatchesGroup, getMoveUpdates, } from "../utils/drag-utils"; @@ -213,35 +215,27 @@ export function BoardView({ // --- Drag state --- const [activeIssue, setActiveIssue] = useState(null); - const isDraggingRef = useRef(false); - const isSettlingRef = useRef(false); - const [settleVersion, setSettleVersion] = useState(0); - - // --- Local columns state --- - // Between drags: follows TQ via useEffect. - // During drag: local-only, driven by onDragOver/onDragEnd. - const [columns, setColumns] = useState>(() => - buildColumns(groupedIssues, groups, grouping), - ); - const columnsRef = useRef(columns); - columnsRef.current = columns; + // Shared drag/settle primitive: owns the local column mirror, the + // dragging/settling locks, the post-move animation-frame throttle, and the + // settle callback. Shared with list-view (and swimlane) so the surfaces + // can't drift apart. Local columns follow TQ between drags via the resync + // effect below; during a drag/settle they are frozen by the locks. + const { + columns, + setColumns, + columnsRef, + isDraggingRef, + isSettlingRef, + recentlyMovedRef, + settleVersion, + beginSettle, + } = useDragSettle(() => buildColumns(groupedIssues, groups, grouping)); useEffect(() => { if (!isDraggingRef.current && !isSettlingRef.current) { setColumns(buildColumns(groupedIssues, groups, grouping)); } - }, [groupedIssues, groups, grouping, settleVersion]); - - // After a cross-column move, lock for one animation frame so dnd-kit's - // collision detection can stabilize before processing the next move. - // Without this, collision oscillates: A→B→A→B… until React bails out. - const recentlyMovedRef = useRef(false); - useEffect(() => { - const id = requestAnimationFrame(() => { - recentlyMovedRef.current = false; - }); - return () => cancelAnimationFrame(id); - }, [columns]); + }, [groupedIssues, groups, grouping, settleVersion, setColumns, isDraggingRef, isSettlingRef]); // --- Issue map --- // Frozen during drag so BoardColumn/DraggableBoardCard props stay @@ -269,7 +263,7 @@ export function BoardView({ const issue = issueMapRef.current.get(event.active.id as string) ?? null; setActiveIssue(issue); }, - [], + [isDraggingRef], ); const handleDragOver = useCallback( @@ -296,7 +290,7 @@ export function BoardView({ return { ...prev, [activeCol]: oldIds, [overCol]: newIds }; }); }, - [groupIds, sortBy], + [groupIds, sortBy, recentlyMovedRef, setColumns], ); const handleDragEnd = useCallback( @@ -359,11 +353,24 @@ export function BoardView({ resetColumns(); return; } - isSettlingRef.current = true; - onMoveIssue(activeId, getMoveUpdates(finalGroup, currentIssue.position), () => { - isSettlingRef.current = false; - setSettleVersion((v) => v + 1); + // Optimistically move the card into the target column *now*. Without + // this, the sortBy != "position" path never touches local columns on + // drop, so onDragOver having been a no-op leaves the card in its origin + // column for the whole request — it only jumps across when the mutation + // settles. That is the "snaps back to origin, then moves" glitch. + // Placement mirrors the cache (insertByPosition) so the settle rebuild + // from TanStack Query is a visual no-op. + setColumns((prev) => { + const fromIds = (prev[activeCol] ?? []).filter((cid) => cid !== activeId); + const toIds = insertIdByPosition( + prev[overCol] ?? [], + activeId, + currentIssue.position, + map, + ); + return { ...prev, [activeCol]: fromIds, [overCol]: toIds }; }); + onMoveIssue(activeId, getMoveUpdates(finalGroup, currentIssue.position), beginSettle()); return; } @@ -379,12 +386,14 @@ export function BoardView({ return; } - isSettlingRef.current = true; - onMoveIssue(activeId, getMoveUpdates(finalGroup, newPosition), () => { - isSettlingRef.current = false; - }); + // beginSettle() holds the lock and returns the onSettled callback that + // releases it and resyncs local columns from the cache: a no-op on + // success (onSuccess already patched the moved card in place), the revert + // on error (onError restored the snapshot). Without it a failed move would + // strand the card at the drop target, since onSettled no longer refetches. + onMoveIssue(activeId, getMoveUpdates(finalGroup, newPosition), beginSettle()); }, - [groupedIssues, groups, grouping, onMoveIssue, groupIds, groupMap, sortBy], + [groupedIssues, groups, grouping, onMoveIssue, groupIds, groupMap, sortBy, beginSettle, columnsRef, isDraggingRef, setColumns], ); return ( diff --git a/packages/views/issues/components/list-view.tsx b/packages/views/issues/components/list-view.tsx index 3e9fecb76..1f12ed61b 100644 --- a/packages/views/issues/components/list-view.tsx +++ b/packages/views/issues/components/list-view.tsx @@ -25,6 +25,7 @@ import { useViewStore } from "@multica/core/issues/stores/view-store-context"; import { useIssueSelectionStore } from "@multica/core/issues/stores/selection-store"; import { StatusHeading } from "./status-heading"; import { ListRow, DraggableListRow, type ChildProgress } from "./list-row"; +import { useDragSettle } from "./use-drag-settle"; import { InfiniteScrollSentinel } from "./infinite-scroll-sentinel"; import { useT } from "../../i18n"; import { @@ -34,6 +35,7 @@ import { buildColumns, computePosition, findColumn, + insertIdByPosition, issueMatchesGroup, getMoveUpdates, } from "../utils/drag-utils"; @@ -113,29 +115,24 @@ export function ListView({ // --- Drag state --- const [activeIssue, setActiveIssue] = useState(null); - const isDraggingRef = useRef(false); - const isSettlingRef = useRef(false); - const [settleVersion, setSettleVersion] = useState(0); - - const [columns, setColumns] = useState>(() => - buildColumns(issues, groups, "status"), - ); - const columnsRef = useRef(columns); - columnsRef.current = columns; + // Shared drag/settle primitive (see use-drag-settle) — same machine as + // board-view, so the two surfaces can't drift apart. + const { + columns, + setColumns, + columnsRef, + isDraggingRef, + isSettlingRef, + recentlyMovedRef, + settleVersion, + beginSettle, + } = useDragSettle(() => buildColumns(issues, groups, "status")); useEffect(() => { if (!isDraggingRef.current && !isSettlingRef.current) { setColumns(buildColumns(issues, groups, "status")); } - }, [issues, groups, settleVersion]); - - const recentlyMovedRef = useRef(false); - useEffect(() => { - const id = requestAnimationFrame(() => { - recentlyMovedRef.current = false; - }); - return () => cancelAnimationFrame(id); - }, [columns]); + }, [issues, groups, settleVersion, setColumns, isDraggingRef, isSettlingRef]); const issueMap = useMemo(() => { const map = new Map(); @@ -165,7 +162,7 @@ export function ListView({ const issue = issueMapRef.current.get(event.active.id as string) ?? null; setActiveIssue(issue); }, - [], + [isDraggingRef], ); const handleDragOver = useCallback( @@ -192,7 +189,7 @@ export function ListView({ return { ...prev, [activeCol]: oldIds, [overCol]: newIds }; }); }, - [groupIds, sortBy], + [groupIds, sortBy, recentlyMovedRef, setColumns], ); const handleDragEnd = useCallback( @@ -253,11 +250,23 @@ export function ListView({ resetColumns(); return; } - isSettlingRef.current = true; - onMoveIssue(activeId, getMoveUpdates(finalGroup, currentIssue.position), () => { - isSettlingRef.current = false; - setSettleVersion((v) => v + 1); + // Optimistically move the row into the target group *now*. Without this + // the sortBy != "position" branch never touched local columns on drop, + // so the row sat in its origin group for the whole request and only + // jumped across when the mutation settled — the same "snaps back, then + // moves" glitch the board view had. Placement mirrors the cache + // (insertIdByPosition) so the settle rebuild is a visual no-op. + setColumns((prev) => { + const fromIds = (prev[activeCol] ?? []).filter((cid) => cid !== activeId); + const toIds = insertIdByPosition( + prev[finalCol] ?? [], + activeId, + currentIssue.position, + map, + ); + return { ...prev, [activeCol]: fromIds, [finalCol]: toIds }; }); + onMoveIssue(activeId, getMoveUpdates(finalGroup, currentIssue.position), beginSettle()); return; } @@ -273,12 +282,12 @@ export function ListView({ return; } - isSettlingRef.current = true; - onMoveIssue(activeId, getMoveUpdates(finalGroup, newPosition), () => { - isSettlingRef.current = false; - }); + // beginSettle() also bumps settleVersion on settle (board-view did, this + // branch did not) so a failed position move reverts instead of stranding + // the row at the drop target. + onMoveIssue(activeId, getMoveUpdates(finalGroup, newPosition), beginSettle()); }, - [issues, groups, onMoveIssue, groupIds, groupMap, sortBy], + [issues, groups, onMoveIssue, groupIds, groupMap, sortBy, beginSettle, setColumns, columnsRef, isDraggingRef], ); const content = ( diff --git a/packages/views/issues/components/swimlane-view.test.tsx b/packages/views/issues/components/swimlane-view.test.tsx index 6529bddb3..cbaeef0df 100644 --- a/packages/views/issues/components/swimlane-view.test.tsx +++ b/packages/views/issues/components/swimlane-view.test.tsx @@ -616,11 +616,34 @@ describe("SwimLaneView", () => { }); }); - expect(mockOnMoveIssue).toHaveBeenCalledWith("orphan-1", { - parent_issue_id: null, - status: "in_progress", - position: 300, + expect(mockOnMoveIssue).toHaveBeenCalledWith( + "orphan-1", + { parent_issue_id: null, status: "in_progress", position: 300 }, + expect.any(Function), + ); + }); + + it("passes a settle callback that releases the lock without error", () => { + const mockOnMoveIssue = vi.fn(); + renderWithI18n( + , + ); + + const targetCellId = "swim:parent:none:in_progress"; + act(() => { + lastOnDragOver({ active: { id: "orphan-1" }, over: { id: targetCellId } }); }); + act(() => { + lastOnDragEnd({ active: { id: "orphan-1" }, over: { id: targetCellId } }); + }); + + // The move carries a settle callback (held from drop until the mutation + // settles); invoking it releases the lock and re-syncs from the cache. + const onSettled = mockOnMoveIssue.mock.calls[0]?.[2] as + | (() => void) + | undefined; + expect(typeof onSettled).toBe("function"); + expect(() => act(() => onSettled?.())).not.toThrow(); }); it("does not call onMoveIssue when drop target equals source cell (no-op)", () => { @@ -665,6 +688,7 @@ describe("SwimLaneView", () => { parent_issue_id: "parent-1", status: "todo", }), + expect.any(Function), ); }); @@ -1068,6 +1092,7 @@ describe("SwimLaneView", () => { expect(mockOnMoveIssue).toHaveBeenCalledWith( "issue-c", expect.objectContaining({ project_id: "proj-1", status: "todo" }), + expect.any(Function), ); }); @@ -1090,6 +1115,7 @@ describe("SwimLaneView", () => { expect(mockOnMoveIssue).toHaveBeenCalledWith( "issue-a", expect.objectContaining({ project_id: null, status: "in_review" }), + expect.any(Function), ); }); @@ -1172,6 +1198,7 @@ describe("SwimLaneView", () => { assignee_id: "user-1", status: "in_review", }), + expect.any(Function), ); }); @@ -1198,6 +1225,7 @@ describe("SwimLaneView", () => { assignee_id: null, status: "done", }), + expect.any(Function), ); }); diff --git a/packages/views/issues/components/swimlane-view.tsx b/packages/views/issues/components/swimlane-view.tsx index ddcf51211..1278de3d9 100644 --- a/packages/views/issues/components/swimlane-view.tsx +++ b/packages/views/issues/components/swimlane-view.tsx @@ -467,7 +467,11 @@ export function SwimLaneView({ activeFilters?: Omit; visibleStatuses?: IssueStatus[]; hiddenStatuses?: IssueStatus[]; - onMoveIssue: (issueId: string, updates: SwimLaneMoveUpdates) => void; + onMoveIssue: ( + issueId: string, + updates: SwimLaneMoveUpdates, + onSettled?: () => void, + ) => void; childProgressMap?: Map; myIssuesScope?: string; myIssuesFilter?: MyIssuesFilter; @@ -782,6 +786,12 @@ export function SwimLaneView({ const [activeIssue, setActiveIssue] = useState(null); const isDraggingRef = useRef(false); + // Settle lock: held from drop until the move mutation settles, so a cache + // change that lands mid-flight (e.g. a membership refetch) does not rebuild + // localCells out from under the optimistic move. Mirrors board-view / + // list-view. settleVersion forces the resync once the lock releases. + const isSettlingRef = useRef(false); + const [settleVersion, setSettleVersion] = useState(0); const issueMap = useMemo(() => { const map = new Map(); @@ -790,7 +800,7 @@ export function SwimLaneView({ }, [mergedIssues]); const issueMapRef = useRef(issueMap); - if (!isDraggingRef.current) { + if (!isDraggingRef.current && !isSettlingRef.current) { issueMapRef.current = issueMap; } @@ -799,10 +809,10 @@ export function SwimLaneView({ localCellsRef.current = localCells; useEffect(() => { - if (!isDraggingRef.current) { + if (!isDraggingRef.current && !isSettlingRef.current) { setLocalCells(cells); } - }, [cells]); + }, [cells, settleVersion]); const recentlyMovedRef = useRef(false); useEffect(() => { @@ -1062,11 +1072,19 @@ export function SwimLaneView({ return; } - onMoveIssue(activeId, { - ...targetLane.moveUpdates, - status: finalOverCell.status as IssueStatus, - position: newPosition, - }); + isSettlingRef.current = true; + onMoveIssue( + activeId, + { + ...targetLane.moveUpdates, + status: finalOverCell.status as IssueStatus, + position: newPosition, + }, + () => { + isSettlingRef.current = false; + setSettleVersion((v) => v + 1); + }, + ); }, [cells, cellSet, laneByKey, laneGroups, onMoveIssue, swimlaneGrouping, viewStoreApi], ); diff --git a/packages/views/issues/components/use-drag-settle.ts b/packages/views/issues/components/use-drag-settle.ts new file mode 100644 index 000000000..01141b134 --- /dev/null +++ b/packages/views/issues/components/use-drag-settle.ts @@ -0,0 +1,68 @@ +import { useCallback, useEffect, useRef, useState } from "react"; + +/** + * Shared drag/settle state machine for the issue boards (board-view, list-view). + * + * All three drag surfaces (board, list, swimlane) follow the same contract: + * + * - Local column state mirrors the TanStack Query cache *between* drags. + * - While dragging, or while a drop is *settling* (the move mutation is + * in flight), that mirror is frozen so an optimistic move isn't clobbered + * by a cache change that lands mid-flight. + * - On settle the lock releases and `settleVersion` bumps, forcing one resync + * from the now-reconciled cache. + * + * This hook owns that primitive so the surfaces can't drift apart (list-view + * once silently lost the optimistic-move half of it). The resync `useEffect` + * itself stays in each caller because its dependency list is data-source + * specific (workspace board vs. status-only list), but it reads `settleVersion` + * and the refs from here. + * + * `initialColumns` is only read once (useState initializer); callers drive + * subsequent updates through their own resync effect + `setColumns`. + */ +export function useDragSettle( + initialColumns: () => Record, +) { + const isDraggingRef = useRef(false); + const isSettlingRef = useRef(false); + // Throttles onDragOver: set true after a local move, cleared one frame later. + const recentlyMovedRef = useRef(false); + const [settleVersion, setSettleVersion] = useState(0); + + const [columns, setColumns] = useState>( + initialColumns, + ); + const columnsRef = useRef(columns); + columnsRef.current = columns; + + useEffect(() => { + const id = requestAnimationFrame(() => { + recentlyMovedRef.current = false; + }); + return () => cancelAnimationFrame(id); + }, [columns]); + + /** + * Engage the settle lock and return the `onSettled` callback to hand to the + * move mutation. The callback releases the lock and triggers a single resync. + */ + const beginSettle = useCallback(() => { + isSettlingRef.current = true; + return () => { + isSettlingRef.current = false; + setSettleVersion((v) => v + 1); + }; + }, []); + + return { + columns, + setColumns, + columnsRef, + isDraggingRef, + isSettlingRef, + recentlyMovedRef, + settleVersion, + beginSettle, + }; +} diff --git a/packages/views/issues/utils/drag-utils.test.ts b/packages/views/issues/utils/drag-utils.test.ts new file mode 100644 index 000000000..2898ebe60 --- /dev/null +++ b/packages/views/issues/utils/drag-utils.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from "vitest"; +import type { Issue } from "@multica/core/types"; +import { insertIdByPosition } from "./drag-utils"; + +function mk(id: string, position: number): Issue { + return { + id, + workspace_id: "ws-1", + number: 1, + identifier: `MUL-${id}`, + title: id, + description: null, + status: "todo", + priority: "none", + assignee_type: null, + assignee_id: null, + creator_type: "member", + creator_id: "user-1", + parent_issue_id: null, + project_id: null, + position, + start_date: null, + due_date: null, + metadata: {}, + labels: [], + created_at: "2025-01-01T00:00:00Z", + updated_at: "2025-01-01T00:00:00Z", + }; +} + +function mapOf(...issues: Issue[]): Map { + return new Map(issues.map((i) => [i.id, i])); +} + +describe("insertIdByPosition", () => { + it("inserts the id at its position-sorted slot", () => { + const map = mapOf(mk("a", 1), mk("c", 3), mk("b", 2)); + expect(insertIdByPosition(["a", "c"], "b", 2, map)).toEqual([ + "a", + "b", + "c", + ]); + }); + + it("appends when the position is the largest", () => { + const map = mapOf(mk("a", 1), mk("z", 9)); + expect(insertIdByPosition(["a"], "z", 9, map)).toEqual(["a", "z"]); + }); + + it("prepends when the position is the smallest", () => { + const map = mapOf(mk("b", 2), mk("a", 1)); + expect(insertIdByPosition(["b"], "a", 1, map)).toEqual(["a", "b"]); + }); + + it("appends into an empty target column", () => { + const map = mapOf(mk("a", 5)); + expect(insertIdByPosition([], "a", 5, map)).toEqual(["a"]); + }); + + it("matches insertByPosition ordering so the settle rebuild is a no-op", () => { + // Same scenario the board's optimistic drop and the cache patch both apply: + // landing a card between two neighbours must produce the same order in the + // id list (board) and the issue list (cache). + const map = mapOf(mk("x", 1), mk("y", 3), mk("moved", 2)); + expect(insertIdByPosition(["x", "y"], "moved", 2, map)).toEqual([ + "x", + "moved", + "y", + ]); + }); +}); diff --git a/packages/views/issues/utils/drag-utils.ts b/packages/views/issues/utils/drag-utils.ts index 6130f9028..854a3a410 100644 --- a/packages/views/issues/utils/drag-utils.ts +++ b/packages/views/issues/utils/drag-utils.ts @@ -66,6 +66,27 @@ export function computePosition(ids: string[], activeId: string, issueMap: Map, +): string[] { + const idx = ids.findIndex((existing) => { + const p = issueMap.get(existing)?.position; + return p !== undefined && p > position; + }); + if (idx === -1) return [...ids, id]; + return [...ids.slice(0, idx), id, ...ids.slice(idx)]; +} + export function findColumn( columns: Record, id: string,