Repository navigation
fix(wework): prevent terminal panel resize lag - #2058
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds drag support to desktop verification commands and automation, and changes bottom-panel resizing to use imperative DOM height updates with pointer-cancel handling. It also adds coverage for resize rendering and height behavior. ChangesBottom panel resize behavior
Desktop drag verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AIVerifyCLI
participant DesktopAutomation
participant DOMElement
AIVerifyCLI->>DesktopAutomation: dispatch drag with selector and target
DesktopAutomation->>DOMElement: validate origin and target
DesktopAutomation->>DOMElement: dispatch pointerdown, pointermove, pointerup
DOMElement-->>DesktopAutomation: return origin text
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wework/src/components/layout/workspace-panels/useResizableWorkspacePanel.ts (1)
158-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCapture the pointer to ensure reliable drag events outside the window.
When the user drags near the edge of the screen, the cursor can easily leave the Tauri webview boundary. Without pointer capture, the webview will stop receiving
pointermoveandpointerupevents, which can leave the panel stuck in a resizing state indefinitely.Call
setPointerCaptureon the event target so that all subsequent pointer events are routed to it until the pointer is released.🛠️ Proposed fix
const handleResizeStart = (event: PointerEvent<HTMLDivElement>) => { event.preventDefault() + event.currentTarget.setPointerCapture(event.pointerId)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/layout/workspace-panels/useResizableWorkspacePanel.ts` around lines 158 - 160, Update handleResizeStart to call setPointerCapture with the event’s pointerId on the event target immediately after preventing the default action, ensuring subsequent resize pointer events remain routed to the target outside the webview boundary.
🧹 Nitpick comments (2)
wework/src/e2e/automation.ts (1)
377-391: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
dispatchDesktopControlPointerEventhelper for consistency and correct event delegation.Dispatching
pointermoveandpointerupdirectly ondocumentbypasses theMouseEventfallback logic provided bydispatchDesktopControlPointerEvent. Furthermore, dispatching ondocumentinstead of a DOM element causes the event's nativetargetto bedocument, which can break React's synthetic event delegation since React listens at the root container rather than the document.Because
desktopControlEventOptionssetsbubbles: true, dispatching these events on the destinationtargetelement will correctly bubble up todocumentfor any global listeners, while preserving the abstraction and React compatibility.♻️ Proposed refactor
const startOptions = { ...desktopControlEventOptions(element), buttons: 1 } const endOptions = { ...desktopControlEventOptions(target), buttons: 1 } dispatchDesktopControlPointerEvent(element, 'pointerdown', startOptions) - document.dispatchEvent(new PointerEvent('pointermove', endOptions)) - document.dispatchEvent(new PointerEvent('pointerup', { ...endOptions, buttons: 0 })) + dispatchDesktopControlPointerEvent(target, 'pointermove', endOptions) + dispatchDesktopControlPointerEvent(target, 'pointerup', { ...endOptions, buttons: 0 }) return element.textContent?.trim() ?? '' }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/e2e/automation.ts` around lines 377 - 391, Update dragDesktopControlElement to dispatch the pointermove and pointerup events through dispatchDesktopControlPointerEvent on the destination target element instead of constructing events on document. Preserve the existing endOptions values and pointerup buttons: 0 behavior while retaining the pointerdown dispatch on the source element.wework/src/components/layout/workspace-panels/useResizableWorkspacePanel.ts (1)
150-156: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClean up side-effects if unmounted during an active resize.
If the panel is unmounted while being dragged (e.g., by a hot-reload or a keyboard shortcut), the
pointermove,pointerup, andpointercancellisteners will remain attached to thedocument, and thedocument.bodycursor will remain stuck asrow-resize.Consider using an
AbortControllerto cleanly remove the event listeners on unmount and revert the global body styles.🛠️ Proposed refactor
Keep track of the abort controller in a ref, and abort it during cleanup:
+ const abortControllerRef = useRef<AbortController | null>(null) + useEffect(() => { return () => { if (resizeFrameRef.current !== null) { window.cancelAnimationFrame(resizeFrameRef.current) } + if (abortControllerRef.current) { + abortControllerRef.current.abort() + document.body.style.cursor = '' + document.body.style.userSelect = '' + } } }, [])Then, update
handleResizeStartandfinishResizeto use thesignal:const finishResize = () => { abortControllerRef.current?.abort() abortControllerRef.current = null // ... (remove the manual document.removeEventListener calls) } // ... abortControllerRef.current = new AbortController() const { signal } = abortControllerRef.current document.addEventListener('pointermove', handleMove, { signal }) document.addEventListener('pointerup', handleUp, { signal }) document.addEventListener('pointercancel', handleCancel, { signal })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/layout/workspace-panels/useResizableWorkspacePanel.ts` around lines 150 - 156, Update the resize lifecycle around handleResizeStart and finishResize to store an AbortController in a ref, register pointermove, pointerup, and pointercancel listeners with its signal, and abort it when resizing finishes or the component unmounts. Ensure cleanup also resets document.body cursor styles and clears the controller ref, while preserving the existing animation-frame cancellation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@wework/src/components/layout/workspace-panels/useResizableWorkspacePanel.ts`:
- Around line 158-160: Update handleResizeStart to call setPointerCapture with
the event’s pointerId on the event target immediately after preventing the
default action, ensuring subsequent resize pointer events remain routed to the
target outside the webview boundary.
---
Nitpick comments:
In `@wework/src/components/layout/workspace-panels/useResizableWorkspacePanel.ts`:
- Around line 150-156: Update the resize lifecycle around handleResizeStart and
finishResize to store an AbortController in a ref, register pointermove,
pointerup, and pointercancel listeners with its signal, and abort it when
resizing finishes or the component unmounts. Ensure cleanup also resets
document.body cursor styles and clears the controller ref, while preserving the
existing animation-frame cancellation.
In `@wework/src/e2e/automation.ts`:
- Around line 377-391: Update dragDesktopControlElement to dispatch the
pointermove and pointerup events through dispatchDesktopControlPointerEvent on
the destination target element instead of constructing events on document.
Preserve the existing endOptions values and pointerup buttons: 0 behavior while
retaining the pointerdown dispatch on the source element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 754f1876-09c9-4cce-b4d5-4e0b3cfa2e5a
📒 Files selected for processing (6)
wework/AGENTS.mdwework/scripts/ai-verify.mjswework/src/components/layout/workspace-panels/BottomWorkspacePanel.tsxwework/src/components/layout/workspace-panels/useResizableWorkspacePanel.test.tsxwework/src/components/layout/workspace-panels/useResizableWorkspacePanel.tswework/src/e2e/automation.ts
|
Addressed the review findings in 6bb6a4d:
Revalidated with 142 focused tests, lint/format checks, pre-push ESLint/TypeScript/unit tests, and real Tauri expand/shrink screenshots. |
What changed
dragverification command and document itWhy
Every animation frame previously updated React state for the whole bottom workspace panel. That repeatedly rerendered the terminal subtree while xterm also observed, fitted, and synchronized its changing container size, creating an expensive render/measure/resize cascade that made the divider lag and eventually feel stuck.
Impact
The terminal divider now tracks the pointer smoothly while preserving the existing min/max bounds and final panel height. Terminal content remains mounted and responsive throughout resizing.
Validation
pnpm --filter wework test— 181 files / 1820 tests passedSummary by CodeRabbit