feat(agent-manager): key diff review by selection with per-session scope - #12709
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if (scope === "staged") return "staged" | ||
| if (scope === "unstaged") return "unstaged" | ||
| if (scope === "session") return `session:${ctx}` | ||
| if (scope === "session") return `session:${sessionId ?? ctx}` |
There was a problem hiding this comment.
WARNING: the ?? ctx fallback can now build a session source from a worktree id
When ctx was a session id this fallback degraded gracefully. Now that ctx is a worktree id (or local), it produces session:<worktreeId>. DiffSourceCatalog.build accepts anything non-empty after session:, so a bare ctx#session id — which composeDiffId yields whenever the session scope is active while the active session is momentarily undefined (webview-ui/agent-manager/diff-review-scope.ts:38) — becomes a snapshot fetch for a session that does not exist. SourceController.runFetch swallows the rejection and returns true, so polling continues and the user sees a permanently empty Session diff with no notice.
The only thing preventing that id from being sent today is that the reset-to-Branch effect (diff-review-scope.ts:60) is created before the watch effect, so it wins the flush. Consider making the missing-session case explicit at both ends instead of falling back to the context id — the new case in tests/unit/diff-scope.test.ts currently pins the fallback as intended behavior.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ): boolean { | ||
| if (diffTarget && normalizePath(diffTarget.directory) === normalizePath(worktreePath)) return true | ||
| if (diffSessionId && orphaned.some((s) => s.id === diffSessionId)) return true | ||
| if (diffCtx && orphaned.some((s) => s.worktreeId === diffCtx || s.id === diffCtx)) return true |
There was a problem hiding this comment.
SUGGESTION: the "diff context is the worktree itself" case isn't actually checked
The updated doc comment says polling stops when the diff context is the worktree being deleted, but diffCtx is only ever compared against session ids here — the deleted worktree's own id never reaches this function. It therefore only works while the worktree still has sessions.
A worktree with no sessions is now a valid diff context (one of the fixes in this PR), and WorktreeDiffController.request() clears this.target for the currently active id (reachable from the Apply dialog, which now requests the very same ctx#branch id the panel is watching). So diffTarget can be undefined while orphaned is empty, this returns false, diffs.stop() is skipped, and the poll interval keeps running git in the removed directory. Passing the worktree id in and comparing it directly to diffCtx would close that gap.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| this.target = resolved ? { sessionId: id, ...resolved } : undefined | ||
| // Clear any stale source notice up front; sources only push a notice when | ||
| // one is active, so a swap away from a noticing source must reset it. | ||
| this.ctx.post({ type: "agentManager.worktreeDiffNotice", sessionId: id, notice: undefined }) |
There was a problem hiding this comment.
SUGGESTION: a notice that stops being reported mid-poll keeps its banner
Clearing on activate handles source swaps, but not the case where the same source stays active and stops reporting. SourceController.runFetch only posts when result.notice !== undefined, so if the user does exactly what the snapshots-disabled banner asks (enable snapshots in the config), the source stops returning the notice and the banner stays up until the context is re-activated. Posting the notice on every fetch — or posting undefined when a fetch reports none — would let it clear itself.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const current = session.currentSessionID() | ||
| if (sel === LOCAL) { | ||
| if (current && localSessionIDs().includes(current) && !isPending(current)) return current | ||
| return localSessionIDs().find((id) => !isPending(id)) |
There was a problem hiding this comment.
SUGGESTION: a pending Local tab silently borrows another session's diff
When the active Local tab is a pending "New Session" tab, this falls through to the first non-pending local session, so the Session scope shows up and renders the diff of a session other than the visible tab, with nothing in the UI naming it. Returning undefined here (hiding the Session scope while a pending tab is active) would match the tab bar. The worktree fallback below has a milder version of the same issue: it picks from the raw managedSessions() order rather than the visible tab order.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| const onWorktreeDiffNotice = (ev: AgentManagerWorktreeDiffNoticeMessage) => { | ||
| setDiffNotices((prev) => ({ ...prev, [ev.sessionId]: ev.notice })) |
There was a problem hiding this comment.
SUGGESTION: notices and diff data are never pruned
diffNotices (like the existing diffDatas / diffFileLoading) now accumulates an entry per ctx#scope plus one per ctx#session:<sid> ever visited, and nothing removes them when a worktree or session disappears. diffDatas holds full patch/before/after text per file, so a long-lived Agent Manager tab retains every diff it ever fetched. reviewOpenByContext / reviewCommentsByContext are already pruned against the live worktree list in AgentManagerApp.tsx:518-530; the same effect could drop these keys by parsed ctx.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // --- Data model --- | ||
|
|
||
| /** Well-known diff source notices → i18n keys (mirrors the standalone viewer). */ | ||
| const DIFF_NOTICE_KEYS: Record<string, string> = { |
There was a problem hiding this comment.
SUGGESTION: third copy of the notice → i18n map
This map now exists here, in diff-viewer/FullScreenDiffView.tsx:69, and in diff-viewer/DiffViewerApp.tsx:28. Exporting a single map — and typing the new prop as the existing DiffViewerNotice union (src/types/messages/extension-messages.ts:1031) rather than string — would keep the three surfaces from drifting and remove the need for the t(map[n] ?? n) fallback, which renders a raw i18n key for any kind that isn't in the map.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| /> | ||
| </div> | ||
| <div class="am-review-diff" ref={setScroller}> | ||
| <Show when={noticeText()}> |
There was a problem hiding this comment.
SUGGESTION: this banner scrolls out of view
Here the notice is rendered inside .am-review-diff, which is the scroll container (ref={setScroller}), so it scrolls away with the file list. DiffPanel and DiffViewerApp both keep it above the scroller. Moving it outside would keep the notice visible while scrolling a review.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge
Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (18 files)
Verified as correct while reviewing: the git scopes genuinely stop refetching on session tab switches ( Fix these issues in Kilo Cloud Reviewed by claude-opus-5 · Input: 90 · Output: 40.2K · Cached: 4.6M Review guidance: REVIEW.md from base branch |
2 participants
The Agent Manager diff review keys its context by a single session id. When a worktree has multiple sessions, switching session tabs swaps the whole diff context: the Branch, Staged, and Unstaged scopes refetch identical data, the scope selection is forgotten per session instead of per worktree, and a worktree whose sessions are gone shows nothing at all, even when its branch still has changes. The Local tab cannot offer the Session scope at all, because local sessions are not resolvable diff contexts, so there is no way to review just the changes of a session running in the workspace.
This re-keys the diff context to the sidebar selection (a worktree id or the local workspace) and treats the Session scope as the only session-dependent view. Git scopes resolve the worktree directly, so they stay stable and cached across session tab switches, and worktrees without an open session still show their branch diff. The Session scope embeds the active session id in the composite diff id (
ctx#session:<sid>), so switching session tabs with the Session scope active swaps to that session's changes while the other scopes are untouched. Scope memory is now per selection, so a picked scope survives tab switches.The Local tab gains the Session scope through the same path: it follows the active local session and diffs against the workspace root, matching what the standalone Changes panel already did. When the active session disappears while the Session scope is selected, the scope falls back to Branch.
Two adjacent issues are fixed with the re-keying. The Session scope previously showed a blank list when snapshot tracking is disabled, because the source's snapshots-disabled notice was dropped on the Agent Manager message path; both review surfaces now show it as a banner instead of the empty state. The Apply dialog listed no files, because it still requested and read diff data by a bare session id while the data had moved to composite
ctx#scopekeys; it now uses the worktree's branch-scoped key.Session scope in the selector
Session scope follows the active session tab
Branch scope on the same context shows every changed file
Implements the session-tab-dependent diff idea from #10711 (comment).