feat: add canvas snap/alignment guidelines for frames #2762
Conversation
|
@reddygtvs is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
|
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. Caution Review failedThe pull request is closed. WalkthroughAdds a snapping system for frame dragging: new SnapManager and types, editorEngine.snap integration, snap-assisted top-bar drag handling that applies snapped positions and toggles guideline visibility, a SnapGuidelines overlay component to render lines, and hides guidelines on clears and drag end. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant TopBar as TopBar Drag Handler
participant Engine as EditorEngine
participant Snap as SnapManager
participant Storage as Frames Store
participant Overlay as SnapGuidelines
U->>TopBar: drag move (dx, dy, modifiers)
TopBar->>Engine: clearElements()
TopBar->>Snap: calculateSnapTarget(frameId, position, dimension)
alt snapping enabled and threshold matched and no Ctrl/Meta
Snap-->>TopBar: SnapTarget(position', snapLines)
TopBar->>Snap: showSnapLines(snapLines)
TopBar->>Storage: updateAndSaveToStorage(frameId, { position: position' })
else no snap
Snap-->>TopBar: null
TopBar->>Snap: hideSnapLines()
TopBar->>Storage: updateAndSaveToStorage(frameId, { position })
end
note over Overlay,Snap: Overlay observes Snap.activeSnapLines and renders guidelines
U-->>TopBar: drag end
TopBar->>Snap: hideSnapLines()
sequenceDiagram
participant App as App
participant Engine as EditorEngine
participant Snap as SnapManager
participant Overlay as SnapGuidelines
App->>Engine: clear()/clearUI()
Engine->>Snap: hideSnapLines()
Overlay-->>Overlay: activeSnapLines empty → render nothing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
![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 (13)
apps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsx (1)
77-77: Consider z-order explicitly for guides (optional).Guidelines render after OverlayButtons and may visually stack above them. With
pointer-events: nonethis won’t break interaction, but if you want buttons to visually dominate, add a lower z-index to the guides container or higher z-index to buttons.Example:
- <SnapGuidelines /> + <SnapGuidelines className="z-10" />(Then accept an optional
classNameprop in SnapGuidelines.)apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar.tsx (1)
44-60: Snapping logic and Ctrl/Cmd override are implemented correctly; one UX caveat.Behavior aligns with the PR description. However, actual snap sensitivity should feel constant in screen pixels regardless of zoom. That is handled inside SnapManager; ensure it uses a scale-adjusted threshold (see my comment in SnapManager).
If needed, you can also give users visual feedback that snapping is disabled while Ctrl/Cmd is held (e.g., temporarily dim guides or show a small “snap off” badge).
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/snap-guidelines.tsx (2)
6-10: Avoid duplicating top bar metrics; move to a shared constant.
TOP_BAR_HEIGHTandTOP_BAR_MARGINare also encoded in TopBar. Drift will misplace lines if either changes. Centralize these values (e.g., a shared visual-config) and import them here and in TopBar.Add a small shared module (example):
// apps/web/client/src/components/store/editor/snap/visual-config.ts export const SNAP_VISUAL_CONFIG = { TOP_BAR_HEIGHT: 28, TOP_BAR_MARGIN: 10, } as const;Then import it here and use it in TopBar.
22-67: Recheck the horizontal “visualOffset” logic across scales and alignment types.Adding
(TOP_BAR_HEIGHT + TOP_BAR_MARGIN) / scaleto all horizontal guides assumes every horizontal line should be pushed down by the top-bar stack. If the line’spositionalready reflects frame bounds (content), this may double-offset; if it reflects the frame’s outer box, it may be correct. Please verify at scales 0.5x, 1x, 2x for:
- top-edge alignment,
- bottom-edge alignment,
- center-horizontal alignment.
If misaligned, consider deriving an explicit “frameOuterToContentYOffset” from the same place that positions TopBar and applying it only when the snapped edge is the top edge. Alternatively, render guidelines relative to the exact rects used to compute snapping to avoid ad-hoc offsets.
apps/web/client/src/components/store/editor/snap/index.ts (3)
75-88: Avoid sorting all candidates on every mousemove; track the best in linear time.This reduces allocations and CPU when many frames exist.
Example change (illustrative, same behavior):
- const snapCandidates: Array<{ position: RectPosition; lines: SnapLine[]; distance: number }> = []; - - for (const otherFrame of otherFrames) { - const candidates = this.calculateSnapCandidates(dragBounds, otherFrame); - snapCandidates.push(...candidates); - } - - if (snapCandidates.length === 0) { - return null; - } - - snapCandidates.sort((a, b) => a.distance - b.distance); - const bestCandidate = snapCandidates[0]; + let bestCandidate: { position: RectPosition; lines: SnapLine[]; distance: number } | null = null; + for (const otherFrame of otherFrames) { + const candidates = this.calculateSnapCandidates(dragBounds, otherFrame); + for (const c of candidates) { + if (!bestCandidate || c.distance < bestCandidate.distance) { + bestCandidate = c; + if (bestCandidate.distance === 0) break; // early exit exact match + } + } + } + if (!bestCandidate) { + return null; + }
98-103: Support showing both axes when both snap within threshold (optional).Tools typically snap independently on X and Y; if both axes qualify, render both guidelines. Current code returns only
lines[0].One approach:
- Track best horizontal and best vertical candidates separately.
- Combine them if both within threshold before returning.
If you prefer minimal changes, keep the current single-line behavior for now.
Also applies to: 226-231
215-223: Stabilize guideline keys to reduce DOM churn (optional).Using
Date.now()changes keys every frame; React will recreate nodes unnecessarily. Encode identity from deterministic parts (type, target frame, rounded position).Apply this diff:
- return { - id: `${type}-${otherFrame.id}-${Date.now()}`, + return { + id: `${type}-${otherFrame.id}-${Math.round(position)}`, type, orientation, position, start, end, frameIds: [otherFrame.id], };apps/web/client/src/components/store/editor/snap/types.ts (6)
3-12: SnapBounds duplicates derivable geometry; consider immutability and documenting invariantsleft/top/right/bottom and centerX/centerY/width/height can drift if any are computed separately. Either document invariants (single source of truth) or expose only one representation and derive the rest at use sites. Also, marking this shape as immutable helps avoid accidental mutation during drag.
Apply immutability via readonly:
-export interface SnapBounds { - left: number; - top: number; - right: number; - bottom: number; - centerX: number; - centerY: number; - width: number; - height: number; -} +export interface SnapBounds { + readonly left: number; + readonly top: number; + readonly right: number; + readonly bottom: number; + readonly centerX: number; + readonly centerY: number; + readonly width: number; + readonly height: number; +}If you already compute bounds from RectPosition/RectDimension in SnapManager, consider removing either width/height or centerX/centerY here to enforce one source of truth.
14-18: “distance” is ambiguous; capture snap delta per axis and make fields readonlyA scalar distance doesn’t convey whether we snapped on X, Y, or both. Explicit dx/dy improves intent and avoids sign ambiguity. Also, make this immutable and the lines array readonly.
-export interface SnapTarget { - position: RectPosition; - snapLines: SnapLine[]; - distance: number; -} +export interface SnapTarget { + readonly position: RectPosition; + readonly snapLines: readonly SnapLine[]; + /** Total scalar distance used to rank candidates (retain for compatibility). */ + readonly distance: number; + /** Pixel delta to apply to reach the snapped position. */ + readonly offset: { dx: number; dy: number }; +}Confirm the drag code in top-bar uses both axes; if it only snaps along one axis per interaction, we can make offset one-dimensional conditionally.
20-28: Avoid type/orientation inconsistencies; add immutability and optional spacing valueorientation is derivable from type (e.g., EDGE_TOP implies horizontal). Keeping both risks mismatch. If you keep orientation, add a runtime assert or derive it in a helper. Also, make fields readonly. For spacing guidelines, consider carrying the spacing amount.
-export interface SnapLine { - id: string; - type: SnapLineType; - orientation: 'horizontal' | 'vertical'; - position: number; - start: number; - end: number; - frameIds: string[]; -} +export interface SnapLine { + readonly id: string; + readonly type: SnapLineType; + readonly orientation: 'horizontal' | 'vertical'; + /** Primary axis coordinate of the guide line. */ + readonly position: number; + /** Extents along the secondary axis. */ + readonly start: number; + readonly end: number; + /** For SPACING lines, the spacing amount in pixels. */ + readonly spacing?: number; + readonly frameIds: readonly string[]; +}If you prefer to eliminate the mismatch entirely, consider a discriminated union keyed by orientation:
type Orientation = 'horizontal' | 'vertical'; type HorizontalSnapLine = Readonly<{ id: string; type: Extract<SnapLineType, 'edge-top' | 'edge-bottom' | 'center-horizontal' | 'spacing'>; orientation: 'horizontal'; position: number; // y start: number; // x1 end: number; // x2 spacing?: number; frameIds: readonly string[]; }>; type VerticalSnapLine = Readonly<{ id: string; type: Extract<SnapLineType, 'edge-left' | 'edge-right' | 'center-vertical' | 'spacing'>; orientation: 'vertical'; position: number; // x start: number; // y1 end: number; // y2 spacing?: number; frameIds: readonly string[]; }>; export type SnapLine = HorizontalSnapLine | VerticalSnapLine;
30-38: Prefer string-literal union over runtime enum for shared libsUsing a TypeScript enum emits runtime code; a union keeps this file type-only and tree-shakeable.
-export enum SnapLineType { - EDGE_LEFT = 'edge-left', - EDGE_RIGHT = 'edge-right', - EDGE_TOP = 'edge-top', - EDGE_BOTTOM = 'edge-bottom', - CENTER_HORIZONTAL = 'center-horizontal', - CENTER_VERTICAL = 'center-vertical', - SPACING = 'spacing', -} +export type SnapLineType = + | 'edge-left' + | 'edge-right' + | 'edge-top' + | 'edge-bottom' + | 'center-horizontal' + | 'center-vertical' + | 'spacing';Optional helper (outside this file) to derive orientation from type:
export const orientationFromType = (t: SnapLineType): 'horizontal' | 'vertical' => t === 'edge-top' || t === 'edge-bottom' || t === 'center-horizontal' ? 'horizontal' : 'vertical';
40-45: SnapFrame doubles state with bounds + position/dimension; make immutable or document derivationCarrying both position/dimension and bounds introduces drift risk on updates. If bounds are cached for perf, document that they’re derived and refresh together; otherwise consider deriving on read. Also, mark fields readonly.
-export interface SnapFrame { - id: string; - position: RectPosition; - dimension: RectDimension; - bounds: SnapBounds; -} +export interface SnapFrame { + readonly id: string; + readonly position: RectPosition; + readonly dimension: RectDimension; + readonly bounds: SnapBounds; +}
47-51: Config is missing the “Ctrl/Cmd disables snapping” behavior; add typed modifiers and defaultsPR summary states Ctrl/Cmd disables snapping. Encode that here to avoid scattering UI-specific checks. Also, readonly fields help treat config as value objects.
-export interface SnapConfig { - threshold: number; - enabled: boolean; - showGuidelines: boolean; -} +export interface SnapConfig { + /** Snap activation threshold in pixels. Default: 12. */ + readonly threshold: number; + readonly enabled: boolean; + readonly showGuidelines: boolean; + /** Modifier keys that temporarily disable snapping (KeyboardEvent.key values). */ + readonly disableWithModifiers?: ReadonlyArray<'Meta' | 'Control'>; +}Optionally centralize defaults in a constants module:
// apps/web/client/src/components/store/editor/snap/constants.ts export const DEFAULT_SNAP_THRESHOLD = 12; export const DEFAULT_DISABLE_WITH_MODIFIERS: ReadonlyArray<'Meta' | 'Control'> = ['Meta', 'Control'];I can follow through by updating SnapManager initialization to use these defaults and wire the modifier check to this config if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/snap-guidelines.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsx(2 hunks)apps/web/client/src/components/store/editor/engine.ts(3 hunks)apps/web/client/src/components/store/editor/snap/index.ts(1 hunks)apps/web/client/src/components/store/editor/snap/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsx (1)
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/snap-guidelines.tsx (1)
SnapGuidelines(11-67)
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/snap-guidelines.tsx (1)
apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(9-13)
apps/web/client/src/components/store/editor/snap/index.ts (2)
apps/web/client/src/components/store/editor/snap/types.ts (5)
SnapConfig(47-51)SnapLine(20-28)SnapBounds(3-12)SnapFrame(40-45)SnapTarget(14-18)apps/web/client/src/components/store/editor/engine.ts (1)
EditorEngine(32-115)
apps/web/client/src/components/store/editor/engine.ts (1)
apps/web/client/src/components/store/editor/snap/index.ts (1)
SnapManager(12-240)
🔇 Additional comments (7)
apps/web/client/src/components/store/editor/engine.ts (3)
25-25: Good integration: SnapManager is part of the EditorEngine public surface.Import and initialization pattern are consistent with other managers. Positioning
snapafterframesensures dependencies are available during construction.Also applies to: 63-63
96-97: Hide guidelines on full clear is correct.Clearing snap lines during
clear()prevents stale guides when projects switch or engine resets.
103-104: UI clear also hides guidelines — consistent with UX expectations.This avoids leftover guides after selection/UI resets.
apps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsx (1)
13-13: Overlay now renders SnapGuidelines — import looks correct.No side effects on existing overlay logic.
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar.tsx (2)
39-43: MutablenewPositionto accommodate snapping is appropriate.This sets up the subsequent snap adjustment correctly.
67-67: Hiding guides on drag end is correct.Prevents ghost lines after interaction.
apps/web/client/src/components/store/editor/snap/types.ts (1)
1-1: Good use ofimport typeUsing
import typeavoids bundling runtime code and keeps this module purely declarative.
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
Awesome work @reddygtvs !
2 participants
Description
Added snap/alignment guidelines for frame positioning. Shows red lines when frames align to edges or centers, with 12px snap threshold. Ctrl/Cmd disables snapping.
Related Issues
related to #2702
Type of Change
Testing
Screenshots (if applicable)
Screen.Recording.-.Aug.26.2025-VEED.mp4
Additional Notes
Open to making any changes related to styling (or otherwise). It was not defined in the feature request, so I ended up going with the standardised red/thin line/12 px snap offset based off what Figma/Adobe are doing for their own implementation.
Important
Adds snapping and alignment guidelines for frames with visual feedback and configurable settings in the canvas editor.
top-bar.tsx.SnapGuidelinescomponent insnap-guidelines.tsxfor rendering snap lines.SnapManagerinengine.tsfor managing snapping logic and configuration.SnapManagerhandles snap configuration, including enabling/disabling and threshold settings.This description was created by
for f8b3eb6. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit