From fecffd369c753f837e8271c55fbba7ae10b821ed Mon Sep 17 00:00:00 2001 From: Yuto Nishida Date: Sun, 26 Jul 2026 00:23:45 -0700 Subject: [PATCH] [papyrus] Source canvas position from silverwood, not a client cache A workstream's canvas coordinate is meant to live only in silverwood (papyrus is stateless). But the client treated its in-memory node positions as authoritative: the 1s reconcile loop never re-pulled position for an on-screen node, and saveAllPositions wrote *every* node's position back in bulk on drag-end and beforeunload. So a move in one instance stayed invisible to another even after a refresh, and any instance could clobber another's update with its stale view. Fix: silverwood is now the source of truth for position. Save only the node(s) actually moved, via onNodeDragStop -> PATCH /sessions/:id {position}; the reconcile adopts silverwood's position for existing nodes, shielding a just-dragged node with an echo-clear guard (resolveReconciledPosition, unit-tested) until our own save round-trips. Drop the bulk POST /state/positions, saveAllPositions, and the beforeunload save. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01GBWyw3DyRvvNmgJpFZBsC5 --- experimental/papyrus/client/src/App.tsx | 136 +++++++++--------- .../papyrus/client/src/canvasPosition.test.ts | 51 +++++++ .../papyrus/client/src/canvasPosition.ts | 49 +++++++ experimental/papyrus/server/routes/api.ts | 15 +- 4 files changed, 170 insertions(+), 81 deletions(-) create mode 100644 experimental/papyrus/client/src/canvasPosition.test.ts create mode 100644 experimental/papyrus/client/src/canvasPosition.ts diff --git a/experimental/papyrus/client/src/App.tsx b/experimental/papyrus/client/src/App.tsx index 062b1d8..719994e 100644 --- a/experimental/papyrus/client/src/App.tsx +++ b/experimental/papyrus/client/src/App.tsx @@ -6,13 +6,14 @@ import { useNodesState, BackgroundVariant, ReactFlowProvider, - NodeChange, - applyNodeChanges, + type Node, + type NodeChange, } from "@xyflow/react"; import "@xyflow/react/dist/style.css"; import { Plus } from "lucide-react"; import { useStore, AgentSession } from "./stores/useStore"; +import { resolveReconciledPosition } from "./canvasPosition"; import { AgentNode } from "./components/AgentNode/index"; import { Sidebar } from "./components/Sidebar"; import { NewSessionModal } from "./components/NewSessionModal"; @@ -51,7 +52,12 @@ function AppContent() { }, [resolvedTheme]); const [nodes, setNodes, onNodesChange] = useNodesState(storeNodes); - const positionUpdateTimeout = useRef | null>(null); + // A node just moved locally: keep its optimistic position and shield it from the + // reconcile loop until silverwood echoes back the value we saved (the expiry is only + // a failure-mode safety valve — see the reconcile's echo-clear guard below). + const recentlyMoved = useRef>( + new Map(), + ); const hasRestoredRef = useRef(false); // True while a node is being dragged, so the reconcile loop doesn't touch nodes // (and snap a drag back). Session/tab updates still flow. @@ -87,9 +93,11 @@ function AppContent() { // Reconcile the client's view from silverwood every second. GET /api/state is a // fresh projection (workstreams + per-session tabs + lock), so nodes/tabs/names // appear, disappear, and update as the forest changes — including from other - // processes. The client invents no durable state. An existing node's position is - // never pulled from the server (positions flow client -> server on drag), so this - // never fights a drag; node mutations are also skipped while a drag is in flight. + // processes. The client invents no durable state: silverwood is authoritative for + // position too, so an existing node's position is re-synced from the server (that is + // how a move in another instance shows up here). The only exception is a node we just + // dragged ourselves — `recentlyMoved` keeps its optimistic position until our own save + // echoes back — and all node mutations are skipped while a drag is in flight. useEffect(() => { if (agents.length === 0) return; let cancelled = false; @@ -134,10 +142,26 @@ function AppContent() { if (store.sessions.has(n.nodeId)) { store.updateSession(n.nodeId, sessionData); if (dragging) return; - // Reflect an external rename/recolor on the canvas node label. const node = store.nodes.find((x) => x.id === n.nodeId); - if (node && (node.data?.label !== label || node.data?.color !== color)) { - store.updateNode(n.nodeId, { data: { ...node.data, label, color } }); + if (!node) return; + + const updates: Partial = {}; + // Reflect an external rename/recolor on the canvas node label. + if (node.data?.label !== label || node.data?.color !== color) { + updates.data = { ...node.data, label, color }; + } + // Adopt silverwood's position (source of truth), except for a node we just + // dragged, until our own save echoes back (see resolveReconciledPosition). + const { position, clearGuard } = resolveReconciledPosition( + node.position, + n.position, + recentlyMoved.current.get(n.nodeId), + Date.now(), + ); + if (clearGuard) recentlyMoved.current.delete(n.nodeId); + if (position) updates.position = position; + if (updates.data || updates.position) { + store.updateNode(n.nodeId, updates); } } else if (!dragging) { store.addSession(n.nodeId, sessionData); @@ -174,70 +198,41 @@ function AppContent() { }; }, [agents.length]); - // Helper to save all positions - accepts nodes directly to avoid sync issues - const saveAllPositions = useCallback((nodesToSave?: typeof nodes) => { - const currentNodes = nodesToSave || useStore.getState().nodes; - if (currentNodes.length === 0) return; - - const positions: Record = {}; - const GRID_SIZE = 24; - currentNodes.forEach((node) => { - // Each node is a workstream; its coordinate is saved to the workstream's KV. - if (node.type === "agent") { - positions[node.id] = { - x: Math.round(node.position.x / GRID_SIZE) * GRID_SIZE, - y: Math.round(node.position.y / GRID_SIZE) * GRID_SIZE, - }; + // Persist a finished drag straight to silverwood — only the node(s) that moved, so one + // instance never rewrites another's positions. A selection drag fires once with every + // dragged node in `dragged`. React Flow snaps to the grid, so `position` is already + // grid-aligned; sending it verbatim lets the reconcile's echo-check match exactly. + const onNodeDragStop = useCallback( + (_: React.MouseEvent, _node: Node, dragged: Node[]) => { + const moved = dragged?.length ? dragged : [_node]; + const GRACE = 10_000; + for (const n of moved) { + if (n.type !== "agent") continue; + const position = { x: n.position.x, y: n.position.y }; + recentlyMoved.current.set(n.id, { ...position, expiry: Date.now() + GRACE }); + fetch(`/api/sessions/${n.id}`, { + method: "PATCH", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ position }), + keepalive: true, // survive a tab close that races the drop + }).catch(console.error); } - }); - if (Object.keys(positions).length > 0) { - fetch("/api/state/positions", { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ positions }), - }).catch(console.error); - } - }, [nodes]); - - // Save positions on window close/refresh - useEffect(() => { - const handleBeforeUnload = () => { - saveAllPositions(); - }; - window.addEventListener("beforeunload", handleBeforeUnload); - return () => window.removeEventListener("beforeunload", handleBeforeUnload); - }, [saveAllPositions]); - - // Save positions when nodes are moved or resized - const handleNodesChange = useCallback((changes: NodeChange[]) => { - onNodesChange(changes); - - // Track drag state so the reconcile loop leaves nodes alone mid-drag. - for (const c of changes) { - if (c.type === "position" && "dragging" in c) { - draggingRef.current = c.dragging === true; - } - } - - const positionChanges = changes.filter( - (c) => c.type === "position" && "dragging" in c && c.dragging === false - ); - // Check for dimension changes - resizing property might be true, false, or undefined - const dimensionChanges = changes.filter( - (c) => c.type === "dimensions" && (!("resizing" in c) || c.resizing === false) - ); + }, + [], + ); - if (positionChanges.length > 0 || dimensionChanges.length > 0) { - if (positionUpdateTimeout.current) { - clearTimeout(positionUpdateTimeout.current); + const handleNodesChange = useCallback( + (changes: NodeChange[]) => { + onNodesChange(changes); + // Track drag state so the reconcile loop leaves nodes alone mid-drag. + for (const c of changes) { + if (c.type === "position" && "dragging" in c) { + draggingRef.current = c.dragging === true; + } } - // Compute updated nodes immediately to avoid sync delay issues - const updatedNodes = applyNodeChanges(changes, nodes); - positionUpdateTimeout.current = setTimeout(() => { - saveAllPositions(updatedNodes); - }, 300); - } - }, [onNodesChange, saveAllPositions, nodes]); + }, + [onNodesChange], + ); const onNodeClick = useCallback( (_: React.MouseEvent, node: any) => { @@ -266,6 +261,7 @@ function AppContent() { nodes={nodes} edges={[]} onNodesChange={handleNodesChange} + onNodeDragStop={onNodeDragStop} onNodeClick={onNodeClick} onPaneClick={onPaneClick} nodeTypes={nodeTypes} diff --git a/experimental/papyrus/client/src/canvasPosition.test.ts b/experimental/papyrus/client/src/canvasPosition.test.ts new file mode 100644 index 0000000..8637e7b --- /dev/null +++ b/experimental/papyrus/client/src/canvasPosition.test.ts @@ -0,0 +1,51 @@ +import { test, expect, describe } from "bun:test"; +import { resolveReconciledPosition, type MoveGuard } from "./canvasPosition"; + +const at = (x: number, y: number) => ({ x, y }); +const guard = (x: number, y: number, expiry: number): MoveGuard => ({ x, y, expiry }); + +describe("resolveReconciledPosition", () => { + test("no guard: adopts silverwood when it differs (cross-instance move)", () => { + // Another instance moved the node; we hold no guard, so silverwood wins. + expect(resolveReconciledPosition(at(0, 0), at(24, 48), undefined, 1000)).toEqual({ + position: at(24, 48), + clearGuard: false, + }); + }); + + test("no guard, already in sync: no update, nothing to clear", () => { + expect(resolveReconciledPosition(at(24, 48), at(24, 48), undefined, 1000)).toEqual({ + clearGuard: false, + }); + }); + + test("guard matches server: our own save propagated — clear guard, no snap", () => { + // We dragged to (96,-72), kept it locally, and silverwood now echoes it back. + const r = resolveReconciledPosition(at(96, -72), at(96, -72), guard(96, -72, 9999), 1000); + expect(r).toEqual({ clearGuard: true }); + }); + + test("guard set, server still stale, before expiry: keep optimistic position", () => { + // Our save hasn't landed yet; a stale in-flight read must NOT snap us back. + const r = resolveReconciledPosition(at(96, -72), at(0, 0), guard(96, -72, 9999), 1000); + expect(r).toEqual({ clearGuard: false }); + expect(r.position).toBeUndefined(); + }); + + test("guard set, server changed by another instance, past expiry: trust server", () => { + // Save apparently failed/clobbered; after the safety expiry, silverwood wins. + const r = resolveReconciledPosition(at(96, -72), at(240, 240), guard(96, -72, 500), 1000); + expect(r).toEqual({ position: at(240, 240), clearGuard: true }); + }); + + test("no server position: leave the node alone, keep any guard", () => { + expect(resolveReconciledPosition(at(10, 10), undefined, guard(96, -72, 9999), 1000)).toEqual({ + clearGuard: false, + }); + }); + + test("guard past expiry but server already matches local: clear guard, no update", () => { + const r = resolveReconciledPosition(at(96, -72), at(96, -72), guard(96, -72, 500), 1000); + expect(r).toEqual({ clearGuard: true }); + }); +}); diff --git a/experimental/papyrus/client/src/canvasPosition.ts b/experimental/papyrus/client/src/canvasPosition.ts new file mode 100644 index 0000000..7c9663c --- /dev/null +++ b/experimental/papyrus/client/src/canvasPosition.ts @@ -0,0 +1,49 @@ +// How the reconcile loop resolves a canvas node's position against silverwood, which is +// the source of truth. The one exception is a node we just dragged locally: we keep its +// optimistic position (recorded as a move-guard) until our own save echoes back, so the +// poll never snaps a fresh drag back to the pre-drag value. + +export interface XY { + x: number; + y: number; +} + +// An optimistic-position guard for a locally-dragged node: the coordinate we saved, plus +// a safety expiry (a timestamp past which we stop waiting for the echo and trust silverwood). +export interface MoveGuard extends XY { + expiry: number; +} + +// Decide, for a node already on the canvas, whether to adopt silverwood's `server` position +// and whether to drop its move-guard. `local` is the node's current on-screen position. +// - No guard: silverwood is authoritative — adopt it if it differs (this is how a move in +// another instance shows up here). +// - Guard matches `server`: our own save propagated — clear the guard (server == local, so +// nothing to adopt). +// - Guard set, `server` still stale, before expiry: keep the optimistic position. +// - Guard set, past expiry: the save failed or was clobbered — clear the guard and adopt. +export function resolveReconciledPosition( + local: XY, + server: XY | undefined, + guard: MoveGuard | undefined, + now: number, +): { position?: XY; clearGuard: boolean } { + if (!server || typeof server.x !== "number" || typeof server.y !== "number") { + return { clearGuard: false }; + } + let guarded = false; + let clearGuard = false; + if (guard) { + if (guard.x === server.x && guard.y === server.y) { + clearGuard = true; // our write propagated + } else if (now > guard.expiry) { + clearGuard = true; // save failed/clobbered → trust server + } else { + guarded = true; + } + } + if (!guarded && (local.x !== server.x || local.y !== server.y)) { + return { position: server, clearGuard }; + } + return { clearGuard }; +} diff --git a/experimental/papyrus/server/routes/api.ts b/experimental/papyrus/server/routes/api.ts index 0fc881b..a0e8c81 100644 --- a/experimental/papyrus/server/routes/api.ts +++ b/experimental/papyrus/server/routes/api.ts @@ -383,17 +383,9 @@ apiRoutes.patch("/sessions/:wsId/sessions/:sessionId", async (c) => { return c.json({ success: true }); }); -// Persist canvas positions into each workstream's papyrus KV (serialized writes). -apiRoutes.post("/state/positions", async (c) => { - const { positions } = await c.req.json(); - await Promise.all( - Object.entries(positions || {}).map(([id, pos]) => sw.setKv(id, "position", pos)), - ); - return c.json({ success: true }); -}); - -// Edit a node's label/color/notes. Label is the workstream name (silverwood -// rename); color + notes are papyrus KV. +// Edit a node's label/color/notes/position. Label is the workstream name (silverwood +// rename); color, notes, and canvas position are papyrus KV. Position is per-node — +// the client saves only the workstream it moved, so one instance never rewrites another's. apiRoutes.patch("/sessions/:sessionId", async (c) => { const id = c.req.param("sessionId"); const u = await c.req.json(); @@ -401,6 +393,7 @@ apiRoutes.patch("/sessions/:sessionId", async (c) => { if (u.customName !== undefined) await sw.rename(id, u.customName); if (u.customColor !== undefined) await sw.setKv(id, "color", u.customColor); if (u.notes !== undefined) await sw.setKv(id, "notes", u.notes); + if (u.position !== undefined) await sw.setKv(id, "position", u.position); } catch (e: any) { return c.json({ error: e.message }, 400); } -- 2.54.0