Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds a context-based SyncBus, Yjs UndoManager integration, session-scoped collaboration state, a CollaborationStartDialog, mock Yjs test utilities, validation/logging utilities, z-index constants, new tests (unit/e2e/component), and multiple consumers updated to use the new hooks and providers. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant App as App/page.tsx
participant Sync as SyncBusProvider
participant Yjs as YjsRoom
participant Board as BoardCanvas
User->>App: Click "Start Collaboration"
App->>Sync: create SyncBus via SyncBusProvider
App->>Yjs: Mount YjsRoom (inside SyncBusProvider)
Yjs->>Sync: subscribeToLocal/RemoteChanges
App->>Board: Render BoardCanvas with syncBusContext
Board->>Sync: emitLocalChange(elements)
Sync->>Yjs: forward remote/local notifications
Yjs->>Board: emitRemoteChange -> update elements
Board->>User: Update canvas
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
|
| Metric | Coverage |
|---|---|
| Lines | 0% |
| Functions | 93.3% |
| Branches | 78.86% |
| Statements | 91.26% |
| Average | 65.9% |
How to view coverage report
- Download the coverage artifact from the link above
- Extract the ZIP file
- Open
index.htmlin your browser
Commit: 4275346938d0f0143f2e896324059ad41651ca1d
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (10)
packages/collaboration/src/adapter/collaboration-context.tsx (1)
50-52: Prefer reusingUndoStatetype instead of duplicating the shape.Line 50 duplicates a type that now exists in
../types. Reusing it avoids drift and keeps the API consistent.Suggested refactor
import type { CollaborationUser, Cursor, ViewportState, ConnectionStatus, UserPresence, SyncState, BoardElement, + UndoState, } from '../types'; @@ - undoState: { canUndo: boolean; canRedo: boolean; undoStackSize: number; redoStackSize: number }; + undoState: UndoState;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/collaboration/src/adapter/collaboration-context.tsx` around lines 50 - 52, The inline undoState property type should be replaced with the shared UndoState type from ../types to avoid duplication: change the undoState declaration to use UndoState (instead of { canUndo: boolean; canRedo: boolean; undoStackSize: number; redoStackSize: number }), add the necessary import for UndoState, and leave the undo and redo method signatures as-is (undo, redo) so the CollaborationContext type reuses the centralized type definition.packages/collaboration/src/sync-bus.tsx (1)
75-78: Memoize context value to prevent unnecessary re-renders.The
valueobject is recreated on every render ofSyncBusProvider, which could cause unnecessary re-renders in consuming components even thoughsyncBusandemitLocalChangeare stable.♻️ Proposed fix using useMemo
+import { + createContext, + useContext, + useCallback, + useState, + useMemo, + type ReactNode, +} from 'react'; export function SyncBusProvider({ children }: { children: ReactNode }) { const [syncBus] = useState(createSyncBus); const emitLocalChange = useCallback((elements: BoardElement[]) => { syncBus.emitLocalChange(elements); }, [syncBus]); - const value = { - syncBus, - emitLocalChange, - }; + const value = useMemo(() => ({ + syncBus, + emitLocalChange, + }), [syncBus, emitLocalChange]); return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/collaboration/src/sync-bus.tsx` around lines 75 - 78, The context `value` object in SyncBusProvider is recreated every render which can trigger unnecessary re-renders in consumers; wrap creation of the `value` object in a memo hook (useMemo) keyed on `syncBus` and `emitLocalChange` so the same reference is returned when those dependencies haven’t changed, i.e. memoize the object assigned to `value` in the `SyncBusProvider` component to reference-stabilize `syncBus` and `emitLocalChange`.tests/e2e/collaboration-undo-clear.spec.ts (2)
78-85: Extract duplicatedsafeClosehelper to reduce code duplication.The
safeClosefunction is defined identically in multiple tests. Consider extracting it to a shared utility or defining it once at the describe block level.♻️ Proposed refactor
+async function safeCloseContexts(...contexts: import('@playwright/test').BrowserContext[]) { + for (const context of contexts) { + try { + await context.close().catch(() => {}); + } catch { + // Ignore cleanup errors + } + } +} + test.describe('Collaboration Undo/Redo', () => { // ... in tests, replace inline safeClose with: - const safeClose = async () => { - try { - await context1.close().catch(() => {}); - await context2.close().catch(() => {}); - } catch { - // Ignore cleanup errors - } - }; + const safeClose = () => safeCloseContexts(context1, context2);Also applies to: 160-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/collaboration-undo-clear.spec.ts` around lines 78 - 85, The safeClose helper is duplicated across tests: consolidate the async function that closes context1 and context2 (currently named safeClose) into a single shared definition—either move it to the top of the relevant describe block or into a test utilities module and import it; update all usages in the e2e collaboration-undo-clear tests (and the other occurrence) to call the shared safeClose to remove duplication and keep the try/await .catch behavior intact.
219-219: Fragile menu button selector may match wrong element.Using
.getByRole('button').filter({ has: page.locator('svg') }).first()is fragile and may select an unintended button if the UI changes. Consider using a more specific test ID or accessible name for the menu button.♻️ Proposed fix using test ID
- const menuButton = page1.getByRole('button').filter({ has: page1.locator('svg') }).first(); + const menuButton = page1.getByTestId('app-menu-button');The
AppMenu.tsxalready hasdata-testid="app-menu-button"on line 204, so this selector would be more reliable.Also applies to: 285-285, 324-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/collaboration-undo-clear.spec.ts` at line 219, Replace the fragile SVG-based button selector (e.g., the menuButton assignment using page1.getByRole('button').filter({ has: page1.locator('svg') }).first()) with the stable test-id selector that AppMenu provides; use page.getByTestId('app-menu-button') or page.locator('[data-testid="app-menu-button"]') (and .first() if needed) so the test targets the intended element reliably, and update the same pattern in the other occurrences in this spec.features/toolbar/components/AppMenu.tsx (1)
123-125: Inconsistent error logging: some handlers useconsole.errorwhilehandleOpenFileuseslogger.error.The
handleOpenFilefunction (line 110) was updated to uselogger.error, buthandleSaveFile,handleExportSvg,handleExportPng, andhandleExportJpgstill useconsole.error. For consistency and to benefit from the logger's analytics integration, consider updating all error handlers.♻️ Proposed fix for consistent logging
const handleSaveFile = async () => { // ... } catch (error) { - console.error('Failed to save file:', error); + logger.error('Failed to save file', error instanceof Error ? error : undefined); posthog.captureException(error); } }; const handleExportSvg = async () => { // ... } catch (error) { - console.error('Failed to export SVG:', error); + logger.error('Failed to export SVG', error instanceof Error ? error : undefined); posthog.captureException(error); } }; const handleExportPng = async (transparent: boolean) => { // ... } catch (error) { - console.error('Failed to export PNG:', error); + logger.error('Failed to export PNG', error instanceof Error ? error : undefined); posthog.captureException(error); } }; const handleExportJpg = async () => { // ... } catch (error) { - console.error('Failed to export JPG:', error); + logger.error('Failed to export JPG', error instanceof Error ? error : undefined); posthog.captureException(error); } };Also applies to: 137-139, 152-153, 165-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/toolbar/components/AppMenu.tsx` around lines 123 - 125, Update the inconsistent error logging by replacing console.error calls with the centralized logger.error in the error handlers for handleSaveFile, handleExportSvg, handleExportPng, and handleExportJpg so they match handleOpenFile; ensure each catch block calls logger.error with a clear message and the caught error object, and continue to call posthog.captureException(error) after logging for analytics.packages/shared/src/logger.ts (2)
1-1: Consider exportingLogLeveltype for downstream consumers.The
LogLeveltype is used internally but not exported. Downstream code usingcreateLoggermay need to reference this type for type-safe log level handling.♻️ Proposed fix
-type LogLevel = 'error' | 'warn' | 'info' | 'debug'; +export type LogLevel = 'error' | 'warn' | 'info' | 'debug';Also applies to: 91-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/logger.ts` at line 1, The LogLevel type is currently internal but should be exported for downstream type safety; update the declaration of the LogLevel alias to be exported and ensure any usages (e.g., the createLogger function signature) import/consume the exported type so external modules can reference it (look for the LogLevel type and the createLogger symbol in this file and export LogLevel alongside existing exports).
18-24: Cache thefalseresult to avoid repeated environment checks.When
getIsDevelopment()returnsfalseon line 23, the result is not cached. Subsequent calls will re-execute the environment checks unnecessarily.♻️ Proposed fix
if (typeof window !== 'undefined') { const hostname = window.location.hostname; cachedIsDev = hostname === 'localhost' || hostname === '127.0.0.1'; return cachedIsDev; } - return false; + cachedIsDev = false; + return cachedIsDev; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/logger.ts` around lines 18 - 24, getIsDevelopment currently returns false without updating the cachedIsDev variable, causing repeated environment checks; update the function (getIsDevelopment) to assign cachedIsDev = false before the final return so the false result is cached, ensuring subsequent calls use cachedIsDev and skip re-evaluating window/location logic (refer to cachedIsDev and getIsDevelopment to locate where to set the cached value).tests/components/collaboration-start-dialog.test.tsx (1)
94-109: Please unskip the timeout reset case with fake timers.The “Copied → Copy” reset is core UX behavior. This can be made deterministic with
vi.useFakeTimers()andvi.advanceTimersByTime(2000)instead of skipping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/components/collaboration-start-dialog.test.tsx` around lines 94 - 109, Unskip the test "reverts to \"Copy\" text after timeout" for CollaborationStartDialog and make it deterministic by enabling fake timers (vi.useFakeTimers()) before rendering, using fireEvent.click(copyButton), asserting 'Copied' with waitFor, then advancing timers with vi.advanceTimersByTime(2000) instead of setTimeout/promise, and finally asserting 'Copy' with waitFor; restore real timers afterwards with vi.useRealTimers() to avoid affecting other tests. Use the existing symbols: CollaborationStartDialog, defaultProps, render, screen, fireEvent, waitFor, and the vitest timer helpers vi.useFakeTimers/vi.advanceTimersByTime/vi.useRealTimers.tests/e2e/collaboration.test.ts (1)
62-69: Add explicit assertions for shared-room join success.This test currently navigates both pages but does not assert room membership/board readiness, so it can pass without validating the intended behavior.
✅ Suggested assertion block
await Promise.all([ page1.waitForLoadState('domcontentloaded'), page2.waitForLoadState('domcontentloaded'), ]); + await expect(page1).toHaveURL(new RegExp(`\\broom=${roomId}\\b`)); + await expect(page2).toHaveURL(new RegExp(`\\broom=${roomId}\\b`)); + await Promise.all([waitForTestBoard(page1), waitForTestBoard(page2)]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/collaboration.test.ts` around lines 62 - 69, After both pages navigate to roomUrl, add explicit assertions to verify each client actually joined the shared room and the board is ready: for page1 and page2 use wait-for-selector/assertions (e.g., presence of the board canvas element, a participant list entry, or a room-id element) and assert the expected text/state (e.g., that both pages show the same room identifier or a participant count >0 and that the board-ready indicator is visible). Reference the existing test variables page1, page2, and roomUrl and ensure these checks run after the waitForLoadState calls so the test fails if join or board initialization did not occur.packages/collaboration/src/hooks/use-undo-redo.ts (1)
8-21: Reuse the shared undo-state contract to avoid drift.This file redefines undo/redo state fields locally. Consider reusing the shared collaboration undo-state type and only extending it with
isCollaborationMode.♻️ Proposed refactor
+import type { UndoState } from '../types'; -interface UndoRedoState { - canUndo: boolean; - canRedo: boolean; - undoStackSize: number; - redoStackSize: number; +interface UndoRedoState extends UndoState { isCollaborationMode: boolean; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/collaboration/src/hooks/use-undo-redo.ts` around lines 8 - 21, This file redefines the undo/redo state locally; replace the locally declared UndoRedoState with the project's shared undo-state type (import the shared undo-state type) and extend it only to add isCollaborationMode, leaving UndoRedoActions and UseUndoRedoReturn intact; update the type alias/definition that currently uses UndoRedoState (and any places using undoStackSize/canUndo/canRedo/redoStackSize) to reference the imported shared type plus the extra isCollaborationMode field so the hook (use-undo-redo.ts) no longer drifts from the central contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/page.tsx`:
- Around line 72-73: The session flags (initiator/disabled) are being written
using useCollaborationSession(roomFromUrl) before router.push actually updates
the URL-derived room key, which can cause flags to be written to the old
session; change the flow to defer writing those flags until the session is keyed
to the desired room — e.g., after calling router.push(newRoom) wait for
roomFromUrl (or the session's current room/id from useCollaborationSession) to
match the target room, then perform the flag writes; apply this same
deferred-write pattern to the other block referenced around lines 106–117 so all
session writes only occur once the session key matches the intended room.
In `@app/test/collaboration/page.tsx`:
- Around line 88-89: The test collaboration page creates a session via
useCollaborationSession(roomFromUrl) but calls markAsInitiator() and
clearDisabled() on a session that may still be bound to the old/null room; defer
applying those writes until the session is for the current room by watching
roomFromUrl and applying the actions only after the session changes to the new
room. Concretely: stop invoking
session.markAsInitiator()/session.clearDisabled() immediately after creating the
session; instead record the intent (e.g., via a ref or state) and in an effect
that depends on the session instance and roomFromUrl, call markAsInitiator() and
clearDisabled() only if the session corresponds to the current roomFromUrl.
Update the code paths around useCollaborationSession, markAsInitiator,
clearDisabled and the component’s effect that performs the navigation so the
writes are performed against the correct session.
In `@features/collaboration/components/collaboration-start-dialog.tsx`:
- Around line 57-65: The rendered collaboration URL (roomUrl) is currently a
truncated span which is not focusable or keyboard/assistive-tech friendly;
update the CollaborationStartDialog UI to render the URL in a focusable,
read-only, selectable control (e.g., an input[type="text"] or textarea) instead
of the <span>, ensure it has a clear accessible name (aria-label or associated
<label>), preserves the same visual styles (classes around the container), and
remains non-editable (readOnly) so users can keyboard-navigate, select, and copy
the URL when clipboard access fails; apply the same change to both places that
render roomUrl.
In `@features/collaboration/components/collaborative-board.tsx`:
- Around line 48-54: The bitwise line "hash = hash & hash" is a no-op and meant
to force a 32-bit integer; locate the hash loop that uses the variables "hash"
and "hashContent" (the function producing the returned base-36 string) and
replace that no-op with a real 32-bit coercion such as "hash |= 0" (or "hash =
hash >>> 0" if you need an unsigned 32-bit value) so the hash stays within
32-bit bounds before returning hash.toString(36).
In `@packages/collaboration/src/adapter/mock-yjs-provider.tsx`:
- Around line 209-219: undo/redo call into undoManager mutates the Yjs document
(yelements) but the provider never refreshes the React state (elements), causing
UI desync; after calling undoManager.undo() or undoManager.redo() inside the
undo and redo callbacks, explicitly trigger the same sync path you use elsewhere
to rebuild React state from the Yjs map (e.g., call the function that converts
yelements -> elements or re-run the observer callback), so
updateElementsFromYElements (or whatever function updates elements from
yelements) is invoked immediately after undoManager.undo()/redo() to
setElements(...) and keep UI in sync with Yjs.
- Around line 125-137: The current change detection only compares array length
and element IDs (data.elements vs prevElements) so remote edits that update an
element's properties but keep the same id are ignored; update the comparison
inside that block to detect property changes as well — e.g., for each element
pair (el, prevEl) compare their shallow/deep equality (or a stable serialization
like JSON.stringify) or a specific version/timestamp field instead of only id,
and if any differ treat it as dataIsDifferent, then call
setIsLocalChange(true)/setTimeout(...false) and return data.elements as before
(referencing data.elements, prevElements, setIsLocalChange).
In `@packages/collaboration/src/hooks/use-collaboration-session.ts`:
- Around line 49-55: State for initiatorKey, dialogSeenKey, and disabledKey is
only read once via useState(() => getSessionItem(...)) and therefore doesn't
update when roomId (and thus those keys) change; update the state whenever
roomId or the derived keys change by adding a useEffect that re-reads
getSessionItem for initiatorKey, dialogSeenKey, and disabledKey and calls
setIsInitiator, setWasDialogSeen, and setWasDisabled respectively (keep existing
getSessionItem and key-building logic and ensure effect depends on roomId or the
three key variables).
In `@packages/collaboration/src/types.ts`:
- Around line 148-149: The id validation currently only checks typeof and
length, so whitespace-only ids like " " pass; update the check in the element
id validation (the block that tests element['id']) to reject strings that are
only whitespace by ensuring trimmed length > 0 or using a regex to test for
non-whitespace (e.g., replace the length check with element['id'].trim().length
> 0 or a non-empty /\S/ test) so whitespace-only ids return false.
In `@tests/components/collaboration-start-dialog.test.tsx`:
- Around line 111-126: The test is spying on console.error but the component
uses logger.error; update the test to spy on the same logger the component uses
(logger.error) so assertions target the actual logging call (replace
vi.spyOn(console, 'error') with vi.spyOn(logger, 'error') and ensure the test
imports the logger from the collaboration component module or the shared logger
module used by CollaborationStartDialog, and call mockRestore() on that spy
afterwards; keep the rest of the test behavior (mockWriteText rejection, render,
click, and expectation for stringContaining('Failed to copy to clipboard'))
unchanged.
In `@tests/e2e/collaboration-undo-clear.spec.ts`:
- Around line 65-117: The test "undo/redo state syncs across two users"
currently only opens two contexts and clicks collaborate buttons then closes;
update it to perform actual drawing and undo/redo actions and assertions: after
clicking collaborateButton1 and collaborateButton2, use page1 to create a
drawable element (e.g., simulate pointer events or call the app's draw API) then
wait for page2 to observe the new element (query the same selector/state on
page2 and assert presence), next trigger undo on page1 (or via UI control) and
assert the element is removed on page2, then trigger redo on page1 and assert
the element is restored on page2; keep using the existing helpers/variables
(page1, page2, collaborateButton1, collaborateButton2, safeClose) and add
appropriate waits/timeouts around network/sync checks to avoid flakiness.
In `@tests/e2e/collaboration.test.ts`:
- Around line 5-9: The helper waitForTestBoard currently swallows selector
timeouts via waitForSelector(...).catch(() => {}), hiding real failures; remove
the empty catch so the timeout error propagates (or replace it with explicit
handling that throws a descriptive error), ensuring waitForTestBoard (the async
function) fails when the board selector isn't found within the 10s timeout so
tests cannot pass silently.
In `@tests/unit/yjs-undo-manager.test.ts`:
- Around line 84-120: Tests currently call mockUndoManager.undo() and .redo()
directly and only validate the mock; instead, set
mockUndoManager.undoStack/redoStack as required and invoke the actual
integration entrypoint that is expected to trigger those calls (the hook or
provider method that your PR added — e.g., the undo/redo handler exposed by the
hook/component), then assert mockUndoManager.undo/redo were called; also ensure
you clear mocks between tests (jest.clearAllMocks()) so the
.not.toHaveBeenCalled() assertions are meaningful. Use the
mockUndoManager.undoStack, mockUndoManager.redoStack, and
mockUndoManager.undo()/redo() symbols to locate and adjust the tests.
- Around line 14-30: The yjs mock currently exports a default constructor but
the code uses namespace imports (import * as Y from 'yjs') and calls new Y.Doc()
in yjs-provider.tsx, so update the vi.mock export to provide a named Doc export
(e.g., replace the default property with Doc) while preserving the UndoManager
export (UndoManager -> mockUndoManager) and the mocked methods (transact,
getMap, destroy, etc.) so new Y.Doc() resolves correctly.
---
Nitpick comments:
In `@features/toolbar/components/AppMenu.tsx`:
- Around line 123-125: Update the inconsistent error logging by replacing
console.error calls with the centralized logger.error in the error handlers for
handleSaveFile, handleExportSvg, handleExportPng, and handleExportJpg so they
match handleOpenFile; ensure each catch block calls logger.error with a clear
message and the caught error object, and continue to call
posthog.captureException(error) after logging for analytics.
In `@packages/collaboration/src/adapter/collaboration-context.tsx`:
- Around line 50-52: The inline undoState property type should be replaced with
the shared UndoState type from ../types to avoid duplication: change the
undoState declaration to use UndoState (instead of { canUndo: boolean; canRedo:
boolean; undoStackSize: number; redoStackSize: number }), add the necessary
import for UndoState, and leave the undo and redo method signatures as-is (undo,
redo) so the CollaborationContext type reuses the centralized type definition.
In `@packages/collaboration/src/hooks/use-undo-redo.ts`:
- Around line 8-21: This file redefines the undo/redo state locally; replace the
locally declared UndoRedoState with the project's shared undo-state type (import
the shared undo-state type) and extend it only to add isCollaborationMode,
leaving UndoRedoActions and UseUndoRedoReturn intact; update the type
alias/definition that currently uses UndoRedoState (and any places using
undoStackSize/canUndo/canRedo/redoStackSize) to reference the imported shared
type plus the extra isCollaborationMode field so the hook (use-undo-redo.ts) no
longer drifts from the central contract.
In `@packages/collaboration/src/sync-bus.tsx`:
- Around line 75-78: The context `value` object in SyncBusProvider is recreated
every render which can trigger unnecessary re-renders in consumers; wrap
creation of the `value` object in a memo hook (useMemo) keyed on `syncBus` and
`emitLocalChange` so the same reference is returned when those dependencies
haven’t changed, i.e. memoize the object assigned to `value` in the
`SyncBusProvider` component to reference-stabilize `syncBus` and
`emitLocalChange`.
In `@packages/shared/src/logger.ts`:
- Line 1: The LogLevel type is currently internal but should be exported for
downstream type safety; update the declaration of the LogLevel alias to be
exported and ensure any usages (e.g., the createLogger function signature)
import/consume the exported type so external modules can reference it (look for
the LogLevel type and the createLogger symbol in this file and export LogLevel
alongside existing exports).
- Around line 18-24: getIsDevelopment currently returns false without updating
the cachedIsDev variable, causing repeated environment checks; update the
function (getIsDevelopment) to assign cachedIsDev = false before the final
return so the false result is cached, ensuring subsequent calls use cachedIsDev
and skip re-evaluating window/location logic (refer to cachedIsDev and
getIsDevelopment to locate where to set the cached value).
In `@tests/components/collaboration-start-dialog.test.tsx`:
- Around line 94-109: Unskip the test "reverts to \"Copy\" text after timeout"
for CollaborationStartDialog and make it deterministic by enabling fake timers
(vi.useFakeTimers()) before rendering, using fireEvent.click(copyButton),
asserting 'Copied' with waitFor, then advancing timers with
vi.advanceTimersByTime(2000) instead of setTimeout/promise, and finally
asserting 'Copy' with waitFor; restore real timers afterwards with
vi.useRealTimers() to avoid affecting other tests. Use the existing symbols:
CollaborationStartDialog, defaultProps, render, screen, fireEvent, waitFor, and
the vitest timer helpers
vi.useFakeTimers/vi.advanceTimersByTime/vi.useRealTimers.
In `@tests/e2e/collaboration-undo-clear.spec.ts`:
- Around line 78-85: The safeClose helper is duplicated across tests:
consolidate the async function that closes context1 and context2 (currently
named safeClose) into a single shared definition—either move it to the top of
the relevant describe block or into a test utilities module and import it;
update all usages in the e2e collaboration-undo-clear tests (and the other
occurrence) to call the shared safeClose to remove duplication and keep the
try/await .catch behavior intact.
- Line 219: Replace the fragile SVG-based button selector (e.g., the menuButton
assignment using page1.getByRole('button').filter({ has: page1.locator('svg')
}).first()) with the stable test-id selector that AppMenu provides; use
page.getByTestId('app-menu-button') or
page.locator('[data-testid="app-menu-button"]') (and .first() if needed) so the
test targets the intended element reliably, and update the same pattern in the
other occurrences in this spec.
In `@tests/e2e/collaboration.test.ts`:
- Around line 62-69: After both pages navigate to roomUrl, add explicit
assertions to verify each client actually joined the shared room and the board
is ready: for page1 and page2 use wait-for-selector/assertions (e.g., presence
of the board canvas element, a participant list entry, or a room-id element) and
assert the expected text/state (e.g., that both pages show the same room
identifier or a participant count >0 and that the board-ready indicator is
visible). Reference the existing test variables page1, page2, and roomUrl and
ensure these checks run after the waitForLoadState calls so the test fails if
join or board initialization did not occur.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
app/page.tsxapp/test/collaboration/page.tsxfeatures/board/components/BoardCanvas.tsxfeatures/collaboration/components/collaboration-start-dialog.tsxfeatures/collaboration/components/collaborative-board.tsxfeatures/collaboration/components/room.tsxfeatures/collaboration/index.tsfeatures/toolbar/components/AppMenu.tsxfeatures/toolbar/components/UndoRedoButtons.tsxpackages/collaboration/package.jsonpackages/collaboration/src/adapter/collaboration-context.tsxpackages/collaboration/src/adapter/index.tspackages/collaboration/src/adapter/mock-yjs-provider.tsxpackages/collaboration/src/adapter/yjs-provider.tsxpackages/collaboration/src/hooks/index.tspackages/collaboration/src/hooks/use-collaboration-session.tspackages/collaboration/src/hooks/use-undo-redo.tspackages/collaboration/src/index.tspackages/collaboration/src/logger.tspackages/collaboration/src/sync-bus.tspackages/collaboration/src/sync-bus.tsxpackages/collaboration/src/test-utils.tspackages/collaboration/src/types.tspackages/shared/src/index.tspackages/shared/src/logger.tsshared/constants/styles.tstests/components/collaboration-start-dialog.test.tsxtests/e2e/collaboration-undo-clear.spec.tstests/e2e/collaboration.test.tstests/unit/clear-board-sync.test.tstests/unit/sync-bus.test.tstests/unit/yjs-undo-manager.test.tstsconfig.json
💤 Files with no reviewable changes (1)
- packages/collaboration/src/sync-bus.ts
features/collaboration/components/collaboration-start-dialog.tsx
Outdated
Show resolved
Hide resolved
- Add CollaborationRoomContext to MockYjsProvider for e2e tests - Move markAsInitiator/clearDisabled to useEffects for proper URL sync - Split useCollaborationRoom to allow mock context override - Change dialog link from span to input for better selection - Fix hash calculation (hash |= 0 instead of hash & hash) - Replace console.error with logger.error - Update mock YJS undo/redo to sync elements after operation - Add data-board attribute for e2e test selectors - Remove coverage thresholds from vitest config
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
features/board/components/BoardCanvas.tsx (1)
85-92:⚠️ Potential issue | 🟠 MajorValidate remote elements before applying to board state.
Remote changes are applied directly to
setValueandlistRender.updatewithout runtime validation. A malformed payload can corrupt board state or trigger runtime failures.Suggested fix
- const unsubscribe = syncBusContext.syncBus.subscribeToRemoteChanges((elements: BoardElement[]) => { - onElementsChange(elements); - listRender.update(elements, { + const unsubscribe = syncBusContext.syncBus.subscribeToRemoteChanges((elements: BoardElement[]) => { + const { valid, invalid } = validateBoardElements(elements); + if (invalid.length > 0) { + logger.warn(`Skipped ${invalid.length} invalid remote board elements`, { + invalidCount: invalid.length, + }); + } + onElementsChange(valid as PlaitElement[]); + listRender.update(valid as PlaitElement[], { board: board, parent: board, parentG: PlaitBoard.getElementHost(board), }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/board/components/BoardCanvas.tsx` around lines 85 - 92, Remote payloads from syncBusContext.syncBus.subscribeToRemoteChanges are applied immediately via onElementsChange and listRender.update which can corrupt state if malformed; add runtime validation before applying: implement/plug a validator (e.g., validateElements(elements)) that checks the payload is an array and each BoardElement has required fields/types (ids, geometry, type, children or content shapes) and return false on any failure; if validation fails, log the error/context and skip calling onElementsChange and listRender.update (or attempt safe sanitization), otherwise proceed to call onElementsChange(elements) and listRender.update(elements, { board, parent: board, parentG: PlaitBoard.getElementHost(board) }).
♻️ Duplicate comments (1)
app/page.tsx (1)
116-121:⚠️ Potential issue | 🟠 MajorSession flags are scoped to the wrong room lifecycle.
Two logic bugs here:
- Line 116-121 marks every URL room as initiator (including users who just joined).
- Disabled state is written after room removal (Line 134-138), so it’s keyed to
nulland not persisted per room.This can incorrectly show initiator-only UI and lose “disabled” intent for a specific room.
Suggested fix
-import { useEffect, Suspense, useCallback, useMemo } from 'react'; +import { useEffect, Suspense, useCallback, useMemo, useRef } from 'react'; @@ const session = useCollaborationSession(roomFromUrl); + const pendingInitiatorRoomRef = useRef<string | null>(null); @@ const handleEnableCollaboration = useCallback(() => { const roomId = crypto.randomUUID(); + pendingInitiatorRoomRef.current = roomId; @@ enableCollaboration(roomId); }, [pathname, searchParams, router, enableCollaboration]); @@ - useEffect(() => { - if (roomFromUrl && session.isInitiator === false) { - session.markAsInitiator(); - session.clearDisabled(); - } - }, [roomFromUrl, session]); + useEffect(() => { + if (!roomFromUrl) return; + if (pendingInitiatorRoomRef.current === roomFromUrl) { + session.markAsInitiator(); + session.clearDisabled(); + pendingInitiatorRoomRef.current = null; + } + }, [roomFromUrl, session]); @@ const handleDisableCollaboration = useCallback(() => { + if (roomFromUrl) { + session.markAsDisabled(); + session.clearInitiator(); + } disableCollaboration(); @@ - }, [disableCollaboration, roomFromUrl, pathname, searchParams, router]); + }, [disableCollaboration, roomFromUrl, pathname, searchParams, router, session]); - - useEffect(() => { - if (!roomFromUrl && session.wasDisabled === false) { - session.markAsDisabled(); - } - }, [roomFromUrl, session]);Also applies to: 123-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/page.tsx` around lines 116 - 121, The effect currently marks every URL room visitor as initiator and clears the disabled flag in the wrong lifecycle; change the logic in the useEffect(s) so flags are tracked per-room id: only call session.markAsInitiator() when the current user actually created/owns the room (e.g., when roomFromUrl.id === session.ownedRoomId or when a creation event sets a createdByCurrentUser boolean) and ensure session.clearDisabled() and any writes to session.disabled are performed when entering a specific room (roomFromUrl != null) and persisted using that room’s id (e.g., key off roomFromUrl.id) instead of after room removal — update the useEffect watching [roomFromUrl, session] and the removal handler code (the block around session.clearDisabled() / disabled writes) to read/write disabled state keyed by room id so initiator and disabled state are per-room, not global.
🧹 Nitpick comments (3)
tests/e2e/collaboration-undo-clear.spec.ts (1)
94-98: Remove redundantsafeClose(...)beforetest.skip()insidetryblocks.These branches already have
finallyteardown (Line 153-Line 155 and Line 290-Line 292). Double-closing is unnecessary and adds noise.♻️ Example cleanup simplification
if (!button1Visible || !button2Visible) { - await safeClose(context1, context2); test.skip(); return; }Apply the same pattern to the other similar branches in this file.
Also applies to: 107-111, 120-124, 214-218, 227-231, 240-244, 253-257, 262-266, 276-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/collaboration-undo-clear.spec.ts` around lines 94 - 98, Remove the redundant calls to safeClose(context1, context2) that occur immediately before test.skip() inside try blocks (e.g., the branch beginning with "if (!button1Visible || !button2Visible) { ... }"); rely on the existing finally teardown (the try/finally that calls safeClose at the end) to perform cleanup instead, and apply the same removal to the other listed branches (lines around 107-111, 120-124, 214-218, 227-231, 240-244, 253-257, 262-266, 276-279) so no double-closing occurs—leave the test.skip() calls in place but delete the pre-skip safeClose(...) calls.tests/e2e/helpers/browser.ts (1)
3-10: GeneralizesafeCloseand close contexts concurrently.This helper is currently fixed to two contexts and performs serial close. A variadic version with
Promise.allSettledkeeps teardown resilient and reusable.♻️ Proposed refactor
export async function safeClose(context1: BrowserContext, context2: BrowserContext): Promise<void> { - try { - await context1.close().catch(() => {}); - await context2.close().catch(() => {}); - } catch { - // Ignore cleanup errors - } + await Promise.allSettled([context1.close(), context2.close()]); }Or make it variadic for broader reuse:
-export async function safeClose(context1: BrowserContext, context2: BrowserContext): Promise<void> { - await Promise.allSettled([context1.close(), context2.close()]); +export async function safeClose(...contexts: BrowserContext[]): Promise<void> { + await Promise.allSettled(contexts.map((context) => context.close())); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpers/browser.ts` around lines 3 - 10, Replace the fixed two-argument safeClose with a variadic version that accepts ...contexts: BrowserContext[] and closes them concurrently using Promise.allSettled; for each context call context.close() (skip null/undefined), then await Promise.allSettled to ensure teardown resilience and swallow any rejection results (no throw) so cleanup never fails; update references to the old signature to pass an array or spread arguments into safeClose if needed.tests/e2e/collaboration.test.ts (1)
77-78: Prefer query-param assertions over runtimeRegExpconstruction.Line 77 and Line 78 can assert
roomdirectly from URL params; this avoids regex escaping pitfalls and is clearer.♻️ Proposed refactor
- await expect(page1).toHaveURL(new RegExp(`room=${roomId}`)); - await expect(page2).toHaveURL(new RegExp(`room=${roomId}`)); + await expect.poll(() => new URL(page1.url()).searchParams.get('room')).toBe(roomId); + await expect.poll(() => new URL(page2.url()).searchParams.get('room')).toBe(roomId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/collaboration.test.ts` around lines 77 - 78, The tests currently use RegExp to check the room query param on page1 and page2; replace those with direct query-param assertions by retrieving each page's URL (via page1.url() and page2.url()), parsing its search params (new URL(...).searchParams) and asserting that searchParams.get('room') equals roomId for both pages (use await on page.url() calls). This avoids regex escaping issues and makes the assertions explicit and robust.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/collaboration/components/collaborative-board.tsx`:
- Around line 55-58: Replace the non-deterministic timestamp fallback in the
catch block that logs "Error generating elements hash" (currently returning
Date.now().toString(36)) with a deterministic fallback computed from the input
payload (e.g., stable-serialize the elements or board state and compute a stable
hash). Capture the same error in logger.error, then produce the fallback by
serializing the elements (or the same data used by the primary hash) and
returning a stable hash string (for example a hex digest from a crypto hash of
JSON.stringify(elements) or a custom stable checksum) so identical inputs always
yield the same fallback value.
In `@packages/collaboration/src/adapter/collaboration-context.tsx`:
- Around line 58-65: The current useCollaborationRoom calls
useContext(CollaborationRoomContext) and only calls
useCollaborationRoomLiveblocks() when the context is missing, causing
conditional hook execution; refactor so hooks run unconditionally by moving the
fallback into a separate provider/component: create a CollaborationRoomProvider
(or a useCollaborationRoomFallback hook used unconditionally) that always
invokes useCollaborationRoomLiveblocks and then returns either the context value
from CollaborationRoomContext or the liveblocks fallback value, and update
useCollaborationRoom to call both useContext(CollaborationRoomContext) and the
fallback hook (or always read from the provider) so hook invocation order/count
is stable across renders.
In `@packages/collaboration/src/adapter/mock-yjs-provider.tsx`:
- Around line 108-109: The MockRoom component currently ignores passed props by
always initializing with createMockRoomId and a fresh Y.Doc; update the
initialization to honor incoming props: initialize roomId with useState(() =>
props.roomId ?? createMockRoomId()) (or the prop name used where MockRoom is
defined) and apply props.initialElements into the Y.Doc after creation (e.g.,
seed the Y.Doc via Y.Array/Y.Map or Y.applyUpdate inside the useState
initializer or a useEffect that runs once) so exported MockRoom actually scopes
to the provided roomId and seeds initialElements; reference the MockRoom
component, the roomId state, the initialElements prop, createMockRoomId and the
ydoc (new Y.Doc()) when making these changes.
- Around line 125-133: The subscribe callback for syncRef.current (inside the
sync handler that calls setElementsState) assumes data.elements is always an
array; validate the incoming BroadcastChannel payload before dereferencing by
checking that data is an object and Array.isArray(data.elements) (or coerce to
an empty array) and bail/ignore or log malformed messages instead of accessing
data.elements directly; update the logic that computes dataIsDifferent (in the
subscribe callback) to operate on a safe array variable so malformed messages do
not throw and break sync handling.
- Around line 235-240: Remove the extraneous connectionStatus property from the
syncState object returned by the useMemo in mock-yjs-provider.tsx because it is
not part of the SyncState interface; update the useMemo initializer for
syncState (the constant named syncState) to only include isConnected, isSyncing,
and lastSyncedAt so the object shape matches the SyncState type.
- Around line 197-210: insertElement, updateElement, and deleteElement capture a
stale elements value in their closures which can drop rapid mutations; fix by
introducing a ref (e.g., latestElementsRef) that is kept in sync with elements
(update the ref inside a useEffect or each setElements call) and then rewrite
those callbacks to read from latestElementsRef.current and call setElements
using the functional updater (or a value derived from the ref) so you can remove
elements from their dependency arrays; update the closure-free callbacks
insertElement, updateElement, and deleteElement to use the ref and functional
setElements to avoid stale-closure races.
In `@tests/e2e/collaboration-undo-clear.spec.ts`:
- Line 365: The test currently swallows failures by appending .catch(() => {})
to page.waitForSelector('[role="dialog"]', { state: 'hidden', timeout: 5000 });
remove the silent catch so the wait throws on timeout, or replace it with an
explicit assert that fails with a clear message when the dialog does not close
(use page.waitForSelector with state: 'hidden' and let the error propagate, or
catch and rethrow a new Error mentioning the cancel-flow and dialog selector);
update the invocation of page.waitForSelector to either be awaited without
.catch or wrapped to throw a descriptive error so the test fails if the dialog
never closes.
In `@tests/e2e/collaboration.test.ts`:
- Around line 79-82: The finally block currently awaits context1.close() then
context2.close(), which lets the first thrown error skip closing the second and
can mask test failures; replace both direct closes with the resilient helper
(call safeClose(context1) and safeClose(context2)) so each context is attempted
to close independently and errors are handled by safeClose, ensuring the second
close still runs even if the first fails.
---
Outside diff comments:
In `@features/board/components/BoardCanvas.tsx`:
- Around line 85-92: Remote payloads from
syncBusContext.syncBus.subscribeToRemoteChanges are applied immediately via
onElementsChange and listRender.update which can corrupt state if malformed; add
runtime validation before applying: implement/plug a validator (e.g.,
validateElements(elements)) that checks the payload is an array and each
BoardElement has required fields/types (ids, geometry, type, children or content
shapes) and return false on any failure; if validation fails, log the
error/context and skip calling onElementsChange and listRender.update (or
attempt safe sanitization), otherwise proceed to call onElementsChange(elements)
and listRender.update(elements, { board, parent: board, parentG:
PlaitBoard.getElementHost(board) }).
---
Duplicate comments:
In `@app/page.tsx`:
- Around line 116-121: The effect currently marks every URL room visitor as
initiator and clears the disabled flag in the wrong lifecycle; change the logic
in the useEffect(s) so flags are tracked per-room id: only call
session.markAsInitiator() when the current user actually created/owns the room
(e.g., when roomFromUrl.id === session.ownedRoomId or when a creation event sets
a createdByCurrentUser boolean) and ensure session.clearDisabled() and any
writes to session.disabled are performed when entering a specific room
(roomFromUrl != null) and persisted using that room’s id (e.g., key off
roomFromUrl.id) instead of after room removal — update the useEffect watching
[roomFromUrl, session] and the removal handler code (the block around
session.clearDisabled() / disabled writes) to read/write disabled state keyed by
room id so initiator and disabled state are per-room, not global.
---
Nitpick comments:
In `@tests/e2e/collaboration-undo-clear.spec.ts`:
- Around line 94-98: Remove the redundant calls to safeClose(context1, context2)
that occur immediately before test.skip() inside try blocks (e.g., the branch
beginning with "if (!button1Visible || !button2Visible) { ... }"); rely on the
existing finally teardown (the try/finally that calls safeClose at the end) to
perform cleanup instead, and apply the same removal to the other listed branches
(lines around 107-111, 120-124, 214-218, 227-231, 240-244, 253-257, 262-266,
276-279) so no double-closing occurs—leave the test.skip() calls in place but
delete the pre-skip safeClose(...) calls.
In `@tests/e2e/collaboration.test.ts`:
- Around line 77-78: The tests currently use RegExp to check the room query
param on page1 and page2; replace those with direct query-param assertions by
retrieving each page's URL (via page1.url() and page2.url()), parsing its search
params (new URL(...).searchParams) and asserting that searchParams.get('room')
equals roomId for both pages (use await on page.url() calls). This avoids regex
escaping issues and makes the assertions explicit and robust.
In `@tests/e2e/helpers/browser.ts`:
- Around line 3-10: Replace the fixed two-argument safeClose with a variadic
version that accepts ...contexts: BrowserContext[] and closes them concurrently
using Promise.allSettled; for each context call context.close() (skip
null/undefined), then await Promise.allSettled to ensure teardown resilience and
swallow any rejection results (no throw) so cleanup never fails; update
references to the old signature to pass an array or spread arguments into
safeClose if needed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
app/page.tsxapp/test/collaboration/page.tsxfeatures/board/components/BoardCanvas.tsxfeatures/collaboration/components/collaboration-start-dialog.tsxfeatures/collaboration/components/collaborative-board.tsxfeatures/toolbar/components/AppMenu.tsxpackages/collaboration/src/adapter/collaboration-context.tsxpackages/collaboration/src/adapter/mock-yjs-provider.tsxpackages/collaboration/src/hooks/use-collaboration-session.tspackages/collaboration/src/hooks/use-undo-redo.tspackages/collaboration/src/sync-bus.tsxpackages/collaboration/src/types.tspackages/shared/src/logger.tstests/components/collaboration-start-dialog.test.tsxtests/e2e/collaboration-undo-clear.spec.tstests/e2e/collaboration.test.tstests/e2e/helpers/browser.tstests/unit/yjs-undo-manager.test.tsvitest.config.mts
💤 Files with no reviewable changes (1)
- vitest.config.mts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/collaboration/src/hooks/use-undo-redo.ts
- features/collaboration/components/collaboration-start-dialog.tsx
- features/toolbar/components/AppMenu.tsx
- app/test/collaboration/page.tsx
- packages/shared/src/logger.ts
- packages/collaboration/src/sync-bus.tsx
- tests/unit/yjs-undo-manager.test.ts
Comment on lines
+55
to
58
| } catch (error) { | ||
| logger.error('Error generating elements hash', error instanceof Error ? error : undefined); | ||
| return Date.now().toString(36); | ||
| } |
Contributor
There was a problem hiding this comment.
Use a deterministic hash fallback instead of timestamp.
When hashing fails, Date.now() makes the hash change even for unchanged input. That defeats dedupe checks and can trigger unnecessary sync/update churn.
Suggested fix
} catch (error) {
logger.error('Error generating elements hash', error instanceof Error ? error : undefined);
- return Date.now().toString(36);
+ // Deterministic fallback to preserve dedupe behavior
+ return elements.map((el) => `${el.id}:${el.type ?? ''}`).join('|||');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@features/collaboration/components/collaborative-board.tsx` around lines 55 -
58, Replace the non-deterministic timestamp fallback in the catch block that
logs "Error generating elements hash" (currently returning
Date.now().toString(36)) with a deterministic fallback computed from the input
payload (e.g., stable-serialize the elements or board state and compute a stable
hash). Capture the same error in logger.error, then produce the fallback by
serializing the elements (or the same data used by the primary hash) and
returning a stable hash string (for example a hex digest from a crypto hash of
JSON.stringify(elements) or a custom stable checksum) so identical inputs always
yield the same fallback value.
Comment on lines
58
to
+65
| export function useCollaborationRoom(): CollaborationRoomContextValue { | ||
| const existingContext = useContext(CollaborationRoomContext); | ||
| if (existingContext) { | ||
| return existingContext; | ||
| } | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| return useCollaborationRoomLiveblocks(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/collaboration/src/adapter/collaboration-context.tsxRepository: Kripu77/thinkix
Length of output: 6043
🏁 Script executed:
# Verify the hook count discrepancy by analyzing both code paths
rg -n "useContext|useYjsCollaboration|useMyPresence|useOthers|useStatus|useRoom|useEffect|useCallback|useMemo" packages/collaboration/src/adapter/collaboration-context.tsx | head -30Repository: Kripu77/thinkix
Length of output: 864
Refactor useCollaborationRoom to avoid conditional hook execution.
The function calls useContext unconditionally at line 59, then conditionally returns without invoking any other hooks when a context exists, but invokes five additional hooks in useCollaborationRoomLiveblocks when context is null. This violates the Rules of Hooks: hook count must remain constant across all renders, not vary based on runtime conditions. The eslint-disable suppresses the warning but does not address the underlying stability risk.
Extract the fallback logic into a separate conditional component or context provider to ensure all hooks execute consistently on every render.
🧰 Tools
🪛 Biome (2.4.4)
[error] 64-64: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/collaboration/src/adapter/collaboration-context.tsx` around lines 58
- 65, The current useCollaborationRoom calls
useContext(CollaborationRoomContext) and only calls
useCollaborationRoomLiveblocks() when the context is missing, causing
conditional hook execution; refactor so hooks run unconditionally by moving the
fallback into a separate provider/component: create a CollaborationRoomProvider
(or a useCollaborationRoomFallback hook used unconditionally) that always
invokes useCollaborationRoomLiveblocks and then returns either the context value
from CollaborationRoomContext or the liveblocks fallback value, and update
useCollaborationRoom to call both useContext(CollaborationRoomContext) and the
fallback hook (or always read from the provider) so hook invocation order/count
is stable across renders.
Comment on lines
+108
to
+109
| const [roomId] = useState(createMockRoomId); | ||
| const [ydoc] = useState(() => new Y.Doc()); |
Contributor
There was a problem hiding this comment.
MockRoom API is currently non-functional (roomId/initialElements are ignored).
Line 285-286 expose roomId and initialElements, but Line 289-290 drops both. This makes room scoping/seeding impossible through the exported component and can mislead test usage.
✅ Minimal fix if props are intentionally unsupported
interface MockRoomProps {
children: ReactNode;
- roomId: string;
- initialElements?: BoardElement[];
}
export function MockRoom({ children }: MockRoomProps) {
return <>{children}</>;
}Also applies to: 283-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/collaboration/src/adapter/mock-yjs-provider.tsx` around lines 108 -
109, The MockRoom component currently ignores passed props by always
initializing with createMockRoomId and a fresh Y.Doc; update the initialization
to honor incoming props: initialize roomId with useState(() => props.roomId ??
createMockRoomId()) (or the prop name used where MockRoom is defined) and apply
props.initialElements into the Y.Doc after creation (e.g., seed the Y.Doc via
Y.Array/Y.Map or Y.applyUpdate inside the useState initializer or a useEffect
that runs once) so exported MockRoom actually scopes to the provided roomId and
seeds initialElements; reference the MockRoom component, the roomId state, the
initialElements prop, createMockRoomId and the ydoc (new Y.Doc()) when making
these changes.
Comment on lines
+125
to
+133
| const unsubscribe = syncRef.current.subscribe((data) => { | ||
| setElementsState(prevElements => { | ||
| const dataIsDifferent = data.elements.length !== prevElements.length || | ||
| data.elements.some((el, i) => { | ||
| const prevEl = prevElements[i]; | ||
| if (!prevEl || el.id !== prevEl.id) return true; | ||
| return JSON.stringify(el) !== JSON.stringify(prevEl); | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
Validate incoming BroadcastChannel payload before dereferencing.
Line 127 assumes data.elements is always a valid array. A malformed message will throw and break sync handling.
✅ Proposed hardening
const unsubscribe = syncRef.current.subscribe((data) => {
+ if (!data || !Array.isArray(data.elements)) return;
setElementsState(prevElements => {
const dataIsDifferent = data.elements.length !== prevElements.length ||🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/collaboration/src/adapter/mock-yjs-provider.tsx` around lines 125 -
133, The subscribe callback for syncRef.current (inside the sync handler that
calls setElementsState) assumes data.elements is always an array; validate the
incoming BroadcastChannel payload before dereferencing by checking that data is
an object and Array.isArray(data.elements) (or coerce to an empty array) and
bail/ignore or log malformed messages instead of accessing data.elements
directly; update the logic that computes dataIsDifferent (in the subscribe
callback) to operate on a safe array variable so malformed messages do not throw
and break sync handling.
Comment on lines
+197
to
+210
| const insertElement = useCallback((element: BoardElement) => { | ||
| setElements([...elements, element]); | ||
| }, [elements, setElements]); | ||
|
|
||
| const updateElement = useCallback((id: string, changes: Record<string, unknown>) => { | ||
| const newElements = elements.map((el) => | ||
| el.id === id ? { ...el, ...changes } : el | ||
| ); | ||
| setElements(newElements); | ||
| }, [elements, setElements]); | ||
|
|
||
| const deleteElement = useCallback((id: string) => { | ||
| setElements(elements.filter((el) => el.id !== id)); | ||
| }, [elements, setElements]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -50 packages/collaboration/src/adapter/mock-yjs-provider.tsxRepository: Kripu77/thinkix
Length of output: 1312
🏁 Script executed:
wc -l packages/collaboration/src/adapter/mock-yjs-provider.tsxRepository: Kripu77/thinkix
Length of output: 119
🏁 Script executed:
sed -n '190,220p' packages/collaboration/src/adapter/mock-yjs-provider.tsxRepository: Kripu77/thinkix
Length of output: 999
🏁 Script executed:
sed -n '100,170p' packages/collaboration/src/adapter/mock-yjs-provider.tsxRepository: Kripu77/thinkix
Length of output: 2194
🏁 Script executed:
sed -n '170,200p' packages/collaboration/src/adapter/mock-yjs-provider.tsxRepository: Kripu77/thinkix
Length of output: 879
🏁 Script executed:
sed -n '177,195p' packages/collaboration/src/adapter/mock-yjs-provider.tsxRepository: Kripu77/thinkix
Length of output: 510
🏁 Script executed:
grep -n "insertElement\|updateElement\|deleteElement" packages/collaboration/src/adapter/mock-yjs-provider.tsx | head -20Repository: Kripu77/thinkix
Length of output: 923
Fix stale closure risk in element mutation callbacks.
insertElement, updateElement, and deleteElement capture elements state in their closure, but setElements has async operations (broadcast, setTimeout) that delay state updates. Two rapid calls can both operate on stale elements, dropping mutations.
Use a ref to sync with the latest elements state and remove it from the dependency arrays:
Proposed fix
const [elements, setElementsState] = useState<BoardElement[]>([]);
+ const elementsRef = useRef<BoardElement[]>([]);
+
+ useEffect(() => {
+ elementsRef.current = elements;
+ }, [elements]);
const insertElement = useCallback((element: BoardElement) => {
- setElements([...elements, element]);
- }, [elements, setElements]);
+ setElements([...elementsRef.current, element]);
+ }, [setElements]);
const updateElement = useCallback((id: string, changes: Record<string, unknown>) => {
- const newElements = elements.map((el) =>
+ const newElements = elementsRef.current.map((el) =>
el.id === id ? { ...el, ...changes } : el
);
setElements(newElements);
- }, [elements, setElements]);
+ }, [setElements]);
const deleteElement = useCallback((id: string) => {
- setElements(elements.filter((el) => el.id !== id));
- }, [elements, setElements]);
+ setElements(elementsRef.current.filter((el) => el.id !== id));
+ }, [setElements]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const insertElement = useCallback((element: BoardElement) => { | |
| setElements([...elements, element]); | |
| }, [elements, setElements]); | |
| const updateElement = useCallback((id: string, changes: Record<string, unknown>) => { | |
| const newElements = elements.map((el) => | |
| el.id === id ? { ...el, ...changes } : el | |
| ); | |
| setElements(newElements); | |
| }, [elements, setElements]); | |
| const deleteElement = useCallback((id: string) => { | |
| setElements(elements.filter((el) => el.id !== id)); | |
| }, [elements, setElements]); | |
| const elementsRef = useRef<BoardElement[]>([]); | |
| useEffect(() => { | |
| elementsRef.current = elements; | |
| }, [elements]); | |
| const insertElement = useCallback((element: BoardElement) => { | |
| setElements([...elementsRef.current, element]); | |
| }, [setElements]); | |
| const updateElement = useCallback((id: string, changes: Record<string, unknown>) => { | |
| const newElements = elementsRef.current.map((el) => | |
| el.id === id ? { ...el, ...changes } : el | |
| ); | |
| setElements(newElements); | |
| }, [setElements]); | |
| const deleteElement = useCallback((id: string) => { | |
| setElements(elementsRef.current.filter((el) => el.id !== id)); | |
| }, [setElements]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/collaboration/src/adapter/mock-yjs-provider.tsx` around lines 197 -
210, insertElement, updateElement, and deleteElement capture a stale elements
value in their closures which can drop rapid mutations; fix by introducing a ref
(e.g., latestElementsRef) that is kept in sync with elements (update the ref
inside a useEffect or each setElements call) and then rewrite those callbacks to
read from latestElementsRef.current and call setElements using the functional
updater (or a value derived from the ref) so you can remove elements from their
dependency arrays; update the closure-free callbacks insertElement,
updateElement, and deleteElement to use the ref and functional setElements to
avoid stale-closure races.
Comment on lines
+235
to
+240
| const syncState: SyncState = useMemo(() => ({ | ||
| isConnected: true, | ||
| isSyncing: false, | ||
| connectionStatus: 'connected' as ConnectionStatus, | ||
| lastSyncedAt: 0, | ||
| }), []); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== SyncState definition ==="
rg -n -A6 -B2 'export interface SyncState' packages/collaboration/src/types.ts
echo
echo "=== syncState literal in mock provider ==="
rg -n -A8 -B2 'const syncState: SyncState' packages/collaboration/src/adapter/mock-yjs-provider.tsx
echo
echo "=== Any connectionStatus usage in syncState block ==="
rg -n -A3 -B3 'connectionStatus' packages/collaboration/src/adapter/mock-yjs-provider.tsxRepository: Kripu77/thinkix
Length of output: 1099
Remove connectionStatus field from syncState object literal—not part of SyncState interface.
Line 238 includes connectionStatus: 'connected' as ConnectionStatus, in the syncState object, but the SyncState interface (defined at packages/collaboration/src/types.ts:174) only declares isConnected, isSyncing, and lastSyncedAt. This will fail TypeScript's structural type checking.
Fix
const syncState: SyncState = useMemo(() => ({
isConnected: true,
isSyncing: false,
- connectionStatus: 'connected' as ConnectionStatus,
lastSyncedAt: 0,
}), []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const syncState: SyncState = useMemo(() => ({ | |
| isConnected: true, | |
| isSyncing: false, | |
| connectionStatus: 'connected' as ConnectionStatus, | |
| lastSyncedAt: 0, | |
| }), []); | |
| const syncState: SyncState = useMemo(() => ({ | |
| isConnected: true, | |
| isSyncing: false, | |
| lastSyncedAt: 0, | |
| }), []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/collaboration/src/adapter/mock-yjs-provider.tsx` around lines 235 -
240, Remove the extraneous connectionStatus property from the syncState object
returned by the useMemo in mock-yjs-provider.tsx because it is not part of the
SyncState interface; update the useMemo initializer for syncState (the constant
named syncState) to only include isConnected, isSyncing, and lastSyncedAt so the
object shape matches the SyncState type.
| const cancelButton = page.getByRole('button', { name: /cancel/i }); | ||
| if (await cancelButton.isVisible({ timeout: 5000 }).catch(() => false)) { | ||
| await cancelButton.click(); | ||
| await page.waitForSelector('[role="dialog"]', { state: 'hidden', timeout: 5000 }).catch(() => {}); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the context around line 365
wc -l tests/e2e/collaboration-undo-clear.spec.tsRepository: Kripu77/thinkix
Length of output: 105
🏁 Script executed:
# Read the area around line 365 with context
sed -n '355,375p' tests/e2e/collaboration-undo-clear.spec.ts | cat -nRepository: Kripu77/thinkix
Length of output: 999
🏁 Script executed:
# Check the beginning of the file to identify the test framework
head -30 tests/e2e/collaboration-undo-clear.spec.ts | cat -nRepository: Kripu77/thinkix
Length of output: 1480
🏁 Script executed:
# Let's check if there are other similar patterns of swallowing errors in this file
rg 'catch\(\(\)\s*=>\s*\{\}' tests/e2e/collaboration-undo-clear.spec.ts -A 1 -B 1Repository: Kripu77/thinkix
Length of output: 215
🏁 Script executed:
# Check the broader context of line 365 (which is line 11 in the snippet) to understand the test flow
sed -n '350,375p' tests/e2e/collaboration-undo-clear.spec.ts | cat -nRepository: Kripu77/thinkix
Length of output: 1216
🏁 Script executed:
# Verify Playwright API support for the proposed fix
web_search
Playwright expect toBeHidden API documentationRepository: Kripu77/thinkix
Length of output: 158
🌐 Web query:
Playwright expect toBeHidden API
💡 Result:
expect(locator).toBeHidden() (Playwright Test)
What it asserts: the Locator either
- matches no DOM element, or
- matches an element that is not visible. [1]
Signature:
await expect(locator).toBeHidden(options?);Options:
timeout?: number— how long Playwright retries the assertion before failing (defaults toexpecttimeout from config). [1]
Example:
import { test, expect } from '@playwright/test';
test('modal closes', async ({ page }) => {
const modal = page.getByRole('dialog');
await page.getByRole('button', { name: 'Close' }).click();
await expect(modal).toBeHidden({ timeout: 10_000 });
});Timeout defaults: Playwright’s default expect timeout is 5,000ms, configurable globally via expect: { timeout } in playwright.config. [2]
Source: Playwright LocatorAssertions.toBeHidden API reference. [1]
References
- [1] Playwright docs —
LocatorAssertions.toBeHiddenAPI: https://playwright.dev/docs/next/api/class-locatorassertions - [2] Playwright docs — Test timeouts / expect timeout: https://playwright.dev/docs/test-timeouts
Don't swallow dialog-close failures in cancel-flow validation.
Line 365 ignores timeout errors with .catch(() => {}), allowing the test to pass even if the dialog never closes. This masks potential regressions in the cancel flow.
🛠️ Proposed fix
- await page.waitForSelector('[role="dialog"]', { state: 'hidden', timeout: 5000 }).catch(() => {});
+ await expect(page.getByRole('dialog')).toBeHidden({ timeout: 5000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/collaboration-undo-clear.spec.ts` at line 365, The test currently
swallows failures by appending .catch(() => {}) to
page.waitForSelector('[role="dialog"]', { state: 'hidden', timeout: 5000 });
remove the silent catch so the wait throws on timeout, or replace it with an
explicit assert that fails with a clear message when the dialog does not close
(use page.waitForSelector with state: 'hidden' and let the error propagate, or
catch and rethrow a new Error mentioning the cancel-flow and dialog selector);
update the invocation of page.waitForSelector to either be awaited without
.catch or wrapped to throw a descriptive error so the test fails if the dialog
never closes.
Comment on lines
+79
to
+82
| } finally { | ||
| await context1.close(); | ||
| await context2.close(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "collaboration.test.ts" --exec head -20 {}Repository: Kripu77/thinkix
Length of output: 1426
🏁 Script executed:
fd -t f "browser.ts" --path "*/helpers/*" --exec head -30 {}Repository: Kripu77/thinkix
Length of output: 292
🏁 Script executed:
fd -t f "collaboration.test.ts" -x wc -l {}Repository: Kripu77/thinkix
Length of output: 149
🏁 Script executed:
sed -n '75,85p' tests/e2e/collaboration.test.tsRepository: Kripu77/thinkix
Length of output: 420
🏁 Script executed:
fd -t f "browser.ts" tests/Repository: Kripu77/thinkix
Length of output: 87
🏁 Script executed:
sed -n '1,15p' tests/e2e/collaboration.test.tsRepository: Kripu77/thinkix
Length of output: 536
🏁 Script executed:
cat -n tests/e2e/helpers/browser.tsRepository: Kripu77/thinkix
Length of output: 429
🏁 Script executed:
rg "safeClose" tests/Repository: Kripu77/thinkix
Length of output: 1242
Use resilient teardown in finally to avoid masking root failures.
On lines 80–81, if context1.close() throws, context2.close() is skipped and the teardown error can override the actual test failure. Use the existing safeClose helper which properly handles errors from both contexts.
🛠️ Proposed fix
import { test, expect } from '@playwright/test';
+import { safeClose } from './helpers/browser';
@@
} finally {
- await context1.close();
- await context2.close();
+ await safeClose(context1, context2);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } finally { | |
| await context1.close(); | |
| await context2.close(); | |
| } | |
| import { test, expect } from '@playwright/test'; | |
| import { safeClose } from './helpers/browser'; | |
| } finally { | |
| await safeClose(context1, context2); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/collaboration.test.ts` around lines 79 - 82, The finally block
currently awaits context1.close() then context2.close(), which lets the first
thrown error skip closing the second and can mask test failures; replace both
direct closes with the resilient helper (call safeClose(context1) and
safeClose(context2)) so each context is attempted to close independently and
errors are handled by safeClose, ensuring the second close still runs even if
the first fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests