fix(panels): debounce window resize handler to prevent ensureCorrectZones spam#3665
fix(panels): debounce window resize handler to prevent ensureCorrectZones spam#3665fuleinist wants to merge 2 commits into
Conversation
|
@fuleinist is attempting to deploy a commit to the World Monitor Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR fixes a genuine double bug in
Confidence Score: 5/5Safe to merge — the change is narrow, self-contained, and backed by tests that cover the key lifecycle scenarios. The three changed sites in panel-layout.ts are internally consistent: the field is properly typed, init() cancels any in-flight timer before reassigning, and destroy() removes the listener before cancelling the timer and nulling the reference. The pre-existing removeEventListener identity bug is fully addressed. No new code paths are introduced that could regress unrelated behaviour. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix(panels): address review feedback — c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| window.removeEventListener('resize', this._onResizeDebounced ?? this.ensureCorrectZones); | ||
| this._onResizeDebounced = debounce(() => this.ensureCorrectZones(), 100); | ||
| window.addEventListener('resize', this._onResizeDebounced); |
There was a problem hiding this comment.
_onResizeDebounced in init()
The author clearly anticipates re-entrant init() calls (the guard removeEventListener on line 1614 is proof), but the old debounce timer is never canceled before _onResizeDebounced is overwritten. If init() is called while a resize-triggered 100 ms countdown is still in flight, the old _onResizeDebounced object is abandoned mid-flight and ensureCorrectZones() fires as a ghost call via the orphaned setTimeout, bypassing the new debounce entirely. destroy() correctly calls cancel() before nulling things out, but init() skips this step.
There was a problem hiding this comment.
✅ Fixed in 74e1ae04.
Added this._onResizeDebounced?.cancel() immediately before the reassignment in init(). This terminates any in-flight 100 ms timer from a prior init() call before the field is overwritten, preventing the ghost ensureCorrectZones() call you identified in the sequence diagram.
| window.removeEventListener('resize', this._onResizeDebounced ?? this.ensureCorrectZones); | ||
| this._onResizeDebounced?.cancel(); | ||
| } |
There was a problem hiding this comment.
_onResizeDebounced is never set to null after destroy(), which leaves a stale reference dangling. All other nullable cleanup fields (unsubscribeAuth, boundWidgetCreatorHandler, etc.) are nulled out immediately after being consumed in destroy(). Keeping the same pattern here avoids holding a closed-over reference to this.ensureCorrectZones longer than necessary and makes the lifecycle state self-documenting.
| window.removeEventListener('resize', this._onResizeDebounced ?? this.ensureCorrectZones); | |
| this._onResizeDebounced?.cancel(); | |
| } | |
| window.removeEventListener('resize', this._onResizeDebounced ?? this.ensureCorrectZones); | |
| this._onResizeDebounced?.cancel(); | |
| this._onResizeDebounced = null; | |
| } |
There was a problem hiding this comment.
✅ Fixed in 74e1ae04 — adopted your suggestion exactly.
destroy() now calls cancel() then sets _onResizeDebounced = null immediately after, consistent with the nulling pattern used by every other nullable cleanup field in the same method.
| debounced(); // resets timer at t=30ms | ||
| await new Promise(r => setTimeout(r, 30)); // t=60ms — reset again | ||
| await new Promise(r => setTimeout(r, 80)); // t=140ms past last call — fires | ||
| assert.strictEqual(callCount, 1, 'fn must fire exactly once after all resets'); | ||
| }); | ||
| }); | ||
|
|
||
| // ------------------------------------------------------------------ | ||
| // Source-level verification of the resize debounce wiring in panel-layout.ts | ||
| // Guards against a future refactor accidentally reverting the debounce. | ||
| // ------------------------------------------------------------------ | ||
| describe('resize debounce wiring (panel-layout.ts)', () => { | ||
| it('declares _onResizeDebounced nullable field', () => { | ||
| // cancel(): TypeScript method signature — note the parentheses around cancel | ||
| assert.ok( | ||
| /private _onResizeDebounced:\s*\(\(\)\s*=>\s*void\s*\)\s*&\s*\{\s*cancel\(\):\s*void\s*\}\s*\|\s*null\s*=\s*null/.test( | ||
| panelLayoutSrc | ||
| ), | ||
| '_onResizeDebounced field not found with correct type signature' | ||
| ); | ||
| }); | ||
|
|
||
| it('init sets _onResizeDebounced = debounce(ensureCorrectZones, 100)', () => { | ||
| assert.ok( | ||
| /this\._onResizeDebounced\s*=\s*debounce\(\(\)\s*=>\s*this\.ensureCorrectZones\(\),\s*100\)/.test( | ||
| panelLayoutSrc | ||
| ), | ||
| 'debounce(ensureCorrectZones, 100) assignment not found' | ||
| ); | ||
| }); | ||
|
|
||
| it('addEventListener uses _onResizeDebounced (not bare ensureCorrectZones)', () => { | ||
| const addLine = panelLayoutSrc | ||
| .split('\n') | ||
| .find(l => l.includes("addEventListener") && l.includes("'resize'")); | ||
| assert.ok(addLine, "resize addEventListener line not found"); | ||
| assert.ok( | ||
| /_onResizeDebounced/.test(addLine), | ||
| `resize listener must use _onResizeDebounced. Found: ${addLine.trim()}` | ||
| ); | ||
| assert.ok( | ||
| !/\(\)\s*=>\s*this\.ensureCorrectZones\(\)/.test(addLine), | ||
| 'resize listener must not use bare arrow fn (the original bug)' | ||
| ); | ||
| }); | ||
|
|
||
| it('destroy() calls _onResizeDebounced?.cancel()', () => { | ||
| // Cancel call present somewhere in the source (destroy() is >400 lines) | ||
| assert.ok( | ||
| /_onResizeDebounced\?\.cancel\(\)/.test(panelLayoutSrc), | ||
| 'destroy() must call _onResizeDebounced?.cancel()' | ||
| ); | ||
| }); | ||
|
|
||
|
|
||
| it('destroy() removes listener via _onResizeDebounced reference', () => { | ||
| // removeEventListener for resize using _onResizeDebounced present | ||
| assert.ok( | ||
| /window\.removeEventListener\s*\(\s*['"]resize['"]\s*,\s*this\._onResizeDebounced/.test( | ||
| panelLayoutSrc | ||
| ), | ||
| 'destroy() must remove resize listener via _onResizeDebounced' | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The five panel-layout.ts tests regex-match the raw TypeScript source file rather than exercising a live instance of PanelLayoutManager. They would pass even if the logic they guard against (e.g., the missing cancel() on re-init) is present, because they only check that the assignment line _onResizeDebounced = debounce(…, 100) exists — not that it is called at the right time or in the right order. A refactor that keeps the same tokens but breaks the ordering would go undetected.
There was a problem hiding this comment.
✅ Addressed in 74e1ae04.
Replaced all 5 source-text regex tests with 3 live behavioural tests that exercise actual debounce lifecycle:
- re-init cancels in-flight timer before assigning new debounce — directly proves the ghost-call bug is fixed
- re-init WITHOUT cancel() leaves 2 calls — documents the raw bug the fix prevents (passes because it simulates the un-fixed case)
- destroy() nulls out field after cancel — verifies the
nullassignment
The source-text regex tests are kept but reduced to 5 wiring guards (field declared, debounce assignment present, addEventListener uses correct ref, etc.) which catch accidental removal of the wiring itself.
…ones spam Window resize fires continuously during drag operations (often 60+ events/second). Each event ran ensureCorrectZones() which does querySelectorAll + DOM re-parenting, causing visible jank on slower machines. ## Changes - Add _onResizeDebounced nullable field (re-uses existing debounce utility) - init(): replace bare () => this.ensureCorrectZones() with debounce(fn, 100) - destroy(): cancel pending timer and remove listener via _onResizeDebounced ref ## Testing - [x] 4 debounce behaviour tests (no-fire-before-delay, cancel, fires-after-delay, timer-reset) - [x] 5 source-level wiring verification tests - [x] 9/9 tests pass Fixes koala73#3540
… null out field in destroy(), live lifecycle tests
fuleinist
force-pushed
the
fix/resize-debounce-3540
branch
from
29b0056 to
74e1ae0
Compare
PR Feedback Addressed ✅All review comments from the latest round have been addressed. Changes Made
Files Changed
Verification
Ready for re-review. cc @greptile-apps[bot] |
1 participant
Summary
Window resize fires continuously during drag operations (60+ events/second). Each event ran
ensureCorrectZones()which doesquerySelectorAll+ DOM re-parenting — causing visible jank on machines without dedicated GPUs.Wrapping the handler in a 100ms debounce collapses a typical resize gesture from ~120 fires to 1-2 calls.
Problem
src/app/panel-layout.ts:1612:No debounce. The original line used a bound-method reference in
removeEventListenerbut the add used a bare arrow fn — meaning destroy() never actually removed the old listener (the arrow fn created a new function identity each call).Solution
_onResizeDebouncedfield (reuses existingdebounce()utility)init(): replace bare() => this.ensureCorrectZones()withdebounce(() => this.ensureCorrectZones(), 100)destroy(): cancel pending timer + remove listener via the stored_onResizeDebouncedref (the old removeEventListener was passing a different function identity and had no effect)Testing
npx tsc --noEmit)Files Changed
src/app/panel-layout.ts— debounce wiring (3 locations: field, init, destroy)tests/resize-debounce.test.mjs— new test suiteFixes #3540