diff --git a/e2e/_verify_collab.spec.ts b/e2e/_verify_collab.spec.ts new file mode 100644 index 0000000..23e8816 --- /dev/null +++ b/e2e/_verify_collab.spec.ts @@ -0,0 +1,116 @@ +import { test, expect, type Page } from '@playwright/test' + +// Manual verification for the collab duplication + probe fixes. Underscore- +// prefixed so it stays OUT of the default CI suite (it needs the real +// collab.noteser.app worker reachable + NEXT_PUBLIC_YJS_WS_URL set on the dev +// server). Run explicitly (rename off the `_` prefix or point the runner at +// it directly): +// NEXT_PUBLIC_YJS_WS_URL=wss://collab.noteser.app/ npm run dev (separate shell) +// npx playwright test e2e/.spec.ts + +const SENTINEL = 'ZQX-UNIQUE-BODY-7731' +const BODY = `# Collab heading\nfirst body line ${SENTINEL}\nsecond body line` + +// Shape of the testHooks bridge this spec touches. Cast inline (rather than +// augmenting the global Window) so it never collides with the canonical +// declaration in src/utils/testHooks.ts under tsc. +type Hooks = { + stores: { + noteStore: { getState(): { + addNote: (i: { title: string; content: string }) => { id: string } + notes: Array<{ id: string; collabId?: string; content: string }> + } } + workspaceStore: { getState(): { openNote: (id: string, o: { preview: boolean }) => void } } + } +} +// NOTE: page.evaluate callbacks run in the BROWSER, so they cast `window` +// inline rather than closing over any Node-side helper. + +async function waitForHooks(page: Page) { + await page.waitForFunction( + () => typeof (window as unknown as { __noteser_test?: unknown }).__noteser_test !== 'undefined', + undefined, { timeout: 20_000 }, + ) +} + +function trackErrors(page: Page): string[] { + const errs: string[] = [] + page.on('console', m => { if (m.type() === 'error') errs.push(m.text()) }) + page.on('pageerror', e => errs.push(String(e))) + return errs +} + +const countOccurrences = (haystack: string, needle: string) => haystack.split(needle).length - 1 + +test('collab: pill connected, body once, live A→B sync, single saved content', async ({ browser }) => { + // ── Context A — seeder ──────────────────────────────────────────────────── + const ctxA = await browser.newContext() + const pageA = await ctxA.newPage() + const errA = trackErrors(pageA) + await pageA.goto('/') + await waitForHooks(pageA) + + const noteId = await pageA.evaluate((content) => { + const h = (window as unknown as { __noteser_test: Hooks }).__noteser_test + const n = h.stores.noteStore.getState().addNote({ title: 'Collab Note', content }) + h.stores.workspaceStore.getState().openNote(n.id, { preview: false }) + return n.id + }, BODY) + + // Editor must show the body (seeded into the fresh room over the wire) and + // exactly once — the doubling bug rendered it twice. + await expect(pageA.locator('.cm-content').first()).toContainText(SENTINEL, { timeout: 20_000 }) + const aText = await pageA.locator('.cm-content').first().innerText() + expect(countOccurrences(aText, SENTINEL)).toBe(1) + + // Pill must report connected ("Live: on") — the probe now dials //. + await expect(pageA.getByTestId('status-bar-collab')).toHaveText(/Live: on/, { timeout: 20_000 }) + + // The note's stable collabId (room) is minted when the binding attaches. + await pageA.waitForFunction( + (id) => !!(window as unknown as { __noteser_test: Hooks }).__noteser_test + .stores.noteStore.getState().notes.find(n => n.id === id)?.collabId, + noteId, { timeout: 20_000 }, + ) + const collabId = await pageA.evaluate( + (id) => (window as unknown as { __noteser_test: Hooks }).__noteser_test + .stores.noteStore.getState().notes.find(n => n.id === id)!.collabId!, + noteId, + ) + + // Saved content must be the SINGLE body, never doubled. + const storedA = await pageA.evaluate( + (id) => (window as unknown as { __noteser_test: Hooks }).__noteser_test + .stores.noteStore.getState().notes.find(n => n.id === id)!.content, + noteId, + ) + expect(countOccurrences(storedA, SENTINEL)).toBe(1) + + // ── Context B — joiner via share link ───────────────────────────────────── + const ctxB = await browser.newContext() + const pageB = await ctxB.newPage() + const errB = trackErrors(pageB) + await pageB.goto(`/?collab=${collabId}&title=Collab%20Note`) + await waitForHooks(pageB) + + // B materialises an EMPTY local note bound to the room and pulls A's content + // over the wire — exactly once. + await expect(pageB.locator('.cm-content').first()).toContainText(SENTINEL, { timeout: 20_000 }) + const bText = await pageB.locator('.cm-content').first().innerText() + expect(countOccurrences(bText, SENTINEL)).toBe(1) + await expect(pageB.getByTestId('status-bar-collab')).toHaveText(/Live: on/, { timeout: 20_000 }) + + // ── Live sync A → B ─────────────────────────────────────────────────────── + const liveToken = 'LIVE-EDIT-FROM-A-4477' + await pageA.locator('.cm-content').first().click() + await pageA.keyboard.press('Control+End') + await pageA.keyboard.type(`\n${liveToken}`) + await expect(pageB.locator('.cm-content').first()).toContainText(liveToken, { timeout: 20_000 }) + + // ── No CSP / WebSocket errors on either client ──────────────────────────── + const offending = [...errA, ...errB].filter(e => /content security policy|csp|websocket|wss:/i.test(e)) + expect(offending, `offending console errors:\n${offending.join('\n')}`).toEqual([]) + + await ctxA.close() + await ctxB.close() +}) diff --git a/src/__tests__/collabNoDoubling.test.tsx b/src/__tests__/collabNoDoubling.test.tsx new file mode 100644 index 0000000..ca9d4d8 --- /dev/null +++ b/src/__tests__/collabNoDoubling.test.tsx @@ -0,0 +1,103 @@ +/** + * @jest-environment jsdom + * + * Regression test for the live-collaboration content-DOUBLING bug. + * + * We mount a REAL CodeMirror EditorView with the REAL yCollab binding (only + * the websocket transport is faked — the provider's awareness is a real + * y-protocols Awareness over the binding's own Y.Doc, and we drive the + * provider's 'sync' event by hand). This exercises the exact path that + * doubled content in production: + * + * editor doc + yCollab observer replaying the seeded Y.Text on top of it. + * + * The fix builds the editor EMPTY when collab is enabled and lets the Y.Text + * be the single content source. These tests pin that: after binding + sync the + * editor holds the note body EXACTLY ONCE — for the seeder (fresh room) and + * for a joiner (room already populated over the wire). + */ + +import * as Y from 'yjs' +import { Awareness } from 'y-protocols/awareness' +import { EditorState } from '@codemirror/state' +import { EditorView } from '@codemirror/view' +import { + createCollabBinding, + type ProviderLike, + type ProviderFactory, +} from '../components/editor/collabExtension' + +// Fake transport: real Awareness (yRemoteSelections needs it), manual 'sync'. +class FakeProvider implements ProviderLike { + awareness: Awareness + private handlers: Array<(s: boolean) => void> = [] + constructor(doc: Y.Doc) { + this.awareness = new Awareness(doc) + } + on(_event: 'sync', cb: (s: boolean) => void) { this.handlers.push(cb) } + off(_event: 'sync', cb: (s: boolean) => void) { + this.handlers = this.handlers.filter(h => h !== cb) + } + destroy() { this.awareness.destroy() } + fireSync() { this.handlers.forEach(h => h(true)) } +} + +let lastProvider: FakeProvider | null = null +const factory: ProviderFactory = (_url, _room, doc) => { + lastProvider = new FakeProvider(doc) + return lastProvider +} + +// Mount a real editor with the binding spliced in, mirroring how the editor +// renders: `editorDoc` is the value CodeMirror starts with ('' when collab is +// enabled — the fix — or the body, which reproduces the old doubling bug). +function mount(editorDoc: string, body: string) { + const binding = createCollabBinding({ + url: 'wss://collab.example.com/token', + room: 'room-1', + initialContent: body, + user: null, + providerFactory: factory, + }) + const view = new EditorView({ + state: EditorState.create({ doc: editorDoc, extensions: [binding.extension] }), + }) + return { binding, view } +} + +describe('collab binding does not double content', () => { + test('SEEDER: empty editor + seed-on-sync → body appears exactly once', () => { + const body = '# Title\nthe body' + const { binding, view } = mount('', body) + expect(view.state.doc.toString()).toBe('') // nothing until sync + lastProvider!.fireSync() + expect(view.state.doc.toString()).toBe(body) // once, not doubled + binding.destroy() + view.destroy() + }) + + test('JOINER: empty editor + content arriving over the wire → body once, no re-seed', () => { + const body = 'remote body' + const { binding, view } = mount('', body) + // Simulate the shared doc being populated by another client before sync. + binding.ytext.insert(0, body) + // The yCollab observer should have already replayed it into the editor. + expect(view.state.doc.toString()).toBe(body) + // Sync now fires: seed-on-empty must NOT append our local copy. + lastProvider!.fireSync() + expect(view.state.doc.toString()).toBe(body) + binding.destroy() + view.destroy() + }) + + test('REGRESSION GUARD: initializing the editor WITH the body reproduces the doubling', () => { + // This is the pre-fix shape (value={initialContent}); it must double, which + // is exactly why the editor is built EMPTY when collab is enabled. + const body = 'hello' + const { binding, view } = mount(body, body) + lastProvider!.fireSync() + expect(view.state.doc.toString()).toBe(body + body) + binding.destroy() + view.destroy() + }) +}) diff --git a/src/__tests__/useCollaboration.test.ts b/src/__tests__/useCollaboration.test.ts index fb7d4b2..775f886 100644 --- a/src/__tests__/useCollaboration.test.ts +++ b/src/__tests__/useCollaboration.test.ts @@ -10,7 +10,7 @@ */ import { renderHook, act } from '@testing-library/react' -import { useCollaboration } from '../hooks/useCollaboration' +import { useCollaboration, buildProbeUrl } from '../hooks/useCollaboration' // Test double for window.WebSocket. Captures whichever instance the // hook constructs so tests can fire open/close events synchronously. @@ -56,6 +56,23 @@ afterEach(() => { else process.env.NEXT_PUBLIC_YJS_WS_URL = ORIGINAL_URL }) +describe('buildProbeUrl', () => { + test('appends a / segment so the worker sees //', () => { + // Configured URL is the bare base + token, with NO room. The worker reads + // the LAST segment as the room and the one before it as the token, so the + // probe must dial `//` — never the bare URL (which the + // worker would read as a single-segment room with no token → 403). + expect(buildProbeUrl('wss://collab.noteser.app/deadbeefToken')).toBe( + 'wss://collab.noteser.app/deadbeefToken/__probe__', + ) + }) + test('does not double the slash when the URL has a trailing slash', () => { + expect(buildProbeUrl('wss://collab.noteser.app/tok/')).toBe( + 'wss://collab.noteser.app/tok/__probe__', + ) + }) +}) + describe('useCollaboration', () => { test('without NEXT_PUBLIC_YJS_WS_URL: status is "off" and no WS is opened', () => { delete process.env.NEXT_PUBLIC_YJS_WS_URL @@ -84,6 +101,20 @@ describe('useCollaboration', () => { expect(result.current.attempts).toBe(0) }) + test('dials the probe room (/__probe__), not the bare configured URL', () => { + // Regression for the false "Live: unreachable" pill: the configured URL is + // the bare `/` with no room. The probe must append a room so + // the worker's `//` auth check passes — the bare URL is read + // as a single-segment room with no token and rejected (403). + process.env.NEXT_PUBLIC_YJS_WS_URL = 'wss://collab.example.com/sometoken' + const { result } = renderHook(() => useCollaboration()) + expect(MockWebSocket.lastConstructorArg).toBe( + 'wss://collab.example.com/sometoken/__probe__', + ) + // The exposed url stays the bare configured value (used by the pill tooltip). + expect(result.current.url).toBe('wss://collab.example.com/sometoken') + }) + test('close before open: status flips to disconnected and attempt counter ticks', () => { process.env.NEXT_PUBLIC_YJS_WS_URL = 'wss://collab.example.com/room' const { result } = renderHook(() => useCollaboration()) diff --git a/src/components/editor/CodeMirrorEditor.tsx b/src/components/editor/CodeMirrorEditor.tsx index 67f8157..af5bf60 100644 --- a/src/components/editor/CodeMirrorEditor.tsx +++ b/src/components/editor/CodeMirrorEditor.tsx @@ -536,6 +536,21 @@ export function CodeMirrorEditor({ const [wikilinkState, setWikilinkState] = useState(null) const [tagState, setTagState] = useState(null) + // When collaboration is enabled the shared Y.Text is the SINGLE source of + // truth for the document content. The collab binding (yCollab) seeds the + // Y.Text with the note body after the first sync and replays it into the + // editor via its observer. If the editor ALSO started life holding the same + // body (value={initialContent}), that replayed insert lands on top of the + // existing text and the body is DOUBLED on screen — and the doubled text is + // what gets saved back to the note (= corruption that would sync to GitHub). + // So when collab is on we build the editor EMPTY and let the Y.Text populate + // it: the seeder sees its body exactly once, a joiner receives the shared + // body over the wire exactly once. When collab is off this is a no-op and + // the editor is byte-for-byte identical to before. getConfiguredUrl() is the + // exact gate the collab effect below uses, so this branches together with it. + const collabEnabled = getConfiguredUrl() != null + const editorInitialValue = collabEnabled ? '' : initialContent + // Live-collaboration (Phase B). A Compartment lets us swap the yCollab // binding in/out per note without rebuilding the whole (memoized) // extension list. The compartment is created ONCE and stays empty unless @@ -1247,7 +1262,7 @@ export function CodeMirrorEditor({ { diff --git a/src/hooks/useCollaboration.ts b/src/hooks/useCollaboration.ts index 364f4e3..88984a7 100644 --- a/src/hooks/useCollaboration.ts +++ b/src/hooks/useCollaboration.ts @@ -36,6 +36,23 @@ export interface CollabState { const MAX_ATTEMPTS = 5 +// Dedicated room the connectivity probe dials. The collab worker requires a +// `//` path (token = second-to-last segment, room = last); +// NEXT_PUBLIC_YJS_WS_URL is configured as the BARE `/` with NO +// room. Opening a socket straight to that bare URL gives the worker a single +// path segment, which it reads as the ROOM with no token → 403 → the probe +// false-reports "unreachable" even though the real document binding (which +// connects to `/`) authenticates fine. Appending a probe room +// makes the probe hit the same `//` shape the worker accepts. +const PROBE_ROOM = '__probe__' + +// Build the URL the connectivity probe actually dials: the configured base +// URL plus a dedicated probe room, mirroring how WebsocketProvider(url, room) +// connects to `/`. Exported for unit testing. +export function buildProbeUrl(url: string): string { + return `${url.replace(/\/+$/, '')}/${PROBE_ROOM}` +} + // Exported so the Phase-B collaboration binding (collabExtension) shares the // exact same gate as the Phase-A connectivity probe: a single source of // truth for "is collab enabled, and at what URL". Returns null unless @@ -89,7 +106,9 @@ export function useCollaboration(): CollabState { setStatus('connecting') let ws: WebSocket try { - ws = new WebSocket(url) + // Dial the probe room (not the bare configured URL) so the worker sees + // the `//` path it requires and accepts the socket. + ws = new WebSocket(buildProbeUrl(url)) } catch { setStatus('error') return