fix(ui): debounce sessions.list reload after sessions.changed push events#73779
fix(ui): debounce sessions.list reload after sessions.changed push events#73779fancymatt wants to merge 1 commit into
Conversation
…ents The dashboard subscribes to sessions.changed push events for real-time UI updates. However, every push event also triggered an immediate full sessions.list reload, causing excessive polling under load. With 97 WebSocket connections, a single session change would fan out to 97 simultaneous sessions.list calls. This creates unnecessary GC pressure on the gateway and contributes to memory growth. This change debounces the sessions.list reload to 5 seconds after the last sessions.changed event. The incremental update from applySessionsChangedEvent keeps the UI responsive immediately; the debounced full reload reconciles any edge cases (e.g., sessions that appeared/disappeared outside the push event's scope). Impact: - Reduces sessions.list calls by ~80-95% under normal load - No change to real-time UI responsiveness (incremental updates unchanged) - No server-side changes required - 5s debounce is conservative; could be tuned or made configurable later
Greptile SummaryThis PR debounces the Confidence Score: 3/5Not safe to merge as-is — the existing test asserting synchronous loadSessions invocation will fail with the new debounce. One P1 finding: the existing sessions.changed test in app-gateway.sessions.node.test.ts checks that loadSessions was called immediately, but the debounce defers the call by 5 seconds. The test will fail without fake-timer support and updated assertions. Two P2 findings (missing fixture field, timer not cleared on disconnect) are non-blocking. ui/src/ui/app-gateway.sessions.node.test.ts — needs fake timers and assertion updates for the debounced behavior
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private toolStreamById = new Map<string, ToolStreamEntry>(); | ||
| private toolStreamOrder: string[] = []; | ||
| refreshSessionsAfterChat = new Set<string>(); | ||
| sessionsRefreshTimer: ReturnType<typeof setTimeout> | null = null; |
There was a problem hiding this comment.
sessionsRefreshTimer is not cleared in disconnectedCallback / handleDisconnected. If the component is torn down while the 5-second timer is pending, the timer fires on the dead host. loadSessions does guard with !state.client || !state.connected, so no network call escapes, but the dangling timer could interfere with test isolation and is inconsistent with how the other polling intervals (nodesPollInterval, logsPollInterval) are cleaned up in handleDisconnected.
Add clearTimeout(host.sessionsRefreshTimer ?? undefined); host.sessionsRefreshTimer = null; to handleDisconnected alongside the other interval teardowns.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/app.ts
Line: 565
Comment:
**Timer not cancelled on disconnect**
`sessionsRefreshTimer` is not cleared in `disconnectedCallback` / `handleDisconnected`. If the component is torn down while the 5-second timer is pending, the timer fires on the dead host. `loadSessions` does guard with `!state.client || !state.connected`, so no network call escapes, but the dangling timer could interfere with test isolation and is inconsistent with how the other polling intervals (`nodesPollInterval`, `logsPollInterval`) are cleaned up in `handleDisconnected`.
Add `clearTimeout(host.sessionsRefreshTimer ?? undefined); host.sessionsRefreshTimer = null;` to `handleDisconnected` alongside the other interval teardowns.
How can I resolve this? If you propose a fix, please make it concise.
|
Codex review: needs changes before merge. Summary Reproducibility: yes. Current main has a source-level reproduction: dispatching any Next step before merge Security Review findings
Review detailsBest possible solution: Request changes on this PR: keep incremental session updates immediate, debounce only the full reconciliation reload, clear pending timers during teardown, add fake-timer coverage for delayed and coalesced reloads, initialize fixtures, and add the active-version changelog entry if this remains a user-facing Control UI performance fix. Do we have a high-confidence way to reproduce the issue? Yes. Current main has a source-level reproduction: dispatching any Is this the best way to solve the issue? No, not as submitted. The debounce is a narrow maintainable direction for the reported fanout, but the patch needs lifecycle cleanup, updated tests, and changelog handling before it is the best fix. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 8283c5d6cc3f. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Debounces the sessions.list reload triggered by sessions.changed push events to reduce fan-out load while keeping the UI responsive via incremental updates.
Changes:
- Added a
sessionsRefreshTimerfield to the app/host to track a debounced reload timer. - Replaced immediate
loadSessionscalls onsessions.changedwith a 5s debounce viasetTimeout. - Added inline documentation describing the debounce rationale.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ui/src/ui/app.ts | Adds timer state to the app so the gateway event handler can debounce reloads. |
| ui/src/ui/app-gateway.ts | Implements the debounce logic for sessions.changed events using the new timer state. |
| if (host.sessionsRefreshTimer != null) { | ||
| clearTimeout(host.sessionsRefreshTimer); | ||
| } | ||
| host.sessionsRefreshTimer = setTimeout(() => { | ||
| host.sessionsRefreshTimer = null; | ||
| void loadSessions(host as unknown as SessionsState); | ||
| }, 5000); |
| host.sessionsRefreshTimer = setTimeout(() => { | ||
| host.sessionsRefreshTimer = null; | ||
| void loadSessions(host as unknown as SessionsState); | ||
| }, 5000); |
Same-time PR openclaw#73779 hit the identical 5 check failures (looks like a plugin-test isolation flake at that point in time). Pushing an empty commit to retrigger.
|
Closing as superseded by #80657. The signed replacement includes this PR's core |
Problem
The dashboard subscribes to
sessions.changedpush events for real-time UI updates. However, every push event also triggered an immediate fullsessions.listreload.With 97 WebSocket connections, a single session change fans out to 97 simultaneous
sessions.listcalls. This creates unnecessary GC pressure on the gateway and contributes to memory growth under load (~5,648 calls/day measured).Fix
Debounce the
sessions.listreload to 5 seconds after the lastsessions.changedevent. The incremental update fromapplySessionsChangedEventkeeps the UI responsive immediately; the debounced full reload reconciles any edge cases (e.g., sessions that appeared/disappeared outside the push event scope).Impact
sessions.listcalls under normal load