fix(canvas): tighten rescue + cap toast + cover paths with tests
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) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang
parent
c2b2e13abe
commit
4fd7f1e84c
@ -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 });
|
||||
});
|
||||
});
|
||||
|
||||
@ -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<typeof vi.fn>;
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
@ -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);
|
||||
|
||||
@ -340,7 +340,13 @@ export const useCanvasStore = create<CanvasState>((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<string>();
|
||||
while (cursor && !seen.has(cursor)) {
|
||||
seen.add(cursor);
|
||||
if (selectedSet.has(cursor)) {
|
||||
hasSelectedAncestor = true;
|
||||
break;
|
||||
@ -511,13 +517,21 @@ export const useCanvasStore = create<CanvasState>((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",
|
||||
);
|
||||
}
|
||||
|
||||
@ -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) {
|
||||
|
||||
Loading…
Reference in New Issue
Block a user