-
Notifications
You must be signed in to change notification settings - Fork 809
Improve area select in new recording flow #1067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors the selection overlay to a two-phase interaction: new zero-sized selection on mousedown outside existing bounds, then resize/drag via mousemove with updated state flags. Removes minimum size defaults, revises control visibility/placement logic, and updates event handling for mousedown/mousemove/mouseup and window resize. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Overlay
participant SelectionState as Selection State
participant Controls
rect rgba(230,240,255,0.5)
note over User,Overlay: Start new selection
User->>Overlay: mousedown (outside selection)
Overlay->>SelectionState: set selecting=true, bounds=(x,y,0,0)
User->>Overlay: mousemove
Overlay->>SelectionState: update bounds (resize)
User->>Overlay: mouseup
Overlay->>SelectionState: set selecting=false
end
rect rgba(240,255,230,0.5)
note over User,Overlay: Drag existing selection
User->>Overlay: mousedown (inside selection)
Overlay->>SelectionState: set dragging=true, capture offset
User->>Overlay: mousemove
Overlay->>SelectionState: update bounds (translate, clamp)
User->>Overlay: mouseup
Overlay->>SelectionState: set dragging=false
end
Overlay->>Controls: compute visibility/position (above/below based on viewport)
Note over Controls,Overlay: Recompute on window resize
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
0, | ||
Math.min( | ||
window.innerWidth - Math.max(0, newBounds.position.x), | ||
newBounds.size.width, | ||
), | ||
), | ||
height: Math.max( | ||
150, | ||
0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0, | |
Math.min( | |
window.innerWidth - Math.max(0, newBounds.position.x), | |
newBounds.size.width, | |
), | |
), | |
height: Math.max( | |
150, | |
0, | |
150, | |
Math.min( | |
window.innerWidth - Math.max(0, newBounds.position.x), | |
newBounds.size.width, | |
), | |
), | |
height: Math.max( | |
150, |
Inconsistent minimum size constraints between drag selection and resize handles will cause unexpected behavior when users create small selections.
View Details
Analysis
Inconsistent minimum size constraints between drag selection and resize handles
What fails: In target-select-overlay.tsx
, the setBounds
function allows drag selections with 0px minimum size while ResizeHandles
component enforces 150px minimum, creating jarring UX inconsistency.
How to reproduce:
- Open area selection mode in the desktop app
- Drag-select a small area (e.g., 50x50px) - this works fine
- Try to resize that selection using the corner handles
- Selection immediately snaps to 150x150px minimum
Result: User can create 50x50px selections via drag, but any resize operation snaps to 150x150px, causing unexpected size jumps.
Expected: Both drag selection and resize operations should enforce the same minimum constraint for consistent user experience.
Fix: Updated setBounds
function to use Math.max(150, ...)
instead of Math.max(0, ...)
for both width and height, matching the resize handles behavior.
This improvement is mainly targetted at users with more than one monitor as it can be wierd having a different area select box per-screen.
TODO:
Summary by CodeRabbit