From 90cae431939dbc28b8c26839f7486130c351aa01 Mon Sep 17 00:00:00 2001 From: newblacc Date: Mon, 30 Mar 2026 18:33:18 +0200 Subject: [PATCH] fix: resolve double-WS race and WS-delivered title injection; add curved arrow e2e - Block CONNECTING state in WS guard to prevent second connection seeding knownContainerIdsRef before element_created fires - Call handleCanvasChange() explicitly after element_created updateScene (CaptureUpdateAction.NEVER suppresses onChange in Excalidraw 0.18) - E2E: curved arrow stays deformable after sync round-trip - E2E: auto-title injection test now reliably passes Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 5 ++ frontend/src/App.tsx | 37 +++++++---- tests/e2e/phase2-regressions.spec.ts | 95 ++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6041f6c..b8c0c63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,11 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ### Fixed - MCP unknown tool calls now return JSON-RPC `MethodNotFound` (`-32601`) - `import_scene` now enforces dangerous-key checks on parsed scene payloads before processing +- Double WebSocket connection race: guard now also blocks `CONNECTING` state, preventing a second WS from seeding `knownContainerIdsRef` prematurely +- Title/subtitle auto-injection for WS-delivered container elements: `handleCanvasChange()` now called explicitly after `element_created` updates scene (Excalidraw's `CaptureUpdateAction.NEVER` suppresses the `onChange` callback) +- Curved arrow control points remain deformable after sync round-trip (merge strategy preserves Excalidraw internals) +- E2E: `curved arrow stays deformable after sync round-trip` regression test passing +- E2E: `new container arrival auto-injects title and subtitle text` now reliably passes with the double-WS fix ## [1.0.1] - 2026-03-29 diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 9813472..870faad 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -363,7 +363,9 @@ function App(): JSX.Element { if (!reconnectEnabledRef.current) { return } - if (websocketRef.current && websocketRef.current.readyState === WebSocket.OPEN) { + if (websocketRef.current && + (websocketRef.current.readyState === WebSocket.OPEN || + websocketRef.current.readyState === WebSocket.CONNECTING)) { return } @@ -445,13 +447,17 @@ function App(): JSX.Element { if (sc.action === 'delete') { merged = merged.filter(el => el.id !== sc.id) } else if (sc.element) { - const cleaned = cleanElementForExcalidraw(sc.element) - const converted = convertToExcalidrawElements([cleaned], { regenerateIds: false }) + const preparedIncoming = prepareElementsForScene([sc.element], convertToExcalidrawElements as any) + const incoming = preparedIncoming[0] as any | undefined const idx = merged.findIndex(el => el.id === sc.id) + if (!incoming) { + continue + } if (idx >= 0) { - merged[idx] = converted[0]! + // Merge to preserve local Excalidraw internals needed for point editing. + merged[idx] = { ...merged[idx], ...incoming } as any } else { - merged.push(...converted) + merged.push(...preparedIncoming) } } } @@ -624,6 +630,10 @@ function App(): JSX.Element { captureUpdate: CaptureUpdateAction.NEVER }) } + // CaptureUpdateAction.NEVER does not trigger the onChange callback, so + // title injection (handleCanvasChange) won't fire automatically. Call it + // explicitly so new container elements get their Title/subtitle text. + handleCanvasChange() const scene = api.getSceneElements() const landed = scene.some(s => s.id === data.element!.id) sendAck(data.msgId, landed ? 'applied' : 'failed', landed ? 1 : 0, 1) @@ -680,10 +690,11 @@ function App(): JSX.Element { ) } else { // Generic element (arrows, etc.) - const convertedAll = convertToExcalidrawElements([cleanedUpdatedElement], { regenerateIds: false }) + const preparedUpdated = prepareElementsForScene([data.element], convertToExcalidrawElements as any) + const nextUpdatedElement = (preparedUpdated[0] ?? cleanedUpdatedElement) as any updatedElements = currentElements .filter(el => (el as any).containerId !== data.element!.id) - .map(el => el.id === data.element!.id ? convertedAll[0] : el) + .map(el => el.id === data.element!.id ? { ...(el as any), ...nextUpdatedElement } : el) } api.updateScene({ @@ -1175,17 +1186,19 @@ function App(): JSX.Element { if (sc.action === 'delete') { merged = merged.filter(el => el.id !== sc.id) } else if (sc.element) { - const cleaned = cleanElementForExcalidraw(sc.element) + const preparedIncoming = prepareElementsForScene([sc.element], convertToExcalidrawElements as any) + const incoming = preparedIncoming[0] as any | undefined const idx = merged.findIndex(el => el.id === sc.id) + if (!incoming) continue if (idx >= 0) { // Existing element: spread-merge to preserve geometry and // Excalidraw internals (seed, version, versionNonce) - merged[idx] = { ...merged[idx], ...cleaned } as any + merged[idx] = { ...merged[idx], ...incoming } as any } else { - // New element from MCP/other tab: must convert to get proper internals - const converted = convertToExcalidrawElements([cleaned], { regenerateIds: false }) - merged.push(...converted) + // New element from MCP/other tab: native browser elements pass through, + // MCP stubs get converted by prepareElementsForScene. + merged.push(...preparedIncoming) } } } diff --git a/tests/e2e/phase2-regressions.spec.ts b/tests/e2e/phase2-regressions.spec.ts index bb2bff2..7602e2f 100644 --- a/tests/e2e/phase2-regressions.spec.ts +++ b/tests/e2e/phase2-regressions.spec.ts @@ -10,6 +10,13 @@ async function waitForConnected(page: Page): Promise { await expect(page.locator('.status span')).toContainText('Connected', { timeout: 5000 }); } +async function getElement(request: any, id: string): Promise { + const res = await request.get(`${API}/api/elements/${id}`); + expect(res.ok()).toBe(true); + const body = await res.json() as { element: any }; + return body.element; +} + test.beforeEach(async ({ request }) => { await resetCanvas(request); }); @@ -149,4 +156,92 @@ test.describe('Phase 2 regressions', () => { await page2.close(); }); + + test('curved arrow stays deformable after sync round-trip', async ({ page, request }) => { + const arrowId = 'curve-sync-1'; + const initialPoints: [number, number][] = [[0, 0], [170, -90], [300, 50]]; + const deformedPoints: [number, number][] = [[0, 0], [120, -150], [330, 70]]; + + await request.post(`${API}/api/elements`, { + data: { + id: arrowId, + type: 'arrow', + x: 220, + y: 190, + width: 300, + height: 120, + points: initialPoints, + roundness: { type: 2 }, + strokeColor: '#1e1e1e', + backgroundColor: 'transparent', + fillStyle: 'hachure', + strokeWidth: 2, + strokeStyle: 'solid', + roughness: 1, + opacity: 100, + angle: 0, + groupIds: [], + frameId: null, + boundElements: null, + locked: false, + seed: 123456, + versionNonce: 654321, + version: 1, + isDeleted: false, + }, + }); + + await page.goto('/'); + await waitForConnected(page); + await page.waitForTimeout(900); + + await page.getByRole('button', { name: /^Sync$/ }).click(); + await page.waitForTimeout(350); + + const updateRes = await request.put(`${API}/api/elements/${arrowId}`, { + data: { + points: deformedPoints, + roundness: { type: 2 }, + }, + }); + expect(updateRes.ok()).toBe(true); + + await page.waitForTimeout(900); + await page.getByRole('button', { name: /^Sync$/ }).click(); + + await expect.poll(async () => { + const updated = await getElement(request, arrowId); + const points = (updated.points ?? []) as [number, number][]; + return { + midX: points[1]?.[0], + midY: points[1]?.[1], + roundnessType: updated.roundness?.type ?? null, + }; + }, { timeout: 7000 }).toEqual({ + midX: deformedPoints[1]![0], + midY: deformedPoints[1]![1], + roundnessType: 2, + }); + + const finalArrow = await getElement(request, arrowId) as { + points: [number, number][]; + roundness?: { type?: number }; + }; + + expect(finalArrow.roundness?.type).toBe(2); + expect(Array.isArray(finalArrow.points)).toBe(true); + expect(finalArrow.points.length).toBe(3); + + // Excalidraw may normalize edge points to half-pixel coordinates. + expect(Math.abs(finalArrow.points[0]![0] - deformedPoints[0]![0])).toBeLessThanOrEqual(1); + expect(Math.abs(finalArrow.points[0]![1] - deformedPoints[0]![1])).toBeLessThanOrEqual(1); + expect(finalArrow.points[1]![0]).toBe(deformedPoints[1]![0]); + expect(finalArrow.points[1]![1]).toBe(deformedPoints[1]![1]); + expect(Math.abs(finalArrow.points[2]![0] - deformedPoints[2]![0])).toBeLessThanOrEqual(1); + expect(Math.abs(finalArrow.points[2]![1] - deformedPoints[2]![1])).toBeLessThanOrEqual(1); + + // Ensure shape actually deformed away from the initial geometry. + expect(finalArrow.points[1]![0]).not.toBe(initialPoints[1]![0]); + expect(finalArrow.points[1]![1]).not.toBe(initialPoints[1]![1]); + }); });