Conversation
When toggling the toolbar's enabled state, the expandable buttons cause a width change that shifts the toggle switch out from under the cursor. This adds position compensation that offsets the toolbar horizontally by the expandable buttons width, clamped to viewport bounds. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Medium Urgency - Implementation is mostly solid, but there's a critical race condition in rapid toggling and several edge cases that could cause incorrect positioning.
| const handleToggleEnabled = createDragAwareHandler(() => { | ||
| const isCurrentlyEnabled = Boolean(props.enabled); | ||
| const edge = snapEdge(); | ||
| const preTogglePosition = position(); | ||
| const expandableWidth = lastKnownExpandableWidth; | ||
| const shouldCompensatePosition = expandableWidth > 0 && edge !== "left"; |
There was a problem hiding this comment.
Race condition with rapid toggling: The logic reads lastKnownExpandableWidth synchronously, but if the user rapidly toggles, the previous timeout (lines 500-509) might not have completed yet, meaning lastKnownExpandableWidth could still be 0 on subsequent toggles after the first enable. This would cause no compensation on the second toggle.
Consider setting lastKnownExpandableWidth synchronously during the first enable if expandableButtonsRef is available, rather than deferring it to the timeout.
| if (expandableWidth > 0) { | ||
| const widthChange = isCurrentlyEnabled ? -expandableWidth : expandableWidth; | ||
| expandedDimensions = { | ||
| width: expandedDimensions.width + widthChange, | ||
| height: expandedDimensions.height, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Missing synchronization: expandedDimensions is mutated here, but this happens before the DOM has actually changed (the toggle happens at line 456). When toggling from enabled→disabled, you're subtracting expandableWidth, but the toolbar is still visually expanded until the CSS animation completes.
This creates a mismatch: expandedDimensions.width no longer reflects the actual visual width during the animation. If a resize event or state change callback fires during the animation, calculations using expandedDimensions will be incorrect.
| if (shouldCompensatePosition) { | ||
| const viewport = getVisualViewport(); | ||
| const positionOffset = isCurrentlyEnabled ? expandableWidth : -expandableWidth; | ||
| const clampMin = viewport.offsetLeft + TOOLBAR_SNAP_MARGIN_PX; | ||
| const clampMax = viewport.offsetLeft + viewport.width - expandedDimensions.width - TOOLBAR_SNAP_MARGIN_PX; | ||
| const compensatedX = clampToViewport( | ||
| preTogglePosition.x + positionOffset, | ||
| clampMin, | ||
| clampMax, | ||
| ); |
There was a problem hiding this comment.
Using updated dimensions for clamping before they're accurate: You calculate clampMax using the already-mutated expandedDimensions.width (line 470), but this assumes the toolbar has already changed size. During the toggle animation, the toolbar hasn't resized yet, so clamping against the future dimensions could cause the toolbar to shift incorrectly.
For disabling (going from wide→narrow), you should use the old width for initial clamping, then let the position settle after the animation. For enabling (narrow→wide), you need the new width, but it's not yet rendered.
| toggleAnimationTimeout = setTimeout(() => { | ||
| setIsToggleAnimating(false); | ||
| const newRatio = getRatioFromPosition( | ||
| edge, | ||
| position().x, | ||
| position().y, | ||
| expandedDimensions.width, | ||
| expandedDimensions.height, | ||
| ); | ||
| setPositionRatio(newRatio); | ||
| saveAndNotify({ | ||
| edge, | ||
| ratio: newRatio, | ||
| collapsed: isCollapsed(), | ||
| enabled: !isCurrentlyEnabled, | ||
| }); | ||
| }, TOOLBAR_COLLAPSE_ANIMATION_DURATION_MS); |
There was a problem hiding this comment.
Position recalculation uses stale position signal: Line 484 reads position().x and position().y inside the timeout callback. However, the user could have dragged the toolbar, or a resize could have occurred during the 150ms animation. This would cause the ratio calculation to be based on wherever the toolbar ended up, not where the compensation placed it.
Consider capturing the compensated position in a variable and using that for the ratio calculation, rather than reading from the signal.
| if (props.enabled && expandableButtonsRef) { | ||
| lastKnownExpandableWidth = expandableButtonsRef.offsetWidth; | ||
| } |
There was a problem hiding this comment.
Measuring expandable width on mount when disabled: If the toolbar mounts with props.enabled === false, this block won't execute (line 886 condition fails). But you correctly handle this later in the deferred width learning (lines 497-510). However, there's a subtle issue: if the toolbar mounts enabled, you measure immediately here, but what if expandableButtonsRef hasn't been assigned yet?
SolidJS ref assignment happens during rendering, but onMount runs after the component returns. There's no guarantee expandableButtonsRef is defined at this point. Consider using createEffect or adding a null check.
| const unsubscribe = props.onSubscribeToStateChanges( | ||
| (state: ToolbarState) => { | ||
| if (isCollapseAnimating()) return; | ||
| if (isCollapseAnimating() || isToggleAnimating()) return; |
There was a problem hiding this comment.
Blocking state change callbacks during toggle animation: This guards against external state changes during animations, which is good. However, this means if another toolbar instance (or the same toolbar in another browser tab) toggles enabled/disabled, this toolbar won't sync until the animation completes. This could cause a brief desync in multi-instance scenarios.
Not necessarily a bug, but worth considering if this is the intended behavior.
| > | ||
| Select | ||
| </Tooltip> | ||
| <div ref={expandableButtonsRef} class="flex items-center"> |
There was a problem hiding this comment.
Ref assignment to wrapper div: The ref is assigned to a wrapper div containing both expandable buttons. This works, but you're measuring the combined width. If there's any gap, margin, or padding between the two buttons at the wrapper level (the flex items-center at line 1077), you'll measure more than just the buttons themselves.
Looking at the code, this seems intentional and correct since both buttons are always shown/hidden together. But verify there's no extra spacing that would throw off measurements.
@react-grab/cli
grab
@react-grab/ami
@react-grab/amp
@react-grab/claude-code
@react-grab/codex
@react-grab/cursor
@react-grab/droid
@react-grab/gemini
@react-grab/opencode
react-grab
@react-grab/relay
@react-grab/utils
commit: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
aidenybai#164) When toggling the toolbar's enabled state, the expandable buttons cause a width change that shifts the toggle switch out from under the cursor. This adds position compensation that offsets the toolbar horizontally by the expandable buttons width, clamped to viewport bounds. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
grid-cols-[0fr]), measuring after the first enable animation completes.Test plan
Made with Cursor
Note
Medium Risk
Touches core toolbar positioning/state persistence and introduces timing-based width measurement and animation gating, which could cause edge-case layout/state desync across snap edges or rapid toggles.
Overview
Fixes toolbar jank when toggling
enabledby compensating the toolbar’sxposition for the width of the expandable buttons (Select/Comment), clamping to viewport bounds and skipping compensation on the left snap edge.Adds toggle-specific animation state/timeouts (
isToggleAnimating,toggleAnimationTimeout) to prevent external state sync from fighting in-progress toggle/collapse animations, and introduces a measuredexpandableButtonsRef/lastKnownExpandableWidthwith a deferred “learn width after first enable” fallback when mounting disabled.Written by Cursor Bugbot for commit ed3c642. This will update automatically on new commits. Configure here.
Summary by cubic
Keeps the toolbar toggle switch under the cursor by compensating for width changes when expandable buttons appear/disappear. Adds edge-aware horizontal offset with viewport clamping for smooth, stable toggles.
Written for commit ed3c642. Summary will update on new commits.