fix: DH-22093: Fix web UI freezing bug from grid-block-events#2646
fix: DH-22093: Fix web UI freezing bug from grid-block-events#2646vbabich merged 1 commit intodeephaven:mainfrom
Conversation
vbabich
changed the title
vbabich
requested review from
a team and
dgodinez-dh
and removed request for
a team
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2646 +/- ##
==========================================
+ Coverage 49.51% 49.55% +0.04%
==========================================
Files 774 774
Lines 43864 43868 +4
Branches 11292 11293 +1
==========================================
+ Hits 21720 21740 +20
+ Misses 22098 22082 -16
Partials 46 46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes a web UI freeze caused by the grid-block-events document class being left behind or removed incorrectly during drag interactions/unmount, by tightening cleanup behavior and adding regression tests.
Changes:
- Track whether a
Gridinstance addedgrid-block-eventsand adjust cleanup logic accordingly. - Reorder
handleMouseUpcleanup to ensure drag timers/cursor overlays are removed even on non-left mouseups (when not actively dragging). - Add test coverage for
grid-block-eventscleanup scenarios (normal drag, right/middle click interactions, unmount during drag, multi-instance cases).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
packages/grid/src/Grid.tsx |
Adds per-instance tracking for grid-block-events ownership and adjusts mouseup cleanup order. |
packages/grid/src/Grid.test.tsx |
Adds regression tests around grid-block-events cleanup and multi-instance/unmount scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // removeDocumentCursor should still clean up | ||
| // BUG: Currently it doesn't because documentCursor is null and | ||
| // the entire function body is guarded by `if (this.documentCursor != null)` | ||
| component.removeDocumentCursor(); | ||
|
|
||
| // After fix, this should be false |
There was a problem hiding this comment.
These comments describe behavior as a current bug ("BUG: Currently it doesn't...") but the implementation in this PR makes removeDocumentCursor() remove grid-block-events even when documentCursor is null. Please update/remove the bug explanation so the test documents the new expected behavior rather than the pre-fix state.
| // removeDocumentCursor should still clean up | |
| // BUG: Currently it doesn't because documentCursor is null and | |
| // the entire function body is guarded by `if (this.documentCursor != null)` | |
| component.removeDocumentCursor(); | |
| // After fix, this should be false | |
| // removeDocumentCursor should clean up grid-block-events | |
| // even if documentCursor is currently null | |
| component.removeDocumentCursor(); | |
| // grid-block-events should be removed after calling removeDocumentCursor |
| expect(hasBlockEventsClass()).toBe(false); | ||
| }); | ||
|
|
||
| it('should remove grid-block-events when right-clicking during drag (case 1)', () => { |
There was a problem hiding this comment.
Test title is misleading: the test asserts grid-block-events remains after the right-click mouseup and is only removed after the subsequent left mouseup. Rename the test (or adjust assertions) so the title matches the behavior being verified.
| it('should remove grid-block-events when right-clicking during drag (case 1)', () => { | |
| it('should keep grid-block-events after right-click during drag until left mouse up (case 1)', () => { |
| function getColumnSeparatorX(columnIndex: VisibleIndex): number { | ||
| const { rowHeaderWidth, columnWidth } = resizableTheme; | ||
| return rowHeaderWidth + columnWidth * (columnIndex + 1) - 2; | ||
| } | ||
|
|
||
| function getColumnHeaderY(): number { | ||
| return Math.floor(resizableTheme.columnHeaderHeight / 2); | ||
| } |
There was a problem hiding this comment.
getColumnSeparatorX / getColumnHeaderY helpers are duplicated from the earlier "column separators" tests in this file. To reduce drift and make future updates easier, consider hoisting these helpers to a shared scope (or reusing the existing ones) instead of redefining them in each describe block.
| document.documentElement.classList.add('grid-block-events'); | ||
| this.hasAddedBlockEvents = true; |
There was a problem hiding this comment.
hasAddedBlockEvents is set to true unconditionally after classList.add('grid-block-events'). If another Grid instance (or any other code) already added grid-block-events, this instance will later remove it in removeDocumentCursor(), reintroducing the cross-instance interference this change is trying to prevent. Consider setting hasAddedBlockEvents only when the class was not present before (or using a shared ref-count / global ownership token) so removal only happens when this instance actually transitioned the class from absent→present.
| document.documentElement.classList.add('grid-block-events'); | |
| this.hasAddedBlockEvents = true; | |
| const docEl = document.documentElement; | |
| const hadBlockEventsClass = docEl.classList.contains('grid-block-events'); | |
| docEl.classList.add('grid-block-events'); | |
| this.hasAddedBlockEvents = !hadBlockEventsClass; |
No description provided.