fix(sessions): prune stale agent session rows - #375
DerrickBarra wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSession loading now reconciles full gateway sessions with ChangesSession reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08cd488ada
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? baseSessions.filter((session) => { | ||
| const key = getSessionKey(session); | ||
| if (!key) return false; | ||
| if (isSessionEligibleWithoutSpawnedConfirmation(session)) return true; |
There was a problem hiding this comment.
When a spawnedBy lookup succeeds, this filter removes every inactive base session that is not a root or cron session, including ordinary keys such as agent:main:telegram:direct:123 and agent:reviewer:discord:channel:456 that sessionTree.ts deliberately renders beneath agent roots. Those sessions do not appear in spawned-child results, so successful refreshes now make existing DM/channel conversations disappear from the sidebar; restrict stale pruning to session types that can actually be governed by spawnedBy.
Useful? React with 👍 / 👎.
| return mergeAuthoritativeSessions( | ||
| baseSessions, | ||
| spawnedResults.map((result) => result.sessions), | ||
| { spawnedByAuthoritative: spawnedResults.some((result) => result.ok) }, |
There was a problem hiding this comment.
When there are multiple agent roots and only one sessions.list({ spawnedBy }) call fails, some(result.ok) still marks the combined data authoritative. The failed root contributes an empty child list, causing its inactive children from the full list to be pruned as stale merely because another root's request succeeded; track authority per root or retain sessions belonging to failed roots.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/contexts/SessionContext.tsx`:
- Around line 328-343: Scope spawned-session authority per root instead of using
aggregate success: in src/contexts/SessionContext.tsx lines 328-343, retain each
rootSessionKey with its lookup result and pass per-root success data to
mergeAuthoritativeSessions; in src/features/sessions/sessionReconciliation.ts
lines 73-107, prune base children only when their root lookup succeeded,
preserving children for failed roots. Add unit coverage for mixed
successful/failed roots in src/features/sessions/sessionReconciliation.test.ts
lines 66-80 and integration coverage confirming a child under a failed root
remains visible in src/contexts/SessionContext.test.tsx lines 524-555.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 414e1dc6-debc-450a-b7d2-211c9614ad39
📒 Files selected for processing (4)
src/contexts/SessionContext.test.tsxsrc/contexts/SessionContext.tsxsrc/features/sessions/sessionReconciliation.test.tssrc/features/sessions/sessionReconciliation.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28e7b01b87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const key = getSessionKey(session); | ||
| if (!key || seen.has(key)) continue; | ||
| if (!isLiveSupplementalSession(session)) continue; | ||
| seen.add(key); |
There was a problem hiding this comment.
When the full list lags after a spawn and the spawnedBy response contains only the session key/label—the API-compatible shape used throughout this repository—isLiveSupplementalSession returns false because all state fields are optional, so this line drops the newly spawned child even though the successful spawnedBy lookup confirms it exists. This regresses the preceding refresh behavior and can make a newly created child disappear from the sidebar until the full list catches up; confirmed spawned rows with unknown state need to remain eligible.
Useful? React with 👍 / 👎.
| const rootKey = getRootAgentSessionKey(key); | ||
| if (rootKey && authoritativeSpawnedByRoots && !authoritativeSpawnedByRoots.has(rootKey)) return true; | ||
| return Boolean(rootKey && spawnedKeys.has(rootKey)); |
There was a problem hiding this comment.
When one root lookup succeeds and another fails, an inactive child with a supported opaque key such as custom-subagent-key and parentSessionKey: 'agent:reviewer:main' reaches this branch with rootKey === null, so it is pruned despite belonging to the failed, non-authoritative root. Fresh evidence in the final tree is that sessionTree.ts explicitly supports opaque keys through parentSessionKey/parentId, while the new preservation logic derives scope only from the key; use the explicit parent candidates to determine the authoritative root as well.
Useful? React with 👍 / 👎.
1 participant
In Plain English
The Agents panel can keep showing old spawned-session rows after the gateway no longer reports them as current children. This PR makes the full session list converge with current
spawnedBytruth while still keeping live children visible during gateway lag or transient spawnedBy lookup failures.What
spawnedBysession results.Why
Closes #372.
Without this, the Agents panel can show stale subagent/session rows after a spawned session is deleted, archived, or no longer returned by the gateway's current child lookup.
How
mergeAuthoritativeSessionsinsrc/features/sessions/sessionReconciliation.ts.SessionContextto fetchspawnedBychild lists with success/failure metadata and reconcile them against the full list.sessionReconciliation.test.tsandSessionContext.test.tsx.Pre-patch proof:
312e27333e14f841b95bf4f2b205a856b4a4c370, adding only the stale-child regression test and runningnpm test -- --run src/contexts/SessionContext.test.tsxfailed becauseOld cached childremained in the document.Validation:
npm test -- --run src/features/sessions/sessionReconciliation.test.ts src/contexts/SessionContext.test.tsxpassed: 2 files, 27 tests.npm run lintpassed.npm run buildpassed. Vite reported existing chunk-size/dynamic-import warnings.npm run build:serverpassed.npm test -- --runpassed: 143 files, 1878 tests. The run emitted existing Reactact(...)and nested-button warning noise from unrelated tests.Type of Change
Checklist
npm run lintpassesnpm run build && npm run build:serversucceedsnpm test -- --runpassesScreenshots
Not applicable; this is a session reconciliation behavior fix covered by tests.
Summary by CodeRabbit