Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a collaborative Canvas sidebar item type with Convex schema, create/update mutations, Yjs document support (notes+canvases as Yjs documents), preview claiming/uploading and metadata fields on sidebar items, a full React Flow-based canvas UI (nodes, hooks, stores, utilities), and test/factory updates; removes the old note block update API in favor of Yjs-driven persistence. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Canvas UI
participant Convex as Convex Backend
participant Yjs as Yjs storage
participant Storage as Blob Storage
User->>UI: createCanvas(name,parent)
UI->>Convex: api.canvases.mutations.createCanvas(args)
Convex->>Convex: validate, generate slug, insert canvases row
Convex->>Yjs: createYjsDocument({ documentId, initialState })
Yjs-->>Convex: persist snapshot row
Convex-->>UI: { canvasId, slug }
UI->>Yjs: start Yjs collaboration (pushUpdate/pushAwareness)
UI->>Convex: claimPreviewGeneration(canvasId)
Convex->>Convex: check locks/cooldown → patch previewLockedUntil if granted
UI->>Storage: upload preview blob (via upload URL)
UI->>Convex: setPreviewImage(canvasId, previewStorageId)
Convex->>Convex: patch canvas preview fields
sequenceDiagram
participant Local as Local client
participant Provider as ConvexYjsProvider
participant Convex as Backend (yjsUpdates/yjsAwareness)
participant Remote as Remote client
Local->>Provider: local Yjs update
Provider->>Convex: pushUpdate(documentId, update)
Convex->>Convex: store update row
Remote->>Convex: getUpdates(documentId, afterSeq)
Convex-->>Remote: return updates
Remote->>Remote: apply updates to local Y.Doc
Local->>Provider: awareness change
Provider->>Convex: pushAwareness(documentId, state)
Convex->>Convex: store awareness row
Remote->>Convex: getAwareness(documentId)
Convex-->>Remote: return awareness states
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
wizard-archive | b8dbdb9 | Commit Preview URL Branch Preview URL |
Apr 06 2026, 10:01 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 54
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
convex/sidebarItems/functions/hardDeleteItem.ts (1)
41-43:⚠️ Potential issue | 🟠 MajorCanvas Yjs documents are not cleaned up on hard delete.
With canvases now using Yjs, restricting cleanup to notes leaves orphanedyjsUpdates/yjsAwarenessrows after canvas deletion.Proposed fix
- if (item.type === SIDEBAR_ITEM_TYPES.notes) { + if ( + item.type === SIDEBAR_ITEM_TYPES.notes || + item.type === SIDEBAR_ITEM_TYPES.canvases + ) { await deleteYjsDocument(ctx, item._id) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/sidebarItems/functions/hardDeleteItem.ts` around lines 41 - 43, The current hardDeleteItem logic only calls deleteYjsDocument for SIDEBAR_ITEM_TYPES.notes, leaving Yjs rows orphaned for canvases; update the conditional in hardDeleteItem (the block that currently checks item.type === SIDEBAR_ITEM_TYPES.notes) to also call deleteYjsDocument for canvas items (e.g., item.type === SIDEBAR_ITEM_TYPES.canvas) — either expand the condition to include canvas or use a set of types (notes | canvas) and invoke deleteYjsDocument(ctx, item._id) for those types so yjsUpdates/yjsAwareness rows are removed on canvas hard-delete.src/features/editor/providers/convex-yjs-provider.ts (2)
201-231:⚠️ Potential issue | 🟠 MajorPending updates can be lost during
destroy()if a push is in-flight.When
pushInFlightis true,flushUpdates()returns immediately without waiting for the current push to complete. Indestroy(), this means:
destroyed = trueis setawait this.flushUpdates()returns immediately (line 203)- The in-flight push eventually completes
finallyblock (line 229) callsscheduleFlush()if there are pending updatesscheduleFlush()(line 234) returns early becausethis.destroyedis trueAny updates accumulated in
pendingUpdatesthat weren't merged into the in-flight batch will be lost.Proposed fix: track and await the in-flight promise
private pushInFlight = false + private inflightPromise: Promise<void> | null = null private awarenessTimer: ReturnType<typeof setTimeout> | null = nullprivate flushUpdates(): Promise<void> { this.clearUpdateTimers() - if (this.pushInFlight) return Promise.resolve() + if (this.pushInFlight) return this.inflightPromise ?? Promise.resolve() if (this.pendingUpdates.length === 0) return Promise.resolve() const merged = this.pendingUpdates.length === 1 ? this.pendingUpdates[0] : Y.mergeUpdates(this.pendingUpdates) this.pendingUpdates = [] this.pushInFlight = true - return this.config + this.inflightPromise = this.config .pushUpdate({ documentId: this.documentId, update: uint8ToArrayBuffer(merged), }) .then(({ seq }) => { if (seq > this.lastAppliedSeq) this.lastAppliedSeq = seq }) .catch((err: unknown) => { console.error('[YJS] push failed for', this.documentId, err) if (this._writable && !this.destroyed) { this.pendingUpdates.unshift(merged) } }) .finally(() => { this.pushInFlight = false + this.inflightPromise = null if (this.pendingUpdates.length > 0) this.scheduleFlush() }) + return this.inflightPromise }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/providers/convex-yjs-provider.ts` around lines 201 - 231, flushUpdates currently returns immediately when this.pushInFlight is true, causing in-flight pushes to complete after destroy() sets destroyed=true and then drop any pendingUpdates; change flushUpdates to track the active push promise (e.g., add a private pushPromise: Promise<void> field), set pushPromise when starting the config.pushUpdate() chain and clear it in finally, and when pushInFlight is true return await this.pushPromise instead of Promise.resolve() so callers (like destroy which calls await flushUpdates()) will wait for the in-flight push to finish; keep existing behavior of re-queuing merged in catch and scheduling flush in finally, and ensure pushInFlight is still set/cleared around the pushPromise lifecycle.
261-261:⚠️ Potential issue | 🟡 MinorComment says "debouncing" but implementation uses throttling.
The awareness handling was changed to a throttle pattern (immediate flush, then rate-limited subsequent flushes). Update the comment to match.
- // -- Awareness debouncing -- + // -- Awareness throttling --🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/providers/convex-yjs-provider.ts` at line 261, The comment "// -- Awareness debouncing --" is incorrect because the code uses a throttle pattern (immediate flush then rate-limited subsequent flushes); update that comment to reflect throttling (e.g., "// -- Awareness throttling --") and, if there are any nearby explanatory comments around the awareness update handler, clarify that the implementation performs an immediate flush followed by rate-limited flushes to match the actual behavior of the awareness update logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/files/functions/updateFile.ts`:
- Around line 50-58: In updateFile, when a new storageId exists but the fetched
metadata is not an image the code currently leaves updates.previewStorageId
unchanged, causing stale previews to persist; modify the branch that runs when
storageId is truthy (where ctx.db.system.get(storageId) is called and metadata
is inspected) so that if metadata?.contentType?.startsWith('image/') is false
you explicitly set updates.previewStorageId = null and clear
updates.previewUpdatedAt (e.g., set to null or undefined) to remove any old
preview; keep the existing behavior for image types.
In `@convex/folders/functions/getFolderContentsForDownload.ts`:
- Around line 119-121: The switch in getFolderContentsForDownload currently
drops SIDEBAR_ITEM_TYPES.canvases with an empty case, which returns success but
omits canvases; update the SIDEBAR_ITEM_TYPES.canvases branch in
getFolderContentsForDownload to either (A) implement a minimal canvas export
path (call the canvas export helper or serialize canvas data into the download
bundle) or (B) explicitly surface unsupported items by adding canvases to an
unsupportedItems array or throwing a clear error (e.g., return { success: false,
unsupportedItems: [...] } or throw a meaningful Error) so callers can handle
incomplete exports; modify the switch case for SIDEBAR_ITEM_TYPES.canvases
accordingly and ensure the function's response shape (success/error or
unsupportedItems) is adjusted wherever getFolderContentsForDownload is consumed.
In `@convex/notes/functions/createNote.ts`:
- Around line 78-95: The code is using the internal _headless option and
accessing editor._tiptapEditor.destroy(); change BlockNoteEditor.create usage to
only use documented options (remove _headless) and perform cleanup via the
documented event-listener cleanup functions: capture the cleanup functions
returned by editor.onMount / editor.onUnmount / editor.onChange (or whatever
mount/unmount listener APIs BlockNoteEditor exposes) and call those to teardown
the editor instead of touching editor._tiptapEditor; keep the existing flow of
converting content with blocksToYDoc and encoding via Y.encodeStateAsUpdate
before calling createYjsDocument, but ensure you call the documented listener
cleanup(s) and editor.destroy() (if a public destroy/unmount API exists) after
initialState is produced and before doc.destroy().
In `@convex/notes/mutations.ts`:
- Around line 77-89: The code uses BlockNoteEditor.create and directly touches
internal fields _headless and _tiptapEditor; replace this with BlockNote's
public server API by constructing the editor via ServerBlockNoteEditor from
`@blocknote/server-util` (or a small adapter wrapping ServerBlockNoteEditor) so
yDocToBlocks and saveTopLevelBlocksForNote consume a supported editor instance,
and ensure proper destruction via the public API; if using internals is
unavoidable, add a concise comment explaining why and encapsulate access to
_headless and _tiptapEditor behind an adapter function (with tests) so only that
adapter touches internals.
In `@convex/sidebarItems/__tests__/previewGeneration.test.ts`:
- Around line 433-446: The test assumes the new note is the first item in
results which is brittle; instead have createNote return the created note's id
(capture the noteId from createNote or its response) and then after calling
api.sidebarItems.queries.getSidebarItemsByLocation filter the returned items for
the one matching that noteId (e.g., compare item.entityId or equivalent) and
assert that that matched item's previewUrl is null; update the test to use
createNote, capture noteId, filter items by that id, and assert on the matched
item's previewUrl rather than items[0].
In `@convex/sidebarItems/functions/applyToDependents.ts`:
- Around line 55-68: The current code calls operation(ctx, row) for every item
in yjsUpdates and yjsAwareness via a single Promise.all, which can overload the
system for large canvases; change this to process rows in bounded batches (e.g.,
batchSize = 50): combine or iterate the arrays (yjsUpdates and yjsAwareness),
split them into chunks, and for each chunk run Promise.all on that chunk and
await it before continuing; ensure you still call operation(ctx, row) for each
row and preserve the original ordering/semantics while awaiting between batches
to limit concurrent mutation execution.
In `@convex/sidebarItems/functions/claimPreviewGeneration.ts`:
- Around line 9-10: The COOLDOWN_MS constant uses an unnecessary multiplication
("1 * 60_000") which is redundant and inconsistent with LEASE_DURATION_MS;
change COOLDOWN_MS to directly use the numeric literal "60_000" (i.e., set
COOLDOWN_MS = 60_000) so both constants follow the same style and remove the
redundant multiplication.
In `@convex/yjsSync/__tests__/yjsSyncHelpers.test.ts`:
- Around line 94-99: Add a parallel test that mirrors the existing
duplicate-creation checks but uses a canvas documentId to exercise the Yjs ID
union path: duplicate the two await createYjsDocument(dbCtx, { documentId:
noteId }) calls and replace noteId with a canvasId (e.g., a different constant
like canvasId) so the test suite validates duplication behavior for canvas
documents as well; ensure the new canvasId is initialized similarly to noteId
and the test names/descriptions indicate it's for the canvas documentId path.
In `@src/features/canvas/components/canvas-color-panel.tsx`:
- Around line 86-92: The effect is re-running because colorRelevantNodes gets a
new array reference each render; stabilize it by memoizing colorRelevantNodes
(e.g., with useMemo) so it only changes when its true inputs change (likely
selectedNodes or whatever source drives it). Update the component to compute
colorRelevantNodes via useMemo and keep the existing useEffect that reads
hasColorSelection, getSelectionColor, getSelectionOpacity, setStrokeColor, and
setStrokeOpacity so the effect only fires when the actual selection changes.
In `@src/features/canvas/components/canvas-remote-cursors.tsx`:
- Around line 131-151: NameLabel creates a new inline style object on every
render which can cause unnecessary re-renders; fix by extracting the static
style properties into a shared constant (e.g., BASE_NAME_LABEL_STYLE) and
composing it with the dynamic properties (backgroundColor and color computed via
getContrastColor(color)) inside NameLabel, and/or wrap the component export with
React.memo(NameLabel) to prevent re-renders when props haven't changed; update
any uses in canvas-remote-selections.tsx to import the memoized/optimized
NameLabel.
- Around line 59-71: The code currently mutates refs and reads DOM style during
render (pinnedRef.current, wasDraggingRef.current,
elementRef.current.style.transform, lerpRef.current) which must be moved into a
useLayoutEffect; create a useLayoutEffect hook that depends on [isDragging,
pinnedPosition] (and elementRef if needed), set pinnedRef.current =
pinnedPosition and perform the drag-start detection: if (isDragging &&
!wasDraggingRef.current && elementRef.current) parse
elementRef.current.style.transform to compute lerpRef.current with from and
startTime, then update wasDraggingRef.current = isDragging at the end of the
effect so all DOM reads/writes happen after render but before paint.
In `@src/features/canvas/components/canvas-toolbar.tsx`:
- Around line 164-180: The stroke size button elements lack an explicit type and
thus default to type="submit"; update the button rendered in the strokes map
(the element that reads onClick={() => setStrokeSize(size)} and uses strokeSize
to determine background) to include type="button" so clicking a size won't
trigger form submission if the toolbar is placed inside a form.
In `@src/features/canvas/components/canvas-viewer.tsx`:
- Around line 246-251: The useEffect that queries wrapperRef.current for
'.react-flow' runs on every render because it lacks a dependency array; update
the effect attached to useEffect to only run when needed (e.g., add an empty
dependency array [] if the element is stable) or replace this approach with a
callback ref pattern that sets canvasContainerRef.current when the element
mounts/changes, or install a MutationObserver inside the effect (with proper
cleanup) to track dynamic additions; target the useEffect, wrapperRef,
canvasContainerRef, and the '.react-flow' query when making the change.
In `@src/features/canvas/components/nodes/embed-content/embed-file-content.tsx`:
- Around line 21-24: The image preview uses alt="" which is inaccessible; update
the img in the EmbedFileContent component (the img with src={downloadUrl}) to
provide meaningful alt text by using the file's descriptive property (e.g.,
attachment.name, fileName, node.label or a passed-in title prop) and fall back
to a generic phrase like "Embedded image" if no name is available; ensure the
chosen property is used consistently so screen readers receive useful context.
In `@src/features/canvas/components/nodes/embed-content/embed-folder-content.tsx`:
- Around line 22-28: The flex container div in the EmbedFolderContent rendering
has a redundant "truncate" class which does nothing for text truncation; remove
the "truncate" class from the outer div (the element rendered with
key={child._id as string} and className that includes "flex items-center gap-1.5
px-1.5 py-1 rounded text-xs") and keep the "truncate" on the inner <span>
({child.name}) so only the span handles overflow truncation.
In `@src/features/canvas/components/nodes/embed-content/embed-map-content.tsx`:
- Line 3: The image currently uses an empty alt which hides map content from
assistive tech; update EmbedMapContent to accept a descriptive label (e.g., add
a prop mapTitle?: string or mapName) and set the <img> alt attribute to that
value with a sensible fallback like "Embedded map" (not an empty string), and
apply the same change to the other embedded map <img> instances referenced
around the block (the images mentioned in lines 14-17) so all map images use
meaningful alt text; also update any callers of EmbedMapContent to pass the map
title where available.
In `@src/features/canvas/components/nodes/embed-content/embed-note-content.tsx`:
- Around line 91-104: Replace the manual string concatenation used for the
wrapper div's className in embed-note-content.tsx with a conditional class
utility (e.g., clsx or a local cn). Specifically, update the className
expression on the top-level div (the one using `h-full${editable ? ' nodrag
nopan' : ''}${selected ? ' nowheel' : ''}`) to call the utility with keys like
'h-full', { 'nodrag nopan': editable, 'nowheel': selected }, and add the
appropriate import for the utility at the top of the file (e.g., import cn from
'clsx' or your project's cn helper). Ensure you remove the string concatenation
and use the utility consistently so spacing and class toggles are correct.
In `@src/features/canvas/components/nodes/rectangle-node.tsx`:
- Around line 20-23: Clamp the opacity prop before converting to CSS by bounding
it to the 0–100 range and then dividing by 100; update the style block in
rectangle-node.tsx (the object where backgroundColor: color and opacity are set)
to compute something like const safeOpacity = Math.max(0, Math.min(100, opacity
?? 100)) / 100 and use safeOpacity for the CSS opacity to ensure values are
always within 0–1; you can place the clamp inline or extract a small helper
clamp function if preferred.
In `@src/features/canvas/components/nodes/sticky-node.tsx`:
- Around line 86-88: The sticky editor's textarea in StickyNode (the <textarea>
element in sticky-node.tsx) is missing the "nowheel" opt-out class so wheel
events over an overflowing note propagate to the canvas; add the "nowheel" CSS
class to that textarea's className (alongside the existing classes like
"bg-transparent outline-none ...") so useCanvasWheel() will skip this descendant
and scrolling the note with the mouse wheel will not pan/zoom the canvas.
- Around line 92-96: The Escape handler in the onKeyDown for the sticky node
needs to stop the event from bubbling to canvas-level shortcuts; update the
onKeyDown callback (in sticky-node.tsx) to call e.stopPropagation() and
e.preventDefault() before invoking handleBlur(label) so Escape only cancels the
sticky edit and does not trigger outer canvas handlers.
In `@src/features/canvas/components/nodes/stroke-node.tsx`:
- Around line 82-92: The selection overlay redundantly recalculates the SVG path
by calling pointsToPathD twice for the same points/size inputs; wrap those
calculations in a memoized value (e.g., useMemo) keyed on points and size so you
compute the pathD once and reuse it for both the main path and the overlay;
update the JSX in stroke-node.tsx to reference the memoized path (from
pointsToPathD(points, size * 0.3) and the other call) and ensure the dependency
array includes points and size so the cache invalidates correctly.
In `@src/features/canvas/hooks/useCanvasAwareness.ts`:
- Around line 36-41: The effect in useCanvasAwareness leaves stale remoteUsers
when provider becomes null; update the effect to explicitly clear
awarenessRef.current and reset remote users when provider is falsy (e.g., call
setRemoteUsers([]) and set awarenessRef.current = null) so old
cursors/selections are removed; also ensure the cleanup path that unregisters
awareness event listeners (the logic around provider.awareness and any on/ off
handlers) clears remote state when the provider is disposed or changed to avoid
lingering presence.
In `@src/features/canvas/hooks/useCanvasDrawing.ts`:
- Around line 68-94: onPointerUp currently takes no event and never explicitly
releases pointer capture; change its signature to accept the PointerEvent (e.g.,
onPointerUp = useCallback((event: PointerEvent) => { ... }), then call
event.currentTarget?.releasePointerCapture(event.pointerId) (or fallback to
(event.target as Element).releasePointerCapture(event.pointerId)) before
clearing points and calling setLocalDrawing/setAwarenessDrawing; also update the
useCallback dependency array if you add any new references. This ensures pointer
capture set in onPointerDown is always explicitly released.
In `@src/features/canvas/hooks/useCanvasDropTarget.ts`:
- Around line 121-139: The code awaits processDataTransferItems and each
uploadRef.current call while mutating nodesMapRef.current, which can write to
the wrong Yjs document if the user switches canvases; to fix, capture the target
canvas/document id upfront (e.g., read currentCanvasId or similar), then
freeze/lock the target by collecting all created node objects (using
crypto.randomUUID, basePosition and result.id) into a local array during the
async work (inside the loop that calls processDataTransferItems and
uploadRef.current) and, after all uploads complete, verify the current
canvas/document id still matches the captured id and perform a single batched
write to nodesMapRef.current to insert all nodes at once; if the id changed,
discard or re-target the nodes appropriately.
In `@src/features/canvas/hooks/useCanvasEraser.ts`:
- Around line 71-82: onPointerMove currently calls testIntersections() on every
pointer event; wrap that call with RAF throttling: create a rafId ref (e.g.,
rafIdRef) and in onPointerMove schedule testIntersections() via
requestAnimationFrame only if no raf is pending, storing the id; ensure
testIntersections reads trailRef/current as before. Also cancel/clear the
pending RAF in onPointerUp (and reset rafIdRef) to avoid running stale work when
erasingRef or pointer interaction ends. This preserves existing logic around
erasingRef/current, trailRef.current.push(pos) and dependencies (reactFlow,
testIntersections).
- Around line 84-94: The onPointerUp callback uses a non-null assertion on
nodesMap.doc (nodesMap.doc!) which can throw if the Yjs document is not
initialized; update onPointerUp to guard early by returning if nodesMap.doc is
null/undefined (or if nodesMap is not ready) before calling
nodesMap.doc.transact, then proceed to delete IDs from nodesMap, clear
markedRef.current and trailRef.current, and call
useCanvasToolStore.getState().setErasingStrokeIds(new Set()) as before;
reference the onPointerUp function and nodesMap.delete to locate where to add
the null/ready check.
In `@src/features/canvas/hooks/useCanvasFileUpload.tsx`:
- Around line 75-107: The code can leave a committed storage blob orphaned if
commitUpload.mutateAsync succeeds but createItem(...) throws; wrap the
createItem + toast logic in a try/catch and on failure perform a compensating
deletion of the committed storage using the existing storage APIs (e.g., call a
deleteCommittedUpload/deleteFileStorage/deleteFile mutation with the storageId
or enqueue a cleanup job) and ensure toast.dismiss(toastId) is run in finally;
reference generateUploadUrl.mutateAsync, uploadFile(...),
trackUpload.mutateAsync, commitUpload.mutateAsync and createItem to locate where
to add the try/catch and compensating delete or cleanup scheduling.
In `@src/features/canvas/hooks/useCanvasHistory.ts`:
- Around line 29-37: syncStore currently captures stale closures because it has
an empty dependency array but passes undo and redo into setHistory while
undo/redo reference syncStore; to fix, stop relying on captured functions—either
store stable function refs (e.g., create undoRef.current and redoRef.current and
pass those to useCanvasToolStore.getState().setHistory) and update those refs
whenever undo/redo are redefined, or remove syncStore entirely and call
useCanvasToolStore.getState().setHistory directly from within the undo and redo
implementations (using undoStackRef.current and redoStackRef.current for
canUndo/canRedo) so setHistory always gets the current behavior; update
references to undo, redo, syncStore, undoStackRef, redoStackRef accordingly.
In `@src/features/canvas/hooks/useCanvasKeyboardShortcuts.ts`:
- Around line 20-27: In useCanvasKeyboardShortcuts, normalize the keyboard event
key before comparisons: read e.key into a local normalized variable (e.g., key =
e.key.toLowerCase()) and use that variable when checking for 'z' and 'y' so
uppercase 'Z' from Shift+Z still matches; update the checks that call undo() and
redo() (and the 'y' branch) to use the normalized key while preserving existing
modifier checks and e.preventDefault() calls.
In `@src/features/canvas/hooks/useCanvasRectangleDraw.ts`:
- Around line 73-74: The size-check uses && so it only rejects rectangles when
both width and height are below MIN_RECT_SIZE, which allows very thin/tall
rectangles; decide intended behavior and either (a) change the condition in
useCanvasRectangleDraw from `rect.width < MIN_RECT_SIZE && rect.height <
MIN_RECT_SIZE` to `rect.width < MIN_RECT_SIZE || rect.height < MIN_RECT_SIZE` to
reject any rectangle with a small dimension, or (b) keep the && and add a
clarifying comment next to the `rectFromPoints`/validation line explaining that
thin/tall shapes are intentionally allowed.
- Around line 22-35: The onPointerDown handler (onPointerDown) should ignore
non-left clicks like onPointerMove does; add a check at the top of onPointerDown
to return early when e.button !== 0 so only left-button presses proceed, then
perform pointer capture, set activeRef.current, compute pos via
reactFlow.screenToFlowPosition, set startRef.current and lastClientPos.current,
and call useCanvasToolStore.getState().setSelectionRect(null); this mirrors the
left-button guard used in useCanvasEraser and ensures right/middle clicks don't
start drawing.
In `@src/features/canvas/hooks/useCanvasSelectionRect.ts`:
- Around line 59-80: Duplicate logic computes adjusted stroke points inside the
reactFlow.setNodes mapping (used when filtering selected 'stroke' nodes) and
again in the RAF callback; extract that into a helper named
getAdjustedStrokePoints(n: Node, strokeData: StrokeNodeData): Array<[number,
number, number]> that computes offsetX/offsetY from n.position and
strokeData.bounds and returns strokeData.points mapped to [x + offsetX, y +
offsetY, p]; then replace the inline offset/points.map blocks in the
selection-end reactFlow.setNodes map and in the RAF callback with calls to
getAdjustedStrokePoints and pass the returned adjustedPoints into
strokePathIntersectsRect and any other consumers.
In `@src/features/canvas/hooks/useCanvasSelectionSync.ts`:
- Around line 36-43: The code in useCanvasSelectionSync
(reactFlowInstance.setNodes) forces draggable to match selection for all nodes
(using selectedSet.has(n.id)), which prevents dragging unselected nodes; either
add a concise inline comment near that block explaining this is an intentional
UX choice (only selected nodes are draggable) or change the update logic so only
selected nodes are set draggable=true and all other nodes are left unchanged
(i.e., update nodes where selectedSet.has(n.id) is true to {...n, draggable:
true} and return n for others) to preserve existing draggability for unselected
nodes; reference reactFlowInstance.setNodes and selectedSet to locate the code.
In `@src/features/canvas/hooks/useNodeEditing.ts`:
- Around line 15-19: startEditing doesn't reset the commit flag, so a previous
Escape-cancelled session (shouldCommitRef.current = false) can leak into the
next session; update startEditing (and the other start-edit handler around the
43-45 region) to set shouldCommitRef.current = true when beginning an edit
session so blurs behave correctly and commits aren't incorrectly dropped. Ensure
you modify the startEditing callback and the analogous handler to reinitialize
shouldCommitRef.current to true right before calling setIsEditing(true).
In `@src/features/canvas/utils/canvas-stroke-utils.ts`:
- Around line 87-103: segmentsIntersect currently rejects all near-parallel
cases by returning false when denom ≈ 0, which misses collinear overlapping
segments; update segmentsIntersect to handle collinear/parallel cases by
computing orientations and using an on-segment check (e.g., orientation(A,B,C),
orientation(A,B,D), orientation(C,D,A), orientation(C,D,B)) and if orientations
indicate collinearity, test whether any endpoint lies on the other segment
(onSegment check using bounding-box comparisons of ax/bx/… and ay/by/…); keep
the existing t/u intersection logic for non-parallel cases (denom not ≈ 0) but
replace the early return with the orientation + on-segment handling to detect
overlapping/edge-aligned intersections.
In
`@src/features/editor/components/extensions/selection-toolbar/selection-toolbar.tsx`:
- Line 16: The inline comment "// TODO: fix this" in selection-toolbar.tsx is
ambiguous—replace it with a clear, actionable note that either links to the
tracking ticket or describes the missing/incorrect behavior and the verification
steps; for example, in the SelectionToolbar component (or near any related
function such as renderToolbar / handleSelectionChange) state what specifically
is broken, how to reproduce it, the expected outcome, and acceptance criteria
(e.g., "See ISSUE-1234: toolbar fails to position on multi-line selection —
reproduce by selecting across lines A/B, expect toolbar to appear above
selection"); keep the comment concise and stable so future reviewers can find
the ticket or tests to validate the fix.
In `@src/features/editor/components/forms/file-form/file-form.tsx`:
- Around line 182-187: The create path currently awaits
generatePdfPreviewIfNeeded(fileUpload.file, newFileId) which, if it throws, will
block navigation and success UI even though the file was created; wrap that call
in a try/catch inside the create flow in the FileForm component (the same place
that handles newFileId after creation), log or report the error (e.g., via
processLogger or UI toast) and swallow it so navigation and success feedback
still occur when the file creation succeeded.
- Around line 164-166: The await on generatePdfPreviewIfNeeded(fileUpload.file,
fileId) blocks the success flow if preview generation fails; make preview
generation non-blocking by either wrapping the call in a try-catch that logs
errors (so onSuccess and the success toast still run) or fire-and-forget it
(call generatePdfPreviewIfNeeded(fileUpload.file, fileId) without await and
attach .catch to log failures). Update the code around the fileUpload/fileId
handling and ensure onSuccess and the success toast are executed regardless of
preview errors.
In `@src/features/editor/components/note-content.tsx`:
- Around line 191-205: Add an explanatory comment above the RAF retry loop (the
MAX_RETRIES / tryPatch logic) describing that BlockNote initializes Tiptap
plugins asynchronously and we need to wait for
instance._tiptapEditor.view.state.plugins to be non-empty before calling
patchYUndoPluginDestroy and patchYSyncAfterTypeChanged; note the chosen
MAX_RETRIES (30) as a safety cap to avoid infinite RAF looping and mention that
retries use requestAnimationFrame to poll until plugins are ready or the cap is
reached.
- Around line 142-146: The useEffect currently skips calling
editor.replaceBlocks when content.length === 0, so clearing content doesn't
propagate; update the effect to run whenever editor or content changes (keep
useEffect dependencies) and call editor.replaceBlocks(editor.document, content)
even when content is empty — or normalize empty content to the expected
empty-block payload (e.g., [] or "") before calling; modify the effect around
useEffect/replaceBlocks to remove the content.length > 0 guard and ensure empty
content is converted to the editor's empty value so the editor is cleared.
In `@src/features/editor/components/viewer/folder/canvas-card.tsx`:
- Around line 98-108: The img rendering branch using canvas.previewUrl should
enable browser lazy-loading to improve folder view performance; update the <img>
element that references canvas.previewUrl (in the canvas card component) to
include loading="lazy" (and consider preserving existing attributes like src,
alt, className) so previews load only as they scroll into view.
In `@src/features/editor/components/viewer/folder/file-card.tsx`:
- Around line 143-151: The image preview can break if previewUrl is stale; add a
local state (e.g., hasPreviewError via useState) in the FileCard component and
use it to conditionally render the <img> only when file.previewUrl is present
and hasPreviewError is false, and attach an onError handler to the img to set
hasPreviewError(true) so the component falls back to rendering the FileIcon;
update any initial state if you want to optimistically show the preview when
previewUrl exists and ensure the fallback logic uses the same FileIcon className
so appearance stays consistent.
In `@src/features/editor/hooks/useYjsReactFlowSync.ts`:
- Around line 95-160: The RAF loop in the useEffect's animate function never
stops; change animate to only call requestAnimationFrame when there are active
targets or springs (check remoteDragRef.current keys length,
springStates.current.size, or springActiveIds.current.size) and
cancelAnimationFrame/clear rafId when none remain so the loop goes idle; also
ensure the effect restarts the loop when remote drag state changes by triggering
the effect (e.g., add the remote-drag state variable that backs remoteDragRef to
the useEffect dependencies or call a restart helper when remoteDragRef is
updated) so requestAnimationFrame is re-queued when new targets appear
(referencing animate, rafId, requestAnimationFrame, cancelAnimationFrame,
remoteDragRef, springStates, springActiveIds).
In `@src/features/previews/hooks/use-claim-and-upload-preview.ts`:
- Around line 42-44: In the catch block inside useClaimAndUploadPreview (the
function in use-claim-and-upload-preview.ts) enhance the logger.error call to
include the itemId (and optional contextual info like preview type or upload
target) along with the caught error so logs show which item failed; update the
catch to log a descriptive message such as "Failed to claim/upload preview for
item" plus itemId and the error object (and keep returning false) to aid
debugging.
In `@src/features/previews/hooks/use-pdf-preview-upload.ts`:
- Around line 6-8: The isPdfFile function currently checks
file.name.endsWith('.pdf') which misses uppercase extensions; change the
extension check to be case-insensitive (for example, compare
file.name.toLowerCase().endsWith('.pdf') or use a case-insensitive regex) so
that isPdfFile(File) returns true for names ending in .pdf, .PDF, or mixed-case
variants while keeping the existing MIME-type check (file.type ===
'application/pdf').
In `@src/features/previews/utils/__tests__/upload-preview.test.ts`:
- Around line 54-61: The test suite is stubbing the global fetch with
vi.stubGlobal('fetch') inside a test which overrides the beforeEach stub and
isn't cleaned up; add an afterEach cleanup to restore globals so later tests
aren't affected — e.g. add afterEach(() => { vi.unstubAllGlobals();
vi.restoreAllMocks(); }) to this test file so any vi.stubGlobal('fetch') or mock
changes made in the test are undone.
- Around line 17-30: The test setup stubs the global fetch via vi.stubGlobal but
never cleans it up, risking test pollution; add an afterEach that calls
vi.unstubAllGlobals() to remove the global fetch stub (and also call
vi.restoreAllMocks() or vi.resetAllMocks() to reset mockGenerateUploadUrl and
mockSetPreviewImage) so the stubbed global and mocks are restored between tests;
locate the setup using mockGenerateUploadUrl, mockSetPreviewImage, and the
vi.stubGlobal('fetch') call and add the cleanup there.
In `@src/features/previews/utils/generate-preview.ts`:
- Around line 44-49: The canvas height is being capped against
PDF_PREVIEW_HEIGHT multiplied by scale, which double-scales the limit; update
the logic in generate-preview.ts around the scale/viewport calculation
(variables: scale, viewport, PDF_PREVIEW_WIDTH, PDF_PREVIEW_HEIGHT,
page.getViewport) to either (A) cap canvas.height against the unscaled pixel
limit by using Math.min(viewport.height, PDF_PREVIEW_HEIGHT) so the preview
height is in device pixels, or (B) compute a new scale that respects both width
and height constraints (e.g., heightScale = PDF_PREVIEW_HEIGHT /
page.getViewport({ scale: 1 }).height; scale = Math.min(widthScale,
heightScale); then call page.getViewport({ scale }) and set canvas.width/height
to viewport.width/height) so the aspect ratio is preserved and the preview fits
both max width and max height.
- Around line 33-37: The current dynamic worker path using new URL(...) is
inconsistent with the project's Vite pattern; import the worker as a URL using
the Vite idiom (e.g., add an import like pdfjsWorkerUrl from
'pdfjs-dist/build/pdf.worker.min.mjs?url' at the top of generate-preview.ts) and
then set pdfjsLib.GlobalWorkerOptions.workerSrc = pdfjsWorkerUrl instead of
building the URL via import.meta.url; update references in the file (pdfjsLib
and pdfjsLib.GlobalWorkerOptions.workerSrc) to use that imported pdfjsWorkerUrl
for consistency with pdf-file-viewer.tsx.
In `@src/features/previews/utils/upload-preview.ts`:
- Around line 25-27: The code currently force-casts the JSON body to {
storageId: Id<'_storage'> } which can mask malformed responses; replace the
direct assertion by parsing the JSON into a temporary value (e.g., const body =
await response.json()), validate that body is an object and has a valid
storageId (e.g., typeof body.storageId === 'string' or whatever Id<'_storage'>
expects), and only then assign storageId from the validated body; if validation
fails, throw or handle an explicit error so callers don’t operate on undefined
storageId (refer to the storageId variable and the response.json() usage in
upload-preview.ts).
In `@src/features/sidebar/components/sidebar-layout.tsx`:
- Around line 149-159: The width-calculation logic (computing displayWidth,
contentWidth, totalDisplay, totalContent and applying them to
outerRef.innerRef.handleRef) is duplicated in the mousemove and mouseup paths;
extract this into a single helper function (e.g., computeAndApplyWidths or
updateSidebarWidths) that accepts finalWidth and startWidth (or derives them
like the existing code) and performs the displayWidth/contentWidth/total* math
and DOM style updates, then call that helper from both handleMouseMove and
handleMouseUp to avoid drift and maintenance issues.
In `@src/shared/hooks/useSpringPosition.ts`:
- Around line 69-109: The RAF loop currently always schedules another frame in
animate, causing perpetual 60fps even when targetRef.current is null or the
spring has settled; update animate (and the surrounding effect) so it stops
calling requestAnimationFrame when targetRef.current is null or when the spring
is idle (detect idle by comparing state.pos to the target and checking vel is
near zero / within a small epsilon after calling stepSpring), canceling/clearing
rafId and resetting prevTimeRef/posRef/velRef as needed; only re-start the RAF
when a new non-null target appears (e.g., trigger requestAnimationFrame again
when targetRef transitions from null to a value). Ensure you reference animate,
requestAnimationFrame/cancelAnimationFrame, stepSpring, targetRef, posRef,
velRef, prevTimeRef, elementRef, optionsRef, and SPRING_DEFAULTS.maxDt when
making the change.
In `@src/shared/utils/color.ts`:
- Around line 1-7: The getContrastColor function must validate and normalize the
hexColor before parsing: in getContrastColor strip the leading '#', expand
3-character hex to 6-character form, ensure the resulting string is exactly 6
hex digits (0-9a-f/A-F), and if validation fails use a safe fallback (e.g.,
return '#000000' or a configured default) instead of proceeding; only then parse
r/g/b and compute luminance to choose '#000000' or '#ffffff'.
In `@src/styles/app.css`:
- Around line 182-184: The reduced-motion override selector currently uses
:not(.allow-motion *) (and ::before/::after variants) which does not exclude the
.allow-motion element itself; update the selector list to also exclude the
element by adding :not(.allow-motion), :not(.allow-motion)::before, and
:not(.allow-motion)::after alongside the existing :not(.allow-motion *) variants
so elements with class ".allow-motion" are not subjected to the
animation-duration override.
---
Outside diff comments:
In `@convex/sidebarItems/functions/hardDeleteItem.ts`:
- Around line 41-43: The current hardDeleteItem logic only calls
deleteYjsDocument for SIDEBAR_ITEM_TYPES.notes, leaving Yjs rows orphaned for
canvases; update the conditional in hardDeleteItem (the block that currently
checks item.type === SIDEBAR_ITEM_TYPES.notes) to also call deleteYjsDocument
for canvas items (e.g., item.type === SIDEBAR_ITEM_TYPES.canvas) — either expand
the condition to include canvas or use a set of types (notes | canvas) and
invoke deleteYjsDocument(ctx, item._id) for those types so
yjsUpdates/yjsAwareness rows are removed on canvas hard-delete.
In `@src/features/editor/providers/convex-yjs-provider.ts`:
- Around line 201-231: flushUpdates currently returns immediately when
this.pushInFlight is true, causing in-flight pushes to complete after destroy()
sets destroyed=true and then drop any pendingUpdates; change flushUpdates to
track the active push promise (e.g., add a private pushPromise: Promise<void>
field), set pushPromise when starting the config.pushUpdate() chain and clear it
in finally, and when pushInFlight is true return await this.pushPromise instead
of Promise.resolve() so callers (like destroy which calls await flushUpdates())
will wait for the in-flight push to finish; keep existing behavior of re-queuing
merged in catch and scheduling flush in finally, and ensure pushInFlight is
still set/cleared around the pushPromise lifecycle.
- Line 261: The comment "// -- Awareness debouncing --" is incorrect because the
code uses a throttle pattern (immediate flush then rate-limited subsequent
flushes); update that comment to reflect throttling (e.g., "// -- Awareness
throttling --") and, if there are any nearby explanatory comments around the
awareness update handler, clarify that the implementation performs an immediate
flush followed by rate-limited flushes to match the actual behavior of the
awareness update logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 585ae647-b5e4-4b67-a273-a773d5312398
⛔ Files ignored due to path filters (2)
convex/_generated/api.d.tsis excluded by!**/_generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (137)
convex/_test/factories.helper.tsconvex/canvases/baseSchema.tsconvex/canvases/functions/createCanvas.tsconvex/canvases/functions/enhanceCanvas.tsconvex/canvases/functions/updateCanvas.tsconvex/canvases/mutations.tsconvex/canvases/schema.tsconvex/canvases/types.tsconvex/files/functions/createFile.tsconvex/files/functions/updateFile.tsconvex/folders/functions/createFolder.tsconvex/folders/functions/getFolderContentsForDownload.tsconvex/gameMaps/__tests__/gameMaps.test.tsconvex/gameMaps/functions/createMap.tsconvex/gameMaps/functions/updateMap.tsconvex/notes/__tests__/noteContentBlockShareConsistency.test.tsconvex/notes/__tests__/noteWorkflows.test.tsconvex/notes/__tests__/notes.test.tsconvex/notes/functions/createNote.tsconvex/notes/functions/updateNoteContent.tsconvex/notes/mutations.tsconvex/schema.tsconvex/sidebarItems/__tests__/previewCleanup.test.tsconvex/sidebarItems/__tests__/previewGeneration.test.tsconvex/sidebarItems/functions/applyToDependents.tsconvex/sidebarItems/functions/claimPreviewGeneration.tsconvex/sidebarItems/functions/defaultItemName.tsconvex/sidebarItems/functions/enhanceSidebarItem.tsconvex/sidebarItems/functions/fetchCampaignSidebarItems.tsconvex/sidebarItems/functions/getSidebarItemById.tsconvex/sidebarItems/functions/getSidebarItemBySlug.tsconvex/sidebarItems/functions/hardDeleteItem.tsconvex/sidebarItems/functions/setPreviewImage.tsconvex/sidebarItems/mutations.tsconvex/sidebarItems/schema/baseFields.tsconvex/sidebarItems/schema/baseValidators.tsconvex/sidebarItems/schema/contentSchema.tsconvex/sidebarItems/schema/schema.tsconvex/sidebarItems/types/baseTypes.tsconvex/sidebarItems/types/types.tsconvex/yjsSync/__tests__/yjsSyncHelpers.test.tsconvex/yjsSync/__tests__/yjsSyncMutations.test.tsconvex/yjsSync/constants.tsconvex/yjsSync/functions/checkYjsAccess.tsconvex/yjsSync/functions/compactUpdates.tsconvex/yjsSync/functions/createYjsDocument.tsconvex/yjsSync/functions/deleteYjsDocument.tsconvex/yjsSync/functions/reconstructYDoc.tsconvex/yjsSync/functions/types.tsconvex/yjsSync/internalMutations.tsconvex/yjsSync/mutations.tsconvex/yjsSync/queries.tsconvex/yjsSync/schema.tse2e/editor-stress.spec.tseslint.config.jspackage.jsonsrc/features/canvas/components/canvas-color-panel.tsxsrc/features/canvas/components/canvas-minimap-node.tsxsrc/features/canvas/components/canvas-remote-cursors.tsxsrc/features/canvas/components/canvas-remote-selections.tsxsrc/features/canvas/components/canvas-strokes.tsxsrc/features/canvas/components/canvas-toolbar.tsxsrc/features/canvas/components/canvas-viewer.tsxsrc/features/canvas/components/nodes/canvas-node-types.tssrc/features/canvas/components/nodes/embed-content/embed-canvas-content.tsxsrc/features/canvas/components/nodes/embed-content/embed-file-content.tsxsrc/features/canvas/components/nodes/embed-content/embed-folder-content.tsxsrc/features/canvas/components/nodes/embed-content/embed-map-content.tsxsrc/features/canvas/components/nodes/embed-content/embed-note-content.tsxsrc/features/canvas/components/nodes/embed-node.tsxsrc/features/canvas/components/nodes/rectangle-node.tsxsrc/features/canvas/components/nodes/resizable-node-wrapper.tsxsrc/features/canvas/components/nodes/sticky-node-constants.tssrc/features/canvas/components/nodes/sticky-node.tsxsrc/features/canvas/components/nodes/stroke-node.tsxsrc/features/canvas/components/nodes/text-node.tsxsrc/features/canvas/hooks/useCanvasAwareness.tssrc/features/canvas/hooks/useCanvasDrawing.tssrc/features/canvas/hooks/useCanvasDropTarget.tssrc/features/canvas/hooks/useCanvasEraser.tssrc/features/canvas/hooks/useCanvasFileUpload.tsxsrc/features/canvas/hooks/useCanvasHistory.tssrc/features/canvas/hooks/useCanvasKeyboardShortcuts.tssrc/features/canvas/hooks/useCanvasLassoSelection.tssrc/features/canvas/hooks/useCanvasOverlayHandlers.tssrc/features/canvas/hooks/useCanvasRectangleDraw.tssrc/features/canvas/hooks/useCanvasSelectionRect.tssrc/features/canvas/hooks/useCanvasSelectionSync.tssrc/features/canvas/hooks/useCanvasWheel.tssrc/features/canvas/hooks/useEmbedItemContent.tssrc/features/canvas/hooks/useNodeEditing.tssrc/features/canvas/index.tssrc/features/canvas/stores/canvas-tool-store.tssrc/features/canvas/utils/canvas-awareness-types.tssrc/features/canvas/utils/canvas-context.tssrc/features/canvas/utils/canvas-stroke-utils.tssrc/features/context-menu/actions.tsxsrc/features/context-menu/menu-registry.tssrc/features/dnd/hooks/useFileDropHandler.tsxsrc/features/dnd/hooks/useNoteEditorDropTarget.tssrc/features/dnd/utils/dnd-registry.tssrc/features/editor/components/create-new-dashboard.tsxsrc/features/editor/components/extensions/selection-toolbar/selection-toolbar.tsxsrc/features/editor/components/forms/file-form/file-form.tsxsrc/features/editor/components/note-content.tsxsrc/features/editor/components/note-view.tsxsrc/features/editor/components/viewer/folder/canvas-card.tsxsrc/features/editor/components/viewer/folder/file-card.tsxsrc/features/editor/components/viewer/folder/item-card.tsxsrc/features/editor/components/viewer/folder/map-card.tsxsrc/features/editor/components/viewer/folder/note-card.tsxsrc/features/editor/components/viewer/note/note-editor.tsxsrc/features/editor/components/viewer/sidebar-item-editor.tsxsrc/features/editor/hooks/__tests__/useConvexYjsCollaboration.test.tssrc/features/editor/hooks/__tests__/useNoteYjsCollaboration.test.tssrc/features/editor/hooks/useConvexYjsCollaboration.tssrc/features/editor/hooks/useNoteYjsCollaboration.tssrc/features/editor/hooks/useYjsReactFlowSync.tssrc/features/editor/providers/__tests__/convex-yjs-provider.test.tssrc/features/editor/providers/convex-yjs-provider.tssrc/features/previews/hooks/use-canvas-preview.tssrc/features/previews/hooks/use-claim-and-upload-preview.tssrc/features/previews/hooks/use-note-preview.tssrc/features/previews/hooks/use-pdf-preview-upload.tssrc/features/previews/utils/__tests__/upload-preview.test.tssrc/features/previews/utils/generate-preview.tssrc/features/previews/utils/upload-preview.tssrc/features/sharing/hooks/useSidebarItemsShare.tssrc/features/sidebar/components/sidebar-layout.tsxsrc/features/sidebar/hooks/useCreateSidebarItem.tssrc/features/sidebar/hooks/useEditSidebarItem.tssrc/features/sidebar/utils/sidebar-item-utils.tssrc/shared/components/color-picker-popover.tsxsrc/shared/hooks/useSpringPosition.tssrc/shared/utils/color.tssrc/styles/app.csssrc/test/factories/sidebar-item-factory.ts
💤 Files with no reviewable changes (6)
- convex/notes/tests/notes.test.ts
- convex/notes/functions/updateNoteContent.ts
- convex/yjsSync/constants.ts
- src/features/editor/hooks/tests/useConvexYjsCollaboration.test.ts
- convex/notes/tests/noteContentBlockShareConsistency.test.ts
- src/features/editor/providers/tests/convex-yjs-provider.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/editor/providers/convex-yjs-provider.ts (1)
146-165:⚠️ Potential issue | 🟠 MajorPotential data loss when destroy() is called during an in-flight push.
If
pushInFlightis true whendestroy()callsflushUpdates(), the method returns the in-flight promise (line 204). When that push completes, itsfinallyblock callsscheduleFlush()(line 230), which returns immediately becausedestroyedis true (line 236). Any updates that accumulated during the in-flight push are lost.🔧 Suggested fix: loop until pending updates are drained
const teardown = async () => { if (this._writable) { - await this.flushUpdates() + // Loop until all pending updates are flushed + while (this.pendingUpdates.length > 0 || this.pushInFlight) { + await this.flushUpdates() + } } await this.flushAwareness()Alternatively, modify
flushUpdates()to handle the destroyed state differently when called directly vs. from a timer, but the loop approach is simpler and more explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/providers/convex-yjs-provider.ts` around lines 146 - 165, The destroy() path can lose updates if a push is in flight because flushUpdates() returns the in-flight promise and scheduleFlush() no-ops when destroyed; fix by looping in destroy() until all pending updates are drained: instead of a single await this.flushUpdates() in the teardown, repeatedly call/await this.flushUpdates() (or check this.pushInFlight and any pending-update buffer/state used by flushUpdates()) until flushUpdates() returns no promise/indicates no work or both pushInFlight is false and pending buffer is empty; keep the rest of teardown (flushAwareness(), removeAwareness) unchanged. Ensure you reference the existing members pushInFlight, destroyed, flushUpdates(), scheduleFlush(), clearAwarenessTimer(), flushAwareness(), and config.removeAwareness when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/editor/providers/convex-yjs-provider.ts`:
- Around line 26-40: Update the ConvexYjsProviderConfig callback signatures to
accept YjsDocumentId instead of string: change the documentId parameter types in
pushUpdate, pushAwareness, and removeAwareness to YjsDocumentId so they match
the class usage; then update any related call sites that pass YjsDocumentId
(e.g., where the provider stores/passes Id<'notes'> | Id<'canvases'>) to satisfy
the new types and ensure imports/exports reference the YjsDocumentId type where
ConvexYjsProviderConfig is defined.
---
Outside diff comments:
In `@src/features/editor/providers/convex-yjs-provider.ts`:
- Around line 146-165: The destroy() path can lose updates if a push is in
flight because flushUpdates() returns the in-flight promise and scheduleFlush()
no-ops when destroyed; fix by looping in destroy() until all pending updates are
drained: instead of a single await this.flushUpdates() in the teardown,
repeatedly call/await this.flushUpdates() (or check this.pushInFlight and any
pending-update buffer/state used by flushUpdates()) until flushUpdates() returns
no promise/indicates no work or both pushInFlight is false and pending buffer is
empty; keep the rest of teardown (flushAwareness(), removeAwareness) unchanged.
Ensure you reference the existing members pushInFlight, destroyed,
flushUpdates(), scheduleFlush(), clearAwarenessTimer(), flushAwareness(), and
config.removeAwareness when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86e5bbfb-4c0f-4d91-baff-daf45c3d9889
📒 Files selected for processing (1)
src/features/editor/providers/convex-yjs-provider.ts
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/features/canvas/components/canvas-color-panel.tsx (1)
58-64:⚠️ Potential issue | 🟠 MajorBatch target can still drift across rapid selection changes.
Line [63] keeps the previous pending
nodeIdswhenever a RAF batch already exists. If selection changes and another color/opacity change is queued before flush, the new change is still applied to the old selection.💡 Proposed fix
const scheduleNodeUpdate = useCallback( - (data: Record<string, unknown>) => { + (nodeIds: Array<string>, data: Record<string, unknown>) => { + if (!nodeIds.length) return + const prev = pendingUpdate.current + const sameTarget = + !!prev && + prev.nodeIds.length === nodeIds.length && + prev.nodeIds.every((id, i) => id === nodeIds[i]) + pendingUpdate.current = { - data: { ...pendingUpdate.current?.data, ...data }, - nodeIds: - pendingUpdate.current?.nodeIds ?? colorRelevantNodes.map((n) => n.id), + nodeIds, + data: sameTarget ? { ...prev!.data, ...data } : data, } if (!rafId.current) { rafId.current = requestAnimationFrame(() => { rafId.current = 0 flushNodeUpdates() }) } }, - [flushNodeUpdates, colorRelevantNodes], + [flushNodeUpdates], ) @@ const handleColorChange = useCallback( (color: string) => { setStrokeColor(color) - if (hasColorSelection) scheduleNodeUpdate({ color }) + if (hasColorSelection) { + scheduleNodeUpdate( + colorRelevantNodes.map((node) => node.id), + { color }, + ) + } }, - [setStrokeColor, scheduleNodeUpdate, hasColorSelection], + [setStrokeColor, scheduleNodeUpdate, hasColorSelection, colorRelevantNodes], ) @@ const handleOpacityChange = useCallback( (opacity: number) => { setStrokeOpacity(opacity) - if (hasColorSelection) scheduleNodeUpdate({ opacity }) + if (hasColorSelection) { + scheduleNodeUpdate( + colorRelevantNodes.map((node) => node.id), + { opacity }, + ) + } }, - [setStrokeOpacity, scheduleNodeUpdate, hasColorSelection], + [setStrokeOpacity, scheduleNodeUpdate, hasColorSelection, colorRelevantNodes], )Also applies to: 83-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/canvas-color-panel.tsx` around lines 58 - 64, scheduleNodeUpdate currently preserves pendingUpdate.current.nodeIds when a RAF batch exists, causing queued changes to apply to an outdated selection; change the assignment inside scheduleNodeUpdate so nodeIds is always set to the current selection (e.g., colorRelevantNodes.map(n => n.id)) instead of using pendingUpdate.current?.nodeIds, and make the same change in the other batching callback at lines 83-97 so every new queued change overwrites nodeIds with the latest selection before the RAF flush.src/features/canvas/hooks/useCanvasFileUpload.tsx (1)
92-117:⚠️ Potential issue | 🟠 MajorCommitted storage can still be orphaned when sidebar item creation fails.
Line 92 commits the upload before
createItem, and the catch at Lines 106-117 only logs/shows an error. This leaves committed blobs without a reachable sidebar reference. Please add compensating cleanup (or deferred commit orchestration) so failedcreateItemdoes not leak storage records.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/hooks/useCanvasFileUpload.tsx` around lines 92 - 117, In useCanvasFileUpload, when commitUpload.mutateAsync({ storageId }) succeeds but createItem(...) fails, the committed blob can be orphaned; add compensating cleanup by invoking the storage deletion/rollback API (e.g., a deleteStorage/deleteBlob or commitUpload.undo/delete method) with the same storageId inside the createItem catch block before returning null, and handle/delete errors (log and optionally toast) so the failure path doesn’t leak storage records; ensure you reference commitUpload, createItem and storageId and preserve the existing error toast flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/auth/component.ts`:
- Around line 49-55: The module currently throws during createAuth module
initialization if process.env.BETTER_AUTH_SECRET is missing, which breaks
preview/analyze; remove the eager check that throws on BETTER_AUTH_SECRET at
top-level and instead defer the validation to runtime by passing a secret getter
or validating inside the auth runtime path (e.g., inside the function that
handles requests or inside the betterAuth consumer) so that createAuth /
betterAuth(...) can be imported during analysis without throwing, but still
raise an error when the secret is actually required at request runtime;
reference the BETTER_AUTH_SECRET env var, the createAuth entry point, and the
betterAuth(...) call to locate where to move the validation.
In `@src/features/canvas/components/canvas-viewer.tsx`:
- Around line 391-394: The code sets nodesDraggable = false while still
supplying onNodeDragStart/onNodeDrag/onNodeDragStop handlers, which is
confusing; either make node dragging explicit or document the intent — add a
concise comment above the nodesDraggable declaration explaining that default
React Flow dragging is disabled intentionally (e.g., because dragging is handled
via selection-based drag, resize controls, or custom handlers) and reference the
handlers onNodeDragStart, onNodeDrag, and onNodeDragStop so future readers
understand why handlers remain while nodesDraggable is false.
- Around line 222-230: The canvas tool hooks are initialized unconditionally
allowing read-only users to trigger mutating handlers; update the code to
respect the canEdit flag by passing canEdit into useCanvasDrawing,
useCanvasEraser, useCanvasLassoSelection, useCanvasRectangleDraw and
useCanvasKeyboardShortcuts (or alternatively gate overlay binding inside
useCanvasOverlayHandlers), and change each hook (e.g., the
onPointerUp/onPointerDown handlers in useCanvasDrawing, useCanvasEraser,
useCanvasRectangleDraw and the undo/redo in useCanvasKeyboardShortcuts) to be
no-ops when canEdit is false; ensure any persisted tool in useCanvasToolStore is
either reset to a non-mutating tool (e.g., 'hand') when canEdit turns false or
that the hooks ignore it so no nodesMap.set/delete calls occur for read-only
users.
In `@src/features/canvas/components/nodes/stroke-node.tsx`:
- Line 43: The opacity handling is inconsistent: opacityOverride is treated as a
0–1 value while data.opacity is treated as 0–100 and divided by 100; update
StrokeNode rendering (the opacity prop set where opacity={opacityOverride ??
(data.opacity ?? 100) / 100}) to use a single 0–1 convention by removing the
0–100 division and treating data.opacity as 0–1, and ensure callers/creators of
stroke data convert/normalize opacity to 0–1 (e.g., when creating/updating
strokes set data.opacity to a 0–1 value) so both opacityOverride and
data.opacity share the same scale.
In `@src/features/canvas/hooks/useCanvasOverlayHandlers.ts`:
- Around line 4-9: The PointerHandlers type declares onPointerCancel but the
effect in useCanvasOverlayHandlers only registers
pointerdown/pointermove/pointerup; add handling for pointercancel by registering
window.addEventListener('pointercancel', handlers.onPointerCancel) (or a wrapper
that calls handlers.onPointerCancel if defined) alongside the other listeners,
and ensure you remove the 'pointercancel' listener in the same cleanup to
prevent stale state. Locate the effect that registers pointer event listeners in
useCanvasOverlayHandlers and mirror the pattern used for
onPointerUp/onPointerMove to wire up and tear down onPointerCancel.
In `@src/features/canvas/hooks/useCanvasWheel.ts`:
- Around line 12-54: The effect captures ref.current but doesn't depend on it,
which can leave listeners on an old element; update the useEffect dependencies
to include ref.current (or ref) alongside reactFlowInstance and ensure the
current element is captured into a local const (already done as const el =
ref.current) so the cleanup removes the listener from that specific element
(keep handler as defined). In short: add ref.current to the dependency array of
the useEffect that defines handler and uses reactFlowInstance, so the effect
re-runs and cleans up when the ref target changes.
---
Duplicate comments:
In `@src/features/canvas/components/canvas-color-panel.tsx`:
- Around line 58-64: scheduleNodeUpdate currently preserves
pendingUpdate.current.nodeIds when a RAF batch exists, causing queued changes to
apply to an outdated selection; change the assignment inside scheduleNodeUpdate
so nodeIds is always set to the current selection (e.g.,
colorRelevantNodes.map(n => n.id)) instead of using
pendingUpdate.current?.nodeIds, and make the same change in the other batching
callback at lines 83-97 so every new queued change overwrites nodeIds with the
latest selection before the RAF flush.
In `@src/features/canvas/hooks/useCanvasFileUpload.tsx`:
- Around line 92-117: In useCanvasFileUpload, when commitUpload.mutateAsync({
storageId }) succeeds but createItem(...) fails, the committed blob can be
orphaned; add compensating cleanup by invoking the storage deletion/rollback API
(e.g., a deleteStorage/deleteBlob or commitUpload.undo/delete method) with the
same storageId inside the createItem catch block before returning null, and
handle/delete errors (log and optionally toast) so the failure path doesn’t leak
storage records; ensure you reference commitUpload, createItem and storageId and
preserve the existing error toast flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5620ce80-bdb6-49e4-8d79-0e84675f90d7
📒 Files selected for processing (19)
convex/auth/component.tssrc/features/canvas/components/canvas-color-panel.tsxsrc/features/canvas/components/canvas-remote-cursors.tsxsrc/features/canvas/components/canvas-viewer.tsxsrc/features/canvas/components/nodes/resizable-node-wrapper.tsxsrc/features/canvas/components/nodes/stroke-node.tsxsrc/features/canvas/hooks/useCanvasEraser.tssrc/features/canvas/hooks/useCanvasFileUpload.tsxsrc/features/canvas/hooks/useCanvasOverlayHandlers.tssrc/features/canvas/hooks/useCanvasSelectionRect.tssrc/features/canvas/hooks/useCanvasStrokeClick.tssrc/features/canvas/hooks/useCanvasWheel.tssrc/features/canvas/hooks/useNodeEditing.tssrc/features/canvas/utils/canvas-stroke-utils.tssrc/features/editor/components/forms/file-form/file-form.tsxsrc/features/editor/components/viewer/folder/note-card.tsxsrc/features/editor/hooks/useYjsReactFlowSync.tssrc/features/previews/utils/upload-preview.tssrc/styles/app.css
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/features/sidebar/utils/sidebar-item-utils.ts (1)
59-60:⚠️ Potential issue | 🟡 MinorMalformed JSDoc comment: duplicate opening markers.
Lines 59-60 have two
/**comment opening markers in a row, which breaks the JSDoc block and leaves an orphaned comment line.Proposed fix
-/** /** * Type guard to check if a sidebar item is a SidebarFile. */Remove the extra
/**on line 59.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sidebar/utils/sidebar-item-utils.ts` around lines 59 - 60, There are duplicate JSDoc opening markers (`/**` twice) that break the comment block; edit the JSDoc block immediately above the exported symbol (the comment before the sidebar item helper in sidebar-item-utils.ts) and remove the extra `/**` so the block starts with a single `/**` and becomes a valid JSDoc comment.convex/yjsSync/__tests__/yjsSyncMutations.test.ts (1)
212-226: 🧹 Nitpick | 🔵 TrivialUse
COMPACT_INTERVAL - 1for consistency in the "before interval" test.The test at Line 179 correctly uses
COMPACT_INTERVAL, but this companion test still uses a hardcoded19. IfCOMPACT_INTERVALchanges, this test's assumption (and the expected row count of20at Line 225) would become incorrect.♻️ Suggested fix
- for (let i = 1; i <= 19; i++) { + for (let i = 1; i <= COMPACT_INTERVAL - 1; i++) { await dmAuth.mutation(api.yjsSync.mutations.pushUpdate, { documentId: noteId, update: makeEmptyYjsUpdate(), }) } await t.run(async (dbCtx) => { const rows = await dbCtx.db .query('yjsUpdates') .withIndex('by_document_seq', (q) => q.eq('documentId', noteId)) .collect() - expect(rows).toHaveLength(20) + expect(rows).toHaveLength(COMPACT_INTERVAL) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/yjsSync/__tests__/yjsSyncMutations.test.ts` around lines 212 - 226, The test currently hardcodes 19 iterations and expects 20 rows; replace the magic number with the constant COMPACT_INTERVAL so the loop runs COMPACT_INTERVAL - 1 times (use COMPACT_INTERVAL - 1 in the for loop bound where dmAuth.mutation(api.yjsSync.mutations.pushUpdate, { documentId: noteId, update: makeEmptyYjsUpdate() }) is called) and update the assertion expect(rows).toHaveLength(20) to expect(rows).toHaveLength(COMPACT_INTERVAL) so the test stays correct if COMPACT_INTERVAL changes.
♻️ Duplicate comments (3)
src/features/canvas/components/canvas-remote-cursors.tsx (1)
130-150: 🧹 Nitpick | 🔵 TrivialConsider memoizing
NameLabelor extracting static styles.
NameLabelcreates a new inline style object on every render. Since it's exported and used per remote user, consider extracting static style properties to a constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/canvas-remote-cursors.tsx` around lines 130 - 150, NameLabel recreates the full inline style object each render; extract the static style properties into a constant (e.g., NAME_LABEL_BASE_STYLE) and only merge the dynamic properties (backgroundColor and color from getContrastColor(color)) inside NameLabel, and optionally wrap/export NameLabel with React.memo to avoid re-renders when props are unchanged; update references in the component to use Object.assign or spread to combine NAME_LABEL_BASE_STYLE with the dynamic fields so only the color-dependent values are recalculated.src/features/canvas/utils/canvas-stroke-utils.ts (1)
97-103:⚠️ Potential issue | 🟠 MajorHandle collinear segment overlaps in hit-testing.
segmentsIntersectreturnsfalsefor every parallel case (whendenom ≈ 0), so overlapping or edge-aligned segments are treated as disjoint. This makes eraser and selection miss strokes when the trail runs along a stroke edge or rectangle boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/utils/canvas-stroke-utils.ts` around lines 97 - 103, The function segmentsIntersect currently bails out on near-zero denom and treats all parallel cases as non-intersecting; update it to detect collinear (parallel + on-line) cases and return true when the 1D projections of the two segments overlap. Specifically, in segmentsIntersect, when Math.abs(denom) < 1e-10 compute whether the segments are collinear by checking the cross product of (bx-ax,by-ay) with (cx-ax,cy-ay) is near zero; if collinear, choose the dominant axis (x or y) from (bx-ax,by-ay), build scalar intervals for [a,b] and [c,d] on that axis (use ax, bx, cx, dx or ay, by, cy, dy accordingly), then return true if the intervals overlap (including touching at endpoints), otherwise return false; leave the existing t/u branch unchanged for the non-collinear case.src/features/editor/hooks/useYjsReactFlowSync.ts (1)
189-209:⚠️ Potential issue | 🟡 MinorClear spring state in drag handlers to prevent snap-back.
When a local drag starts, the RAF loop clears the spring entry (lines 137-140), but only if the loop is running. If the spring loop has stopped (
springs.size === 0) and a node retains stale spring state from a prior remote drag, starting a local drag won't immediately clear it. WhenremoteDragPositionsnext changes and restarts the loop, the stale spring may cause unexpected behavior.Consider clearing spring state directly in the drag handlers:
🛠️ Suggested fix
const onNodeDragStart: OnNodeDrag = useCallback((_event, _node, nodes) => { - for (const n of nodes) draggingIds.current.add(n.id) + for (const n of nodes) { + draggingIds.current.add(n.id) + springStates.current.delete(n.id) + springActiveIds.current.delete(n.id) + } }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/hooks/useYjsReactFlowSync.ts` around lines 189 - 209, The drag handlers (onNodeDragStart and onNodeDragStop) need to proactively clear any stale spring state so nodes don't snap back when the RAF spring loop (springs / remoteDragPositions loop) restarts; inside onNodeDragStart and when ending in onNodeDragStop, remove the node's entry from the springs map and from any related remoteDragPositions state (and/or reset its spring data) before mutating nodesMap, and keep the existing uses of draggingIds.current and suppressNodeObserver.current; target the functions onNodeDragStart, onNodeDragStop and the springs / remoteDragPositions data structures referenced in the module and ensure the clear happens synchronously in those handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/_test/factories.helper.ts`:
- Around line 175-195: The insertSidebarItem helper currently bypasses TS checks
by casting data with "as any"; replace that with a proper typed insert payload
so schema mismatches are caught: introduce or reuse a specific insert type
(e.g., SidebarItemInsert<TTable> or a Convex insert-row mapping) and type the
local variable data to that type, then pass it to ctx.db.insert(table, data)
inside t.run; update the insertSidebarItem signature if needed so the generic
TTable can map to the correct insert row type (referencing insertSidebarItem,
data, table, t.run, and ctx.db.insert).
In `@src/features/canvas/components/canvas-color-panel.tsx`:
- Around line 36-39: The filter that builds colorRelevantNodes currently checks
for n.data?.color !== undefined but getSelectionColor only counts truthy colors,
causing mismatch for empty-string colors; update colorRelevantNodes to use a
truthy check (e.g., filter(n => !!n.data?.color)) so it matches
getSelectionColor's logic (and make the same change at the other occurrence
around getSelectionColor lines 145-153) or alternatively change
getSelectionColor to accept empty-string as a valid color—pick one consistent
approach and apply it to both colorRelevantNodes and the getSelectionColor
logic.
In `@src/features/canvas/components/canvas-minimap-node.tsx`:
- Around line 46-53: The minimap SVG in canvas-minimap-node.tsx uses the default
preserveAspectRatio which causes strokes to scale differently after non-uniform
resizes; to match StrokePreview behavior, set preserveAspectRatio="none" on the
SVG element (the same element that already sets viewBox and overflow) so the
minimap stroke scaling matches the main StrokePreview rendering.
In `@src/features/canvas/components/canvas-viewer.tsx`:
- Around line 381-388: The useMemo return object references setEditingEmbedId
but the dependency array omits it; add setEditingEmbedId to the dependency array
of the useMemo that returns updateNodeData, onResizeEnd, remoteHighlights,
canEdit, canvasUser, and editingEmbedId so the exhaustive-deps lint rule is
satisfied—locate the useMemo where those symbols are returned and append
setEditingEmbedId to the array.
In `@src/features/canvas/components/nodes/embed-content/embed-canvas-content.tsx`:
- Around line 103-118: Edge rendering in embed-canvas-content.tsx currently
draws straight center-to-center lines (edges.map, using
nodePositions.get(edge.source) / .get(edge.target)), which reduces visual
fidelity compared to the main canvas' routed edges; to improve this, replace the
simple <line> rendering with a routed path: compute edge endpoints and control
points (or use existing edge routing logic from the main canvas renderer) to
construct an SVG path (e.g., "M x1 y1 C cx1 cy1, cx2 cy2, x2 y2") and render a
<path> instead of <line>, or add a flag to choose simple vs. routed rendering so
the embedded preview can opt into bezier/handle-based routing while still
falling back to center-to-center when routing data is missing.
In `@src/features/canvas/components/nodes/resizable-node-wrapper.tsx`:
- Around line 36-69: The current logic in subscribeShift adds/removes global
keydown/keyup listeners only while shiftSubscribers.size > 0, causing Shift
state (shiftPressed) to go stale when no handles are mounted; change the
behavior so the window listeners for onShiftDown and onShiftUp are registered
once and never removed (or otherwise ensure they remain active independent of
shiftSubscribers.size), keeping shiftPressed updated at all times; update
subscribeShift to only add/remove callbacks from shiftSubscribers without
attaching/removing event listeners, referencing the functions/variables
shiftPressed, onShiftDown, onShiftUp, subscribeShift, shiftSubscribers and
notifyShiftSubscribers to locate the code to change.
In `@src/features/canvas/hooks/useCanvasKeyboardShortcuts.ts`:
- Around line 20-28: The handler in useCanvasKeyboardShortcuts is checking both
e.code and e.key which breaks non-QWERTY layouts; update the keyboard logic to
drop the physical key checks (remove any comparisons against the variable code
or 'KeyZ'/'KeyY') and rely solely on e.key.toLowerCase() (the existing key
variable) when detecting undo/redo (the branches that call undo() and redo());
keep e.preventDefault() and the shiftKey checks as-is so Ctrl/Cmd+z vs
Ctrl/Cmd+Shift+z behave correctly.
In `@src/features/canvas/hooks/useCanvasSelectionRect.ts`:
- Around line 59-65: The mouse-up/deselection path can see a stale
lastFlowRectRef.current because the RAF that writes it may be cancelled; before
canceling rafRef.current (and/or at the start of the mouse-up handler) copy the
previous selection rect into lastFlowRectRef.current (e.g., set
lastFlowRectRef.current = prevRect or wasActive ? prevRect : null) so the
deselection branch that reads lastFlowRectRef.current has the correct previous
value; update the write at the top of the handler (where prevRect and
rafRef.current are handled) and ensure the same fix is applied to the other
affected sections referenced (the blocks around lines 67-69 and 103-132 that
also rely on lastFlowRectRef.current).
In `@src/features/editor/components/forms/file-form/file-form.tsx`:
- Around line 185-192: Replace the raw console.error call in the create flow
with the module's error handler: when calling
generatePdfPreviewIfNeeded(fileUpload.file, newFileId as Id<'files'>) attach a
.catch that calls handleError(err, 'PDF preview generation failed') instead of
console.error so it matches the update flow's handling; locate this in
file-form.tsx around the generatePdfPreviewIfNeeded invocation and reuse the
existing handleError utility.
In `@src/features/editor/components/note-content.tsx`:
- Around line 128-142: The useEffect that creates the BlockNoteEditor uses
`content` but intentionally omits it from the dependency array; add an inline
ESLint suppression comment above the useEffect such as `//
eslint-disable-next-line react-hooks/exhaustive-deps` and a short comment
documenting the intent (e.g., "create editor only once; intentionally omit
content from deps") so the `useEffect` (which calls `BlockNoteEditor.create`
with `editorSchema`, sets state via `setEditor`, and invokes
`onEditorChangeRef.current`) does not trigger exhaustive-deps warnings.
In `@src/features/editor/hooks/useYjsReactFlowSync.ts`:
- Line 197: The code uses non-null assertions nodesMap.doc! and edgesMap.doc!
(in the transact/remove calls and in handlers onNodesDelete and onEdgesDelete)
which will throw if the Yjs docs aren't initialized; fix by adding explicit
null/undefined guards (e.g., if (!nodesMap?.doc) return or if (!edgesMap?.doc)
return) before any use of .doc or calling .transact/remove, and update
onNodesDelete and onEdgesDelete to early-return when their respective .doc is
missing instead of using the ! operator.
In `@src/features/sidebar/components/forms/color-picker.tsx`:
- Line 3: The file imports the same module using two different path styles;
consolidate to the alias import to keep paths consistent: replace the relative
import for DEFAULT_ITEM_COLOR (currently from '../../utils/sidebar-item-utils')
with the alias import used elsewhere
(~/features/sidebar/utils/sidebar-item-utils) and remove the duplicate import so
only one import of DEFAULT_ITEM_COLOR from that alias remains in the
color-picker.tsx module.
---
Outside diff comments:
In `@convex/yjsSync/__tests__/yjsSyncMutations.test.ts`:
- Around line 212-226: The test currently hardcodes 19 iterations and expects 20
rows; replace the magic number with the constant COMPACT_INTERVAL so the loop
runs COMPACT_INTERVAL - 1 times (use COMPACT_INTERVAL - 1 in the for loop bound
where dmAuth.mutation(api.yjsSync.mutations.pushUpdate, { documentId: noteId,
update: makeEmptyYjsUpdate() }) is called) and update the assertion
expect(rows).toHaveLength(20) to expect(rows).toHaveLength(COMPACT_INTERVAL) so
the test stays correct if COMPACT_INTERVAL changes.
In `@src/features/sidebar/utils/sidebar-item-utils.ts`:
- Around line 59-60: There are duplicate JSDoc opening markers (`/**` twice)
that break the comment block; edit the JSDoc block immediately above the
exported symbol (the comment before the sidebar item helper in
sidebar-item-utils.ts) and remove the extra `/**` so the block starts with a
single `/**` and becomes a valid JSDoc comment.
---
Duplicate comments:
In `@src/features/canvas/components/canvas-remote-cursors.tsx`:
- Around line 130-150: NameLabel recreates the full inline style object each
render; extract the static style properties into a constant (e.g.,
NAME_LABEL_BASE_STYLE) and only merge the dynamic properties (backgroundColor
and color from getContrastColor(color)) inside NameLabel, and optionally
wrap/export NameLabel with React.memo to avoid re-renders when props are
unchanged; update references in the component to use Object.assign or spread to
combine NAME_LABEL_BASE_STYLE with the dynamic fields so only the
color-dependent values are recalculated.
In `@src/features/canvas/utils/canvas-stroke-utils.ts`:
- Around line 97-103: The function segmentsIntersect currently bails out on
near-zero denom and treats all parallel cases as non-intersecting; update it to
detect collinear (parallel + on-line) cases and return true when the 1D
projections of the two segments overlap. Specifically, in segmentsIntersect,
when Math.abs(denom) < 1e-10 compute whether the segments are collinear by
checking the cross product of (bx-ax,by-ay) with (cx-ax,cy-ay) is near zero; if
collinear, choose the dominant axis (x or y) from (bx-ax,by-ay), build scalar
intervals for [a,b] and [c,d] on that axis (use ax, bx, cx, dx or ay, by, cy, dy
accordingly), then return true if the intervals overlap (including touching at
endpoints), otherwise return false; leave the existing t/u branch unchanged for
the non-collinear case.
In `@src/features/editor/hooks/useYjsReactFlowSync.ts`:
- Around line 189-209: The drag handlers (onNodeDragStart and onNodeDragStop)
need to proactively clear any stale spring state so nodes don't snap back when
the RAF spring loop (springs / remoteDragPositions loop) restarts; inside
onNodeDragStart and when ending in onNodeDragStop, remove the node's entry from
the springs map and from any related remoteDragPositions state (and/or reset its
spring data) before mutating nodesMap, and keep the existing uses of
draggingIds.current and suppressNodeObserver.current; target the functions
onNodeDragStart, onNodeDragStop and the springs / remoteDragPositions data
structures referenced in the module and ensure the clear happens synchronously
in those handlers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 731ebcae-c1b0-4161-b301-61142b546140
📒 Files selected for processing (49)
convex/_test/factories.helper.tsconvex/canvases/functions/createCanvas.tsconvex/canvases/functions/updateCanvas.tsconvex/folders/functions/getFolderContentsForDownload.tsconvex/gameMaps/__tests__/gameMaps.test.tsconvex/notes/__tests__/notes.test.tsconvex/notes/__tests__/persistBlocks.test.tsconvex/notes/functions/createNote.tsconvex/sidebarItems/__tests__/previewCleanup.test.tsconvex/sidebarItems/__tests__/previewGeneration.test.tsconvex/sidebarItems/__tests__/sidebarItemValidation.test.tsconvex/sidebarItems/functions/claimPreviewGeneration.tsconvex/sidebarItems/functions/enhanceSidebarItem.tsconvex/sidebarItems/functions/getSidebarItemById.tsconvex/sidebarItems/functions/getSidebarItemBySlug.tsconvex/sidebarItems/functions/setPreviewImage.tsconvex/sidebarItems/schema/baseFields.tsconvex/sidebarItems/types/baseTypes.tsconvex/sidebarItems/validation.tsconvex/yjsSync/__tests__/yjsSyncMutations.test.tsconvex/yjsSync/functions/compactUpdates.tssrc/features/canvas/components/canvas-color-panel.tsxsrc/features/canvas/components/canvas-minimap-node.tsxsrc/features/canvas/components/canvas-remote-cursors.tsxsrc/features/canvas/components/canvas-viewer.tsxsrc/features/canvas/components/nodes/embed-content/embed-canvas-content.tsxsrc/features/canvas/components/nodes/resizable-node-wrapper.tsxsrc/features/canvas/components/nodes/stroke-node.tsxsrc/features/canvas/hooks/useCanvasEraser.tssrc/features/canvas/hooks/useCanvasFileUpload.tsxsrc/features/canvas/hooks/useCanvasKeyboardShortcuts.tssrc/features/canvas/hooks/useCanvasOverlayHandlers.tssrc/features/canvas/hooks/useCanvasSelectionRect.tssrc/features/canvas/hooks/useCanvasStrokeClick.tssrc/features/canvas/hooks/useCanvasWheel.tssrc/features/canvas/hooks/useNodeEditing.tssrc/features/canvas/utils/canvas-stroke-utils.tssrc/features/dnd/utils/__tests__/dnd-registry.test.tssrc/features/dnd/utils/dnd-registry.tssrc/features/editor/components/forms/file-form/file-form.tsxsrc/features/editor/components/note-content.tsxsrc/features/editor/components/viewer/folder/note-card.tsxsrc/features/editor/components/viewer/map/map-viewer.tsxsrc/features/editor/hooks/useYjsReactFlowSync.tssrc/features/previews/utils/upload-preview.tssrc/features/sidebar/components/forms/color-picker.tsxsrc/features/sidebar/components/sidebar-root/droppable-root.tsxsrc/features/sidebar/utils/sidebar-item-utils.tssrc/styles/app.css
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/features/sidebar/utils/sidebar-item-utils.ts (1)
59-60:⚠️ Potential issue | 🟡 MinorDuplicate JSDoc comment opener creates malformed documentation.
There's a duplicated
/**on lines 59-60. This results in malformed JSDoc and the comment forisFileis broken.Proposed fix
-/** /** * Type guard to check if a sidebar item is a SidebarFile. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sidebar/utils/sidebar-item-utils.ts` around lines 59 - 60, The JSDoc opener is duplicated which breaks the comment for isFile; edit the comment block in sidebar-item-utils.ts (the JSDoc immediately above the isFile function) and remove the extra `/**` so there is only a single JSDoc opener, ensuring the comment is well-formed and applies to the isFile declaration.convex/yjsSync/__tests__/yjsSyncMutations.test.ts (1)
212-226: 🧹 Nitpick | 🔵 TrivialInconsistent use of hardcoded values vs
COMPACT_INTERVALconstant.The "triggers compaction at COMPACT_INTERVAL" test correctly uses the imported constant, but this test still hardcodes
19and20. IfCOMPACT_INTERVALchanges, this test will become incorrect.♻️ Use COMPACT_INTERVAL for consistency
it('does not trigger compaction before interval', async () => { const ctx = await setupCampaignContext(t) const dmAuth = asDm(ctx) const { noteId } = await dmAuth.mutation(api.notes.mutations.createNote, { campaignId: ctx.campaignId, name: 'No Compact Note', parentId: null, }) - for (let i = 1; i <= 19; i++) { + for (let i = 1; i < COMPACT_INTERVAL; i++) { await dmAuth.mutation(api.yjsSync.mutations.pushUpdate, { documentId: noteId, update: makeEmptyYjsUpdate(), }) } await t.run(async (dbCtx) => { const rows = await dbCtx.db .query('yjsUpdates') .withIndex('by_document_seq', (q) => q.eq('documentId', noteId)) .collect() - expect(rows).toHaveLength(20) + expect(rows).toHaveLength(COMPACT_INTERVAL) }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/yjsSync/__tests__/yjsSyncMutations.test.ts` around lines 212 - 226, Replace the hardcoded 19/20 with the COMPACT_INTERVAL constant: import/use COMPACT_INTERVAL in this test and change the loop to run for i = 1; i <= COMPACT_INTERVAL - 1; i++ when calling dmAuth.mutation(api.yjsSync.mutations.pushUpdate, ...) and assert expect(rows).toHaveLength(COMPACT_INTERVAL) after t.run; reference the existing symbols dmAuth.mutation, api.yjsSync.mutations.pushUpdate, makeEmptyYjsUpdate, noteId, and t.run to locate where to make the change.convex/_test/factories.helper.ts (1)
450-499: 🧹 Nitpick | 🔵 TrivialConsider adding 'canvas' to
setupFolderTreeleaf types.The
leafTypeparameter supports'note' | 'file' | 'gameMap'but not'canvas'. If tests need to create folder trees with canvas leaves, this helper would need updating.♻️ Optional: Add canvas support to setupFolderTree
export async function setupFolderTree( t: T, campaignId: Id<'campaigns'>, creatorProfileId: Id<'userProfiles'>, opts: { depth: number inheritShares?: Array<boolean> - leafType?: 'note' | 'file' | 'gameMap' + leafType?: 'note' | 'file' | 'gameMap' | 'canvas' }, ) { const { depth, inheritShares = [], leafType = 'note' } = opts // ... existing code ... case 'gameMap': { const { mapId } = await createGameMap(t, campaignId, creatorProfileId, { parentId: leafParent, }) return { folders, leaf: mapId } } + case 'canvas': { + const { canvasId } = await createCanvas(t, campaignId, creatorProfileId, { + parentId: leafParent, + }) + return { folders, leaf: canvasId } + } default: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_test/factories.helper.ts` around lines 450 - 499, The setupFolderTree helper's leafType union omits 'canvas', so tests cannot create folder trees with canvas leaves; update the setupFolderTree function to include 'canvas' in the leafType union and add a case that calls createCanvas(t, campaignId, creatorProfileId, { parentId: leafParent }) and returns { folders, leaf: canvasId }; also update the default/exhaustive handling if needed and any callers/types that reference setupFolderTree's opts.leafType to accept 'canvas'.
♻️ Duplicate comments (5)
src/features/editor/components/forms/file-form/file-form.tsx (1)
185-192:⚠️ Potential issue | 🟡 MinorUse
handleErrorinstead ofconsole.errorin create flow.Line 189-191 still uses raw
console.error, while Line 166-167 uses the sharedhandleErrorpath. Please align create flow with the same error handler for consistency and centralized logging.♻️ Proposed fix
if (fileUpload.file) { generatePdfPreviewIfNeeded( fileUpload.file, newFileId as Id<'files'>, - ).catch((err: unknown) => - console.error('PDF preview generation failed:', err), - ) + ).catch((err: unknown) => + handleError(err, 'PDF preview generation failed'), + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/components/forms/file-form/file-form.tsx` around lines 185 - 192, Replace the raw console.error call inside the create flow where generatePdfPreviewIfNeeded(...).catch(...) is used with the shared error handler: call handleError with the caught error and a descriptive context (e.g., "PDF preview generation failed") so the create flow uses handleError instead of console.error; locate the promise catch attached to generatePdfPreviewIfNeeded and swap console.error('PDF preview generation failed:', err) for handleError(err, 'PDF preview generation failed') while keeping newFileId and fileUpload.file unchanged.src/features/canvas/components/nodes/resizable-node-wrapper.tsx (1)
50-69:⚠️ Potential issue | 🟠 MajorReset
shiftPressedon window blur.If the tab loses focus while Shift is held,
onShiftUpnever fires andshiftPressedstaystrueuntil another key event. The next resize then starts withkeepAspectRatiostuck on. Add ablurhandler that clears the flag and notifies subscribers.Possible fix
function onShiftUp(e: KeyboardEvent) { if (e.key === 'Shift' && shiftPressed) { shiftPressed = false notifyShiftSubscribers() } } + +function resetShift() { + if (!shiftPressed) return + shiftPressed = false + notifyShiftSubscribers() +} function subscribeShift(cb: () => void) { if (shiftSubscribers.size === 0) { window.addEventListener('keydown', onShiftDown) window.addEventListener('keyup', onShiftUp) + window.addEventListener('blur', resetShift) } shiftSubscribers.add(cb) return () => { shiftSubscribers.delete(cb) if (shiftSubscribers.size === 0) { window.removeEventListener('keydown', onShiftDown) window.removeEventListener('keyup', onShiftUp) + window.removeEventListener('blur', resetShift) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/nodes/resizable-node-wrapper.tsx` around lines 50 - 69, The shiftPressed flag can remain true if the tab loses focus while Shift is held; update subscribeShift/onShiftUp logic to also attach a window 'blur' handler (e.g., onWindowBlur) when the first subscriber is added and remove it when the last subscriber is removed; implement onWindowBlur to set shiftPressed = false and call notifyShiftSubscribers(), and ensure the same cleanup that removes onShiftDown/onShiftUp also removes the blur listener in subscribeShift's teardown.src/features/canvas/utils/canvas-stroke-utils.ts (1)
87-103:⚠️ Potential issue | 🟠 MajorHandle collinear overlaps in
segmentsIntersect.The
denom ≈ 0 => falsepath still treats parallel/collinear segments as disjoint. Eraser, lasso, and rect hit-testing then miss cases where the trail runs exactly along a stroke edge or rectangle boundary. Add orientation/on-segment checks before falling back to thet/utest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/utils/canvas-stroke-utils.ts` around lines 87 - 103, The segmentsIntersect function currently returns false when denom ≈ 0, missing collinear overlaps; modify segmentsIntersect to, when Math.abs(denom) < 1e-10, perform collinearity and on-segment checks: compute orientation (cross products) for endpoints (e.g., orient(ax,ay,bx,by, cx,cy) etc.) and if collinear, check if any endpoint of one segment lies on the other segment using a between/projection test (compare min/max for x and y or use dot product) before returning false; keep existing t and u intersection logic for non-parallel cases so segmentsIntersect still returns true for overlapping collinear segments as well as proper crossings.src/features/canvas/components/canvas-color-panel.tsx (1)
58-64:⚠️ Potential issue | 🟠 MajorDon't reuse a pending batch for a different selection.
pendingUpdate.current.nodeIdsis kept whenever a RAF batch already exists. If the selection changes before that frame and the user makes another color/opacity edit, the second update is still flushed to the old node set.Possible fix
const scheduleNodeUpdate = useCallback( (data: Record<string, unknown>) => { + const nodeIds = colorRelevantNodes.map((n) => n.id) + const pending = pendingUpdate.current + const sameTarget = + pending?.nodeIds.length === nodeIds.length && + pending.nodeIds.every((id, index) => id === nodeIds[index]) + pendingUpdate.current = { - data: { ...pendingUpdate.current?.data, ...data }, - nodeIds: - pendingUpdate.current?.nodeIds ?? colorRelevantNodes.map((n) => n.id), + data: sameTarget ? { ...pending?.data, ...data } : data, + nodeIds, } if (!rafId.current) { rafId.current = requestAnimationFrame(() => { rafId.current = 0 flushNodeUpdates()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/canvas-color-panel.tsx` around lines 58 - 64, scheduleNodeUpdate currently reuses pendingUpdate.current.nodeIds from an earlier RAF batch, causing edits to flush to the wrong selection; update the logic in scheduleNodeUpdate so nodeIds are always derived from the current selection (colorRelevantNodes.map(n => n.id)) instead of reusing pendingUpdate.current?.nodeIds — i.e., when setting pendingUpdate.current, always set nodeIds: colorRelevantNodes.map((n) => n.id) (or compute a stable currentSelectionIds variable and use that) so a new edit within the same frame targets the active selection rather than the previous one.src/features/canvas/hooks/useCanvasSelectionRect.ts (1)
59-69:⚠️ Potential issue | 🟠 MajorPreserve the last rect even when mouse-up beats the RAF.
Lines 62-69 cancel the pending frame and then rely on
lastFlowRectRef.currentfor the deselection pass. A drag that starts and ends within one frame never writes that ref, so strokes outside the finished rect stay selected. Cleanup should also clearlastFlowRectRef.currentso the next session can't reuse a stale rect.Also applies to: 160-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/hooks/useCanvasSelectionRect.ts` around lines 59 - 69, The deselection path in useCanvasSelectionRect cancels a pending RAF and then relies on lastFlowRectRef.current (and prevRect) which can be stale if a drag starts and ends within one frame; update the cleanup so when handling !userSelectionRect you clear lastFlowRectRef.current (and reset prevRect appropriately) immediately after performing the deselection logic so the next session cannot reuse a stale rect; apply the same fix to the duplicate block around the other deselection branch (the block referenced at the second occurrence) to ensure lastFlowRectRef.current is always cleared whenever the selection is finalized or cancelled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/notes/functions/createNote.ts`:
- Around line 75-80: The code repeats the same condition `content &&
content.length > 0`; consolidate by combining the two blocks into one: wrap the
call to saveTopLevelBlocksForNote and the initialization of `initialState`
inside a single `if (content && content.length > 0)` block so
`saveTopLevelBlocksForNote(ctx, { noteId, content })` is awaited and then
`initialState` is set in the same scope (ensure `initialState` remains defined
as `undefined` outside when there is no content and that any later uses of
`initialState` still see the correct value).
In `@convex/sidebarItems/__tests__/previewGeneration.test.ts`:
- Around line 247-254: The test currently allows a 5000ms tolerance when
asserting note!.previewUpdatedAt which is overly generous; update the assertion
in previewGeneration.test.ts (inside the t.run callback where note is fetched
via dbCtx.db.get with noteId/storageId) to use a tighter bound such as 1000ms
(expect(Math.abs(now - note!.previewUpdatedAt!)).toBeLessThan(1000)) or, if
there are known timing reasons, add an inline comment documenting why 5000ms is
required so future readers understand the rationale.
In `@src/features/canvas/components/canvas-viewer.tsx`:
- Around line 470-478: The component CanvasDropOverlay is receiving a prop named
ref which is deprecated; update it to use React.forwardRef by converting
CanvasDropOverlay into a forwardRef component that accepts (props, ref:
React.Ref<HTMLDivElement | null>) so the DOM ref is passed via the second
parameter instead of a prop, remove the ref entry from the props shape ({
isDropTarget, isFileDropTarget }) and update all call sites to pass the ref as a
normal JSX ref (ref={...}) rather than a prop; alternatively, if you prefer not
to forward refs, rename the prop to a non-reserved name like containerRef or
overlayRef across the component and its usages to avoid React reserved ref
semantics.
In `@src/features/canvas/components/nodes/stroke-node.tsx`:
- Around line 63-90: The resize currently only persists width/height/position in
onResizeEnd (nodesMap.set) while StrokePreview and pointsToPathD render scaled
geometry from width/height, causing hit-testing to use stale data.points and
data.bounds; update onResizeEnd (or the resize commit path) to compute
scaledPoints and scaledBounds from the original data.points/bounds using the
width/height scale factors and call updateNodeData(nodeId, { points:
scaledPoints, bounds: scaledBounds }) so the stored geometry matches the
rendered geometry (alternatively, adjust hit-testing to apply the same display
scale, but prefer recomputing via updateNodeData).
In `@src/features/canvas/hooks/useCanvasEraser.ts`:
- Around line 90-107: onPointerUp currently cancels the pending RAF and clears
trail/marked without flushing the final eraser segment, so a fast lift can miss
the last intersections; modify onPointerUp in useCanvasEraser.ts to append the
pointer-up coordinate to trailRef.current (or otherwise ensure the final point
is included) and call testIntersections() synchronously before
cancelling/clearing the RAF/marked state (use eraserRafRef, trailRef, markedRef,
testIntersections, nodesMap.transact and useCanvasToolStore.setErasingStrokeIds
to ensure the final segment is processed and marked deletions are applied) so
the last segment is evaluated even if the RAF hasn't fired yet.
In `@src/features/canvas/hooks/useCanvasFileUpload.tsx`:
- Around line 46-58: Extract the repeated call to isTextFile(file.type,
file.name) into a local boolean variable (e.g., const isText =
isTextFile(file.type, file.name)) at the top of useCanvasFileUpload or just
before toast creation, then use that variable in the ToastContent
message/progress and the subsequent try branch; update references to isTextFile
in the toast loading call, the progress prop, and the if (isTextFile(...))
condition to use isText so you avoid redundant evaluations and improve
readability.
In `@src/features/editor/hooks/useYjsReactFlowSync.ts`:
- Around line 193-205: The drag bookkeeping (draggingIds.current.delete(...)) is
inside the persistence guard and can be skipped if nodesMap is unavailable; move
the deletion out so it always runs: in useYjsReactFlowSync's onNodeDragStop
callback, after the early nodesMap check and after suppressNodeObserver.current
= true, iterate over the incoming nodes and call
draggingIds.current.delete(n.id) for each before entering
nodesMap.doc!.transact, then keep the existing transact block to update nodesMap
(get/set) and remove the deletions from inside that loop.
---
Outside diff comments:
In `@convex/_test/factories.helper.ts`:
- Around line 450-499: The setupFolderTree helper's leafType union omits
'canvas', so tests cannot create folder trees with canvas leaves; update the
setupFolderTree function to include 'canvas' in the leafType union and add a
case that calls createCanvas(t, campaignId, creatorProfileId, { parentId:
leafParent }) and returns { folders, leaf: canvasId }; also update the
default/exhaustive handling if needed and any callers/types that reference
setupFolderTree's opts.leafType to accept 'canvas'.
In `@convex/yjsSync/__tests__/yjsSyncMutations.test.ts`:
- Around line 212-226: Replace the hardcoded 19/20 with the COMPACT_INTERVAL
constant: import/use COMPACT_INTERVAL in this test and change the loop to run
for i = 1; i <= COMPACT_INTERVAL - 1; i++ when calling
dmAuth.mutation(api.yjsSync.mutations.pushUpdate, ...) and assert
expect(rows).toHaveLength(COMPACT_INTERVAL) after t.run; reference the existing
symbols dmAuth.mutation, api.yjsSync.mutations.pushUpdate, makeEmptyYjsUpdate,
noteId, and t.run to locate where to make the change.
In `@src/features/sidebar/utils/sidebar-item-utils.ts`:
- Around line 59-60: The JSDoc opener is duplicated which breaks the comment for
isFile; edit the comment block in sidebar-item-utils.ts (the JSDoc immediately
above the isFile function) and remove the extra `/**` so there is only a single
JSDoc opener, ensuring the comment is well-formed and applies to the isFile
declaration.
---
Duplicate comments:
In `@src/features/canvas/components/canvas-color-panel.tsx`:
- Around line 58-64: scheduleNodeUpdate currently reuses
pendingUpdate.current.nodeIds from an earlier RAF batch, causing edits to flush
to the wrong selection; update the logic in scheduleNodeUpdate so nodeIds are
always derived from the current selection (colorRelevantNodes.map(n => n.id))
instead of reusing pendingUpdate.current?.nodeIds — i.e., when setting
pendingUpdate.current, always set nodeIds: colorRelevantNodes.map((n) => n.id)
(or compute a stable currentSelectionIds variable and use that) so a new edit
within the same frame targets the active selection rather than the previous one.
In `@src/features/canvas/components/nodes/resizable-node-wrapper.tsx`:
- Around line 50-69: The shiftPressed flag can remain true if the tab loses
focus while Shift is held; update subscribeShift/onShiftUp logic to also attach
a window 'blur' handler (e.g., onWindowBlur) when the first subscriber is added
and remove it when the last subscriber is removed; implement onWindowBlur to set
shiftPressed = false and call notifyShiftSubscribers(), and ensure the same
cleanup that removes onShiftDown/onShiftUp also removes the blur listener in
subscribeShift's teardown.
In `@src/features/canvas/hooks/useCanvasSelectionRect.ts`:
- Around line 59-69: The deselection path in useCanvasSelectionRect cancels a
pending RAF and then relies on lastFlowRectRef.current (and prevRect) which can
be stale if a drag starts and ends within one frame; update the cleanup so when
handling !userSelectionRect you clear lastFlowRectRef.current (and reset
prevRect appropriately) immediately after performing the deselection logic so
the next session cannot reuse a stale rect; apply the same fix to the duplicate
block around the other deselection branch (the block referenced at the second
occurrence) to ensure lastFlowRectRef.current is always cleared whenever the
selection is finalized or cancelled.
In `@src/features/canvas/utils/canvas-stroke-utils.ts`:
- Around line 87-103: The segmentsIntersect function currently returns false
when denom ≈ 0, missing collinear overlaps; modify segmentsIntersect to, when
Math.abs(denom) < 1e-10, perform collinearity and on-segment checks: compute
orientation (cross products) for endpoints (e.g., orient(ax,ay,bx,by, cx,cy)
etc.) and if collinear, check if any endpoint of one segment lies on the other
segment using a between/projection test (compare min/max for x and y or use dot
product) before returning false; keep existing t and u intersection logic for
non-parallel cases so segmentsIntersect still returns true for overlapping
collinear segments as well as proper crossings.
In `@src/features/editor/components/forms/file-form/file-form.tsx`:
- Around line 185-192: Replace the raw console.error call inside the create flow
where generatePdfPreviewIfNeeded(...).catch(...) is used with the shared error
handler: call handleError with the caught error and a descriptive context (e.g.,
"PDF preview generation failed") so the create flow uses handleError instead of
console.error; locate the promise catch attached to generatePdfPreviewIfNeeded
and swap console.error('PDF preview generation failed:', err) for
handleError(err, 'PDF preview generation failed') while keeping newFileId and
fileUpload.file unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b048339b-9adc-4a78-ba00-1b9623af9fa9
📒 Files selected for processing (49)
convex/_test/factories.helper.tsconvex/canvases/functions/createCanvas.tsconvex/canvases/functions/updateCanvas.tsconvex/folders/functions/getFolderContentsForDownload.tsconvex/gameMaps/__tests__/gameMaps.test.tsconvex/notes/__tests__/notes.test.tsconvex/notes/__tests__/persistBlocks.test.tsconvex/notes/functions/createNote.tsconvex/sidebarItems/__tests__/previewCleanup.test.tsconvex/sidebarItems/__tests__/previewGeneration.test.tsconvex/sidebarItems/__tests__/sidebarItemValidation.test.tsconvex/sidebarItems/functions/claimPreviewGeneration.tsconvex/sidebarItems/functions/enhanceSidebarItem.tsconvex/sidebarItems/functions/getSidebarItemById.tsconvex/sidebarItems/functions/getSidebarItemBySlug.tsconvex/sidebarItems/functions/setPreviewImage.tsconvex/sidebarItems/schema/baseFields.tsconvex/sidebarItems/types/baseTypes.tsconvex/sidebarItems/validation.tsconvex/yjsSync/__tests__/yjsSyncMutations.test.tsconvex/yjsSync/functions/compactUpdates.tssrc/features/canvas/components/canvas-color-panel.tsxsrc/features/canvas/components/canvas-minimap-node.tsxsrc/features/canvas/components/canvas-remote-cursors.tsxsrc/features/canvas/components/canvas-viewer.tsxsrc/features/canvas/components/nodes/embed-content/embed-canvas-content.tsxsrc/features/canvas/components/nodes/resizable-node-wrapper.tsxsrc/features/canvas/components/nodes/stroke-node.tsxsrc/features/canvas/hooks/useCanvasEraser.tssrc/features/canvas/hooks/useCanvasFileUpload.tsxsrc/features/canvas/hooks/useCanvasKeyboardShortcuts.tssrc/features/canvas/hooks/useCanvasOverlayHandlers.tssrc/features/canvas/hooks/useCanvasSelectionRect.tssrc/features/canvas/hooks/useCanvasStrokeClick.tssrc/features/canvas/hooks/useCanvasWheel.tssrc/features/canvas/hooks/useNodeEditing.tssrc/features/canvas/utils/canvas-stroke-utils.tssrc/features/dnd/utils/__tests__/dnd-registry.test.tssrc/features/dnd/utils/dnd-registry.tssrc/features/editor/components/forms/file-form/file-form.tsxsrc/features/editor/components/note-content.tsxsrc/features/editor/components/viewer/folder/note-card.tsxsrc/features/editor/components/viewer/map/map-viewer.tsxsrc/features/editor/hooks/useYjsReactFlowSync.tssrc/features/previews/utils/upload-preview.tssrc/features/sidebar/components/forms/color-picker.tsxsrc/features/sidebar/components/sidebar-root/droppable-root.tsxsrc/features/sidebar/utils/sidebar-item-utils.tssrc/styles/app.css
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/sidebar/utils/sidebar-item-utils.ts (1)
59-67:⚠️ Potential issue | 🟡 MinorFix malformed JSDoc comment - duplicate opening marker.
There's a duplicate
/**at lines 59-60 creating invalid JSDoc syntax.📝 Proposed fix
-/** /** * Type guard to check if a sidebar item is a SidebarFile. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sidebar/utils/sidebar-item-utils.ts` around lines 59 - 67, The JSDoc for the isFile type guard is malformed due to a duplicate opening `/**`; edit the JSDoc above the exported function isFile to remove the extra `/**` so it begins with a single valid `/**` and maintains the existing description, leaving the function signature, isSidebarItemType call, and SIDEBAR_ITEM_TYPES reference unchanged.
♻️ Duplicate comments (15)
src/features/editor/components/forms/file-form/file-form.tsx (1)
185-192:⚠️ Potential issue | 🟡 MinorUse
handleErrorin create flow instead ofconsole.error.Line 190 still uses raw
console.error, which is inconsistent with the update path and bypasses centralized error handling.♻️ Proposed fix
if (fileUpload.file) { generatePdfPreviewIfNeeded( fileUpload.file, newFileId as Id<'files'>, - ).catch((err: unknown) => - console.error('PDF preview generation failed:', err), - ) + ).catch((err: unknown) => + handleError(err, 'PDF preview generation failed'), + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/components/forms/file-form/file-form.tsx` around lines 185 - 192, The PDF preview generation catch currently logs with console.error; replace that with the centralized error handler by calling handleError(...) inside the catch for generatePdfPreviewIfNeeded(fileUpload.file, newFileId as Id<'files'>), passing a descriptive message (e.g., "PDF preview generation failed for new file") and the caught error so the create flow uses the same error handling as the update path; ensure you import or reference handleError in this scope and preserve the promise chain (i.e., .catch((err) => handleError('PDF preview generation failed for new file', err))).convex/folders/functions/getFolderContentsForDownload.ts (1)
116-118:⚠️ Potential issue | 🟠 MajorDon’t silently omit canvases from exports.
Line 117 currently skips canvas items with
break, so downloads can succeed while returning incomplete content. This should either explicitly fail or report unsupported items to callers.Minimal safe fix (explicit failure)
// TODO: add canvas -> img export - case SIDEBAR_ITEM_TYPES.canvases: - break + case SIDEBAR_ITEM_TYPES.canvases: { + throw new Error('Canvas export is not supported yet') + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/folders/functions/getFolderContentsForDownload.ts` around lines 116 - 118, The switch in getFolderContentsForDownload currently ignores canvases (case SIDEBAR_ITEM_TYPES.canvases: break) which returns incomplete exports; change this to explicitly fail or report unsupported items instead of breaking — e.g., in the getFolderContentsForDownload switch for SIDEBAR_ITEM_TYPES.canvases throw or return a descriptive error/unsupported-item result (including the item id/context) so callers know canvases are not handled; ensure the error type/message is consistent with surrounding error handling in this function.src/features/editor/components/note-content.tsx (2)
196-214: 🧹 Nitpick | 🔵 TrivialRAF retry loop would benefit from an explanatory comment.
The retry loop waits for Tiptap plugins to initialize before patching. This is a workaround for BlockNote's async plugin initialization. Consider adding a comment explaining why this is necessary.
📝 Add explanatory comment
let cancelled = false let retries = 0 const MAX_RETRIES = 30 + // BlockNote/Tiptap may initialize plugins asynchronously after editor creation. + // Wait for plugins to be available before applying Y.js undo patches. const tryPatch = () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/components/note-content.tsx` around lines 196 - 214, Add an inline explanatory comment above the RAF retry loop (the cancelled/retries/MAX_RETRIES variables and the tryPatch function) that briefly explains this is a deliberate workaround to wait for BlockNote/Tiptap plugins to initialize asynchronously before calling patchYUndoPluginDestroy and patchYSyncAfterTypeChanged on instance._tiptapEditor.view; mention why requestAnimationFrame is used, the MAX_RETRIES guard, and that the loop avoids patching until plugins exist to prevent race conditions.
128-142: 🧹 Nitpick | 🔵 TrivialAdd
eslint-disablecomment for intentionally omitted dependency.The
contentvariable is used inside this effect but intentionally excluded from the dependency array (editor creation should only happen once). This will trigger anexhaustive-depslint warning.📝 Suggested fix
onEditorChangeRef.current?.(null, null) } + // eslint-disable-next-line react-hooks/exhaustive-deps -- content used only for initial editor creation }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/components/note-content.tsx` around lines 128 - 142, The useEffect that creates the editor references content but intentionally omits it from dependencies, causing an eslint exhaustive-deps warning; to silence this, add an inline eslint disable comment (e.g., // eslint-disable-next-line react-hooks/exhaustive-deps) immediately above the useEffect call that contains BlockNoteEditor.create({ schema: editorSchema, initialContent, }) so the hook runs only once; keep the rest of the effect (setEditor(instance), onEditorChangeRef.current?.(instance, null), and cleanup destroying instance._tiptapEditor and calling onEditorChangeRef.current?.(null, null)) unchanged.convex/notes/functions/createNote.ts (1)
81-91:⚠️ Potential issue | 🟠 MajorUse
ServerBlockNoteEditorhere instead of client internals.This Convex mutation is server-side, but it instantiates
BlockNoteEditorand reaches_headless/_tiptapEditor. BlockNote's server-processing docs route backend Yjs conversion throughServerBlockNoteEditor, so this path is tightly coupled to client internals and can break note creation on library updates. (blocknotejs.org)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/notes/functions/createNote.ts` around lines 81 - 91, The code currently instantiates client internals via BlockNoteEditor.create({_headless: true}) and accesses _tiptapEditor, which is fragile for server-side conversion; replace this with ServerBlockNoteEditor (imported from blocknote) and use ServerBlockNoteEditor.create(...) when creating the editor used by blocksToYDoc so you never touch client-only internals. Update the code paths that call BlockNoteEditor.create, blocksToYDoc(editor, content, 'document'), editor._tiptapEditor.destroy() and editor.destroy() to instead create/destroy a ServerBlockNoteEditor instance and pass that to blocksToYDoc, keep the Y.encodeStateAsUpdate and uint8ToArrayBuffer flow the same, and remove any references to _headless or _tiptapEditor.src/features/canvas/hooks/useCanvasKeyboardShortcuts.ts (1)
20-28:⚠️ Potential issue | 🟠 MajorDrop
event.codefrom undo/redo detection.These branches still mix physical-key
codewith layout-awarekey. On non-QWERTY layouts,codetracks the physical key whilekeytracks the produced character, soCtrl/Cmd+YandCtrl/Cmd+Zcan dispatch the wrong history action. Match only normalizede.keyfor editor shortcuts. (developer.mozilla.org)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/hooks/useCanvasKeyboardShortcuts.ts` around lines 20 - 28, The undo/redo keyboard handling currently mixes KeyboardEvent.code with KeyboardEvent.key which breaks non-QWERTY layouts; in useCanvasKeyboardShortcuts.ts remove the use of the local `code` variable and change the undo/redo conditionals (the branches that call `undo()` and `redo()`) to match only the normalized `key` (e.g., `key === 'z'` and `key === 'y'`) while keeping the `e.shiftKey` checks and preventDefault/dispatch behavior intact so only character-aware `e.key` drives history actions.src/features/editor/hooks/useYjsReactFlowSync.ts (1)
193-206:⚠️ Potential issue | 🟠 MajorGuard detached Yjs docs and clear bookkeeping outside the transaction path.
draggingIds.current.delete(...)only runs insidenodesMap.doc!.transact(...), and these handlers restoresuppress*Observeronly after the Yjs call succeeds. If a map is temporarily detached, or a transaction throws, drag state can stay stuck and the observer can remain permanently suppressed. Clear drag ids before the guard, usenodesMap?.doc/edgesMap?.doc, and restore the suppression flags infinally.Also applies to: 211-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/hooks/useYjsReactFlowSync.ts` around lines 193 - 206, The onNodeDragStop handler can leave drag state or observer suppression stuck if nodesMap.doc is detached or a transaction throws; move draggingIds.current.delete(n.id) out of the Yjs transaction so deletion happens before attempting the transaction, use safe optional access like nodesMap?.doc (and edgesMap?.doc in the corresponding edge handler), and wrap the suppressNodeObserver.current toggling and the transact call in a try/finally so suppressNodeObserver.current is always reset; apply the same pattern to the other handler referenced (lines ~211-231) ensuring you use nodesMap.get / nodesMap.set inside the transaction but clear bookkeeping and reset suppression in finally.src/features/sidebar/components/forms/color-picker.tsx (1)
3-3: 🧹 Nitpick | 🔵 TrivialUse one import path for the sidebar color helpers.
Line 11 already imports the same module through the alias path. Pull
DEFAULT_ITEM_COLORinto that import too so this file does not mix alias and relative paths for one module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sidebar/components/forms/color-picker.tsx` at line 3, The file mixes two import paths for the same module; remove the separate relative import of DEFAULT_ITEM_COLOR and add DEFAULT_ITEM_COLOR to the existing import that already brings in the sidebar color helpers (the alias import used elsewhere in this file), so all symbols (including DEFAULT_ITEM_COLOR) come from the single alias import for the sidebar-item-utils module.src/features/canvas/hooks/useCanvasFileUpload.tsx (1)
92-116:⚠️ Potential issue | 🟠 MajorClean up committed uploads when
createItemfails.After
commitUploadsucceeds, this catch still only logs and returnsnull. That leaves a durable storage blob/file record with no sidebar item pointing at it. Please call the existing storage cleanup path, or enqueue cleanup, before surfacing the error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/hooks/useCanvasFileUpload.tsx` around lines 92 - 116, When createItem fails after commitUpload.mutateAsync, the code currently only logs and returns null, leaving the committed storage orphaned; modify the catch block for createItem (the one handling createError and referencing storageId and file.name) to call the existing storage cleanup path (or enqueue a cleanup job) with storageId before showing the toast and returning, e.g., invoke the project's cleanup API/function that handles durable storage cleanup (the function used elsewhere for removing or enqueuing cleanup of committed storage) so the orphaned blob is removed or scheduled for deletion prior to surfacing the error.src/features/canvas/components/canvas-minimap-node.tsx (1)
45-56:⚠️ Potential issue | 🟡 MinorAdd
preserveAspectRatio="none"for consistency with main stroke rendering.If the main
StrokePreviewcomponent usespreserveAspectRatio="none", this minimap SVG should match to ensure strokes display consistently after non-uniform resizes.Suggested fix
<svg x={x} y={y} width={width} height={height} viewBox={`${data.bounds.x} ${data.bounds.y} ${safeWidth} ${safeHeight}`} + preserveAspectRatio="none" overflow="visible" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/canvas-minimap-node.tsx` around lines 45 - 56, The minimap SVG in the CanvasMinimapNode component is missing preserveAspectRatio="none", causing inconsistent rendering versus the main StrokePreview; update the <svg> returned in the CanvasMinimapNode (the JSX that uses x, y, width, height, viewBox and safeWidth/safeHeight) to include preserveAspectRatio="none" so non-uniform resizes match the main stroke rendering.src/features/canvas/components/nodes/resizable-node-wrapper.tsx (1)
36-70:⚠️ Potential issue | 🟠 MajorShift state can become stale when no resize handles are mounted.
When all subscribers unmount (lines 65-68), the keyup listener is removed. If the user releases Shift after this,
shiftPressedremainstrue. The next resize will incorrectly usekeepAspectRatio=true.Additionally, window blur events should reset the shift state to handle alt-tab scenarios.
Suggested fix: keep listeners attached permanently
let shiftPressed = false const shiftSubscribers = new Set<() => void>() +let listenersAttached = false + +function resetShift() { + if (!shiftPressed) return + shiftPressed = false + notifyShiftSubscribers() +} function notifyShiftSubscribers() { for (const cb of shiftSubscribers) cb() } function onShiftDown(e: KeyboardEvent) { if (e.key === 'Shift' && !shiftPressed) { shiftPressed = true notifyShiftSubscribers() } } function onShiftUp(e: KeyboardEvent) { if (e.key === 'Shift' && shiftPressed) { - shiftPressed = false - notifyShiftSubscribers() + resetShift() } } function subscribeShift(cb: () => void) { - if (shiftSubscribers.size === 0) { + if (!listenersAttached) { window.addEventListener('keydown', onShiftDown) window.addEventListener('keyup', onShiftUp) + window.addEventListener('blur', resetShift) + listenersAttached = true } shiftSubscribers.add(cb) return () => { shiftSubscribers.delete(cb) - if (shiftSubscribers.size === 0) { - window.removeEventListener('keydown', onShiftDown) - window.removeEventListener('keyup', onShiftUp) - } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/nodes/resizable-node-wrapper.tsx` around lines 36 - 70, The shift tracking currently removes key listeners when the last subscriber is removed causing shiftPressed to stay true if Shift is released while listeners are removed; change subscribeShift/remove logic so the global listeners (onShiftDown, onShiftUp) are attached once and never removed (i.e., don’t remove listeners when shiftSubscribers becomes empty), and add a window 'blur' handler that clears shiftPressed and calls notifyShiftSubscribers (use onShiftDown/onShiftUp names and the shiftPressed and shiftSubscribers symbols) so alt-tab/blur resets state; keep subscribeShift returning an unsubscribe that only removes the callback from shiftSubscribers but does not detach the global event listeners.src/features/canvas/components/canvas-color-panel.tsx (1)
58-64:⚠️ Potential issue | 🟠 MajorReset the pending batch when the selection target changes.
pendingUpdate.current?.nodeIds ?? ...keeps the first target set until the RAF flushes. If the user changes selection and tweaks color/opacity again within the same frame, the second change is merged into the first batch and applied to the wrong nodes.Possible fix
const scheduleNodeUpdate = useCallback( (data: Record<string, unknown>) => { + const nodeIds = colorRelevantNodes.map((n) => n.id) + const sameTarget = + !!pendingUpdate.current && + pendingUpdate.current.nodeIds.length === nodeIds.length && + pendingUpdate.current.nodeIds.every((id, index) => id === nodeIds[index]) + pendingUpdate.current = { - data: { ...pendingUpdate.current?.data, ...data }, - nodeIds: - pendingUpdate.current?.nodeIds ?? colorRelevantNodes.map((n) => n.id), + data: sameTarget + ? { ...pendingUpdate.current!.data, ...data } + : data, + nodeIds, } if (!rafId.current) { rafId.current = requestAnimationFrame(() => { rafId.current = 0 flushNodeUpdates()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/canvas-color-panel.tsx` around lines 58 - 64, The current scheduleNodeUpdate merges new edits into pendingUpdate.current but reuses pendingUpdate.current.nodeIds via the `??` fallback, causing later changes within the same RAF to be applied to stale nodeIds; update scheduleNodeUpdate so nodeIds are derived from the current selection instead of preserving the old pending nodeIds (i.e., compute nodeIds as colorRelevantNodes.map(n => n.id) rather than using `pendingUpdate.current?.nodeIds ?? ...`) so a changed selection resets the pending batch target.src/features/canvas/hooks/useCanvasEraser.ts (1)
90-107:⚠️ Potential issue | 🟠 MajorFlush the final eraser segment before clearing the trail.
A quick erase gesture can end before the scheduled frame runs. Pointer-up currently cancels that frame and clears state immediately, so the last segment never goes through
testIntersections()and crossed strokes are missed.Possible fix
- const onPointerUp = useCallback(() => { + const onPointerUp = useCallback((e: React.PointerEvent) => { erasingRef.current = false if (eraserRafRef.current) { cancelAnimationFrame(eraserRafRef.current) eraserRafRef.current = 0 } + const pos = reactFlow.screenToFlowPosition({ + x: e.clientX, + y: e.clientY, + }) + trailRef.current.push(pos) + testIntersections() const marked = markedRef.current if (nodesMap.doc && marked.size > 0) { nodesMap.doc.transact(() => { for (const id of marked) { nodesMap.delete(id) @@ markedRef.current = new Set() trailRef.current = [] useCanvasToolStore.getState().setErasingStrokeIds(new Set()) - }, [nodesMap]) + }, [nodesMap, reactFlow, testIntersections])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/hooks/useCanvasEraser.ts` around lines 90 - 107, Pointer-up currently cancels the pending eraser RAF and clears state before the last trail segment is processed, so call the same final processing path synchronously in onPointerUp to flush the last segment: before cancelling eraserRafRef and resetting trailRef/markedRef, run the eraser frame handler (the logic that calls testIntersections and updates markedRef) so markedRef contains any final hits and nodesMap deletions happen, then proceed to cancelAnimationFrame(eraserRafRef.current), clear erasingRef/eraserRafRef/trailRef and call useCanvasToolStore.getState().setErasingStrokeIds(new Set()); reference onPointerUp, eraserRafRef, trailRef, markedRef, testIntersections, and nodesMap.delete for locating where to insert the synchronous flush.src/features/canvas/hooks/useCanvasSelectionRect.ts (1)
59-69:⚠️ Potential issue | 🟠 MajorDon't cancel away the only flow-space selection rect.
A drag that starts and ends within one frame cancels the pending RAF before
lastFlowRectRef.currentis written. The mouse-up branch then has no flow-space rect to replay, so the built-in box selection can leave non-intersecting stroke nodes selected.Also applies to: 103-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/hooks/useCanvasSelectionRect.ts` around lines 59 - 69, The code cancels the pending RAF unconditionally which can prevent lastFlowRectRef.current from being populated when a drag starts and ends within one frame; update the logic in useCanvasSelectionRect so you do not cancel rafRef.current when there was an active selection (wasActive === true) but lastFlowRectRef.current is still empty — either move the cancelAnimationFrame call until after lastFlowRectRef.current is written or guard the cancel with a check like if (rafRef.current && (lastFlowRectRef.current || !wasActive)) to ensure the RAF has a chance to compute and set lastFlowRectRef.current before being canceled (apply the same guard to the mirrored block around lines 103-132).src/features/canvas/components/canvas-viewer.tsx (1)
341-350:⚠️ Potential issue | 🔴 CriticalPersist resized stroke geometry, not just the display box.
StrokeNoderenders resized strokes by scaling the SVG towidth/height, whileuseCanvasSelectionRectanduseCanvasEraserhit-test againstdata.pointsanddata.bounds. Persisting onlywidth,height, andpositionmakes the rendered path drift away from the interactive path after the first resize.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/canvas-viewer.tsx` around lines 341 - 350, Persisted node update only sets width/height/position causing StrokeNode visual scaling to diverge from hit-test data; in onResizeEnd compute scaleX = width / existing.width and scaleY = height / existing.height (guard against zero), then scale the node's stroke geometry in existing.data by mapping data.points (x *= scaleX, y *= scaleY) and scaling data.bounds (x, y, width, height) before calling nodesMap.set; update the stored node to include the new data object plus the width, height, and position so useCanvasSelectionRect/useCanvasEraser hit the transformed path correctly (referencing onResizeEnd, nodesMap, StrokeNode, data.points, data.bounds).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/folders/functions/getFolderContentsForDownload.ts`:
- Around line 71-73: The suffix-check for note names currently uses
child.name.endsWith('.md') which is case-sensitive and will produce names like
Readme.MD.md; change the check in getFolderContentsForDownload (the noteName
assignment) to perform a case-insensitive test (e.g., test
child.name.toLowerCase().endsWith('.md') or use a case-insensitive regex like
/\.md$/i) so existing .MD/.Md variants are recognized and you don't append an
extra .md.
In `@convex/notes/__tests__/notes.test.ts`:
- Around line 39-40: The tests currently assert typeof result!.blockMeta ===
'object' which also passes for null; update the assertions for result!.blockMeta
in notes.test.ts (the checks around result!.blockMeta at the existing expect
lines and the similar pair at line 86) to explicitly reject null — e.g., add an
explicit non-null assertion (expect(result!.blockMeta).not.toBeNull()) before or
instead of the typeof check so null cannot satisfy the test while preserving the
array check (Array.isArray(result!.blockMeta) should remain false).
In `@convex/notes/__tests__/persistBlocks.test.ts`:
- Around line 56-108: The test suite lacks a positive round-trip for
persistNoteBlocks: add a test that creates a note via setupCampaignContext/asDm,
sends a non-empty Yjs update using api.yjsSync.mutations.pushUpdate (use or
extend makeYjsUpdate to include at least one paragraph/text delta), then call
api.notes.mutations.persistNoteBlocks with that documentId and assert it returns
expected data (or non-null) and that the DB 'blocks' rows were inserted by
querying the blocks table (same pattern as existing empty test using
dbCtx.db.query('blocks')). Ensure you reference the same helpers
(setupCampaignContext, asDm, makeYjsUpdate, pushUpdate, persistNoteBlocks) and
assert on block count and expected block fields to validate the conversion.
In `@convex/sidebarItems/__tests__/previewCleanup.test.ts`:
- Around line 33-35: Add a new test case to previewCleanup.test.ts mirroring
existing preview cleanup tests but using the canvas sidebar item type: use the
test context from createTestContext(), create a canvas via the createCanvas
factory (ensure createCanvas is exported from convex/_test/factories.helper.ts),
add it to the sidebar as a preview, perform the hard delete action used by the
other tests, and assert that the preview cleanup behavior matches other item
types; reference and reuse the same helper setup and assertions used in the
existing preview cleanup tests so the new test for canvas follows the same
structure and expectations.
In `@convex/sidebarItems/__tests__/previewGeneration.test.ts`:
- Around line 21-425: Add tests exercising the new "canvas" sidebar item type by
mirroring existing note tests: use createCanvas (or the project helper that
creates a canvas) to create a canvas and then call
api.sidebarItems.mutations.claimPreviewGeneration and
api.sidebarItems.mutations.setPreviewImage to assert claim/upload behavior
(claimed true/false, previewLockedUntil, previewStorageId, previewUpdatedAt, old
blob deletion, permission checks for edit/view/no-share, NOT_FOUND/auth checks).
Also add a case in the enhanceBase previewUrl resolution tests that creates a
canvas, patches previewStorageId, then queries
api.sidebarItems.queries.getSidebarItemsByLocation to assert that the canvas
item._id yields a previewUrl string when storage is set and null when not.
In `@convex/sidebarItems/functions/setPreviewImage.ts`:
- Around line 36-46: Keep the current try/catch in setPreviewImage around
ctx.storage.delete to avoid failing the mutation on delete errors, but add (1) a
short TODO comment next to the ctx.storage.delete call noting the need for a
periodic reconciliation/cleanup job to remove orphaned blobs, and (2) implement
(or create a new task placeholder for) a scheduled cleanup function (e.g.,
reconcilePreviewStorage or cleanupOrphanedPreviews) that scans storage entries
against active previewStorageId references and deletes unreferenced blobs;
reference the ctx.storage.delete call, the oldPreviewStorageId and
previewStorageId variables, and the setPreviewImage function when adding the
TODO and creating the cleanup task placeholder.
In `@convex/sidebarItems/validation.ts`:
- Around line 295-311: The .unique() call on the by_campaign_slug query will
throw if duplicate {campaignId, slug} pairs exist; remove .unique() in
queryTable (the ctx.db.query(...).withIndex('by_campaign_slug', ...) chain) and
instead return all matches, then compute conflict by finding any returned item
whose id !== excludeId (use the existing campaignId and slug inputs and the
variable excludeId) so create/rename operations treat duplicates as conflicts
instead of throwing; update the code that currently awaits
Promise.all([queryTable('notes'), ...]) to handle arrays and set conflict =
firstMatchFromAnyTable.where(item => item.id !== excludeId).
In `@convex/yjsSync/__tests__/yjsSyncMutations.test.ts`:
- Line 15: The test "does not trigger compaction before interval" (and similarly
the test block around lines 167-179) still uses hardcoded loop bounds and
expected counts (looping to 19 and expecting 20), so replace those hardcoded
values with expressions derived from COMPACT_INTERVAL: loop up to
COMPACT_INTERVAL - 1 and assert the expected row count equals COMPACT_INTERVAL
(or use COMPACT_INTERVAL - 1 / COMPACT_INTERVAL as appropriate for each
assertion); update any iterations, expectedCounts, or magic numbers in the test
functions (e.g., the loop bounds and expect(...) calls) to reference
COMPACT_INTERVAL so the tests track the constant instead of a fixed 19/20.
In `@convex/yjsSync/functions/compactUpdates.ts`:
- Line 23: The code sets maxSeq from the last array element (const maxSeq =
updates[updates.length - 1].seq) which wrongly assumes reconstructYDoc returns
updates sorted; change this to compute the maximum seq explicitly from the
updates array (e.g., use a reduce/Math.max over updates.map(u => u.seq)) so
maxSeq is correct regardless of ordering; update the compactUpdates logic that
uses maxSeq and leave a short comment referencing reconstructYDoc to clarify the
ordering contract if you choose to keep it.
In `@src/features/canvas/components/canvas-color-panel.tsx`:
- Around line 120-131: The color swatch buttons (in the canvas-color-panel
component) only indicate selection visually; update the button elements (where
handleColorChange is used and activeColor is compared to color) to expose
selection to assistive tech by adding an aria-pressed attribute set to
(activeColor === color) so the active swatch reads as pressed/selected to screen
readers; ensure the boolean value is passed (true/false) rather than a string.
In `@src/features/canvas/components/nodes/embed-content/embed-canvas-content.tsx`:
- Around line 224-264: The query cursor must be keyed to the canvasId so we
don't fetch or apply updates from the previous canvas: change the getUpdates
call so its query key/params include the canvasId (not just documentId and
afterSeq separately) or build a memoized params object that embeds canvasId with
afterSeq, ensuring the query cache is unique per canvas; when resetting in the
canvasId useEffect set afterSeq and lastAppliedSeqRef and then trigger a new
query for the new canvasId (so updatesResult corresponds to the new canvas), and
in the update application effect (which references updatesResult.data, doc,
lastAppliedSeqRef, canvasIdRef) guard by checking the query’s embedded canvasId
matches canvasIdRef.current before applying entries and updating
lastAppliedSeqRef and setAfterSeq. Use the existing symbols: afterSeq,
setAfterSeq, canvasIdRef, lastAppliedSeqRef,
useAuthQuery(api.yjsSync.queries.getUpdates), updatesResult, and the two
useEffect blocks to implement this.
In `@src/features/canvas/hooks/useCanvasStrokeClick.ts`:
- Around line 12-61: The onPaneClick callback is being recreated on every zoom
change which may cause needless re-renders; instead store zoom in a ref and read
from that ref inside onPaneClick so the callback can be stable. Create a zoomRef
updated in an effect when zoom changes, remove zoom from the onPaneClick
dependency array, and have onPaneClick use zoomRef.current when computing
threshold (STROKE_HIT_PADDING_PX / zoomRef.current) before calling
pointNearStrokePath and reactFlow.setNodes so behavior remains correct but the
callback identity stays stable.
In `@src/features/canvas/hooks/useNodeEditing.ts`:
- Around line 41-46: The Enter-key commit in handleKeyDown currently fires even
during IME composition; add a guard that checks e.nativeEvent.isComposing (or
e.isComposing) and only proceed to preventDefault/stopPropagation/commit when
composition is not active; update the Enter branch in handleKeyDown that
manipulates shouldCommitRef.current and calls commitEdit(value) to first return
early if e.nativeEvent.isComposing is true so IME input isn't committed
prematurely.
In `@src/features/editor/components/note-content.tsx`:
- Line 221: Add an ESLint disable comment to suppress the exhaustive-deps
warning for the useEffect that intentionally omits user.name and user.color:
locate the useEffect hook whose dependency array is "}, [doc, provider])" in the
NoteContent component and add a comment like "// eslint-disable-next-line
react-hooks/exhaustive-deps" immediately above that line so the linter knows the
omission of user.name and user.color is intentional while keeping doc and
provider as dependencies.
---
Outside diff comments:
In `@src/features/sidebar/utils/sidebar-item-utils.ts`:
- Around line 59-67: The JSDoc for the isFile type guard is malformed due to a
duplicate opening `/**`; edit the JSDoc above the exported function isFile to
remove the extra `/**` so it begins with a single valid `/**` and maintains the
existing description, leaving the function signature, isSidebarItemType call,
and SIDEBAR_ITEM_TYPES reference unchanged.
---
Duplicate comments:
In `@convex/folders/functions/getFolderContentsForDownload.ts`:
- Around line 116-118: The switch in getFolderContentsForDownload currently
ignores canvases (case SIDEBAR_ITEM_TYPES.canvases: break) which returns
incomplete exports; change this to explicitly fail or report unsupported items
instead of breaking — e.g., in the getFolderContentsForDownload switch for
SIDEBAR_ITEM_TYPES.canvases throw or return a descriptive error/unsupported-item
result (including the item id/context) so callers know canvases are not handled;
ensure the error type/message is consistent with surrounding error handling in
this function.
In `@convex/notes/functions/createNote.ts`:
- Around line 81-91: The code currently instantiates client internals via
BlockNoteEditor.create({_headless: true}) and accesses _tiptapEditor, which is
fragile for server-side conversion; replace this with ServerBlockNoteEditor
(imported from blocknote) and use ServerBlockNoteEditor.create(...) when
creating the editor used by blocksToYDoc so you never touch client-only
internals. Update the code paths that call BlockNoteEditor.create,
blocksToYDoc(editor, content, 'document'), editor._tiptapEditor.destroy() and
editor.destroy() to instead create/destroy a ServerBlockNoteEditor instance and
pass that to blocksToYDoc, keep the Y.encodeStateAsUpdate and uint8ToArrayBuffer
flow the same, and remove any references to _headless or _tiptapEditor.
In `@src/features/canvas/components/canvas-color-panel.tsx`:
- Around line 58-64: The current scheduleNodeUpdate merges new edits into
pendingUpdate.current but reuses pendingUpdate.current.nodeIds via the `??`
fallback, causing later changes within the same RAF to be applied to stale
nodeIds; update scheduleNodeUpdate so nodeIds are derived from the current
selection instead of preserving the old pending nodeIds (i.e., compute nodeIds
as colorRelevantNodes.map(n => n.id) rather than using
`pendingUpdate.current?.nodeIds ?? ...`) so a changed selection resets the
pending batch target.
In `@src/features/canvas/components/canvas-minimap-node.tsx`:
- Around line 45-56: The minimap SVG in the CanvasMinimapNode component is
missing preserveAspectRatio="none", causing inconsistent rendering versus the
main StrokePreview; update the <svg> returned in the CanvasMinimapNode (the JSX
that uses x, y, width, height, viewBox and safeWidth/safeHeight) to include
preserveAspectRatio="none" so non-uniform resizes match the main stroke
rendering.
In `@src/features/canvas/components/canvas-viewer.tsx`:
- Around line 341-350: Persisted node update only sets width/height/position
causing StrokeNode visual scaling to diverge from hit-test data; in onResizeEnd
compute scaleX = width / existing.width and scaleY = height / existing.height
(guard against zero), then scale the node's stroke geometry in existing.data by
mapping data.points (x *= scaleX, y *= scaleY) and scaling data.bounds (x, y,
width, height) before calling nodesMap.set; update the stored node to include
the new data object plus the width, height, and position so
useCanvasSelectionRect/useCanvasEraser hit the transformed path correctly
(referencing onResizeEnd, nodesMap, StrokeNode, data.points, data.bounds).
In `@src/features/canvas/components/nodes/resizable-node-wrapper.tsx`:
- Around line 36-70: The shift tracking currently removes key listeners when the
last subscriber is removed causing shiftPressed to stay true if Shift is
released while listeners are removed; change subscribeShift/remove logic so the
global listeners (onShiftDown, onShiftUp) are attached once and never removed
(i.e., don’t remove listeners when shiftSubscribers becomes empty), and add a
window 'blur' handler that clears shiftPressed and calls notifyShiftSubscribers
(use onShiftDown/onShiftUp names and the shiftPressed and shiftSubscribers
symbols) so alt-tab/blur resets state; keep subscribeShift returning an
unsubscribe that only removes the callback from shiftSubscribers but does not
detach the global event listeners.
In `@src/features/canvas/hooks/useCanvasEraser.ts`:
- Around line 90-107: Pointer-up currently cancels the pending eraser RAF and
clears state before the last trail segment is processed, so call the same final
processing path synchronously in onPointerUp to flush the last segment: before
cancelling eraserRafRef and resetting trailRef/markedRef, run the eraser frame
handler (the logic that calls testIntersections and updates markedRef) so
markedRef contains any final hits and nodesMap deletions happen, then proceed to
cancelAnimationFrame(eraserRafRef.current), clear
erasingRef/eraserRafRef/trailRef and call
useCanvasToolStore.getState().setErasingStrokeIds(new Set()); reference
onPointerUp, eraserRafRef, trailRef, markedRef, testIntersections, and
nodesMap.delete for locating where to insert the synchronous flush.
In `@src/features/canvas/hooks/useCanvasFileUpload.tsx`:
- Around line 92-116: When createItem fails after commitUpload.mutateAsync, the
code currently only logs and returns null, leaving the committed storage
orphaned; modify the catch block for createItem (the one handling createError
and referencing storageId and file.name) to call the existing storage cleanup
path (or enqueue a cleanup job) with storageId before showing the toast and
returning, e.g., invoke the project's cleanup API/function that handles durable
storage cleanup (the function used elsewhere for removing or enqueuing cleanup
of committed storage) so the orphaned blob is removed or scheduled for deletion
prior to surfacing the error.
In `@src/features/canvas/hooks/useCanvasKeyboardShortcuts.ts`:
- Around line 20-28: The undo/redo keyboard handling currently mixes
KeyboardEvent.code with KeyboardEvent.key which breaks non-QWERTY layouts; in
useCanvasKeyboardShortcuts.ts remove the use of the local `code` variable and
change the undo/redo conditionals (the branches that call `undo()` and `redo()`)
to match only the normalized `key` (e.g., `key === 'z'` and `key === 'y'`) while
keeping the `e.shiftKey` checks and preventDefault/dispatch behavior intact so
only character-aware `e.key` drives history actions.
In `@src/features/canvas/hooks/useCanvasSelectionRect.ts`:
- Around line 59-69: The code cancels the pending RAF unconditionally which can
prevent lastFlowRectRef.current from being populated when a drag starts and ends
within one frame; update the logic in useCanvasSelectionRect so you do not
cancel rafRef.current when there was an active selection (wasActive === true)
but lastFlowRectRef.current is still empty — either move the
cancelAnimationFrame call until after lastFlowRectRef.current is written or
guard the cancel with a check like if (rafRef.current &&
(lastFlowRectRef.current || !wasActive)) to ensure the RAF has a chance to
compute and set lastFlowRectRef.current before being canceled (apply the same
guard to the mirrored block around lines 103-132).
In `@src/features/editor/components/forms/file-form/file-form.tsx`:
- Around line 185-192: The PDF preview generation catch currently logs with
console.error; replace that with the centralized error handler by calling
handleError(...) inside the catch for
generatePdfPreviewIfNeeded(fileUpload.file, newFileId as Id<'files'>), passing a
descriptive message (e.g., "PDF preview generation failed for new file") and the
caught error so the create flow uses the same error handling as the update path;
ensure you import or reference handleError in this scope and preserve the
promise chain (i.e., .catch((err) => handleError('PDF preview generation failed
for new file', err))).
In `@src/features/editor/components/note-content.tsx`:
- Around line 196-214: Add an inline explanatory comment above the RAF retry
loop (the cancelled/retries/MAX_RETRIES variables and the tryPatch function)
that briefly explains this is a deliberate workaround to wait for
BlockNote/Tiptap plugins to initialize asynchronously before calling
patchYUndoPluginDestroy and patchYSyncAfterTypeChanged on
instance._tiptapEditor.view; mention why requestAnimationFrame is used, the
MAX_RETRIES guard, and that the loop avoids patching until plugins exist to
prevent race conditions.
- Around line 128-142: The useEffect that creates the editor references content
but intentionally omits it from dependencies, causing an eslint exhaustive-deps
warning; to silence this, add an inline eslint disable comment (e.g., //
eslint-disable-next-line react-hooks/exhaustive-deps) immediately above the
useEffect call that contains BlockNoteEditor.create({ schema: editorSchema,
initialContent, }) so the hook runs only once; keep the rest of the effect
(setEditor(instance), onEditorChangeRef.current?.(instance, null), and cleanup
destroying instance._tiptapEditor and calling onEditorChangeRef.current?.(null,
null)) unchanged.
In `@src/features/editor/hooks/useYjsReactFlowSync.ts`:
- Around line 193-206: The onNodeDragStop handler can leave drag state or
observer suppression stuck if nodesMap.doc is detached or a transaction throws;
move draggingIds.current.delete(n.id) out of the Yjs transaction so deletion
happens before attempting the transaction, use safe optional access like
nodesMap?.doc (and edgesMap?.doc in the corresponding edge handler), and wrap
the suppressNodeObserver.current toggling and the transact call in a try/finally
so suppressNodeObserver.current is always reset; apply the same pattern to the
other handler referenced (lines ~211-231) ensuring you use nodesMap.get /
nodesMap.set inside the transaction but clear bookkeeping and reset suppression
in finally.
In `@src/features/sidebar/components/forms/color-picker.tsx`:
- Line 3: The file mixes two import paths for the same module; remove the
separate relative import of DEFAULT_ITEM_COLOR and add DEFAULT_ITEM_COLOR to the
existing import that already brings in the sidebar color helpers (the alias
import used elsewhere in this file), so all symbols (including
DEFAULT_ITEM_COLOR) come from the single alias import for the sidebar-item-utils
module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f78dd3f9-cd83-4934-a8a2-5b60eb2175e5
📒 Files selected for processing (49)
convex/_test/factories.helper.tsconvex/canvases/functions/createCanvas.tsconvex/canvases/functions/updateCanvas.tsconvex/folders/functions/getFolderContentsForDownload.tsconvex/gameMaps/__tests__/gameMaps.test.tsconvex/notes/__tests__/notes.test.tsconvex/notes/__tests__/persistBlocks.test.tsconvex/notes/functions/createNote.tsconvex/sidebarItems/__tests__/previewCleanup.test.tsconvex/sidebarItems/__tests__/previewGeneration.test.tsconvex/sidebarItems/__tests__/sidebarItemValidation.test.tsconvex/sidebarItems/functions/claimPreviewGeneration.tsconvex/sidebarItems/functions/enhanceSidebarItem.tsconvex/sidebarItems/functions/getSidebarItemById.tsconvex/sidebarItems/functions/getSidebarItemBySlug.tsconvex/sidebarItems/functions/setPreviewImage.tsconvex/sidebarItems/schema/baseFields.tsconvex/sidebarItems/types/baseTypes.tsconvex/sidebarItems/validation.tsconvex/yjsSync/__tests__/yjsSyncMutations.test.tsconvex/yjsSync/functions/compactUpdates.tssrc/features/canvas/components/canvas-color-panel.tsxsrc/features/canvas/components/canvas-minimap-node.tsxsrc/features/canvas/components/canvas-remote-cursors.tsxsrc/features/canvas/components/canvas-viewer.tsxsrc/features/canvas/components/nodes/embed-content/embed-canvas-content.tsxsrc/features/canvas/components/nodes/resizable-node-wrapper.tsxsrc/features/canvas/components/nodes/stroke-node.tsxsrc/features/canvas/hooks/useCanvasEraser.tssrc/features/canvas/hooks/useCanvasFileUpload.tsxsrc/features/canvas/hooks/useCanvasKeyboardShortcuts.tssrc/features/canvas/hooks/useCanvasOverlayHandlers.tssrc/features/canvas/hooks/useCanvasSelectionRect.tssrc/features/canvas/hooks/useCanvasStrokeClick.tssrc/features/canvas/hooks/useCanvasWheel.tssrc/features/canvas/hooks/useNodeEditing.tssrc/features/canvas/utils/canvas-stroke-utils.tssrc/features/dnd/utils/__tests__/dnd-registry.test.tssrc/features/dnd/utils/dnd-registry.tssrc/features/editor/components/forms/file-form/file-form.tsxsrc/features/editor/components/note-content.tsxsrc/features/editor/components/viewer/folder/note-card.tsxsrc/features/editor/components/viewer/map/map-viewer.tsxsrc/features/editor/hooks/useYjsReactFlowSync.tssrc/features/previews/utils/upload-preview.tssrc/features/sidebar/components/forms/color-picker.tsxsrc/features/sidebar/components/sidebar-root/droppable-root.tsxsrc/features/sidebar/utils/sidebar-item-utils.tssrc/styles/app.css
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 (2)
src/features/dnd/utils/__tests__/dnd-registry.test.ts (1)
24-36:⚠️ Potential issue | 🟡 MinorAdd tests for new canvas drop-zone branches.
This suite was updated for the moved
SIDEBAR_ROOT_TYPEimport, but it still doesn’t assert the newly added canvas behavior (embed,self_embed, canvas target key/highlight/file acceptance). Please add focused tests for those paths to cover the new DnD logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/dnd/utils/__tests__/dnd-registry.test.ts` around lines 24 - 36, Add unit tests covering the new canvas drop-zone branches: write focused tests that call resolveDropTarget and resolveDropOutcome with drag items representing canvas embeds (use getDragItemId to create/embed ids if needed) to assert the returned target types include the new 'embed' and 'self_embed' outcomes; also add assertions for getDropTargetKey and getHighlightId to verify canvas-specific target keys/highlight ids are produced, and use canDropFilesOnTarget to confirm file-acceptance behavior for canvas targets. Target the symbols resolveDropTarget, resolveDropOutcome, getDropTargetKey, getHighlightId, canDropFilesOnTarget and reuse existing constants like SIDEBAR_ROOT_TYPE, MAP_DROP_ZONE_TYPE and EMPTY_EDITOR_DROP_TYPE to construct scenarios for external embed, self-embed, and file-drop acceptance.src/features/dnd/utils/dnd-registry.ts (1)
210-211:⚠️ Potential issue | 🟡 MinorMake
trashed_itemrejection text target-agnostic.
trashed_itemis now used by canvas embedding (Line 352), but the current message says “link,” which is misleading outside note-editor link flows.💡 Proposed fix
case 'trashed_item': - return 'Cannot link to a trashed item' + return 'Cannot use a trashed item here'Also applies to: 352-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/dnd/utils/dnd-registry.ts` around lines 210 - 211, The rejection message for the 'trashed_item' case is link-specific; change it to a target-agnostic phrase used by both note-editor link flows and canvas embedding (refer to the 'trashed_item' case in dnd-registry.ts and the other occurrence used by canvas embedding). Replace the returned string "Cannot link to a trashed item" with a generic message such as "The item is trashed and cannot be used" (or equivalent) in the 'trashed_item' case so all consumers get a neutral, accurate rejection text.
♻️ Duplicate comments (24)
src/features/canvas/hooks/useCanvasFileUpload.tsx (2)
46-58: 🧹 Nitpick | 🔵 TrivialExtract
isTextFileresult to a local variable to avoid repeated calls.
isTextFile(file.type, file.name)is evaluated three times with identical arguments (lines 50, 52, and 58). Extract to a local variable for clarity and to avoid redundant computation.+ const isText = isTextFile(file.type, file.name) + const toastId = toast.loading( <ToastContent title={file.name} - message={ - isTextFile(file.type, file.name) ? 'Processing...' : 'Uploading... 0%' - } - progress={isTextFile(file.type, file.name) ? undefined : 0} + message={isText ? 'Processing...' : 'Uploading... 0%'} + progress={isText ? undefined : 0} />, { duration: Infinity, style: TOAST_STYLE }, ) try { - if (isTextFile(file.type, file.name)) { + if (isText) {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/hooks/useCanvasFileUpload.tsx` around lines 46 - 58, The call is repeating isTextFile(file.type, file.name) multiple times; capture its result once (e.g., const isText = isTextFile(file.type, file.name)) before creating the toast and use that variable in the ToastContent props and the subsequent try block so ToastContent.title/message/progress and the if (isTextFile(...)) check all reference the single local variable (look for the toast.loading(...) call that constructs ToastContent and the following if block).
93-117:⚠️ Potential issue | 🟡 MinorStorage blob remains orphaned if
createItemfails aftercommitUploadsucceeds.The inner try-catch logs the orphaned storage (line 108) but doesn't perform cleanup. The
fileStoragerecord exists in the database without a referencing sidebar item. Consider adding a compensating deletion (e.g., calling adeleteFileStoragemutation with thestorageId) or scheduling a background cleanup job for unreferenced storage records.🛡️ Suggested compensation pattern
} catch (createError) { // Storage is committed but sidebar item creation failed — storage is orphaned console.error('Orphaned storage after failed createItem:', storageId) + // Attempt to clean up orphaned storage + try { + await deleteFileStorage.mutateAsync({ storageId }) + } catch (cleanupError) { + console.error('Failed to cleanup orphaned storage:', cleanupError) + } toast.error(This requires adding a
deleteFileStoragemutation. Alternatively, implement a scheduled job to periodically clean upfileStoragerecords without corresponding sidebar items.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/hooks/useCanvasFileUpload.tsx` around lines 93 - 117, The catch block after createItem in useCanvasFileUpload.tsx currently logs orphaned storage but doesn't clean it up; update the error handling to invoke a compensating deletion (e.g., call the deleteFileStorage mutation with storageId) when createItem fails after commitUpload, await and handle errors from that deletion (log failures and surface a toast if needed), and if immediate deletion isn't possible implement scheduling of a background cleanup job for unreferenced fileStorage records; ensure you reference storageId and the createItem failure path so orphaned records are removed or tracked for cleanup.src/features/canvas/hooks/useNodeEditing.ts (1)
41-46:⚠️ Potential issue | 🟠 MajorGuard Enter commit while IME composition is active.
Enter currently commits without checking composition state, so IME users can save incomplete composed text. Add an early return before the commit branch.
Suggested patch
const handleKeyDown = useCallback( (e: React.KeyboardEvent, value: string) => { if (e.key === 'Enter' && !e.shiftKey) { + if (e.nativeEvent.isComposing) return e.preventDefault() e.stopPropagation() shouldCommitRef.current = false commitEdit(value) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/hooks/useNodeEditing.ts` around lines 41 - 46, The Enter key handler in useNodeEditing commits even when IME composition is active; add an early guard before the commit branch to skip handling if composition is in progress (check e.isComposing || a compositionRef/composing flag used in the hook). In the keydown handler where you currently check if (e.key === 'Enter' && !e.shiftKey) and call commitEdit(value), return early when IME composition is active (e.isComposing === true or compositionRef.current === true) so shouldCommitRef/commitEdit are not invoked during composition.convex/notes/__tests__/persistBlocks.test.ts (1)
56-108:⚠️ Potential issue | 🟡 MinorAdd a non-empty Yjs round-trip test for
persistNoteBlocks.Current cases only prove empty input behavior. Please add one positive path that pushes a non-empty Yjs update, runs
persistNoteBlocks, and asserts rows are actually inserted intoblocksfor thatnoteId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/notes/__tests__/persistBlocks.test.ts` around lines 56 - 108, Add a positive round-trip test that pushes a non-empty Yjs update, calls persistNoteBlocks, and asserts blocks were inserted: reuse setupCampaignContext and asDm to create a note (like in existing tests), create a non-empty Yjs update (e.g., use makeYjsUpdate with a text/paragraph payload or build a Y.Doc with text and serialize to an update), call dmAuth.mutation(api.yjsSync.mutations.pushUpdate, { documentId: noteId, update: <non-empty update> }), then call dmAuth.mutation(api.notes.mutations.persistNoteBlocks, { documentId: noteId }) and inside t.run query the blocks table (db.query('blocks').filter(q => q.eq(q.field('noteId'), noteId)).collect()) and assert blocks.length > 0 and optionally validate block content.convex/notes/__tests__/notes.test.ts (1)
39-40:⚠️ Potential issue | 🟡 MinorStrengthen
blockMetaassertions to explicitly rejectnull.
typeof ... === 'object'passes fornull, so these checks can succeed on invalid data. Add a non-null assertion before the type check in both locations.Suggested fix
+ expect(result!.blockMeta).not.toBeNull() expect(typeof result!.blockMeta).toBe('object') expect(Array.isArray(result!.blockMeta)).toBe(false) @@ + expect(result!.blockMeta).not.toBeNull() expect(typeof result!.blockMeta).toBe('object') expect(Array.isArray(result!.blockMeta)).toBe(false)Also applies to: 86-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/notes/__tests__/notes.test.ts` around lines 39 - 40, The tests currently use typeof result!.blockMeta === 'object' which will be true for null; update the assertions around result!.blockMeta (both where you check typeof and Array.isArray — currently lines using expect(typeof result!.blockMeta).toBe('object') and expect(Array.isArray(result!.blockMeta)).toBe(false)) to first assert result!.blockMeta is not null (e.g., expect(result!.blockMeta).not.toBeNull()) before the typeof check and the Array.isArray check so null values are explicitly rejected; apply the same non-null assertion to the duplicate assertions around lines 86-87 as well.convex/yjsSync/__tests__/yjsSyncMutations.test.ts (1)
202-226:⚠️ Potential issue | 🟡 MinorDe-hardcode the companion pre-interval test as well.
Line 212 and Line 225 still use fixed
19/20, so this test can silently become incorrect ifCOMPACT_INTERVALchanges.Suggested fix
- for (let i = 1; i <= 19; i++) { + for (let i = 1; i <= COMPACT_INTERVAL - 1; i++) { await dmAuth.mutation(api.yjsSync.mutations.pushUpdate, { documentId: noteId, update: makeEmptyYjsUpdate(), }) } @@ - expect(rows).toHaveLength(20) + expect(rows).toHaveLength(COMPACT_INTERVAL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/yjsSync/__tests__/yjsSyncMutations.test.ts` around lines 202 - 226, The test "does not trigger compaction before interval" hardcodes 19/20; replace those magic numbers with the shared COMPACT_INTERVAL constant so the test tracks the real compaction threshold. Import or reference COMPACT_INTERVAL (the constant used by the yjs compaction logic) in the test, change the loop to iterate for (let i = 1; i <= COMPACT_INTERVAL - 1; i++) when calling api.yjsSync.mutations.pushUpdate, and assert expect(rows).toHaveLength(COMPACT_INTERVAL) instead of 20; keep using noteId, dmAuth, and makeEmptyYjsUpdate as before.src/features/canvas/hooks/useCanvasSelectionRect.ts (1)
59-69:⚠️ Potential issue | 🟠 MajorStale
lastFlowRectRef.currentwhen RAF is cancelled before firing.When a drag starts and ends within a single frame, the RAF at line 103 is cancelled (lines 62-65) before it can update
lastFlowRectRef.current. The deselection branch at line 68 then reads a stale or null value, causing false-positive stroke selections to never get corrected.Consider computing the flow rect from
prevRectdirectly when the selection ends, rather than relying on the RAF-populated ref:🐛 Proposed fix
const wasActive = prevRect !== null + const previousDomRect = prevRect prevRect = userSelectionRect if (rafRef.current) { cancelAnimationFrame(rafRef.current) rafRef.current = 0 } if (!userSelectionRect) { - if (wasActive && lastFlowRectRef.current) { - const selRect = lastFlowRectRef.current + const state = storeApi.getState() + let selRect = lastFlowRectRef.current + if (!selRect && wasActive && previousDomRect && state.domNode) { + const bounds = state.domNode.getBoundingClientRect() + const topLeft = reactFlow.screenToFlowPosition({ + x: previousDomRect.x + bounds.left, + y: previousDomRect.y + bounds.top, + }) + const bottomRight = reactFlow.screenToFlowPosition({ + x: previousDomRect.x + previousDomRect.width + bounds.left, + y: previousDomRect.y + previousDomRect.height + bounds.top, + }) + selRect = { + x: topLeft.x, + y: topLeft.y, + width: bottomRight.x - topLeft.x, + height: bottomRight.y - topLeft.y, + } + } + if (wasActive && selRect) { reactFlow.setNodes((nodes) =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/hooks/useCanvasSelectionRect.ts` around lines 59 - 69, The deselection branch reads lastFlowRectRef.current which can be stale if the RAF (managed by rafRef) was cancelled before it updated that ref; update the branch in useCanvasSelectionRect to compute the flow rectangle from the previous selection (prevRect) when userSelectionRect becomes null (i.e., when wasActive is true) instead of relying solely on lastFlowRectRef.current—use prevRect to derive selRect (with lastFlowRectRef.current as a fallback) and proceed with the existing deselect logic so short-lived drags that never let the RAF fire still clear selections correctly.src/features/canvas/components/canvas-minimap-node.tsx (1)
46-56:⚠️ Potential issue | 🟡 MinorMissing
preserveAspectRatio="none"for minimap stroke consistency.
StrokePreviewusespreserveAspectRatio="none"(line 37 in stroke-node.tsx), but this minimap SVG uses the defaultpreserveAspectRatio. After a non-uniform resize, the minimap stroke will no longer match the canvas shape.🐛 Proposed fix
<svg x={x} y={y} width={width} height={height} viewBox={`${data.bounds.x} ${data.bounds.y} ${safeWidth} ${safeHeight}`} + preserveAspectRatio="none" overflow="visible" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/canvas-minimap-node.tsx` around lines 46 - 56, The SVG in canvas-minimap-node.tsx is missing preserveAspectRatio="none", causing minimap strokes to distort after non-uniform resizes; update the SVG element (the one using props x, y, width, height and viewBox with data.bounds and safeWidth/safeHeight) to include preserveAspectRatio="none" to match StrokePreview and ensure stroke rendering remains consistent with the canvas.src/features/canvas/hooks/useCanvasEraser.ts (1)
90-107:⚠️ Potential issue | 🟠 MajorFinal eraser segment not flushed on pointer-up.
When the user lifts the pointer,
onPointerUpcancels the pending RAF (lines 92-95) but never callstestIntersections()for the accumulated trail. A fast erase gesture that ends before the RAF fires can miss the final crossed strokes.🐛 Proposed fix
- const onPointerUp = useCallback(() => { + const onPointerUp = useCallback((e: React.PointerEvent) => { erasingRef.current = false if (eraserRafRef.current) { cancelAnimationFrame(eraserRafRef.current) eraserRafRef.current = 0 } + // Flush final segment before deleting + const pos = reactFlow.screenToFlowPosition({ + x: e.clientX, + y: e.clientY, + }) + trailRef.current.push(pos) + testIntersections() + const marked = markedRef.current if (nodesMap.doc && marked.size > 0) { nodesMap.doc.transact(() => { for (const id of marked) { nodesMap.delete(id) } }) } markedRef.current = new Set() trailRef.current = [] useCanvasToolStore.getState().setErasingStrokeIds(new Set()) - }, [nodesMap]) + }, [nodesMap, reactFlow, testIntersections])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/hooks/useCanvasEraser.ts` around lines 90 - 107, onPointerUp currently cancels the pending RAF and clears state without flushing the final accumulated trail, so ensure the final segment is processed by calling the same intersection test routine before cancelling/clearing: invoke testIntersections() (the function used by the RAF) against trailRef.current/markedRef.current right when pointer lifts (e.g., at the start of onPointerUp), then proceed to cancelAnimationFrame(eraserRafRef.current) and clear eraserRafRef, trailRef, and markedRef; reference symbols: onPointerUp, testIntersections, eraserRafRef, trailRef, markedRef, nodesMap, useCanvasToolStore.setErasingStrokeIds.src/features/canvas/utils/canvas-stroke-utils.ts (1)
87-103:⚠️ Potential issue | 🟠 Major
segmentsIntersectreturnsfalsefor collinear overlapping segments.When
denom ≈ 0(parallel segments), the function returnsfalseeven if the segments overlap. This causes hit-testing to miss cases where the eraser trail or selection edge runs exactly along a stroke boundary.🐛 Proposed fix using orientation-based intersection
+function cross( + ax: number, ay: number, + bx: number, by: number, + cx: number, cy: number, +): number { + return (bx - ax) * (cy - ay) - (by - ay) * (cx - ax) +} + +function onSegment( + ax: number, ay: number, + bx: number, by: number, + px: number, py: number, +): boolean { + return ( + px >= Math.min(ax, bx) - 1e-10 && + px <= Math.max(ax, bx) + 1e-10 && + py >= Math.min(ay, by) - 1e-10 && + py <= Math.max(ay, by) + 1e-10 + ) +} + export function segmentsIntersect( ax: number, ay: number, bx: number, by: number, cx: number, cy: number, dx: number, dy: number, ): boolean { - const denom = (bx - ax) * (dy - cy) - (by - ay) * (dx - cx) - if (Math.abs(denom) < 1e-10) return false - - const t = ((cx - ax) * (dy - cy) - (cy - ay) * (dx - cx)) / denom - const u = ((cx - ax) * (by - ay) - (cy - ay) * (bx - ax)) / denom - return t >= 0 && t <= 1 && u >= 0 && u <= 1 + const o1 = cross(ax, ay, bx, by, cx, cy) + const o2 = cross(ax, ay, bx, by, dx, dy) + const o3 = cross(cx, cy, dx, dy, ax, ay) + const o4 = cross(cx, cy, dx, dy, bx, by) + + // Collinear cases + if (Math.abs(o1) < 1e-10 && onSegment(ax, ay, bx, by, cx, cy)) return true + if (Math.abs(o2) < 1e-10 && onSegment(ax, ay, bx, by, dx, dy)) return true + if (Math.abs(o3) < 1e-10 && onSegment(cx, cy, dx, dy, ax, ay)) return true + if (Math.abs(o4) < 1e-10 && onSegment(cx, cy, dx, dy, bx, by)) return true + + // General case + return (o1 > 0) !== (o2 > 0) && (o3 > 0) !== (o4 > 0) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/utils/canvas-stroke-utils.ts` around lines 87 - 103, segmentsIntersect currently bails out when denom ≈ 0 (parallel/collinear) which misses collinear overlapping segments; update segmentsIntersect to handle the collinear case by detecting when denom is nearly zero and then checking whether the four points are collinear (use the same cross-product/orientation test used for denom) and if so determine interval overlap by comparing projections of the segments on an axis (e.g., compare min/max of ax/bx with cx/dx or ay/by with cy/dy depending on dominant axis) to return true for overlapping ranges; keep the existing t/u logic for the non-parallel case and only run the collinear overlap check when Math.abs(denom) < 1e-10.src/features/canvas/components/canvas-viewer.tsx (3)
470-478: 🧹 Nitpick | 🔵 Trivial
refas a regular prop is a legacy pattern.In React 19, using
refas a destructured prop in a function component works but may cause confusion. Consider usingforwardRefor renaming to a non-reserved name likeoverlayRef.♻️ Proposed fix using forwardRef
-function CanvasDropOverlay({ - ref, - isDropTarget, - isFileDropTarget, -}: { - ref: React.RefObject<HTMLDivElement | null> - isDropTarget: boolean - isFileDropTarget: boolean -}) { +import { forwardRef } from 'react' + +const CanvasDropOverlay = forwardRef< + HTMLDivElement, + { isDropTarget: boolean; isFileDropTarget: boolean } +>(function CanvasDropOverlay({ isDropTarget, isFileDropTarget }, ref) { // ... component body -} +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/canvas-viewer.tsx` around lines 470 - 478, The component CanvasDropOverlay currently takes a prop named ref which shadows the reserved React ref pattern; update CanvasDropOverlay to accept a forwarded ref (use React.forwardRef) or rename the prop to a non-reserved name like overlayRef throughout the component usage and type signature, and adjust any internal references and parent callers that pass the prop (preserve isDropTarget and isFileDropTarget props unchanged); ensure the exported component uses forwardRef if keeping the external ref behavior so React refs work correctly.
222-230:⚠️ Potential issue | 🟠 MajorMutation hooks not gated by
canEdit— read-only users may still trigger mutations.The hooks
useCanvasDrawing,useCanvasEraser,useCanvasLassoSelection, anduseCanvasRectangleDraw(lines 224-230) are initialized unconditionally withoutcanEditchecks. While some UI interactions are gated (e.g., line 234),useCanvasOverlayHandlers(line 246) may still bind pointer handlers from these hooks regardless ofcanEdit, allowing mutations via:
drawing.onPointerUp()→nodesMap.set()eraser.onPointerUp()→nodesMap.delete()rectangleDraw.onPointerUp()→nodesMap.set()Pass
canEditto these hooks and have them return no-op handlers when editing is disabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/canvas-viewer.tsx` around lines 222 - 230, The four canvas mutation hooks (useCanvasDrawing, useCanvasEraser, useCanvasLassoSelection, useCanvasRectangleDraw) are created unconditionally and can still expose pointer handlers that call nodesMap.set/delete even when the user is read-only; update the calls to pass the canEdit flag into each hook and change their implementations to return no-op handlers (e.g., onPointerDown/Move/Up, start/stop, etc.) when canEdit is false so that useCanvasOverlayHandlers (and any binding code) cannot trigger mutations like nodesMap.set() or nodesMap.delete() for read-only users.
381-389: 🧹 Nitpick | 🔵 TrivialConsider adding
setEditingEmbedIdtouseMemodependency array.While
useStatesetters are guaranteed stable by React, omittingsetEditingEmbedIdfrom the dependency array may trigger ESLintexhaustive-depswarnings. Adding it for completeness has no runtime impact.♻️ Proposed fix
[ updateNodeData, onResizeEnd, remoteHighlights, canEdit, canvasUser, editingEmbedId, + setEditingEmbedId, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/canvas-viewer.tsx` around lines 381 - 389, The useMemo dependency array that currently lists updateNodeData, onResizeEnd, remoteHighlights, canEdit, canvasUser, and editingEmbedId should also include the state setter setEditingEmbedId to satisfy exhaustive-deps; update the dependency array (where useMemo is declared) to add setEditingEmbedId—this has no runtime impact but prevents ESLint warnings and ensures completeness.src/features/canvas/components/nodes/stroke-node.tsx (2)
63-93:⚠️ Potential issue | 🔴 CriticalStroke resize does not update underlying geometry — hit-testing will drift from rendered display.
The
StrokeNoderenders geometry scaled viawidth/heightprops (lines 63-64), butonResizeEndincanvas-viewer.tsxonly persistswidth,height, andpositionwithout updatingdata.pointsordata.bounds. Hit-testing tools (click, eraser, selection) use the originaldata.pointstranslated bydata.bounds, so after resize the interactive and rendered geometry diverge.Either update the stroke data during resize (recompute scaled points/bounds) or adjust all hit-testing logic to account for the display scale factor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/nodes/stroke-node.tsx` around lines 63 - 93, Stroke rendering is being scaled by width/height in StrokeNode but onResizeEnd (in canvas-viewer.tsx) only saves width/height/position, leaving data.points and data.bounds unchanged so hit-testing drifts; fix by, in the resize handler (onResizeEnd), computing scaleX = newWidth / data.bounds.width and scaleY = newHeight / data.bounds.height (handle zero-safe), map over data.points to multiply each point.x by scaleX and point.y by scaleY, update data.bounds (x/y can be set to new position, width/height to newWidth/newHeight) and also scale any stroke size/pressure field if present (e.g., data.size) before persisting the updated stroke object; ensure StrokeNode continues to use data.points and data.bounds so hit-testing matches rendered output.
40-44:⚠️ Potential issue | 🟡 MinorInconsistent opacity scale between
opacityOverrideanddata.opacity.
opacityOverrideexpects a 0–1 value (e.g.,0.3for erasing), butdata.opacityis treated as 0–100 and divided by 100. If a caller storesdata.opacityin the 0–1 range, the stroke will be nearly invisible.🐛 Proposed fix — normalize to 0–1 convention
d={d} fill={color} - opacity={opacityOverride ?? (data.opacity ?? 100) / 100} + opacity={opacityOverride ?? data.opacity ?? 1}Ensure stroke creators store
data.opacityas 0–1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/nodes/stroke-node.tsx` around lines 40 - 44, The opacity handling is inconsistent: opacityOverride is 0–1 but data.opacity is treated as 0–100; update the StrokeNode rendering to normalize to a 0–1 convention by computing a single opacity value used in the <path> element (use opacityOverride if provided, otherwise use data.opacity normalized to 0–1). In practice, change the logic around opacityOverride and data.opacity in stroke-node.tsx (the code that currently computes opacity as opacityOverride ?? (data.opacity ?? 100) / 100) to: choose opacityOverride when present; else if data.opacity is present treat values >1 as legacy 0–100 and divide by 100, otherwise use the 0–1 value as-is (and fallback to 1 when undefined) so both sources produce a 0–1 opacity for the path.convex/notes/functions/createNote.ts (2)
2-3:⚠️ Potential issue | 🟠 MajorUse
ServerBlockNoteEditorfor this backend Yjs bootstrap.This path is doing server-side document conversion, but it still constructs
BlockNoteEditorand tears it down through underscore-prefixed internals. BlockNote’s server-side docs recommendServerBlockNoteEditorfrom@blocknote/server-util, note thatServerBlockNoteEditor.createaccepts the same editor options, and exposeblocksToYDoc/blocksToYXmlFragmentfor Yjs conversion. (blocknotejs.org)Also applies to: 81-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/notes/functions/createNote.ts` around lines 2 - 3, The code is constructing a client-side BlockNoteEditor and using underscore internals for server-side Yjs conversion; replace this with the server utility: import ServerBlockNoteEditor from '@blocknote/server-util' and use ServerBlockNoteEditor.create(...) (passing the same editor options) instead of new BlockNoteEditor(...), then use the exposed conversion helpers (blocksToYDoc or blocksToYXmlFragment) from the package for Yjs output rather than touching internal teardown methods; update all occurrences (e.g., where BlockNoteEditor and blocksToYDoc are used) including the related block between lines ~81-92 to use the server APIs.
75-80: 🧹 Nitpick | 🔵 TrivialCollapse the repeated content guard.
content && content.length > 0is checked twice in the same flow. Keeping the block save andinitialStatebootstrap under one guard removes the duplication and keeps the happy path together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/notes/functions/createNote.ts` around lines 75 - 80, The code duplicates the content guard `content && content.length > 0`; collapse them by moving the initialState bootstrap into the same conditional that calls saveTopLevelBlocksForNote so both operations run under a single `if (content && content.length > 0)` block; update the block that references saveTopLevelBlocksForNote(ctx, { noteId, content }) to also initialize/set `initialState` (and any related variables) there, removing the second redundant `if` that checks content, while leaving references to `noteId` and `content` intact.src/features/canvas/components/nodes/embed-content/embed-canvas-content.tsx (1)
219-246:⚠️ Potential issue | 🟠 MajorKey the local Yjs session state to
canvasId.Because this reset only happens in
useEffect, the first render after acanvasIdchange still reuses the previousdoc/nodes/edges/afterSeq. That can briefly paint the old canvas and callgetUpdatesfor the newdocumentIdwith the previous canvas’s cursor, which can skip the new canvas’s initial history. Keep the cursor/snapshot keyed bycanvasId(or gate rendering/query args until local state belongs to the currentcanvasId) instead of storingafterSeqas an unscoped state value.🔧 Suggested direction
- const [afterSeq, setAfterSeq] = useState<number | undefined>(undefined) + const [cursor, setCursor] = useState<{ + canvasId: Id<'canvases'> + afterSeq: number | undefined + }>({ canvasId, afterSeq: undefined }) const lastAppliedSeqRef = useRef(-1) - const canvasIdRef = useRef(canvasId) useEffect(() => { setIsLoading(true) - setAfterSeq(undefined) + setCursor({ canvasId, afterSeq: undefined }) lastAppliedSeqRef.current = -1 - canvasIdRef.current = canvasId const d = new Y.Doc() setDoc(d) @@ const updatesResult = useAuthQuery(api.yjsSync.queries.getUpdates, { documentId: canvasId, - afterSeq, + afterSeq: cursor.canvasId === canvasId ? cursor.afterSeq : undefined, }) useEffect(() => { if (!updatesResult.data || !doc) return - if (canvasIdRef.current !== canvasId) return @@ if (updatesResult.data.length > 0) { - setAfterSeq(lastAppliedSeqRef.current) + setCursor({ canvasId, afterSeq: lastAppliedSeqRef.current }) }You will also want to gate the rendered
nodes/edgesthe same way so a canvas switch cannot flash the previous snapshot for one paint.Also applies to: 248-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/canvas/components/nodes/embed-content/embed-canvas-content.tsx` around lines 219 - 246, The local Yjs session state in useReadOnlyYjsCanvas (doc, nodes, edges, afterSeq, lastAppliedSeqRef) is not scoped to the current canvasId and can cause flashing/incorrect getUpdates queries; fix by keying that state to canvasId — e.g., replace independent states with a single keyed structure or include canvasId in their shape (like {canvasId, doc, nodes, edges, afterSeq}) or keep a Map<canvasId, State> and select the current entry; additionally gate rendering and the updates query so you only pass afterSeq and render nodes/edges when the stored state’s canvasId matches the current canvasId (or when you have initialized a fresh doc for that canvas) to prevent reusing previous canvas snapshot during the first render after a switch.convex/folders/functions/getFolderContentsForDownload.ts (2)
71-73:⚠️ Potential issue | 🟡 MinorMake the
.mdsuffix check case-insensitive.Line 71 currently treats
Readme.MDas missing the extension and exportsReadme.MD.md.Proposed fix
- const noteName = child.name.endsWith('.md') + const noteName = child.name.toLowerCase().endsWith('.md') ? child.name : `${child.name}.md`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/folders/functions/getFolderContentsForDownload.ts` around lines 71 - 73, The extension check for noteName is case-sensitive and will append ".md" to names like "Readme.MD"; update the condition in getFolderContentsForDownload (where noteName and child.name are used) to perform a case-insensitive check — e.g., test child.name.toLowerCase().endsWith('.md') or use a case-insensitive regex like /\.md$/i — so existing .MD/.Md/etc. extensions are recognized and not duplicated.
116-118:⚠️ Potential issue | 🟠 MajorDo not silently skip canvases in download output.
Line 117-Line 118 drops canvases without signaling partial export, which can produce incomplete archives with a success response.
Proposed minimal safe behavior (explicit failure until canvas export exists)
- // TODO: add canvas -> img export - case SIDEBAR_ITEM_TYPES.canvases: - break + case SIDEBAR_ITEM_TYPES.canvases: { + throwClientError( + ERROR_CODE.VALIDATION_FAILED, + 'Canvas export is not supported yet', + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/folders/functions/getFolderContentsForDownload.ts` around lines 116 - 118, The switch case for SIDEBAR_ITEM_TYPES.canvases in getFolderContentsForDownload silently drops canvases (case in getFolderContentsForDownload.ts), causing successful-but-partial exports; change this to explicitly fail instead of break — e.g., detect the canvases case in the switch and throw/return an error (with a clear message like "canvas export not implemented") so callers receive a non-success response until a proper canvas->img export is implemented; ensure the thrown/returned error is propagated by the surrounding getFolderContentsForDownload function so the partial archive cannot be produced silently.src/features/sidebar/components/forms/color-picker.tsx (1)
3-11: 🧹 Nitpick | 🔵 TrivialUse one import path style for
sidebar-item-utilsin this file.Line 3 and Line 11 import from the same module via different path styles. Consolidate to a single alias import for consistency.
Proposed fix
-import { DEFAULT_ITEM_COLOR } from '../../utils/sidebar-item-utils' +import { + DEFAULT_ITEM_COLOR, + validateHexColorOrDefault, +} from '~/features/sidebar/utils/sidebar-item-utils' @@ -import { validateHexColorOrDefault } from '~/features/sidebar/utils/sidebar-item-utils'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sidebar/components/forms/color-picker.tsx` around lines 3 - 11, The file imports DEFAULT_ITEM_COLOR and validateHexColorOrDefault from the same module using two different path styles; consolidate them to a single import path style (use the project alias style used elsewhere, e.g., the '~/features/sidebar/utils/sidebar-item-utils' form) so that DEFAULT_ITEM_COLOR and validateHexColorOrDefault are imported from the same module path in color-picker.tsx.convex/sidebarItems/validation.ts (1)
295-313:⚠️ Potential issue | 🟠 MajorHandle slug checks against all matches and avoid
.unique()hard-fail paths.Line 301 can throw if duplicate
{ campaignId, slug }rows exist, and Line 311 only inspects the first non-null hit. That combination can either 500 or miss a real conflict whenexcludeIdmatches the first item but another table still conflicts.Proposed fix
- const queryTable = (table: SidebarItemTable) => + const queryTable = (table: SidebarItemTable) => ctx.db .query(table) .withIndex('by_campaign_slug', (q) => q.eq('campaignId', campaignId).eq('slug', slug), ) // check deleted items as well since deleted items can be accessed by slug - .unique() + .collect() - const [note, folder, map, file, canvas] = await Promise.all([ + const [notes, folders, maps, files, canvases] = await Promise.all([ queryTable('notes'), queryTable('folders'), queryTable('gameMaps'), queryTable('files'), queryTable('canvases'), ]) - const conflict = note ?? folder ?? map ?? file ?? canvas - if (!conflict) return false - return excludeId ? conflict._id !== excludeId : true + const matches = [...notes, ...folders, ...maps, ...files, ...canvases] + if (matches.length === 0) return false + return excludeId ? matches.some((item) => item._id !== excludeId) : trueIn Convex, what is the exact behavior of `query(...).withIndex(...).unique()` when multiple documents match, and what pattern is recommended for conflict detection queries that must not throw on duplicates?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/sidebarItems/validation.ts` around lines 295 - 313, queryTable currently uses ctx.db.query(...).withIndex(...).unique(), which throws if multiple rows match and later code only inspects the first non-null hit (variable conflict), risking a 500 or missing conflicts; change queryTable to not call .unique() (use .collect() to fetch all matches) and then update the conflict detection to iterate all results from notes/folders/gameMaps/files/canvases to check if any returned item has an _id !== excludeId (or any non-excluded match) before returning true/false; reference queryTable, ctx.db.query(...).withIndex(...).unique(), conflict, and excludeId.convex/sidebarItems/__tests__/previewCleanup.test.ts (1)
33-35:⚠️ Potential issue | 🟡 MinorAdd the missing canvas hard-delete cleanup case.
This suite covers note/file/map/folder paths, but the new
canvasesitem type also carries preview storage in this PR. Without a canvas case, regressions in its hard-delete cleanup path will still pass unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/sidebarItems/__tests__/previewCleanup.test.ts` around lines 33 - 35, The test suite "preview cleanup on hard delete" is missing a case for the new canvases item type; add a test that follows the existing pattern (using createTestContext()) to create a canvases item with preview storage, perform the hard-delete flow, and assert that the preview blob is removed; mirror the assertions and setup used for note/file/map/folder tests so the canvases hard-delete cleanup path is covered.convex/sidebarItems/__tests__/previewGeneration.test.ts (1)
21-221: 🧹 Nitpick | 🔵 TrivialCanvas preview paths are still missing from this suite.
This PR introduces
canvasas a sidebar item type, but these tests still only exercise notes/folders. Please mirror at least onecreateCanvaspath for claim/upload/previewUrlresolution so the new canvas preview flow is covered.Also applies to: 223-380, 382-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/sidebarItems/__tests__/previewGeneration.test.ts` around lines 21 - 221, Tests currently only exercise notes/folders; add equivalent canvas scenarios to cover the new canvas preview flow by calling createCanvas (use the same setupCampaignContext/asDm/asPlayer helpers) and invoking api.sidebarItems.mutations.claimPreviewGeneration with the returned canvasId; mirror at least one positive claim path (DM can claim), one denied path (player with view-only share), and preview state assertions similar to note tests (check previewLockedUntil, previewUpdatedAt, COOLDOWN_MS behavior and previewUrl resolution after upload). Reuse existing helpers like createSidebarShare, expectPermissionDenied, expectNotFound, and t.run/db.patch to simulate lock/cooldown conditions so the canvas branch in claimPreviewGeneration and preview URL resolution is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 941afcad-22a3-4266-99d3-2655e68176c8
📒 Files selected for processing (49)
convex/_test/factories.helper.tsconvex/canvases/functions/createCanvas.tsconvex/canvases/functions/updateCanvas.tsconvex/folders/functions/getFolderContentsForDownload.tsconvex/gameMaps/__tests__/gameMaps.test.tsconvex/notes/__tests__/notes.test.tsconvex/notes/__tests__/persistBlocks.test.tsconvex/notes/functions/createNote.tsconvex/sidebarItems/__tests__/previewCleanup.test.tsconvex/sidebarItems/__tests__/previewGeneration.test.tsconvex/sidebarItems/__tests__/sidebarItemValidation.test.tsconvex/sidebarItems/functions/claimPreviewGeneration.tsconvex/sidebarItems/functions/enhanceSidebarItem.tsconvex/sidebarItems/functions/getSidebarItemById.tsconvex/sidebarItems/functions/getSidebarItemBySlug.tsconvex/sidebarItems/functions/setPreviewImage.tsconvex/sidebarItems/schema/baseFields.tsconvex/sidebarItems/types/baseTypes.tsconvex/sidebarItems/validation.tsconvex/yjsSync/__tests__/yjsSyncMutations.test.tsconvex/yjsSync/functions/compactUpdates.tssrc/features/canvas/components/canvas-color-panel.tsxsrc/features/canvas/components/canvas-minimap-node.tsxsrc/features/canvas/components/canvas-remote-cursors.tsxsrc/features/canvas/components/canvas-viewer.tsxsrc/features/canvas/components/nodes/embed-content/embed-canvas-content.tsxsrc/features/canvas/components/nodes/resizable-node-wrapper.tsxsrc/features/canvas/components/nodes/stroke-node.tsxsrc/features/canvas/hooks/useCanvasEraser.tssrc/features/canvas/hooks/useCanvasFileUpload.tsxsrc/features/canvas/hooks/useCanvasKeyboardShortcuts.tssrc/features/canvas/hooks/useCanvasOverlayHandlers.tssrc/features/canvas/hooks/useCanvasSelectionRect.tssrc/features/canvas/hooks/useCanvasStrokeClick.tssrc/features/canvas/hooks/useCanvasWheel.tssrc/features/canvas/hooks/useNodeEditing.tssrc/features/canvas/utils/canvas-stroke-utils.tssrc/features/dnd/utils/__tests__/dnd-registry.test.tssrc/features/dnd/utils/dnd-registry.tssrc/features/editor/components/forms/file-form/file-form.tsxsrc/features/editor/components/note-content.tsxsrc/features/editor/components/viewer/folder/note-card.tsxsrc/features/editor/components/viewer/map/map-viewer.tsxsrc/features/editor/hooks/useYjsReactFlowSync.tssrc/features/previews/utils/upload-preview.tssrc/features/sidebar/components/forms/color-picker.tsxsrc/features/sidebar/components/sidebar-root/droppable-root.tsxsrc/features/sidebar/utils/sidebar-item-utils.tssrc/styles/app.css
Merged
ntietje1
added a commit
that referenced
this pull request
Apr 8, 2026
* add knip to CI * add preview deployments * fix site url env var * fix site_url for preview deps, also move e2e to separate job * attempt fix env var issue * remove duplicate comment * add back comment, move tooltip * add manual dispatch option to e2e test * improve preview deployments * prevent duplicate deployments to prod * fix sub-sub-domain issue * fix hardcoded url * fixes * add github repo env var * fix order of operations * attempt env var fix * fix * resolve type issue * Note yjs (#30) * add yjs live collab for notes * optimize collab performance * remove unused dep * fix missing env var * add yet another missing env var for preview * improvements * fix viewer strict mode issue * add tests, fix small issues * address comments * fixes * fix strict mode issue * fixes * resolve nits * add redo test * re-use convex preview * fix depreciated flag * add missing env var * fix env var handling * fix comments and undo bug * add canvases (#33) * add xy-flow * add live canvas features * add free-hand drawing * fix stroke impl, add more modes * add undo/redo * add color picker and selection improvements * add better zoom controls * refactor * selection fixes + minimap adjustment * improve multi select indicator * fix sidebar layout issue * improve color picker * add basic embedding and dnd * add resizing * add embedded content * add better prveiews with client side image generation for notes * add convex eslint plugin * fix ssr issue * add preview generation * fixes * improve canvas embeds * address comments * fixes * explicitly pass auth secret * fixes * more fixes * improvements * improvements * fix pre-mature throw * fix test * add resize awareness and fix canvas external file drop * address comments * fix flakey test * address comments again * fix claim token in tests * attempt fix env var issue * fix * add resend api key to preview dep
This was referenced Apr 12, 2026
Merged
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
Collaboration
Tests