Hardening follow-ups for the WebGL longtask auto-fallback that landed in #83 (commit e87b03b). The trip logic and sticky-disable design are correct; these are the rough edges flagged in review.
1. PerformanceObserver not disconnected on teardown
The observer set up in _initWebGL() (src/web/public/app.js, ~line 617+) is only disconnect()ed on the trip path (after threshold fires). If the terminal is torn down or rebuilt before tripping, the observer outlives its addon. The if (!this._webglAddon) return guard prevents action, but the observer object lingers — a minor leak per re-init. Matters for the 24+ hour sessions this repo cares about.
Fix: add disconnect() calls in the terminal disposal path and onContextLoss handler. Mirror in memory-leak-prevention.test.ts.
2. Hard-coded thresholds
200ms / 3 occurrences / 30s rolling window are inlined. Tunable knobs help triage on user reports without a redeploy.
Fix: hoist to named constants in src/config/ (or terminal-limits.ts) so they're discoverable and editable from one place.
3. No tests
No unit tests for the trip logic, no integration test for renderer-swap correctness (scrollback survival on swap, sticky-disable across reloads, ?webgl=force reset). The PR's test plan was all-manual unchecked boxes.
Fix: at minimum a unit test for the rolling-window logic (3 longtasks within 30s ⇒ trip, 3 spread over 60s ⇒ no trip), and an entry in memory-leak-prevention.test.ts asserting the observer is disconnected on terminal teardown.
Hardening follow-ups for the WebGL longtask auto-fallback that landed in #83 (commit
e87b03b). The trip logic and sticky-disable design are correct; these are the rough edges flagged in review.1.
PerformanceObservernot disconnected on teardownThe observer set up in
_initWebGL()(src/web/public/app.js, ~line 617+) is onlydisconnect()ed on the trip path (after threshold fires). If the terminal is torn down or rebuilt before tripping, the observer outlives its addon. Theif (!this._webglAddon) returnguard prevents action, but the observer object lingers — a minor leak per re-init. Matters for the 24+ hour sessions this repo cares about.Fix: add
disconnect()calls in the terminal disposal path andonContextLosshandler. Mirror inmemory-leak-prevention.test.ts.2. Hard-coded thresholds
200ms / 3 occurrences / 30s rolling window are inlined. Tunable knobs help triage on user reports without a redeploy.
Fix: hoist to named constants in
src/config/(orterminal-limits.ts) so they're discoverable and editable from one place.3. No tests
No unit tests for the trip logic, no integration test for renderer-swap correctness (scrollback survival on swap, sticky-disable across reloads,
?webgl=forcereset). The PR's test plan was all-manual unchecked boxes.Fix: at minimum a unit test for the rolling-window logic (3 longtasks within 30s ⇒ trip, 3 spread over 60s ⇒ no trip), and an entry in
memory-leak-prevention.test.tsasserting the observer is disconnected on terminal teardown.