feat: add drag-to-select for multiple webframes #2853
Conversation
Implement drag selection functionality allowing users to select multiple webframes by clicking and dragging on the canvas, similar to Figma and tldraw. - Add DragSelectOverlay component for visual selection rectangle - Implement drag selection logic in Canvas component with proper coordinate transformation - Add visual feedback with teal outline for selected frames - Support multi-select with Shift key modifier - Integrate with existing frame selection system for chat context 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds drag-to-select in DESIGN mode: new editor state flag, drag lifecycle (start/move/end) with throttled moves, selection rectangle overlay, selection intersection utilities, per-frame drag-highlight props, and centralized multi-frame drag move/snapping helper. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Canvas
participant State as EditorState
participant Overlay as DragSelectOverlay
participant Frames
participant TopBar as TopBar/Helpers
participant Frame as FrameView
User->>Canvas: mousedown (DESIGN, LMB)
Canvas->>State: set isDragSelecting = true
Canvas->>Canvas: record dragStart/dragEnd (container coords)
Canvas->>Overlay: render selection rect
loop dragging (throttled ~16ms)
User-->>Canvas: mousemove
Canvas->>Canvas: update dragEnd, compute selection rect → canvas coords
Canvas->>Frames: compute framesInDragSelection (ids)
Frames->>Frame: pass isInDragSelection per-frame
Canvas->>Overlay: update overlay position/size
end
User->>Canvas: mouseup
Canvas->>Canvas: finalize intersecting frames (Shift-aware)
Canvas->>Frames: apply selection
Canvas->>State: set isDragSelecting = false
Canvas->>Overlay: hide overlay
Note right of TopBar: TopBar drag uses createMouseMoveHandler for direct frame dragging & snapping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| const handleCanvasMouseMove = (event: React.MouseEvent<HTMLDivElement>) => { | ||
| if (!isDragSelecting || !containerRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| const rect = containerRef.current.getBoundingClientRect(); | ||
| const x = event.clientX - rect.left; | ||
| const y = event.clientY - rect.top; | ||
| setDragSelectEnd({ x, y }); | ||
| }; |
There was a problem hiding this comment.
Mouse move handler has no bounds checking and will cause performance issues. The handler executes on every mouse movement even when not drag selecting, and when drag selecting, it performs expensive DOM operations (getBoundingClientRect) on every pixel movement without throttling. This will cause UI lag and poor user experience. Add early return when not drag selecting and consider throttling the coordinate updates.
| const handleCanvasMouseMove = (event: React.MouseEvent<HTMLDivElement>) => { | |
| if (!isDragSelecting || !containerRef.current) { | |
| return; | |
| } | |
| const rect = containerRef.current.getBoundingClientRect(); | |
| const x = event.clientX - rect.left; | |
| const y = event.clientY - rect.top; | |
| setDragSelectEnd({ x, y }); | |
| }; | |
| const handleCanvasMouseMove = useCallback( | |
| throttle((event: React.MouseEvent<HTMLDivElement>) => { | |
| if (!isDragSelecting || !containerRef.current) { | |
| return; | |
| } | |
| const rect = containerRef.current.getBoundingClientRect(); | |
| const x = event.clientX - rect.left; | |
| const y = event.clientY - rect.top; | |
| setDragSelectEnd({ x, y }); | |
| }, 16), // ~60fps | |
| [isDragSelecting] | |
| ); | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| onMouseDown={handleCanvasMouseDown} | ||
| onMouseMove={handleCanvasMouseMove} | ||
| onMouseUp={handleCanvasMouseUp} | ||
| onMouseLeave={handleCanvasMouseUp} |
There was a problem hiding this comment.
Mouse leave handler will incorrectly terminate drag selection when user drags outside the canvas container but is still dragging. This breaks the expected drag selection behavior where users should be able to drag beyond the visible canvas area. The onMouseLeave should not call handleCanvasMouseUp, or should only do so when the mouse button is not pressed.
| onMouseLeave={handleCanvasMouseUp} | |
| onMouseLeave={(e) => { | |
| // Only terminate drag if no mouse button is pressed | |
| if (e.buttons === 0) { | |
| handleCanvasMouseUp(e); | |
| } | |
| }} |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Changed from border-2 (2px) to border (1px) for a more subtle appearance
Replace Tailwind's blue-500 with Onlook's design system colors from @onlook/ui/tokens
| if (editorEngine.state.editorMode === EditorMode.DESIGN && event.button === 0) { | ||
| const rect = containerRef.current.getBoundingClientRect(); | ||
| const x = event.clientX - rect.left; | ||
| const y = event.clientY - rect.top; | ||
|
|
||
| setIsDragSelecting(true); | ||
| setDragSelectStart({ x, y }); | ||
| setDragSelectEnd({ x, y }); | ||
|
|
||
| // Clear existing selections if not shift-clicking | ||
| if (!event.shiftKey) { | ||
| editorEngine.clearUI(); | ||
| editorEngine.frames.deselectAll(); | ||
| } | ||
| } else { | ||
| editorEngine.clearUI(); | ||
| } |
There was a problem hiding this comment.
Logic error: The drag selection logic only triggers when event.button === 0 (left mouse button), but the else clause at line 53-55 calls editorEngine.clearUI() for ALL other mouse buttons including right-click and middle-click. This will incorrectly clear the UI when users right-click to open context menus or middle-click to pan. The else clause should either be removed or should only handle specific cases where clearing UI is actually desired.
| if (editorEngine.state.editorMode === EditorMode.DESIGN && event.button === 0) { | |
| const rect = containerRef.current.getBoundingClientRect(); | |
| const x = event.clientX - rect.left; | |
| const y = event.clientY - rect.top; | |
| setIsDragSelecting(true); | |
| setDragSelectStart({ x, y }); | |
| setDragSelectEnd({ x, y }); | |
| // Clear existing selections if not shift-clicking | |
| if (!event.shiftKey) { | |
| editorEngine.clearUI(); | |
| editorEngine.frames.deselectAll(); | |
| } | |
| } else { | |
| editorEngine.clearUI(); | |
| } | |
| if (editorEngine.state.editorMode === EditorMode.DESIGN && event.button === 0) { | |
| const rect = containerRef.current.getBoundingClientRect(); | |
| const x = event.clientX - rect.left; | |
| const y = event.clientY - rect.top; | |
| setIsDragSelecting(true); | |
| setDragSelectStart({ x, y }); | |
| setDragSelectEnd({ x, y }); | |
| // Clear existing selections if not shift-clicking | |
| if (!event.shiftKey) { | |
| editorEngine.clearUI(); | |
| editorEngine.frames.deselectAll(); | |
| } | |
| } | |
| // Removed the else clause that was incorrectly clearing UI for all non-left clicks |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
- Add throttling to mouse move handler (16ms/~60fps) to prevent performance issues - Fix mouse leave handler to only terminate drag when no button is pressed - Fix logic error that was clearing UI on right/middle clicks - Add proper cleanup for throttled function Addresses review feedback from Graphite
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (1)
64-67: Keep highlight thickness constant across zoom; avoid inline styles.Currently the outline scales with canvas zoom. Compute width inversely to
editorEngine.canvas.scaleand prefer classes over inline styles.- <div className="relative" style={{ - outline: isSelected ? '2px solid rgb(94, 234, 212)' : 'none', - borderRadius: '4px', - }}> + <div + className={`relative ${isSelected ? 'rounded' : ''}`} + style={{ + outline: isSelected + ? `${Math.max(1, 2 / editorEngine.canvas.scale)}px solid rgb(94, 234, 212)` + : 'none', + borderRadius: 4, + }} + >Please verify that this still matches the intended design tokens (e.g., if there is a themed ring/outline class to use instead of a raw color).
apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsx (3)
1-1: Avoid duplicating client boundaries.Per our guideline, keep a single client boundary at the feature entry. Since Canvas is already a client component, this file doesn’t need its own
"use client".-'use client'; +If this component is imported anywhere outside the Canvas client tree, keep
"use client"here.
23-31: Ensure overlay renders above frames.Frames use fixed/transform stacking; add an explicit z-index so the selection rectangle is never hidden.
- <div - className="absolute border-2 border-blue-500 bg-blue-500/10 pointer-events-none" + <div + className="absolute z-40 border-2 border-blue-500 bg-blue-500/10 pointer-events-none"
13-34:observeris unnecessary here.The overlay renders from props only and doesn’t dereference observables; drop
observerto avoid extra MobX tracking.-import { observer } from 'mobx-react-lite'; +// no mobx imports needed -export const DragSelectOverlay = observer(({ startX, startY, endX, endY, isSelecting }: DragSelectOverlayProps) => { +export const DragSelectOverlay = ({ startX, startY, endX, endY, isSelecting }: DragSelectOverlayProps) => { ... -}); +};Confirm this component isn’t subscribed to store state via context elsewhere.
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (2)
58-67: Prevent default during drag to avoid accidental text/image selection.- const handleCanvasMouseMove = (event: React.MouseEvent<HTMLDivElement>) => { + const handleCanvasMouseMove = (event: React.MouseEvent<HTMLDivElement>) => { if (!isDragSelecting || !containerRef.current) { return; } - + event.preventDefault();If you see selection artifacts in iframes, consider switching to Pointer Events and calling
setPointerCaptureonpointerdown.
69-117: Robustness: finalize on outside mouseup and ignore tiny drags.
- Capture mouseup outside the container to avoid stuck state.
- Treat sub-threshold rectangles as clicks (no selection).
- const handleCanvasMouseUp = (event: React.MouseEvent<HTMLDivElement>) => { + const handleCanvasMouseUp = (event: React.MouseEvent<HTMLDivElement> | MouseEvent) => { if (!isDragSelecting) { return; } // Calculate which frames are within the selection rectangle const selectionRect = { left: Math.min(dragSelectStart.x, dragSelectEnd.x), top: Math.min(dragSelectStart.y, dragSelectEnd.y), right: Math.max(dragSelectStart.x, dragSelectEnd.x), bottom: Math.max(dragSelectStart.y, dragSelectEnd.y), }; + const width = selectionRect.right - selectionRect.left; + const height = selectionRect.bottom - selectionRect.top; + if (width < 3 && height < 3) { + setIsDragSelecting(false); + return; + }And when starting a drag, attach a one-off document mouseup:
setDragSelectStart({ x, y }); setDragSelectEnd({ x, y }); + const onUp = (e: MouseEvent) => { + handleCanvasMouseUp(e); + document.removeEventListener('mouseup', onUp); + }; + document.addEventListener('mouseup', onUp);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/canvas/index.tsx(3 hunks)apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/index.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/index.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/index.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/index.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/index.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-16T19:22:52.423Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.423Z
Learning: Applies to apps/web/client/src/**/*.tsx : observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Applied to files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsx
🧬 Code graph analysis (2)
apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (1)
apps/web/client/src/components/store/editor/frames/manager.ts (1)
isSelected(77-79)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (2)
apps/web/client/src/app/project/[id]/_components/canvas/frames.tsx (1)
Frames(8-19)apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsx (1)
DragSelectOverlay(13-34)
🔇 Additional comments (4)
apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (1)
20-20: Selection reactivity looks good.Using
editorEngine.frames.isSelected(frame.id)will re-render under MobX whenselectedflips.apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (3)
29-31: State for drag-select looks fine.Simple, explicit shape for start/end points.
252-255:onMouseLeave→ finalize is fine, but keep document-level mouseup as primary.Using both prevents stuck overlay when the pointer exits the canvas abruptly.
260-266: Pass through; consider matching overlay z-index with other overlays.Once
DragSelectOverlayaddsz-40, this usage is good. No prop/state leaks spotted.
| onMouseLeave={(e) => { | ||
| // Only terminate drag if no mouse button is pressed | ||
| if (e.buttons === 0) { | ||
| handleCanvasMouseUp(e); | ||
| } | ||
| }} |
There was a problem hiding this comment.
The current onMouseLeave handler only terminates drag selection when no mouse button is pressed (e.buttons === 0), but doesn't handle the case where the user drags outside the canvas and then releases the mouse button elsewhere. This could leave the application in a perpetual drag selecting state.
Consider adding a global mouse up event listener when drag selecting starts:
useEffect(() => {
if (isDragSelecting) {
const handleGlobalMouseUp = () => {
setIsDragSelecting(false);
};
window.addEventListener('mouseup', handleGlobalMouseUp);
return () => window.removeEventListener('mouseup', handleGlobalMouseUp);
}
}, [isDragSelecting]);This would ensure the drag selection state is properly reset regardless of where the mouse button is released.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (1)
34-57: Fix target check: drag-select rarely starts; use currentTarget/canvas-container and preventDefault.Strict equality with
event.targetblocks drags when clicking the inner canvas or descendants. Useevent.currentTarget/canvas container id and callpreventDefault()to avoid text selection. Also guardcontainerRef.current.- const handleCanvasMouseDown = (event: React.MouseEvent<HTMLDivElement>) => { - if (event.target !== containerRef.current) { - return; - } - - // Start drag selection only in design mode and left mouse button - if (editorEngine.state.editorMode === EditorMode.DESIGN && event.button === 0) { - const rect = containerRef.current.getBoundingClientRect(); - const x = event.clientX - rect.left; - const y = event.clientY - rect.top; - - setIsDragSelecting(true); - setDragSelectStart({ x, y }); - setDragSelectEnd({ x, y }); - - // Clear existing selections if not shift-clicking - if (!event.shiftKey) { - editorEngine.clearUI(); - editorEngine.frames.deselectAll(); - } - } else if (event.button === 0) { - // Only clear UI for left clicks that don't start drag selection - editorEngine.clearUI(); - } - }; + const handleCanvasMouseDown = (event: React.MouseEvent<HTMLDivElement>) => { + const target = event.target as HTMLElement | null; + const isBackground = + target === event.currentTarget || + target?.id === EditorAttributes.CANVAS_CONTAINER_ID; + if (!isBackground) return; + if (!containerRef.current) return; + + // Start drag selection only in design mode and left mouse button + if (editorEngine.state.editorMode === EditorMode.DESIGN && event.button === 0) { + event.preventDefault(); // avoid text selection + const rect = containerRef.current.getBoundingClientRect(); + const x = event.clientX - rect.left; + const y = event.clientY - rect.top; + + setIsDragSelecting(true); + setDragSelectStart({ x, y }); + setDragSelectEnd({ x, y }); + + // Clear existing selections if not shift-clicking + if (!event.shiftKey) { + editorEngine.clearUI(); + editorEngine.frames.deselectAll(); + } + } else if (event.button === 0) { + // Only clear UI for left clicks that don't start drag selection + editorEngine.clearUI(); + } + };
🧹 Nitpick comments (3)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (3)
60-73: Don’t pass React events into throttled handler; throttle plain coords instead.Safer and cheaper to throttle numeric coords; avoids any gotchas with synthetic events and makes cancellation explicit.
- const handleCanvasMouseMove = useCallback( - throttle((event: React.MouseEvent<HTMLDivElement>) => { - if (!isDragSelecting || !containerRef.current) { - return; - } - - const rect = containerRef.current.getBoundingClientRect(); - const x = event.clientX - rect.left; - const y = event.clientY - rect.top; - setDragSelectEnd({ x, y }); - }, 16), // ~60fps - [isDragSelecting] - ); + const handleCanvasMouseMoveThrottled = useMemo( + () => + throttle((clientX: number, clientY: number) => { + if (!containerRef.current) return; + const rect = containerRef.current.getBoundingClientRect(); + const x = clientX - rect.left; + const y = clientY - rect.top; + setDragSelectEnd({ x, y }); + }, 16), + [] + ); + const handleCanvasMouseMove = useCallback( + (event: React.MouseEvent<HTMLDivElement>) => { + if (!isDragSelecting) return; + handleCanvasMouseMoveThrottled(event.clientX, event.clientY); + }, + [isDragSelecting, handleCanvasMouseMoveThrottled] + );- onMouseMove={handleCanvasMouseMove} + onMouseMove={handleCanvasMouseMove}- handleCanvasMouseMove.cancel?.(); // Clean up throttled function + handleCanvasMouseMoveThrottled.cancel?.(); // Clean up throttled functionAlso applies to: 247-251, 258-258
6-6: Reduce bundle size: import throttle by path.Avoid pulling all of lodash on the client.
-import { throttle } from 'lodash'; +import throttle from 'lodash/throttle'; +// or: import throttle from 'lodash-es/throttle';
34-73: Allow dragging outside the canvas: use Pointer Events with capture.Dragging stops updating once the cursor leaves the container. Switch to
onPointerDown/Move/Upand callsetPointerCaptureonpointerdownto keep receiving events beyond bounds; release onpointerup.- onMouseDown={handleCanvasMouseDown} - onMouseMove={handleCanvasMouseMove} - onMouseUp={handleCanvasMouseUp} + onPointerDown={handleCanvasPointerDown} + onPointerMove={handleCanvasPointerMove} + onPointerUp={handleCanvasPointerUp}// New handlers (outline) const handleCanvasPointerDown = (e: React.PointerEvent<HTMLDivElement>) => { // ...same background checks... if (editorEngine.state.editorMode === EditorMode.DESIGN && e.button === 0) { (e.currentTarget as HTMLElement).setPointerCapture?.(e.pointerId); e.preventDefault(); // compute start/end... } }; const handleCanvasPointerMove = (e: React.PointerEvent<HTMLDivElement>) => { if (!isDragSelecting) return; // update end using e.clientX/Y (use throttled coords as above) }; const handleCanvasPointerUp = (e: React.PointerEvent<HTMLDivElement>) => { if (!isDragSelecting) return; (e.currentTarget as HTMLElement).releasePointerCapture?.(e.pointerId); // finalize selection... };Also applies to: 252-266
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx(3 hunks)apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsx
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx
🧬 Code graph analysis (1)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (3)
apps/web/client/src/app/project/[id]/_components/canvas/hotkeys/index.tsx (1)
HotkeysArea(8-119)apps/web/client/src/app/project/[id]/_components/canvas/frames.tsx (1)
Frames(8-19)apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsx (1)
DragSelectOverlay(14-37)
🔇 Additional comments (2)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (2)
74-121: Confirm: intersect vs contain semantics for multi-select.Current logic selects frames that intersect the rect. If you intended “containment only,” swap the predicate as below.
- // Check if frame intersects with selection rectangle - return !( - frameLeft > canvasSelectionRect.right || - frameRight < canvasSelectionRect.left || - frameTop > canvasSelectionRect.bottom || - frameBottom < canvasSelectionRect.top - ); + // Check if frame is fully contained within selection rectangle + return ( + frameLeft >= canvasSelectionRect.left && + frameRight <= canvasSelectionRect.right && + frameTop >= canvasSelectionRect.top && + frameBottom <= canvasSelectionRect.bottom + );
252-266: LGTM: onMouseLeave no longer aborts active drags.Good guard with
e.buttons === 0to prevent premature termination.
- Add real-time teal outline feedback for frames during drag selection - Prevent element hover outlines from showing while drag selecting - Add isDragSelecting state to prevent hover events during drag - Show frames with semi-transparent teal outline as they enter selection area - Frames now show visual feedback immediately during drag, not just after
- Use Onlook's teal color tokens for consistent styling - Selected frames show teal-400 outline and text (unchanged) - Frames in drag selection show teal-500 outline and text (one tier darker) - Remove opacity change, use solid colors for clarity - Pass isInDragSelection prop through to TopBar for text color changes
- Pass isInDragSelection prop through FrameView to FrameComponent - Add teal-500 outline to iframe when frame is in drag selection - Ensures the gray outer border changes to teal when dragged over
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (4)
108-123: Mouse‑move handler: throttling and early return look good.Runs at ~60fps only during drag; avoids unnecessary work otherwise. Nice.
311-318: Good fix: don’t end drag on mouseleave while pressed.This matches expected canvas behavior when dragging outside bounds.
35-63: Drag‑select rarely starts: broaden target check and prevent default.Strict
event.target !== containerRef.currentblocks most clicks (children/overlays). Also prevent default to avoid text selection. This was flagged earlier and still applies.-const handleCanvasMouseDown = (event: React.MouseEvent<HTMLDivElement>) => { - if (event.target !== containerRef.current) { - return; - } +const handleCanvasMouseDown = (event: React.MouseEvent<HTMLDivElement>) => { + const target = event.target as HTMLElement | null; + const isBackground = + target === event.currentTarget || + (target instanceof HTMLElement && target.id === EditorAttributes.CANVAS_CONTAINER_ID); + if (!isBackground) return; // Start drag selection only in design mode and left mouse button if (editorEngine.state.editorMode === EditorMode.DESIGN && event.button === 0) { + event.preventDefault(); const rect = containerRef.current.getBoundingClientRect(); const x = event.clientX - rect.left; const y = event.clientY - rect.top;
125-175: End‑of‑drag can get stuck if mouseup happens outside the canvas. Add a global mouseup fallback.Releasing the mouse outside the container leaves
isDragSelectingand the overlay active.Apply this diff near the existing effect setup to register a temporary global listener while dragging:
useEffect(() => { const div = containerRef.current; if (div) { div.addEventListener('wheel', handleWheel, { passive: false }); div.addEventListener('mousedown', middleMouseButtonDown); div.addEventListener('mouseup', middleMouseButtonUp); return () => { div.removeEventListener('wheel', handleWheel); div.removeEventListener('mousedown', middleMouseButtonDown); div.removeEventListener('mouseup', middleMouseButtonUp); handleCanvasMouseMove.cancel?.(); // Clean up throttled function }; } -}, [handleWheel, middleMouseButtonDown, middleMouseButtonUp, handleCanvasMouseMove]); +}, [handleWheel, middleMouseButtonDown, middleMouseButtonUp, handleCanvasMouseMove]); + +useEffect(() => { + if (!isDragSelecting) return; + const onGlobalMouseUp = () => { + setIsDragSelecting(false); + setFramesInSelection(new Set()); + editorEngine.state.isDragSelecting = false; + }; + window.addEventListener('mouseup', onGlobalMouseUp); + return () => window.removeEventListener('mouseup', onGlobalMouseUp); +}, [isDragSelecting, editorEngine.state]);
🧹 Nitpick comments (4)
apps/web/client/src/components/store/editor/state/index.ts (2)
18-18: Add reset of drag‑select flag on store clear.
isDragSelectingis global UI state; not clearing it can leave hover suppression and visuals stuck after resets/navigation.Apply this diff to clear it:
clear() { this.hotkeysOpen = false; this.publishOpen = false; this.branchTab = null; this.manageBranchId = null; this.resetCanvasScrollingDebounced.cancel(); + this.isDragSelecting = false; }
38-40: Consider hiding overlay while drag‑selecting.If overlays should be hidden during drag selection (similar to panning/scrolling), fold
isDragSelectingintoshouldHideOverlay. If not desired, ignore.- return this._canvasScrolling || this.canvasPanning + return this._canvasScrolling || this.canvasPanning || this.isDragSelectingapps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsx (1)
303-307: Avoid double outlines (wrapper + iframe) during drag‑select.
FrameViewwrapper already draws a teal outline; the iframe adds another whenisInDragSelection, causing a thicker/doubled border. Drop the iframe’s drag‑select outline and keep wrapper as the single source of truth.className={cn( 'backdrop-blur-sm transition outline outline-4', isActiveBranch && 'outline-teal-400', isActiveBranch && !isSelected && 'outline-dashed', - !isActiveBranch && isInDragSelection && 'outline-teal-500', )}apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (1)
6-6: Import throttle from the path module to reduce bundle size.Avoid
lodashbarrel import in client bundles.-import { throttle } from 'lodash'; +import throttle from 'lodash/throttle';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx(3 hunks)apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/canvas/frames.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/canvas/index.tsx(3 hunks)apps/web/client/src/components/store/editor/state/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsxapps/web/client/src/app/project/[id]/_components/canvas/frames.tsxapps/web/client/src/app/project/[id]/_components/canvas/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsxapps/web/client/src/components/store/editor/state/index.tsapps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsxapps/web/client/src/app/project/[id]/_components/canvas/frames.tsxapps/web/client/src/app/project/[id]/_components/canvas/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsxapps/web/client/src/app/project/[id]/_components/canvas/frames.tsxapps/web/client/src/app/project/[id]/_components/canvas/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsxapps/web/client/src/components/store/editor/state/index.tsapps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsxapps/web/client/src/app/project/[id]/_components/canvas/frames.tsxapps/web/client/src/app/project/[id]/_components/canvas/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsxapps/web/client/src/app/project/[id]/_components/canvas/frames.tsxapps/web/client/src/app/project/[id]/_components/canvas/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsxapps/web/client/src/components/store/editor/state/index.tsapps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsxapps/web/client/src/app/project/[id]/_components/canvas/frames.tsxapps/web/client/src/app/project/[id]/_components/canvas/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx
🧬 Code graph analysis (4)
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx (2)
packages/models/src/project/frame.ts (1)
Frame(4-16)apps/web/client/src/components/store/editor/frames/manager.ts (1)
isSelected(77-79)
apps/web/client/src/app/project/[id]/_components/canvas/frames.tsx (3)
apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)apps/web/client/src/components/store/editor/frames/manager.ts (1)
FrameData(13-17)apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (1)
FrameView(15-86)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (3)
apps/web/client/src/app/project/[id]/_components/canvas/hotkeys/index.tsx (1)
HotkeysArea(8-119)apps/web/client/src/app/project/[id]/_components/canvas/frames.tsx (1)
Frames(8-23)apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsx (1)
DragSelectOverlay(14-37)
apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (6)
packages/models/src/project/frame.ts (1)
Frame(4-16)apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)apps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsx (2)
IFrameView(24-29)FrameComponent(60-317)apps/web/client/src/components/store/editor/frames/manager.ts (1)
isSelected(77-79)apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx (1)
TopBar(13-226)packages/ui/tokens.ts (1)
colors(1-112)
🔇 Additional comments (4)
apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx (1)
96-99: Good guard to suppress hover during drag‑select.Early‑returning when
state.isDragSelectingavoids noisy hover and unnecessary IPC to frames. LGTM.apps/web/client/src/app/project/[id]/_components/canvas/frames.tsx (1)
8-19: Prop wiring looks correct.Passing
isInDragSelection={framesInDragSelection.has(id)}is straightforward and keepsFrameViewdecoupled from selection set internals.apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx (1)
132-137: Consistent highlight for drag‑selected but unselected frames.Using teal styling when
!isSelected && isInDragSelectionaligns with the iframe/frame wrapper visuals.apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (1)
65-71: Single place to render drag‑select outline is good; ensure no conflicting inner outlines.Wrapper outline uses design tokens and looks good. After removing the iframe’s drag‑select outline (see view.tsx suggestion), visuals will be consistent and avoid double borders.
- Add window-level mouseup event listener when drag selecting starts - Ensures drag selection properly terminates even when mouse released outside canvas - Prevents application from getting stuck in drag selecting state - Addresses Graphite review feedback about edge case handling
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (1)
35-63: Fix drag-start target check; allow clicks on canvas descendants and prevent text selectionStrict
event.target !== containerRef.currentblocks drag-start when clicking the inner canvas or descendants. Use a background containment check and callpreventDefault()when starting a drag. This was flagged earlier and remains unfixed.- if (event.target !== containerRef.current) { - return; - } + const target = event.target as HTMLElement | null; + const isBackground = + target === event.currentTarget || + (target instanceof HTMLElement && target.id === EditorAttributes.CANVAS_CONTAINER_ID); + if (!isBackground) return; - // Start drag selection only in design mode and left mouse button - if (editorEngine.state.editorMode === EditorMode.DESIGN && event.button === 0) { + // Start drag selection only in design mode and left mouse button + if (editorEngine.state.editorMode === EditorMode.DESIGN && event.button === 0) { + event.preventDefault(); // avoid native text selection during drag const rect = containerRef.current.getBoundingClientRect(); const x = event.clientX - rect.left; const y = event.clientY - rect.top;
🧹 Nitpick comments (5)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (5)
6-6: Import throttle in a tree‑shakable way to avoid pulling all of lodashSwitch to the per‑method import to reduce bundle size.
-import { throttle } from 'lodash'; +import throttle from 'lodash/throttle';
27-33: Cache container rect during drag to cut repeated layouts; clear on drag end
getBoundingClientRect()on every mousemove is costly. Cache it at drag start and reuse during the drag; reset on mouseup. Keeps ~60fps smoother on large canvases.@@ - const containerRef = useRef<HTMLDivElement>(null); + const containerRef = useRef<HTMLDivElement>(null); + const containerRectRef = useRef<DOMRect | null>(null); @@ - if (editorEngine.state.editorMode === EditorMode.DESIGN && event.button === 0) { - const rect = containerRef.current.getBoundingClientRect(); + if (editorEngine.state.editorMode === EditorMode.DESIGN && event.button === 0) { + const rect = (containerRectRef.current = containerRef.current!.getBoundingClientRect()); const x = event.clientX - rect.left; const y = event.clientY - rect.top; @@ - const rect = containerRef.current.getBoundingClientRect(); + const rect = containerRectRef.current ?? containerRef.current.getBoundingClientRect(); const x = event.clientX - rect.left; const y = event.clientY - rect.top; @@ - const handleGlobalMouseUp = (event: MouseEvent) => { + const handleGlobalMouseUp = (event: MouseEvent) => { // Calculate which frames are within the selection rectangle @@ setIsDragSelecting(false); setFramesInSelection(new Set()); editorEngine.state.isDragSelecting = false; + containerRectRef.current = null; };Also applies to: 35-63, 108-123, 258-313
258-313: Add a small movement threshold to avoid accidental selections on clickPrevents zero/1‑px drags from triggering selection scans.
- const handleGlobalMouseUp = (event: MouseEvent) => { + const handleGlobalMouseUp = (event: MouseEvent) => { + const dx = Math.abs(dragSelectStart.x - dragSelectEnd.x); + const dy = Math.abs(dragSelectStart.y - dragSelectEnd.y); + const MIN_DRAG = 3; // px + if (dx < MIN_DRAG && dy < MIN_DRAG) { + setIsDragSelecting(false); + setFramesInSelection(new Set()); + editorEngine.state.isDragSelecting = false; + return; + } // Calculate which frames are within the selection rectangle
65-106: DRY up intersection logic (compute once; reuse for live preview and final select)The AABB intersection is duplicated. Extract a helper to reduce bugs and keep behavior consistent.
@@ - const updateFramesInSelection = useCallback((start: { x: number; y: number }, end: { x: number; y: number }) => { + const getIntersectingFrameIds = useCallback((canvasSelectionRect: { + left: number; top: number; right: number; bottom: number; + }) => { + const ids = new Set<string>(); + editorEngine.frames.getAll().forEach(fd => { + const f = fd.frame; + const frameRect = { + left: f.position.x, + top: f.position.y, + right: f.position.x + f.dimension.width, + bottom: f.position.y + f.dimension.height, + }; + const intersects = !( + frameRect.left > canvasSelectionRect.right || + frameRect.right < canvasSelectionRect.left || + frameRect.top > canvasSelectionRect.bottom || + frameRect.bottom < canvasSelectionRect.top + ); + if (intersects) ids.add(f.id); + }); + return ids; + }, [editorEngine.frames]); + + const updateFramesInSelection = useCallback((start: { x: number; y: number }, end: { x: number; y: number }) => { const selectionRect = { left: Math.min(start.x, end.x), top: Math.min(start.y, end.y), right: Math.max(start.x, end.x), bottom: Math.max(start.y, end.y), }; // Convert selection rect to canvas coordinates - const canvasSelectionRect = { + const canvasSelectionRect = { left: (selectionRect.left - position.x) / scale, top: (selectionRect.top - position.y) / scale, right: (selectionRect.right - position.x) / scale, bottom: (selectionRect.bottom - position.y) / scale, }; - // Find all frames that intersect with the selection rectangle - const allFrames = editorEngine.frames.getAll(); - const intersectingFrameIds = new Set<string>(); - - allFrames.forEach(frameData => { - const frame = frameData.frame; - const frameLeft = frame.position.x; - const frameTop = frame.position.y; - const frameRight = frame.position.x + frame.dimension.width; - const frameBottom = frame.position.y + frame.dimension.height; - - // Check if frame intersects with selection rectangle - const intersects = !( - frameLeft > canvasSelectionRect.right || - frameRight < canvasSelectionRect.left || - frameTop > canvasSelectionRect.bottom || - frameBottom < canvasSelectionRect.top - ); - - if (intersects) { - intersectingFrameIds.add(frame.id); - } - }); - - setFramesInSelection(intersectingFrameIds); + setFramesInSelection(getIntersectingFrameIds(canvasSelectionRect)); - }, [position, scale, editorEngine.frames]); + }, [position, scale, getIntersectingFrameIds]); @@ - // Find all frames that intersect with the selection rectangle - const allFrames = editorEngine.frames.getAll(); - const selectedFrames = allFrames.filter(frameData => { - const frame = frameData.frame; - const frameLeft = frame.position.x; - const frameTop = frame.position.y; - const frameRight = frame.position.x + frame.dimension.width; - const frameBottom = frame.position.y + frame.dimension.height; - - // Check if frame intersects with selection rectangle - return !( - frameLeft > canvasSelectionRect.right || - frameRight < canvasSelectionRect.left || - frameTop > canvasSelectionRect.bottom || - frameBottom < canvasSelectionRect.top - ); - }); + const ids = getIntersectingFrameIds(canvasSelectionRect); + const selectedFrames = editorEngine.frames.getAll() + .filter(fd => ids.has(fd.frame.id));Also applies to: 270-294
125-128: Remove no‑op mouseup handler and redundant mouseleave blockMouseup is handled globally; these are now dead code and can confuse future maintenance.
- const handleCanvasMouseUp = (event: React.MouseEvent<HTMLDivElement>) => { - // Mouse up is now handled by the global listener in useEffect - // This function is kept for consistency but the logic is in the global handler - }; @@ - onMouseUp={handleCanvasMouseUp} - onMouseLeave={(e) => { - // Only terminate drag if no mouse button is pressed - if (e.buttons === 0) { - handleCanvasMouseUp(e); - } - }} + // onMouseUp handled globally via window 'mouseup' + // onMouseLeave not needed with global handlerAlso applies to: 321-327
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx(3 hunks)apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsx
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx
🧬 Code graph analysis (1)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (2)
apps/web/client/src/app/project/[id]/_components/canvas/frames.tsx (1)
Frames(8-23)apps/web/client/src/app/project/[id]/_components/canvas/overlay/drag-select.tsx (1)
DragSelectOverlay(14-37)
🔇 Additional comments (1)
apps/web/client/src/app/project/[id]/_components/canvas/index.tsx (1)
258-313: Global mouseup handler is a solid additionCorrectly resolves the “mouseup outside canvas” edge case. Nice.
|
Adds the ability to click and drag to select multiple frames. What is missing is the ability to add a webview's branch as context to the chat. I tried building that capability (where when you select a webview topbar it adds that branch into the chat context) in #2840 but the AI was giving me a really hard time so I scrapped it. @Kitenite up to you if you think this is the right PR to add this kind of functionality, but at the very least this "drag" rectangle behavior could be the start of allowing people to arrange the frames on a canvas or add multiple frames in-context at the same time.
|
| const handleGlobalMouseUp = (event: MouseEvent) => { | ||
| // Calculate which frames are within the selection rectangle | ||
| const selectionRect = { | ||
| left: Math.min(dragSelectStart.x, dragSelectEnd.x), | ||
| top: Math.min(dragSelectStart.y, dragSelectEnd.y), | ||
| right: Math.max(dragSelectStart.x, dragSelectEnd.x), | ||
| bottom: Math.max(dragSelectStart.y, dragSelectEnd.y), | ||
| }; | ||
|
|
||
| // Convert selection rect to canvas coordinates | ||
| const canvasSelectionRect = { | ||
| left: (selectionRect.left - position.x) / scale, | ||
| top: (selectionRect.top - position.y) / scale, | ||
| right: (selectionRect.right - position.x) / scale, | ||
| bottom: (selectionRect.bottom - position.y) / scale, | ||
| }; | ||
|
|
||
| // Find all frames that intersect with the selection rectangle | ||
| const allFrames = editorEngine.frames.getAll(); | ||
| const selectedFrames = allFrames.filter(frameData => { | ||
| const frame = frameData.frame; | ||
| const frameLeft = frame.position.x; | ||
| const frameTop = frame.position.y; | ||
| const frameRight = frame.position.x + frame.dimension.width; | ||
| const frameBottom = frame.position.y + frame.dimension.height; | ||
|
|
||
| // Check if frame intersects with selection rectangle | ||
| return !( | ||
| frameLeft > canvasSelectionRect.right || | ||
| frameRight < canvasSelectionRect.left || | ||
| frameTop > canvasSelectionRect.bottom || | ||
| frameBottom < canvasSelectionRect.top | ||
| ); | ||
| }); | ||
|
|
||
| // Select the frames if any were found in the selection | ||
| if (selectedFrames.length > 0) { | ||
| editorEngine.frames.select( | ||
| selectedFrames.map(fd => fd.frame), | ||
| event.shiftKey // multiselect if shift is held | ||
| ); | ||
| } | ||
|
|
||
| setIsDragSelecting(false); | ||
| setFramesInSelection(new Set()); | ||
| editorEngine.state.isDragSelecting = false; | ||
| }; | ||
|
|
||
| window.addEventListener('mouseup', handleGlobalMouseUp); | ||
| return () => window.removeEventListener('mouseup', handleGlobalMouseUp); | ||
| } |
There was a problem hiding this comment.
Logic consistency error: The frame intersection calculation is duplicated in both updateFramesInSelection (lines 65-106) and the global mouseup handler (lines 261-311). These two implementations could diverge over time, leading to inconsistent selection behavior where the visual feedback during drag doesn't match the final selection result.
| const handleGlobalMouseUp = (event: MouseEvent) => { | |
| // Calculate which frames are within the selection rectangle | |
| const selectionRect = { | |
| left: Math.min(dragSelectStart.x, dragSelectEnd.x), | |
| top: Math.min(dragSelectStart.y, dragSelectEnd.y), | |
| right: Math.max(dragSelectStart.x, dragSelectEnd.x), | |
| bottom: Math.max(dragSelectStart.y, dragSelectEnd.y), | |
| }; | |
| // Convert selection rect to canvas coordinates | |
| const canvasSelectionRect = { | |
| left: (selectionRect.left - position.x) / scale, | |
| top: (selectionRect.top - position.y) / scale, | |
| right: (selectionRect.right - position.x) / scale, | |
| bottom: (selectionRect.bottom - position.y) / scale, | |
| }; | |
| // Find all frames that intersect with the selection rectangle | |
| const allFrames = editorEngine.frames.getAll(); | |
| const selectedFrames = allFrames.filter(frameData => { | |
| const frame = frameData.frame; | |
| const frameLeft = frame.position.x; | |
| const frameTop = frame.position.y; | |
| const frameRight = frame.position.x + frame.dimension.width; | |
| const frameBottom = frame.position.y + frame.dimension.height; | |
| // Check if frame intersects with selection rectangle | |
| return !( | |
| frameLeft > canvasSelectionRect.right || | |
| frameRight < canvasSelectionRect.left || | |
| frameTop > canvasSelectionRect.bottom || | |
| frameBottom < canvasSelectionRect.top | |
| ); | |
| }); | |
| // Select the frames if any were found in the selection | |
| if (selectedFrames.length > 0) { | |
| editorEngine.frames.select( | |
| selectedFrames.map(fd => fd.frame), | |
| event.shiftKey // multiselect if shift is held | |
| ); | |
| } | |
| setIsDragSelecting(false); | |
| setFramesInSelection(new Set()); | |
| editorEngine.state.isDragSelecting = false; | |
| }; | |
| window.addEventListener('mouseup', handleGlobalMouseUp); | |
| return () => window.removeEventListener('mouseup', handleGlobalMouseUp); | |
| } | |
| const handleGlobalMouseUp = (event: MouseEvent) => { | |
| // Calculate which frames are within the selection rectangle | |
| const selectionRect = { | |
| left: Math.min(dragSelectStart.x, dragSelectEnd.x), | |
| top: Math.min(dragSelectStart.y, dragSelectEnd.y), | |
| right: Math.max(dragSelectStart.x, dragSelectEnd.x), | |
| bottom: Math.max(dragSelectStart.y, dragSelectEnd.y), | |
| }; | |
| // Convert selection rect to canvas coordinates | |
| const canvasSelectionRect = { | |
| left: (selectionRect.left - position.x) / scale, | |
| top: (selectionRect.top - position.y) / scale, | |
| right: (selectionRect.right - position.x) / scale, | |
| bottom: (selectionRect.bottom - position.y) / scale, | |
| }; | |
| // Use the same function that updateFramesInSelection uses to find intersecting frames | |
| const selectedFrames = getFramesInSelectionRectangle(canvasSelectionRect); | |
| // Select the frames if any were found in the selection | |
| if (selectedFrames.length > 0) { | |
| editorEngine.frames.select( | |
| selectedFrames.map(fd => fd.frame), | |
| event.shiftKey // multiselect if shift is held | |
| ); | |
| } | |
| setIsDragSelecting(false); | |
| setFramesInSelection(new Set()); | |
| editorEngine.state.isDragSelecting = false; | |
| }; | |
| window.addEventListener('mouseup', handleGlobalMouseUp); | |
| return () => window.removeEventListener('mouseup', handleGlobalMouseUp); | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx (2)
1-1: Missing 'use client' — this component uses hooks and eventsThis file is in app/ and uses hooks; it must be a client component. Without this, Next.js will error at build/runtime.
+'use client'; import { useEditorEngine } from '@/components/store/editor';
193-199: Security: add rel when using target="_blank"Prevent
window.openerhijacking by addingrel="noopener noreferrer".- target="_blank" + target="_blank" + rel="noopener noreferrer"
🧹 Nitpick comments (5)
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/helpers.ts (3)
61-74: Avoid persisting to storage on every mousemove
updateAndSaveToStorageinside a per‑move loop can thrash storage/history and hurt perf when multi‑selecting. Prefer in‑memory updates during drag and a single save on mouseup, or throttle/RAF‑batch writes.Is
updateAndSaveToStorageinternally debounced? If not, I can propose a RAF‑batched approach that commits once inendMove.
10-13: Type import for React event typingsExplicitly import the React types to make
React.MouseEventresilient to TS config changes.+import type React from 'react'; export function createMouseMoveHandler( startEvent: React.MouseEvent<HTMLDivElement, MouseEvent>, options: MouseMoveHandlerOptions ) {
85-87: Prefer Pointer Events + pointer capture for robust drag (touch, pen, iframe edges)Mouse events miss touch/pen and can lose moves over iframes. Switching to
pointerdown/move/upwithe.currentTarget.setPointerCapture(e.pointerId)improves reliability across devices.I can draft a pointer‑events version if desired.
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx (2)
145-176: Internationalize user‑facing strings per guidelinesHardcoded labels (“Go back”, “Go forward”, “Refresh Page”, “Preview in new tab”) should use next‑intl.
+import { useTranslations } from 'next-intl'; … export const TopBar = observer( ({ frame, isInDragSelection = false }: { frame: Frame; isInDragSelection?: boolean }) => { + const t = useTranslations('TopBar'); … - <HoverOnlyTooltip content="Go back" side="top" className="mb-1" hideArrow> + <HoverOnlyTooltip content={t('goBack')} side="top" className="mb-1" hideArrow> … - <HoverOnlyTooltip content="Go forward" side="top" className="mb-1" hideArrow> + <HoverOnlyTooltip content={t('goForward')} side="top" className="mb-1" hideArrow> … - <HoverOnlyTooltip content="Refresh Page" side="top" className="mb-1" hideArrow> + <HoverOnlyTooltip content={t('refreshPage')} side="top" className="mb-1" hideArrow> … - <HoverOnlyTooltip content="Preview in new tab" side="top" hideArrow className="mb-1"> + <HoverOnlyTooltip content={t('previewInNewTab')} side="top" hideArrow className="mb-1">Also applies to: 192-193
75-78: Minor: define clearElements before usage to avoid TDZ gotchasIt’s safe due to closure timing, but defining
clearElementsabovehandleMouseDownimproves readability and avoids TDZ confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/helpers.ts(1 hunks)apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/helpers.tsapps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/helpers.tsapps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/helpers.tsapps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/helpers.tsapps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx
🧬 Code graph analysis (2)
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/helpers.ts (2)
apps/web/client/src/components/store/editor/engine.ts (1)
EditorEngine(33-138)packages/models/src/project/frame.ts (1)
Frame(4-16)
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx (4)
packages/models/src/project/frame.ts (1)
Frame(4-16)apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)apps/web/client/src/components/store/editor/frames/manager.ts (1)
isSelected(77-79)apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/helpers.ts (1)
createMouseMoveHandler(10-87)
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/helpers.ts
Show resolved
Hide resolved
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar/index.tsx
Show resolved
Hide resolved
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
|
|
||
| const allFrames = editorEngine.frames.getAll(); | ||
| return allFrames.filter(frameData => | ||
| intersectingFrameIds.includes(frameData.frame.id) |
There was a problem hiding this comment.
Consider using a Set for frame ID lookups in getSelectedFrameData to improve performance, especially if the frame list grows large.
Implement drag selection functionality allowing users to select multiple webframes by clicking and dragging on the canvas, similar to Figma and tldraw.
🤖 Generated with Claude Code
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Add drag-to-select functionality for multiple frames on the canvas with visual feedback and multi-select support.
Canvascomponent, allowing users to select multiple frames by dragging a selection rectangle.DragSelectOverlaycomponent for visual feedback of the selection rectangle.isDragSelectingstate toStateManagerinstate/index.tsto manage drag selection state.FrameViewandTopBarcomponents to handle drag selection state and provide visual feedback with teal outlines.createMouseMoveHandlerinhelpers.tsto manage frame dragging logic.getFramesInSelectionandgetSelectedFrameDatainselection-utils.tsto calculate frames within the selection rectangle.This description was created by
for f9bf313. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit