mirror of
https://github.com/stablyai/orca.git
synced 2026-10-26 16:02:42 +00:00
fix(dashboard): unstick bold codex inline-agent rows after retention
Auto-ack walked only `agentStatusByPaneKey`, but the inline agents list renders the union of live + retained. When codex's TUI title flips back to a shell label within ms of `Stop`, `onAgentExited → removeAgentStatus` tears down the live `done` row before auto-ack can see it. The retention sync snapshots it into `retainedAgentsByPaneKey` carrying the post-Stop `stateStartedAt`, and the row stays bold forever. Extend the auto-ack scan to also iterate retained entries so any row the user is actually looking at gets acked, regardless of which map it lives in. Extract the scan into a pure `computeAutoAckTargets` helper so the race is testable without a DOM. The asymmetry is independent of the title heuristic — OSC 133 (PR replacing the heuristic) does not obviate this fix; see docs/codex-agent-row-bold-stuck.md for the full analysis. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -0,0 +1,302 @@
|
||||
# Codex inline-agent row stays bold after returning from another workspace
|
||||
|
||||
## Context — how this relates to OSC 133 shell integration
|
||||
|
||||
This bug surfaces a structural asymmetry in the dashboard's ack flow. The
|
||||
*trigger* is the title-revert heuristic in `pty-connection.ts:onAgentExited`,
|
||||
which is independently scheduled for replacement.
|
||||
|
||||
**Title heuristic (current, brittle):**
|
||||
`useAppStore.getState().removeAgentStatus(cacheKey)` fires when the OSC
|
||||
terminal title reverts to a shell label (`bash`/`zsh`/`pwsh`). Added in
|
||||
PR #1019 (2026-04-24) as a stopgap because agent CLIs don't emit a hook on
|
||||
Ctrl+C / clean `exit`. It produces both:
|
||||
|
||||
- **false positives** — codex's TUI redraws to a shell title mid-turn, firing
|
||||
a spurious `removeAgentStatus` while the agent is still running (the race
|
||||
this doc is about).
|
||||
- **false negatives** — Ctrl+C'd Codex/Gemini/OpenCode never revert, so the
|
||||
row lingers 30 minutes until `AGENT_STATUS_STALE_AFTER_MS` decays it.
|
||||
|
||||
**OSC 133 shell integration (in flight, branch
|
||||
`brennanb2025/foreground-process-agent-exit`):** Replaces the title heuristic
|
||||
with `OSC 133;D` markers emitted by shell prompt hooks (zsh `precmd`,
|
||||
PowerShell `PSConsoleHostReadLine`). Cross-platform by construction (works on
|
||||
Windows ConPTY where `tcgetpgrp`-based polling does not), deletes both
|
||||
`agent-foreground-poller.ts` and the `onAgentExited → removeAgentStatus` line
|
||||
in the same PR. Design doc:
|
||||
`docs/design/shell-integration-osc-133.md` (in the sibling worktree
|
||||
`/Users/thebr/orca/workspaces/orca/foreground-process-agent-exit`).
|
||||
|
||||
**Why this fix still matters once OSC 133 lands:** the ack-scan asymmetry is
|
||||
independent of the title heuristic. The inline agents list renders
|
||||
`live ∪ retained` (see `useWorktreeAgentRows`), but the auto-ack scan only
|
||||
walked `live`. Any retention path — explicit user dismissal today, OSC 133's
|
||||
`;D` handler tomorrow, or any future "drop the live row on real process exit"
|
||||
mechanism — that migrates a `done` row to retained while the user is on the
|
||||
tab would re-trigger the same bug. The retained walk closes that gap once,
|
||||
regardless of how `removeAgentStatus` ends up being called.
|
||||
|
||||
**Scope of this PR:** fix the user-visible regression now. Do not block on
|
||||
OSC 133.
|
||||
|
||||
## Summary
|
||||
|
||||
When a codex turn finishes while the user is on a different workspace, the
|
||||
inline-agent row in the original workspace bolds (correct), but stays bold
|
||||
forever after the user returns — even after clicking the codex terminal. The
|
||||
row only clears if the user explicitly clicks the agent row in the sidebar.
|
||||
|
||||
The root cause is a three-step trap:
|
||||
|
||||
1. Codex's `Stop` hook delivers `state=done`, `setAgentStatus` updates with a
|
||||
fresh `stateStartedAt`. Row goes bold.
|
||||
2. Codex's TUI title reverts to a non-agent value (the title agent tracker
|
||||
sees this as "agent exited"), firing `onAgentExited` →
|
||||
`removeAgentStatus(cacheKey)`. Per `agent-status.ts:259-263` the ack
|
||||
timestamp is wiped in lockstep with the live entry.
|
||||
3. `useRetainedAgentsSync` snapshots the `done` row into
|
||||
`retainedAgentsByPaneKey`. The inline agents list keeps showing it
|
||||
(it reads retained entries) — but `useAutoAckViewedAgent` only walks
|
||||
`agentStatusByPaneKey` (live), so the retained row never gets auto-acked.
|
||||
`WorktreeCardAgents.tsx:74` evaluates
|
||||
`(ackAt = 0) < (entry.stateStartedAt = doneTimestamp)` → bold forever.
|
||||
|
||||
For Claude Code the same retention path runs but the user's experience differs
|
||||
because Claude's "bold while finished" window typically closes via auto-ack
|
||||
*before* `onAgentExited` deletes the ack — i.e. there's a race between (1) and
|
||||
(2), and codex tends to lose it because its title flips to shell quickly after
|
||||
Stop. The first session in a session sometimes also wins the race; the second
|
||||
reliably loses it. The bug is timing-dependent but trivially reproducible by
|
||||
running two sequential turns.
|
||||
|
||||
## Repro (Cmd+D split, codex)
|
||||
|
||||
1. New workspace.
|
||||
2. Cmd+D to open a second pane (paneKey suffix `:2`).
|
||||
3. Run `codex` in the new pane.
|
||||
4. Send "sleep 10 and then say hi".
|
||||
5. Switch to a different workspace before codex finishes.
|
||||
6. Codex finishes. Inline-agent row in the original workspace bolds.
|
||||
7. Switch back. Click anywhere in the codex pane.
|
||||
8. Row stays bold. Hover popup says "Done". Only clicking the row in the
|
||||
sidebar clears it.
|
||||
|
||||
The auto-opened initial pane (`:1`) exhibits the same bug under the same
|
||||
conditions; the cmd+d split is just the path that currently produces the
|
||||
fastest reliable repro.
|
||||
|
||||
## Evidence — log capture from /electron repro
|
||||
|
||||
Logs were added at every relevant call site (see "Logging instrumentation"
|
||||
below). The following is the trimmed event sequence for the stuck-bold pane
|
||||
(`dfa8fa3b-5e39-41d5-9e19-0f244429c3ec:2`, workspace "Stingray"):
|
||||
|
||||
### Main process — codex hooks fired exactly three times
|
||||
```
|
||||
[bold-debug-main] codex hook received {
|
||||
paneKey: 'dfa8fa3b-5e39-41d5-9e19-0f244429c3ec:2',
|
||||
tabId: 'dfa8fa3b-5e39-41d5-9e19-0f244429c3ec',
|
||||
worktreeId: '...demo-project/Stingray',
|
||||
state: 'working'
|
||||
}
|
||||
[bold-debug-main] codex hook received { ...same paneKey..., state: 'working' }
|
||||
[bold-debug-main] codex hook received { ...same paneKey..., state: 'done' }
|
||||
```
|
||||
No post-Stop pings. Hook surface is clean. Disproves earlier hypothesis #1
|
||||
("codex emits late state events after Stop").
|
||||
|
||||
### Renderer — the trap closes in four log lines
|
||||
```
|
||||
[bold-debug] ipc agentStatus:set received {
|
||||
paneKey: dfa8fa3b-5e39-41d5-9e19-0f244429c3ec:2, state: done, knownPaneKeys: Array(2)
|
||||
}
|
||||
[bold-debug] setAgentStatus {
|
||||
paneKey: dfa8fa3b-5e39-41d5-9e19-0f244429c3ec:2,
|
||||
agentType: codex, prevState: working, nextState: done,
|
||||
prevStartedAt: 1778042951947
|
||||
}
|
||||
[bold-debug] removeAgentStatus {
|
||||
paneKey: dfa8fa3b-5e39-41d5-9e19-0f244429c3ec:2,
|
||||
prevAgentType: codex, prevState: done,
|
||||
prevAckAt: 1778042951947
|
||||
}
|
||||
[bold-debug] useRetainedAgentsSync { toRetain: Array(1), consumedSuppressedPaneKeys: Array(0) }
|
||||
[bold-debug] autoAck: scan {
|
||||
activeTabId: dfa8fa3b-5e39-41d5-9e19-0f244429c3ec,
|
||||
prefix: dfa8fa3b-5e39-41d5-9e19-0f244429c3ec:,
|
||||
liveCount: 1, retainedCount: 1, toAck: Array(0)
|
||||
}
|
||||
```
|
||||
|
||||
Line-by-line:
|
||||
|
||||
1. `setAgentStatus working → done` advances `stateStartedAt` to a NEW
|
||||
timestamp (state changed, so the carry-forward in `agent-status.ts:166-167`
|
||||
doesn't apply). Row is now bold (`ackAt < newStateStartedAt`).
|
||||
2. `removeAgentStatus` fires immediately after — `prevAckAt: 1778042951947`
|
||||
was the ack from a prior auto-ack on the `working` state. Per
|
||||
`agent-status.ts:259-263`, this ack is deleted in lockstep with the live
|
||||
entry.
|
||||
3. `useRetainedAgentsSync` picks up the disappearance:
|
||||
`toRetain: Array(1)`. The row now lives in `retainedAgentsByPaneKey`,
|
||||
`state: 'done'`, frozen `stateStartedAt`.
|
||||
4. `autoAck: scan` runs (the user is back on the tab). It sees
|
||||
`retainedCount: 1` but `toAck: Array(0)` — the retained entry is invisible
|
||||
to the scan because `useAutoAckViewedAgent.ts:114` iterates only
|
||||
`s.agentStatusByPaneKey` (live), not the retained map.
|
||||
|
||||
The `liveCount: 1` in that final scan is leftover from another pane in the
|
||||
same tab; the stuck pane is in retainedCount. After this point the only
|
||||
log activity is `autoAck: skip — refs unchanged` and `WorktreeCardAgents
|
||||
memo` re-renders with `unvisited: true` indefinitely.
|
||||
|
||||
### First session in the same dev run (Porpoise) cleared correctly
|
||||
The ordering of (`setAgentStatus done`) and (`removeAgentStatus`) was
|
||||
reversed for the first session. Auto-ack scanned the live `done` entry
|
||||
in time, acked it, and the retained snapshot inherited a non-zero ack
|
||||
implicitly via the rendering path (the retained row's
|
||||
`stateStartedAt` was the carry-forward of `working`'s start, not a
|
||||
post-bold timestamp). This is a pure timing race — not a structural
|
||||
difference.
|
||||
|
||||
## Where each piece comes from in code
|
||||
|
||||
- **The bold check** — `WorktreeCardAgents.tsx:70-77`
|
||||
```ts
|
||||
out[a.paneKey] = ackAt < a.entry.stateStartedAt
|
||||
```
|
||||
This runs against the merged live+retained list from `useWorktreeAgentRows`.
|
||||
- **Live-only walk in auto-ack** — `useAutoAckViewedAgent.ts:114`
|
||||
```ts
|
||||
for (const [paneKey, entry] of Object.entries(s.agentStatusByPaneKey)) { ... }
|
||||
```
|
||||
Retained entries are not considered.
|
||||
- **Ack cleanup on remove** — `agent-status.ts:259-263`
|
||||
```ts
|
||||
let nextAck = s.acknowledgedAgentsByPaneKey
|
||||
if (paneKey in nextAck) {
|
||||
nextAck = { ...nextAck }
|
||||
delete nextAck[paneKey]
|
||||
}
|
||||
```
|
||||
- **Retain on disappear** — `useRetainedAgents.ts:101-132`
|
||||
Snapshots `state: 'done'` rows into `retainedAgentsByPaneKey` when they
|
||||
vanish from `agentStatusByPaneKey`.
|
||||
- **Title-revert removes the live entry** — `pty-connection.ts:291-307`
|
||||
`onAgentExited` calls `removeAgentStatus(cacheKey)` when the OSC title
|
||||
reverts to a plain shell label. Codex's TUI exits to shell quickly after
|
||||
Stop, so this fires almost immediately.
|
||||
- **Click-the-terminal does NOT ack** — `TerminalPane.tsx:759-761`
|
||||
Only `clearTerminalTabUnread` and `clearWorktreeUnread`. Acks happen
|
||||
exclusively through (a) explicit click on the agent row or (b)
|
||||
`useAutoAckViewedAgent`.
|
||||
|
||||
## Fix
|
||||
|
||||
Two minimally-invasive options:
|
||||
|
||||
### Option A — make auto-ack walk retained entries too (preferred)
|
||||
|
||||
In `useAutoAckViewedAgent.ts`:
|
||||
|
||||
1. Add `s.retainedAgentsByPaneKey` to the ref-equality guard so retain
|
||||
transitions re-trigger the scan.
|
||||
2. After the live walk, also iterate `Object.entries(s.retainedAgentsByPaneKey)`
|
||||
and push to `toAck` when `paneKey.startsWith(prefix)` and
|
||||
`ackAt < entry.entry.stateStartedAt`. No dedupe needed: when a paneKey
|
||||
appears in BOTH maps (paneKey reuse mid-frame), `acknowledgeAgents`'s
|
||||
per-key `prev < now` short-circuit (`ui.ts:328-337`) collapses the second
|
||||
push into a no-op.
|
||||
|
||||
Why preferred:
|
||||
- Keeps the existing semantics: "while the user is looking at the tab, all
|
||||
visible agent rows on that tab are acked, regardless of provenance."
|
||||
- The retain pool has a bounded lifetime (dismissed on user action / worktree
|
||||
prune), so the scan stays cheap.
|
||||
- Single-file change. No store-shape change.
|
||||
|
||||
### Option B — preserve ack across `removeAgentStatus`
|
||||
|
||||
Stop wiping `acknowledgedAgentsByPaneKey[paneKey]` in
|
||||
`agent-status.ts:259-263`. Two reasons this is rejected:
|
||||
|
||||
1. **It does not fix the bug.** The `working → done` transition mints a
|
||||
fresh `stateStartedAt` (the carry-forward at `agent-status.ts:168-169`
|
||||
only applies within the same state). Even with the prior ack preserved,
|
||||
`ackAt(=working.stateStartedAt) < newDoneStateStartedAt` keeps the row
|
||||
bold. Preservation is structurally insufficient.
|
||||
2. **It leaks acks across paneKey reuse.** A stale ack would silently
|
||||
suppress unvisited signals on a future re-spawn of the same paneKey.
|
||||
The original ack-cleanup was intentional for that reason; resurrecting
|
||||
it would require a second cleanup hook on retained-row dismissal.
|
||||
|
||||
## Logging instrumentation in this branch (to remove on cleanup)
|
||||
|
||||
All gated on `console.debug('[bold-debug] ...')` — useful for future
|
||||
re-investigation, but should be stripped before merge.
|
||||
|
||||
| File | Site | What it logs |
|
||||
|---|---|---|
|
||||
| `useAutoAckViewedAgent.ts` | `maybeAck` | every guard skip + scan with full inspected rows (paneKey, agentType, state, ackAt, stateStartedAt, prefixMatch, unvisited), `liveCount`, `retainedCount`, `toAck` |
|
||||
| `agent-status.ts` | `setAgentStatus` | paneKey, agentType, prevState→nextState, prevStartedAt vs new stateStartedAt, carriedForward flag, updatedAt |
|
||||
| `agent-status.ts` | `removeAgentStatus` | paneKey, prevAgentType, prevState, prevAckAt |
|
||||
| `WorktreeCardAgents.tsx` | `unvisitedByPaneKey` memo | every render: worktreeId + per-row paneKey/agentType/state/ackAt/stateStartedAt/unvisited/isRetained |
|
||||
| `useRetainedAgents.ts` | `useRetainedAgentsSync` | toRetain[] + consumedSuppressedPaneKeys when non-empty |
|
||||
| `ui.ts` | `acknowledgeAgents` | called paneKeys + their prior ack values |
|
||||
| `useIpcEvents.ts` | `agentStatus:set` IPC | paneKey, tabId, worktreeId, state, knownPaneKeys (codex only) |
|
||||
| `main/index.ts` | hook listener | `[bold-debug-main]` paneKey, tabId, worktreeId, state (codex only) |
|
||||
|
||||
## Cleanup
|
||||
|
||||
After fix lands:
|
||||
1. Remove all `[bold-debug]` and `[bold-debug-main]` logs. Specifically:
|
||||
- `useAutoAckViewedAgent.ts` — six skip-path `console.debug` calls
|
||||
(refs-unchanged, feature-gate, activeView, visibilityState, !hasFocus,
|
||||
no activeTabId), the success-path scan log, AND the `inspected[]`
|
||||
accumulator built up purely to feed that log.
|
||||
- `agent-status.ts` — `setAgentStatus` debug call AND the `__prevState` /
|
||||
`__prevStartedAt` capture that feeds it; `removeAgentStatus` has TWO
|
||||
debug calls (the `skip — not in live map` early-return at line 261 and
|
||||
the success-path log at line 264) AND the prevAgentType/prevState
|
||||
reads that feed them.
|
||||
- `WorktreeCardAgents.tsx` — the `debugRows[]` accumulator + the
|
||||
`console.debug` in the `unvisitedByPaneKey` memo, AND the
|
||||
`liveAgentStatusByPaneKey` selector that exists only to compute the
|
||||
`isRetained` field on debug rows.
|
||||
- `useRetainedAgents.ts` — the `toRetain.map(...)` log shape construction
|
||||
plus the conditional `console.debug` block.
|
||||
- `ui.ts` — the `acknowledgeAgents` debug call AND the `priorAcks` map
|
||||
it constructs.
|
||||
- `useIpcEvents.ts`, `main/index.ts` — the codex-only `agentStatus:set`
|
||||
and hook-listener `[bold-debug-main]` log calls.
|
||||
|
||||
2. Add a regression test at
|
||||
`src/renderer/src/hooks/useAutoAckViewedAgent.test.ts` (new file — the
|
||||
existing `agent-status-ack-cleanup.test.ts` is store-slice-scoped and a
|
||||
poor fit for a hook test that needs `document.visibilityState` /
|
||||
`document.hasFocus` / focus-event mocks). Test sequence: stub
|
||||
`document.visibilityState='visible'` and `document.hasFocus()=true`,
|
||||
render the hook against a real test store with the experimental flag
|
||||
on, set an `activeTabId` matching the paneKey prefix, then simulate the
|
||||
exact race: `setAgentStatus(paneKey, working)` → manual ack via
|
||||
`acknowledgeAgents([paneKey])` → `setAgentStatus(paneKey, done)` →
|
||||
`removeAgentStatus(paneKey)` → `retainAgents([snapshot])` → trigger
|
||||
one tracked-slice change so the scan reruns → assert
|
||||
`acknowledgedAgentsByPaneKey[paneKey] >= retained.entry.stateStartedAt`.
|
||||
|
||||
## Files involved
|
||||
|
||||
- `src/renderer/src/hooks/useAutoAckViewedAgent.ts` — auto-ack scan (THE FIX)
|
||||
- `src/renderer/src/components/sidebar/WorktreeCardAgents.tsx` — bold check
|
||||
- `src/renderer/src/components/dashboard/DashboardAgentRow.tsx` —
|
||||
`isUnvisited → font-semibold`
|
||||
- `src/renderer/src/store/slices/agent-status.ts` — `setAgentStatus`,
|
||||
`removeAgentStatus`, ack cleanup
|
||||
- `src/renderer/src/store/slices/ui.ts` — `acknowledgeAgents`
|
||||
- `src/renderer/src/components/sidebar/useWorktreeAgentRows.ts` — row aggregation
|
||||
- `src/renderer/src/components/dashboard/useRetainedAgents.ts` — retain sync
|
||||
- `src/renderer/src/components/terminal-pane/pty-connection.ts` —
|
||||
`onAgentExited → removeAgentStatus`
|
||||
- `src/main/agent-hooks/server.ts` — `normalizeCodexEvent` (state mapping)
|
||||
- `src/main/codex/hook-service.ts` — codex hook install (events subscribed)
|
||||
@@ -0,0 +1,175 @@
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import { computeAutoAckTargets } from './useAutoAckViewedAgent'
|
||||
import { createTestStore, makeTab } from '../store/slices/store-test-helpers'
|
||||
import type { RetainedAgentEntry } from '../store/slices/agent-status'
|
||||
|
||||
// Why: regression coverage for the codex inline-agent row that stayed bold
|
||||
// after returning from another workspace (docs/codex-agent-row-bold-stuck.md).
|
||||
// The race: codex emits `Stop` (state=done), then its TUI title reverts to a
|
||||
// shell label; pty-connection.ts:onAgentExited fires removeAgentStatus before
|
||||
// the live `done` row could be auto-acked. The retention sync snapshots the
|
||||
// `done` row into retainedAgentsByPaneKey carrying a fresh stateStartedAt.
|
||||
// Pre-fix: useAutoAckViewedAgent only walked the live map, so the retained
|
||||
// row's stateStartedAt > ackAt forever. Fix: walk both maps.
|
||||
//
|
||||
// We test the pure helper (computeAutoAckTargets) rather than the hook so the
|
||||
// vitest 'node' environment doesn't need to mock document.visibilityState,
|
||||
// document.hasFocus, or the focus/visibilitychange event surface. The hook's
|
||||
// gate logic is unchanged by this fix — only the scan body was extended.
|
||||
|
||||
describe('computeAutoAckTargets — codex retain race regression', () => {
|
||||
afterEach(() => {
|
||||
vi.useRealTimers()
|
||||
})
|
||||
|
||||
it('acks a paneKey whose live row was retained mid-frame', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.setSystemTime(new Date('2026-05-05T12:00:00.000Z'))
|
||||
const store = createTestStore()
|
||||
const paneKey = 'tab-codex:2'
|
||||
const activeTabId = 'tab-codex'
|
||||
|
||||
// 1. Codex starts working, user acks it (e.g. by clicking the row).
|
||||
store.getState().setAgentStatus(paneKey, {
|
||||
state: 'working',
|
||||
prompt: 'sleep 10 then say hi',
|
||||
agentType: 'codex'
|
||||
})
|
||||
store.getState().acknowledgeAgents([paneKey])
|
||||
const workingAck = store.getState().acknowledgedAgentsByPaneKey[paneKey]
|
||||
expect(workingAck).toBeGreaterThan(0)
|
||||
|
||||
// 2. Time advances; codex Stop fires → state=done with a fresh
|
||||
// stateStartedAt (carry-forward only applies within the same state).
|
||||
vi.setSystemTime(new Date('2026-05-05T12:00:10.000Z'))
|
||||
store.getState().setAgentStatus(paneKey, {
|
||||
state: 'done',
|
||||
prompt: 'sleep 10 then say hi',
|
||||
agentType: 'codex'
|
||||
})
|
||||
const liveDone = store.getState().agentStatusByPaneKey[paneKey]
|
||||
expect(liveDone.stateStartedAt).toBeGreaterThan(workingAck)
|
||||
|
||||
// 3. The codex TUI title reverts to a plain shell label, so
|
||||
// onAgentExited → removeAgentStatus tears down the live entry AND
|
||||
// wipes the prior ack (per agent-status.ts cleanup contract).
|
||||
const retentionSnapshot: RetainedAgentEntry = {
|
||||
entry: liveDone,
|
||||
worktreeId: 'wt-1',
|
||||
tab: makeTab({ id: activeTabId, worktreeId: 'wt-1' }),
|
||||
agentType: 'codex',
|
||||
startedAt: liveDone.stateStartedAt
|
||||
}
|
||||
store.getState().removeAgentStatus(paneKey)
|
||||
expect(store.getState().agentStatusByPaneKey[paneKey]).toBeUndefined()
|
||||
expect(store.getState().acknowledgedAgentsByPaneKey[paneKey]).toBeUndefined()
|
||||
|
||||
// 4. The retention sync picks up the live→gone transition and stashes a
|
||||
// snapshot with the post-Stop stateStartedAt — same as
|
||||
// collectRetainedAgentsOnDisappear in useRetainedAgents.ts.
|
||||
store.getState().retainAgents([retentionSnapshot])
|
||||
expect(store.getState().retainedAgentsByPaneKey[paneKey]).toBeDefined()
|
||||
|
||||
// 5. The user is back on the codex tab. computeAutoAckTargets must see
|
||||
// the retained row and surface it for ack — pre-fix this returned [].
|
||||
const targets = computeAutoAckTargets(store.getState(), activeTabId)
|
||||
expect(targets).toEqual([paneKey])
|
||||
})
|
||||
|
||||
it('returns no targets once the retained row has been acked', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.setSystemTime(new Date('2026-05-05T12:00:00.000Z'))
|
||||
const store = createTestStore()
|
||||
const paneKey = 'tab-codex:2'
|
||||
const activeTabId = 'tab-codex'
|
||||
|
||||
store.getState().setAgentStatus(paneKey, {
|
||||
state: 'done',
|
||||
prompt: 'p',
|
||||
agentType: 'codex'
|
||||
})
|
||||
const doneEntry = store.getState().agentStatusByPaneKey[paneKey]
|
||||
store.getState().retainAgents([
|
||||
{
|
||||
entry: doneEntry,
|
||||
worktreeId: 'wt-1',
|
||||
tab: makeTab({ id: activeTabId, worktreeId: 'wt-1' }),
|
||||
agentType: 'codex',
|
||||
startedAt: doneEntry.stateStartedAt
|
||||
}
|
||||
])
|
||||
store.getState().removeAgentStatus(paneKey)
|
||||
|
||||
// First scan: the retained row is unvisited.
|
||||
expect(computeAutoAckTargets(store.getState(), activeTabId)).toEqual([paneKey])
|
||||
|
||||
// Simulate the ack effect.
|
||||
vi.setSystemTime(new Date('2026-05-05T12:00:01.000Z'))
|
||||
store.getState().acknowledgeAgents([paneKey])
|
||||
|
||||
// Second scan: idempotent — nothing to ack.
|
||||
expect(computeAutoAckTargets(store.getState(), activeTabId)).toEqual([])
|
||||
})
|
||||
|
||||
it('skips retained rows whose paneKey is on a different tab', () => {
|
||||
const store = createTestStore()
|
||||
const paneKey = 'tab-other:0'
|
||||
store.getState().setAgentStatus(paneKey, {
|
||||
state: 'done',
|
||||
prompt: 'p',
|
||||
agentType: 'codex'
|
||||
})
|
||||
const entry = store.getState().agentStatusByPaneKey[paneKey]
|
||||
store.getState().retainAgents([
|
||||
{
|
||||
entry,
|
||||
worktreeId: 'wt-1',
|
||||
tab: makeTab({ id: 'tab-other', worktreeId: 'wt-1' }),
|
||||
agentType: 'codex',
|
||||
startedAt: entry.stateStartedAt
|
||||
}
|
||||
])
|
||||
store.getState().removeAgentStatus(paneKey)
|
||||
|
||||
// Active tab differs — the retained row must NOT be acked while the user
|
||||
// is looking at a different tab; the bold-until-viewed signal must
|
||||
// survive the tab switch.
|
||||
expect(computeAutoAckTargets(store.getState(), 'tab-codex')).toEqual([])
|
||||
})
|
||||
|
||||
it('acks a paneKey present in BOTH live and retained without throwing', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.setSystemTime(new Date('2026-05-05T12:00:00.000Z'))
|
||||
const store = createTestStore()
|
||||
const paneKey = 'tab-codex:2'
|
||||
const activeTabId = 'tab-codex'
|
||||
|
||||
// Construct a (rare) state where retainedAgentsByPaneKey and
|
||||
// agentStatusByPaneKey both contain the same paneKey — e.g. the
|
||||
// retention sync hadn't observed the new live entry yet. The merged
|
||||
// scan should report the paneKey at most twice (acknowledgeAgents
|
||||
// collapses duplicates), never throw.
|
||||
store.getState().setAgentStatus(paneKey, {
|
||||
state: 'done',
|
||||
prompt: 'p1',
|
||||
agentType: 'codex'
|
||||
})
|
||||
const liveDone = store.getState().agentStatusByPaneKey[paneKey]
|
||||
store.getState().retainAgents([
|
||||
{
|
||||
entry: liveDone,
|
||||
worktreeId: 'wt-1',
|
||||
tab: makeTab({ id: activeTabId, worktreeId: 'wt-1' }),
|
||||
agentType: 'codex',
|
||||
startedAt: liveDone.stateStartedAt
|
||||
}
|
||||
])
|
||||
|
||||
const targets = computeAutoAckTargets(store.getState(), activeTabId)
|
||||
// Two pushes, same paneKey — duplicates are intentional and harmless;
|
||||
// acknowledgeAgents short-circuits per key.
|
||||
expect(targets.length).toBeLessThanOrEqual(2)
|
||||
expect(targets.every((k) => k === paneKey)).toBe(true)
|
||||
expect(targets.includes(paneKey)).toBe(true)
|
||||
})
|
||||
})
|
||||
@@ -1,5 +1,52 @@
|
||||
import { useEffect } from 'react'
|
||||
import { useAppStore } from '@/store'
|
||||
import type { AgentStatusEntry } from '../../../shared/agent-status-types'
|
||||
import type { RetainedAgentEntry } from '@/store/slices/agent-status'
|
||||
|
||||
/**
|
||||
* Pure helper used by the hook below — exported so the regression test for
|
||||
* the codex-row-stays-bold race (docs/codex-agent-row-bold-stuck.md) can
|
||||
* exercise the decision against a real test store without needing a DOM.
|
||||
*
|
||||
* Returns the list of paneKeys that should be acked given the active tab.
|
||||
* Walks BOTH the live agent map AND the retained snapshot map: the inline
|
||||
* agents list renders the union, so the ack scan must too. A paneKey may
|
||||
* appear in both maps simultaneously (paneKey reuse mid-frame); duplicate
|
||||
* pushes are harmless because acknowledgeAgents short-circuits per key.
|
||||
*/
|
||||
export function computeAutoAckTargets(
|
||||
state: {
|
||||
agentStatusByPaneKey: Record<string, AgentStatusEntry>
|
||||
retainedAgentsByPaneKey: Record<string, RetainedAgentEntry>
|
||||
acknowledgedAgentsByPaneKey: Record<string, number>
|
||||
},
|
||||
activeTabId: string
|
||||
): string[] {
|
||||
const prefix = `${activeTabId}:`
|
||||
const targets: string[] = []
|
||||
for (const [paneKey, entry] of Object.entries(state.agentStatusByPaneKey)) {
|
||||
if (!paneKey.startsWith(prefix)) {
|
||||
continue
|
||||
}
|
||||
const ackAt = state.acknowledgedAgentsByPaneKey[paneKey] ?? 0
|
||||
// Why: use stateStartedAt (not updatedAt) so tool/prompt pings within the
|
||||
// same state don't re-trigger ack work — keeping the comparison aligned
|
||||
// with WorktreeCardAgents' is-unvisited rule.
|
||||
if (ackAt < entry.stateStartedAt) {
|
||||
targets.push(paneKey)
|
||||
}
|
||||
}
|
||||
for (const [paneKey, retained] of Object.entries(state.retainedAgentsByPaneKey)) {
|
||||
if (!paneKey.startsWith(prefix)) {
|
||||
continue
|
||||
}
|
||||
const ackAt = state.acknowledgedAgentsByPaneKey[paneKey] ?? 0
|
||||
if (ackAt < retained.entry.stateStartedAt) {
|
||||
targets.push(paneKey)
|
||||
}
|
||||
}
|
||||
return targets
|
||||
}
|
||||
|
||||
// Why: an agent row counts as "already seen" when the user is actually looking
|
||||
// at the tab it lives on. Without this effect, ack only fires via an explicit
|
||||
@@ -10,16 +57,26 @@ import { useAppStore } from '@/store'
|
||||
// The effect subscribes directly to the store (not via React selectors) so it
|
||||
// sees every state change with no re-render amplification up the component
|
||||
// tree. A reference-equality guard inside the callback bails out immediately
|
||||
// when none of the five slices we care about (activeView, activeTabId,
|
||||
// agentStatusByPaneKey, acknowledgedAgentsByPaneKey, settings) have changed —
|
||||
// so the Object.entries walk only runs for updates that could legitimately
|
||||
// affect the ack decision.
|
||||
// when none of the six slices we care about (activeView, activeTabId,
|
||||
// agentStatusByPaneKey, retainedAgentsByPaneKey, acknowledgedAgentsByPaneKey,
|
||||
// settings) have changed — so the Object.entries walk only runs for updates
|
||||
// that could legitimately affect the ack decision.
|
||||
//
|
||||
// It acks whenever:
|
||||
// - activeView is 'terminal' (the user isn't on Settings/Tasks), AND
|
||||
// - activeTabId identifies a live tab, AND
|
||||
// - at least one agentStatusByPaneKey entry has paneKey prefixed by
|
||||
// `${activeTabId}:` AND its ackAt < stateStartedAt.
|
||||
// - at least one agentStatusByPaneKey entry OR retainedAgentsByPaneKey
|
||||
// entry has paneKey prefixed by `${activeTabId}:` AND its
|
||||
// ackAt < stateStartedAt.
|
||||
//
|
||||
// Why both maps: the inline-agents list renders the union of live + retained
|
||||
// rows (see useWorktreeAgentRows), so the ack scan must too. Without the
|
||||
// retained walk, a `done` row whose live entry was torn down by the
|
||||
// title-revert path (see pty-connection.ts:onAgentExited) migrates to the
|
||||
// retained map carrying a fresh `done.stateStartedAt` and never gets
|
||||
// auto-acked — leaving the inline row bold forever even while the user
|
||||
// stares at the terminal. Codex hits this race reliably because its TUI
|
||||
// reverts to a shell title within milliseconds of `Stop`.
|
||||
//
|
||||
// The ack ALSO requires the OS window to be visible and focused
|
||||
// (document.visibilityState === 'visible' && document.hasFocus()) —
|
||||
@@ -43,6 +100,7 @@ export function useAutoAckViewedAgent(): void {
|
||||
let lastActiveView: unknown = undefined
|
||||
let lastActiveTabId: unknown = undefined
|
||||
let lastAgentStatus: unknown = undefined
|
||||
let lastRetained: unknown = undefined
|
||||
let lastAcknowledged: unknown = undefined
|
||||
// Why: settings is tracked so flipping `experimentalAgentDashboard`
|
||||
// alone re-evaluates the feature gate below — without this, a pure
|
||||
@@ -59,6 +117,7 @@ export function useAutoAckViewedAgent(): void {
|
||||
s.activeView === lastActiveView &&
|
||||
s.activeTabId === lastActiveTabId &&
|
||||
s.agentStatusByPaneKey === lastAgentStatus &&
|
||||
s.retainedAgentsByPaneKey === lastRetained &&
|
||||
s.acknowledgedAgentsByPaneKey === lastAcknowledged &&
|
||||
s.settings === lastSettings
|
||||
) {
|
||||
@@ -107,26 +166,10 @@ export function useAutoAckViewedAgent(): void {
|
||||
lastActiveView = s.activeView
|
||||
lastActiveTabId = s.activeTabId
|
||||
lastAgentStatus = s.agentStatusByPaneKey
|
||||
lastRetained = s.retainedAgentsByPaneKey
|
||||
lastAcknowledged = s.acknowledgedAgentsByPaneKey
|
||||
lastSettings = s.settings
|
||||
const prefix = `${activeTabId}:`
|
||||
const toAck: string[] = []
|
||||
for (const [paneKey, entry] of Object.entries(s.agentStatusByPaneKey)) {
|
||||
if (!paneKey.startsWith(prefix)) {
|
||||
continue
|
||||
}
|
||||
const ackAt = s.acknowledgedAgentsByPaneKey[paneKey] ?? 0
|
||||
// Why: use stateStartedAt (not updatedAt) so tool/prompt pings
|
||||
// within the same state don't re-trigger ack work on every event —
|
||||
// acknowledgeAgents short-circuits anyway when the value is
|
||||
// unchanged, but keeping the comparison in sync with the
|
||||
// "is-unvisited" rule in WorktreeCardAgents avoids a stutter
|
||||
// where we ack on an updatedAt-bump that didn't cross a state
|
||||
// transition.
|
||||
if (ackAt < entry.stateStartedAt) {
|
||||
toAck.push(paneKey)
|
||||
}
|
||||
}
|
||||
const toAck = computeAutoAckTargets(s, activeTabId)
|
||||
if (toAck.length > 0) {
|
||||
s.acknowledgeAgents(toAck)
|
||||
}
|
||||
@@ -137,7 +180,7 @@ export function useAutoAckViewedAgent(): void {
|
||||
// Why: store.subscribe fires on every state change. The reference-
|
||||
// equality guard above bails out immediately for the common case
|
||||
// (terminal output, timers, etc.) so the Object.entries walk only runs
|
||||
// when one of the five slices we read has actually changed.
|
||||
// when one of the six slices we read has actually changed.
|
||||
const unsubscribe = useAppStore.subscribe(maybeAck)
|
||||
// Why: focus/visibility don't flow through the zustand store, so a
|
||||
// late-arriving transition that failed the gate above never re-evaluates
|
||||
|
||||
Reference in New Issue
Block a user