fix(sidebar): clear task spinner when PTY has no activity for 15s#1407
fix(sidebar): clear task spinner when PTY has no activity for 15s#1407naaa760 wants to merge 3 commits intogeneralaction:mainfrom
Conversation
|
@naaa760 is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR addresses a UX bug where the sidebar task spinner would remain active indefinitely after Codex (or other agents) stopped producing output. It introduces a 15-second PTY inactivity timeout ( Key changes:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| src/renderer/lib/activityStore.ts | Adds inactivityTimers map and armInactivityTimer helper; correctly cleans up the new timers in all existing paths. However, armInactivityTimer is called after an explicit idle signal during the BUSY_HOLD_MS window, resetting a 15 s countdown that should only act as a fallback for silent agents. |
| src/renderer/lib/activityConstants.ts | Adds INACTIVITY_CLEAR_MS = 15_000. Value is reasonable; missing an explanatory comment that the other constants have. |
| src/renderer/lib/activityClassifier.ts | Adds three Codex idle patterns (What would you like, Type your message, Enter your message) — all reasonable heuristics for detecting a waiting prompt. |
Sequence Diagram
sequenceDiagram
participant PTY as PTY Data Stream
participant ACS as applyClassifiedSignal
participant SB as setBusy
participant IT as inactivityTimer (15s)
participant ST as softClearTimer (6s)
PTY->>ACS: chunk arrives
ACS->>ACS: classifyActivity → busy
ACS->>SB: setBusy(true) — clears inactivityTimer
SB-->>IT: clearTimeout
ACS->>IT: armInactivityTimer (reset 15s)
PTY->>ACS: chunk arrives
ACS->>ACS: classifyActivity → neutral
ACS->>ST: armTimer (soft clear 6s)
ACS->>IT: armInactivityTimer (reset 15s)
note over PTY,IT: Agent goes silent — no PTY data for 15s
IT->>SB: setBusy(false) — spinner cleared ✓
note over PTY,IT: --- Alternative: idle pattern detected ---
PTY->>ACS: chunk arrives
ACS->>ACS: classifyActivity → idle
ACS->>SB: setBusy(false) — schedules hold timer (≤2s)
note over ACS,IT: state still true during hold window
ACS->>IT: armInactivityTimer reset (unnecessary — already idle)
SB-->>IT: hold timer fires → clearNow() clears inactivityTimer
Last reviewed commit: 99a72e3
| if (this.states.get(wsId)) { | ||
| this.armInactivityTimer(wsId); | ||
| } |
There was a problem hiding this comment.
Inactivity timer armed after explicit idle signal
When signal === 'idle', setBusy(wsId, false, true) is called but — if the task became busy within the last BUSY_HOLD_MS (2 s) — the state is not immediately set to false; a hold timer is scheduled instead and this.states.get(wsId) still returns true. Execution then falls through to armInactivityTimer, which resets the 15-second countdown even though we have already positively identified that the agent is idle.
This is logically wrong: the inactivity timer is a fallback for when the agent goes silent without sending a recognisable idle signal. Resetting it after we have just detected an idle signal is counterproductive — if the hold timer were somehow superseded (e.g. by a neutral chunk arriving between the two lines and calling armTimer), the spinner could stay active for another 15 seconds despite a confirmed idle detection.
The inactivity timer should only be (re-)armed for busy and neutral signals:
private applyClassifiedSignal(wsId: string, provider: string, chunk: string) {
const sampledChunk = sampleActivityChunk((chunk || '').toString());
const signal = classifyActivity(provider, sampledChunk);
if (signal === 'busy') {
this.setBusy(wsId, true, true);
if (this.states.get(wsId)) {
this.armInactivityTimer(wsId);
}
} else if (signal === 'idle') {
this.setBusy(wsId, false, true);
} else if (this.states.get(wsId)) {
this.armTimer(wsId);
this.armInactivityTimer(wsId);
}
}| export const BUSY_HOLD_MS = 2_000; | ||
| // How long we wait on neutral output before clearing busy if no further busy arrives | ||
| export const CLEAR_BUSY_MS = 6_000; | ||
| export const INACTIVITY_CLEAR_MS = 15_000; |
There was a problem hiding this comment.
Missing explanatory comment for new constant
All neighbouring constants have a short inline or preceding comment explaining their purpose (e.g. // How long we wait on neutral output before clearing busy if no further busy arrives). INACTIVITY_CLEAR_MS is the only one that lacks one, making it harder to understand at a glance why this specific timeout exists.
| export const INACTIVITY_CLEAR_MS = 15_000; | |
| // How long with no PTY data while busy before the spinner is force-cleared | |
| export const INACTIVITY_CLEAR_MS = 15_000; |
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!
summary
Fixes
Fixes #1401
Snapshot
Type of change
[x] Bug fix (non-breaking change which fixes an issue)
Mandatory Tasks
[ ] I have self-reviewed the code
[ ] A decent size PR without self-review might be rejected
Checklist
[ ] I have read the contributing guide
[ ] My code follows the style guidelines of this project (pnpm run format)
[ ] I have commented my code, particularly in hard-to-understand areas
[ ] I have checked if my PR needs changes to the documentation
[ ] I have checked if my changes generate no new warnings (pnpm run lint)
[ ] I have added tests that prove my fix is effective or that my feature works
[ ] I haven't checked if new and existing unit tests pass locally with my changes