fix(panels): debounce window resize handler to prevent ensureCorrectZones spam#4000
fix(panels): debounce window resize handler to prevent ensureCorrectZones spam#4000fuleinist wants to merge 4 commits into
Conversation
…ones spam Window resize fires continuously during drag operations (60+ events/sec). 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(): cancel any in-flight timer before reassigning (prevents ghost calls) - destroy(): cancel pending timer and remove listener via _onResizeDebounced ref - destroy(): null out field after cancel (consistent with other cleanup fields) Testing: - 4 debounce behaviour tests (no-fire-before-delay, cancel, fires-after-delay, timer-reset) - 3 live lifecycle tests (re-init cancels in-flight, re-init without cancel leaves ghost, destroy nulls field) - 5 source-level wiring guards - 9/9 tests pass Fixes koala73#3540
|
@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 debounces the
Confidence Score: 4/5Safe to merge — the debounce wiring is correct and the init()/destroy() lifecycle properly cancels pending timers. The core fix is sound: _onResizeDebounced is cancelled before overwrite on re-init and cancelled + nulled in destroy(), preventing both ghost calls and listener leaks. The two minor gaps — a dead-code fallback that can never match a registered listener, and a lifecycle test that does not fully mirror the two-closure replacement that runs in production — do not affect runtime correctness but leave the codebase slightly misleading for future maintainers. Both changed files are low-risk; src/app/panel-layout.ts is worth a second glance on the removeEventListener fallback arms. Important Files Changed
Sequence DiagramsequenceDiagram
participant W as window
participant PLM as PanelLayoutManager
participant D as _onResizeDebounced
participant EZ as ensureCorrectZones()
Note over PLM: init()
PLM->>W: removeEventListener('resize', _onResizeDebounced)
PLM->>D: cancel() [clears in-flight timer]
PLM->>D: debounce(ensureCorrectZones, 100ms)
PLM->>W: addEventListener('resize', _onResizeDebounced)
Note over W: resize fires (60+/sec)
W->>D: call()
W->>D: call()
W->>D: call()
Note over D: resets 100ms timer each call
Note over D: 100ms idle elapses
D->>EZ: ensureCorrectZones()
Note over PLM: destroy()
PLM->>W: removeEventListener('resize', _onResizeDebounced)
PLM->>D: cancel() [clears pending timer]
PLM->>PLM: "_onResizeDebounced = null"
Reviews (1): Last reviewed commit: "fix(panels): debounce window resize hand..." | Re-trigger Greptile |
| destroyCheckoutOverlay(); | ||
|
|
||
| window.removeEventListener('resize', this.ensureCorrectZones); | ||
| window.removeEventListener('resize', this._onResizeDebounced ?? this.ensureCorrectZones); |
There was a problem hiding this comment.
Dead-code fallback in
removeEventListener calls
Both the destroy() and init() re-entry paths fall back to this.ensureCorrectZones when _onResizeDebounced is null, but the bare this.ensureCorrectZones method reference was never registered as the resize listener — not in the old code (which used () => this.ensureCorrectZones(), an anonymous arrow fn), and not in the new code (which always uses _onResizeDebounced). The fallback can therefore never match a registered listener, making both removeEventListener(…, … ?? this.ensureCorrectZones) calls silent no-ops whenever _onResizeDebounced is null. The same pattern appears at line 1629 in init(). Consider dropping the ?? this.ensureCorrectZones arm to avoid misleading future readers into thinking the bare method reference is ever meaningful here.
There was a problem hiding this comment.
Thanks — addressed in f9d6ed4: f9d6ed4
Dropped the ?? this.ensureCorrectZones fallback from both removeEventListener call sites. The bare method reference was never registered as a listener (old code used an anonymous arrow fn, new code uses _onResizeDebounced), so the fallback was always dead code. Both sites now pass this._onResizeDebounced directly, which is correct whether null (silent no-op) or not.
There was a problem hiding this comment.
Thanks — addressed in f9d6ed4. Dropped ?? this.ensureCorrectZones from both
emoveEventListener calls in destroy() and init(). The bare method reference was never registered as a listener (old code used an anonymous arrow fn, new code uses _onResizeDebounced), so the fallback was indeed dead code. Both spots in panel-layout.ts now pass _onResizeDebounced directly (relying on ?. chaining for the null case). All 12 tests pass. Commit: f9d6ed4
| { | ||
| cancel() { | ||
| clearTimeout(timer); | ||
| timer = null; | ||
| }, | ||
| } | ||
| ); | ||
| return fn; | ||
| } | ||
|
|
||
| it('re-init cancels the in-flight timer before assigning a new debounce', async () => { | ||
| tracker.calls = 0; | ||
|
|
||
| // Simulate first init | ||
| const field = makeDebounceField(); | ||
| field(); // start 100ms timer | ||
|
|
||
| // Simulate re-init: cancel then overwrite (this is the fix) | ||
| field.cancel(); | ||
| field(); // start a new 100ms timer | ||
|
|
||
| // Advance past the second timer's delay | ||
| await new Promise(r => setTimeout(r, 150)); | ||
|
|
||
| // Exactly one call — the old in-flight timer was cancelled |
There was a problem hiding this comment.
First lifecycle test doesn't exercise the actual re-init scenario it claims to prove
The test titled "re-init cancels the in-flight timer before assigning a new debounce" calls field() twice on the same makeDebounceField() instance (with a cancel() in between). This exercises cancel() + reuse of the existing closure, but not the real re-init path where _onResizeDebounced = debounce(...) creates an entirely new closure. To mirror the production fix accurately, the test should create two separate makeDebounceField() instances: fire the first, cancel it, fire the second, then assert only one call fires — exactly mirroring the second test that documents the bug. As written, the first test doesn't distinguish between "cancel worked" and "the re-init safety was wired correctly".
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Thanks — addressed in f9d6ed4: f9d6ed4
Rewrote the test to use two separate makeDebounceField() instances: fire first, firstField.cancel(), then const secondField = makeDebounceField(); secondField();. This now mirrors the production re-init path (_onResizeDebounced.cancel(); _onResizeDebounced = debounce(...)) and actually exercises the new-closure safety. All 12 tests still pass.
There was a problem hiding this comment.
Thanks — addressed in f9d6ed4. Rewrote the 're-init cancels in-flight timer' test to use two separate makeDebounceField() instances (cancel first, then create new closure) — mirrors the production fix where _onResizeDebounced = debounce(...) assigns an entirely new closure. Now actually exercises the two-closure replacement path, not just cancel() reuse. All 12 tests pass. Commit: f9d6ed4
…t in lifecycle test - panel-layout.ts: remove `?? this.ensureCorrectZones` from both removeEventListener calls. The bare method reference was never registered as a listener (old code used an anonymous arrow fn, new code uses _onResizeDebounced), so the fallback was always dead code that misled readers. - resize-debounce.test.mjs: rewrite 're-init cancels in-flight timer' test to use two separate makeDebounceField() instances. The previous version reused a single instance and only proved cancel() works, not that the re-init safety was wired correctly. Now mirrors the production fix (cancel old, assign new closure). All 12 tests pass.
Dropping the ?? this.ensureCorrectZones fallback in f9d6ed4 passed _onResizeDebounced (which can be null) directly to removeEventListener. That broke typecheck (TS2769) because removeEventListener does not accept null. Guard both calls with an if so the listener is only removed when we actually have one registered. destroy() keeps the optional-chained cancel() + null assignment to satisfy the source-level wiring test. Verified: typecheck clean, 12/12 tests pass.
|
Follow-up to f9d6ed4: TypeScript flagged the unguarded |
Summary
Window resize fires continuously during drag operations (60+ events/sec). Each event ran
ensureCorrectZones()which doesquerySelectorAll+ DOM re-parenting, causing visible jank on slower machines.Changes
_onResizeDebouncednullable field (re-uses existing debounce utility)init(): cancel any in-flight timer before reassigning (prevents ghost calls on re-entry)destroy(): cancel pending timer and remove listener via_onResizeDebouncedrefdestroy(): null out field after cancel (consistent with other cleanup fields)Testing
Fixes #3540