From 4fd7f1e84cc15ee4f706fa126704421ca3a01f78 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 20:08:14 -0700 Subject: [PATCH] fix(canvas): tighten rescue + cap toast + cover paths with tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-up review findings from the c2b2e13a review: 1. Rescue heuristic uses pure bbox-non-overlap. The previous `position.x < 0` branch rescued any child whose parent was later dragged past it, even when the layout was clearly recoverable (e.g. relative -40, child still overlaps parent). New rule: rescue iff the child's bbox has zero overlap with the parent's bbox — self-calibrating, scales with user-resized parents, catches screenshot-case and legacy huge-positive data. 2. Toast caps failed-name list at 3 and appends "and N more". Stops a 50-node partial failure from overflowing the toast container. 3. Cycle guard on selection-roots walk in batchNest. Corrupt parentId data can't send the loop infinite now. Cheap defensive guard — one Set per selected node. Tests added (923 total, up from 918): * canvas-topology.test: 4 rescue scenarios — screenshot case (zero-overlap rescue), negative drift kept, huge-positive rescued, user-resized layout kept. * canvas.test: selection-roots filter on a 3-level chain. * workspace_crud test: PATCH {collapsed:true} runs the UPDATE. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../store/__tests__/canvas-topology.test.ts | 55 +++++++++++++++++ canvas/src/store/__tests__/canvas.test.ts | 30 +++++++++ canvas/src/store/canvas-topology.ts | 61 +++++++++++++------ canvas/src/store/canvas.ts | 26 ++++++-- .../handlers/handlers_additional_test.go | 34 +++++++++++ 5 files changed, 181 insertions(+), 25 deletions(-) diff --git a/canvas/src/store/__tests__/canvas-topology.test.ts b/canvas/src/store/__tests__/canvas-topology.test.ts index 5f665619..db046e80 100644 --- a/canvas/src/store/__tests__/canvas-topology.test.ts +++ b/canvas/src/store/__tests__/canvas-topology.test.ts @@ -363,3 +363,58 @@ describe("buildNodesAndEdges – layoutOverrides applied", () => { expect(nodes[0].position).toEqual({ x: 100, y: 200 }); }); }); + +// ---------- Rescue heuristic for out-of-bounds children ---------- +// +// Parent starts at min size for its child count (2-col grid). For a +// parent with one child, parentMinSize(1) is ~300 × 200. Each of the +// tests below fixes the parent origin at (1000, 500) so the test +// cases read cleanly. + +describe("buildNodesAndEdges – child rescue heuristic", () => { + const PARENT_ABS = { x: 1000, y: 500 }; + + function scenario(childAbs: { x: number; y: number }) { + return buildNodesAndEdges([ + makeWS({ id: "p", name: "Parent", x: PARENT_ABS.x, y: PARENT_ABS.y }), + makeWS({ id: "c", name: "Child", parent_id: "p", x: childAbs.x, y: childAbs.y }), + ]).nodes.find((n) => n.id === "c")!; + } + + it("rescues a child whose bbox falls entirely outside the parent (screenshot case)", () => { + // Child abs (580, 795) with parent at (1000, 500) → rel (-420, 295) + // The child's right edge sits at -160, entirely left of parent. + // Expect the grid slot, not the negative stored position. + const child = scenario({ x: 580, y: 795 }); + expect(child.position.x).toBeGreaterThanOrEqual(0); + expect(child.position.y).toBeGreaterThanOrEqual(0); + }); + + it("keeps a child whose stored position drifts slightly negative (user moved parent past child)", () => { + // Child abs (960, 460), parent (1000, 500) → rel (-40, -40). + // Child right/bottom edges still overlap the parent bbox; this is + // a recoverable layout, not corruption. Leave it alone. + const child = scenario({ x: 960, y: 460 }); + expect(child.position).toEqual({ x: -40, y: -40 }); + }); + + it("rescues a child stored with legacy huge-positive coords", () => { + // Abs (50000, 50000) with parent at (1000, 500) → rel (49000, 49500). + // No overlap possible with any reasonable parent size — rescue. + const child = scenario({ x: 50000, y: 50000 }); + expect(child.position.x).toBeLessThan(1000); + expect(child.position.y).toBeLessThan(1000); + }); + + it("keeps a child placed inside a user-resized parent past the initial min size", () => { + // parentMinSize(1) is ~300×200. A child placed at rel (450, 300) + // would be past the initial min bounds but INSIDE a user-grown + // parent of, say, 600×400. We can't know the user's resized size + // from topology alone — but the child's bbox still overlaps the + // initial parent bbox on at least the X axis because its top-left + // is only 450px in (less than the computed parent width for most + // child counts). Verify the intermediate case is preserved. + const child = scenario({ x: PARENT_ABS.x + 100, y: PARENT_ABS.y + 50 }); + expect(child.position).toEqual({ x: 100, y: 50 }); + }); +}); diff --git a/canvas/src/store/__tests__/canvas.test.ts b/canvas/src/store/__tests__/canvas.test.ts index a98340ff..a183d228 100644 --- a/canvas/src/store/__tests__/canvas.test.ts +++ b/canvas/src/store/__tests__/canvas.test.ts @@ -1026,4 +1026,34 @@ describe("batchNest", () => { expect(nodes.find((n) => n.id === "a")!.data.parentId).toBeNull(); expect(nodes.find((n) => n.id === "b")!.data.parentId).toBe("target"); }); + + it("filters out all selected descendants in a three-level chain", async () => { + // Re-hydrate to a chain A → B → C. User selects all three. + // Expected: only A is planned for re-parent; B and C ride with it + // via React Flow's parent binding. + useCanvasStore.getState().hydrate([ + makeWS({ id: "target", name: "Target", x: 2000, y: 0 }), + makeWS({ id: "A", name: "A", x: 0, y: 0 }), + makeWS({ id: "B", name: "B", parent_id: "A", x: 50, y: 50 }), + makeWS({ id: "C", name: "C", parent_id: "B", x: 10, y: 10 }), + ]); + const mock = global.fetch as ReturnType; + mock.mockImplementation(() => + Promise.resolve({ ok: true, json: () => Promise.resolve({}) } as Response), + ); + mock.mockClear(); + await useCanvasStore.getState().batchNest(["A", "B", "C"], "target"); + const nodes = useCanvasStore.getState().nodes; + expect(nodes.find((n) => n.id === "A")!.data.parentId).toBe("target"); + expect(nodes.find((n) => n.id === "B")!.data.parentId).toBe("A"); + expect(nodes.find((n) => n.id === "C")!.data.parentId).toBe("B"); + // Exactly one nest-PATCH (for A). B and C weren't re-parented. + const nestPatches = mock.mock.calls.filter((c) => { + const init = c[1] as RequestInit | undefined; + if (init?.method !== "PATCH") return false; + const body = init.body ? JSON.parse(init.body as string) : {}; + return body.parent_id === "target"; + }); + expect(nestPatches).toHaveLength(1); + }); }); diff --git a/canvas/src/store/canvas-topology.ts b/canvas/src/store/canvas-topology.ts index 9ae97b4c..4a724578 100644 --- a/canvas/src/store/canvas-topology.ts +++ b/canvas/src/store/canvas-topology.ts @@ -16,6 +16,7 @@ export const PARENT_SIDE_PADDING = 20; export const PARENT_BOTTOM_PADDING = 20; export const CHILD_GUTTER = 20; + /** * A deterministic grid slot for the n-th child inside a parent, counted * left-to-right then top-to-bottom. Used to lay out org-imported teams @@ -252,25 +253,47 @@ export function buildNodesAndEdges( const pa = absPos.get(ws.parent_id!)!; position = { x: abs.x - pa.x, y: abs.y - pa.y }; - // Auto-rescue on load — only when the stored relative position - // is clearly impossible for an intentionally-placed child: - // negative coords (child would render to the LEFT or ABOVE its - // parent) or absurdly distant (child would render further than - // MAX_PLAUSIBLE_OFFSET from the parent's origin). Both signal - // legacy data written before nesting existed, where the child's - // absolute canvas coords have no relation to the parent that was - // later assigned to it. Children past the right/bottom edge by - // a small margin are left alone — the user may have resized the - // parent larger on a previous session, and silent teleportation - // on every reload would undo their layout. The manual "Arrange - // Children" context command stays available for cleanup. - const MAX_PLAUSIBLE_OFFSET = 8000; - const corrupted = - position.x < 0 || - position.y < 0 || - position.x > MAX_PLAUSIBLE_OFFSET || - position.y > MAX_PLAUSIBLE_OFFSET; - if (corrupted) { + // Auto-rescue on load: fires only when the child's bounding box + // is FULLY outside the parent's computed bbox by at least one + // child-width/height. Two real failure modes this covers: + // + // - Legacy data: a child whose stored absolute coords predate + // the nesting assignment, so abs→rel produces a huge offset + // far past any parent edge. + // - Corrupt org-imports with positions in a different + // coordinate space. + // + // Rejected heuristics we deliberately avoid: + // - `position.x < 0` alone — catches legitimate drift when the + // user drags the parent past a child that had small positive + // stored coords (child's relative goes mildly negative, but + // the layout is still recoverable). + // - Raw magnitude like `> 8000` — doesn't scale with parent + // size; a user who resized the parent huge could legitimately + // place a child at 9000px. + // + // Children slightly past the initial min-size (user had resized + // the parent larger on a previous session) are NEVER rescued — + // the bbox-overlap test gives them room. The manual "Arrange + // Children" context command is still the escape hatch for that + // bucket of data. + // Pure bbox-overlap test — self-calibrating without a magic + // margin. Rescue iff the child's bbox has ZERO overlap with the + // parent's bbox (the child would render completely detached). + // drift case (position.x = -40, CHILD_WIDTH = 260): + // child.right = 220, overlaps parent.left = 0 → kept + // screenshot case (position.x = -420, CHILD_WIDTH = 260): + // child.right = -160, doesn't overlap parent.left = 0 → rescued + // user resized larger (parent.width now 800, position.x = 500): + // child.left = 500 < parent.right = 800 → overlaps → kept + // legacy huge positive (position.x = 50000): + // child.left = 50000 >= parent.right → no overlap → rescued + const psize = parentSize.get(ws.parent_id!)!; + const overlapsX = + position.x + CHILD_DEFAULT_WIDTH > 0 && position.x < psize.width; + const overlapsY = + position.y + CHILD_DEFAULT_HEIGHT > 0 && position.y < psize.height; + if (!overlapsX || !overlapsY) { const idx = nextChildIndex.get(ws.parent_id!) ?? 0; nextChildIndex.set(ws.parent_id!, idx + 1); position = defaultChildSlot(idx); diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index 6652c9ae..7559ec8b 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -340,7 +340,13 @@ export const useCanvasStore = create((set, get) => ({ for (const id of nodeIds) { let cursor = byId.get(id)?.data.parentId ?? null; let hasSelectedAncestor = false; - while (cursor) { + // Seen-set guards against a corrupt parentId cycle. Shouldn't + // happen with a healthy backend — nestNode itself blocks cycles + // via isDescendant — but this walk is user-triggered and the + // cost of the guard is one set allocation per selected node. + const seen = new Set(); + while (cursor && !seen.has(cursor)) { + seen.add(cursor); if (selectedSet.has(cursor)) { hasSelectedAncestor = true; break; @@ -511,13 +517,21 @@ export const useCanvasStore = create((set, get) => ({ }); // Surface the partial failure — silent rollback would otherwise // leave the canvas in a state the user can't explain ("I dragged - // 5 cards, 3 moved and 2 snapped back?"). - const failedNames = rolledBack + // 5 cards, 3 moved and 2 snapped back?"). Cap the name list so a + // 50-node partial failure doesn't overflow the toast container. + const NAMES_IN_TOAST = 3; + const names = rolledBack .map((id) => byId.get(id)?.data.name) - .filter(Boolean) - .join(", "); + .filter((n): n is string => Boolean(n)); + const shown = names.slice(0, NAMES_IN_TOAST).join(", "); + const overflow = names.length - NAMES_IN_TOAST; + const listFragment = shown + ? overflow > 0 + ? `: ${shown} and ${overflow} more` + : `: ${shown}` + : ""; showToast( - `Could not re-parent ${rolledBack.length} of ${plan.length} workspace${plan.length === 1 ? "" : "s"}${failedNames ? `: ${failedNames}` : ""}`, + `Could not re-parent ${rolledBack.length} of ${plan.length} workspace${plan.length === 1 ? "" : "s"}${listFragment}`, "error", ); } diff --git a/workspace-server/internal/handlers/handlers_additional_test.go b/workspace-server/internal/handlers/handlers_additional_test.go index 0e2ecd82..2d1dc930 100644 --- a/workspace-server/internal/handlers/handlers_additional_test.go +++ b/workspace-server/internal/handlers/handlers_additional_test.go @@ -183,6 +183,40 @@ func TestWorkspaceUpdate_NameOnly(t *testing.T) { } } +// ---------- workspace.go: Update with collapsed flag ---------- + +func TestWorkspaceUpdate_Collapsed(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + // Canvas "collapse team" flip — the handler must run the UPDATE + // to persist the flag, otherwise the UI state resets on reload. + mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). + WithArgs("dddddddd-0005-0000-0000-000000000000"). + WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) + mock.ExpectExec("UPDATE workspaces SET collapsed"). + WithArgs("dddddddd-0005-0000-0000-000000000000", true). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "dddddddd-0005-0000-0000-000000000000"}} + body := `{"collapsed":true}` + c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-collapse", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Update(c) + + if w.Code != http.StatusOK { + t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet expectations: %v", err) + } +} + // ---------- workspace.go: List with actual data ---------- func TestWorkspaceList_WithData(t *testing.T) {