-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat(agent-manager): key diff review by selection with per-session scope #12709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "kilo-code": minor | ||
| --- | ||
|
|
||
| Make the Agent Manager diff review follow the sidebar selection instead of a single session. Switching session tabs inside a worktree no longer refetches the Branch, Staged, and Unstaged scopes, the Session scope now swaps to the active session's changes on tab switch, and the Local tab gains the Session scope so sessions running in the workspace can be reviewed on their own. The Session scope shows a notice when snapshots are disabled instead of a blank list, worktrees without an open session now still show their branch diff, and the Apply dialog lists the worktree's changes again. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,18 @@ | ||
| /** | ||
| * Composite diff-source keying for Agent Manager. | ||
| * | ||
| * Agent Manager keys diff sources by *context* (a session id, or the `local` | ||
| * Agent Manager keys diff sources by *context* (a worktree id, or the `local` | ||
| * workspace pseudo-context) while the standalone Changes viewer keys by | ||
| * *scope* (branch / staged / unstaged / session). To expose scopes in Agent | ||
| * Manager we compose the two into a single id the SourceController can build. | ||
| * The context is the sidebar selection, so it stays stable when the user | ||
| * switches between session tabs of the same worktree; only the Session scope | ||
| * follows the active session, carried inside the id. | ||
| * | ||
| * ctx = "local" | "<sessionId>" | ||
| * ctx = "local" | "<worktreeId>" | ||
| * scope = "branch" | "staged" | "unstaged" | "session" | ||
| * id = `${ctx}#${scope}` | ||
| * id = `${ctx}#${scope}` (git scopes) | ||
| * id = `${ctx}#session:<sid>` (session scope, sid = active session id) | ||
| * | ||
| * `ctx#branch` is the default and reproduces the pre-scope behavior exactly. | ||
| */ | ||
|
|
@@ -18,8 +22,10 @@ export type DiffScope = "branch" | "staged" | "unstaged" | "session" | |
| export const DEFAULT_DIFF_SCOPE: DiffScope = "branch" | ||
|
|
||
| const SEP = "#" | ||
| const SESSION_TOKEN = "session:" | ||
|
|
||
| export function composeDiffId(ctx: string, scope: DiffScope): string { | ||
| export function composeDiffId(ctx: string, scope: DiffScope, sessionId?: string): string { | ||
| if (scope === "session" && sessionId) return `${ctx}${SEP}${SESSION_TOKEN}${sessionId}` | ||
| return `${ctx}${SEP}${scope}` | ||
| } | ||
|
|
||
|
|
@@ -28,11 +34,13 @@ export function composeDiffId(ctx: string, scope: DiffScope): string { | |
| * id (no separator) by assuming the default branch scope, which keeps the | ||
| * pre-scope messages working unchanged. | ||
| */ | ||
| export function parseDiffId(id: string): { ctx: string; scope: DiffScope } { | ||
| export function parseDiffId(id: string): { ctx: string; scope: DiffScope; sessionId?: string } { | ||
| const idx = id.lastIndexOf(SEP) | ||
| if (idx === -1) return { ctx: id, scope: DEFAULT_DIFF_SCOPE } | ||
| const scope = id.slice(idx + SEP.length) | ||
| if (isDiffScope(scope)) return { ctx: id.slice(0, idx), scope } | ||
| const token = id.slice(idx + SEP.length) | ||
| const ctx = id.slice(0, idx) | ||
| if (token.startsWith(SESSION_TOKEN)) return { ctx, scope: "session", sessionId: token.slice(SESSION_TOKEN.length) } | ||
| if (isDiffScope(token)) return { ctx, scope: token } | ||
| return { ctx: id, scope: DEFAULT_DIFF_SCOPE } | ||
| } | ||
|
|
||
|
|
@@ -46,12 +54,13 @@ export function normalizeScope(value: unknown): DiffScope { | |
|
|
||
| /** | ||
| * Map a scope to the underlying standalone-viewer source id the catalog knows | ||
| * how to build. `branch` maps to the workspace source; `session` is handled | ||
| * separately because it needs the session id embedded in the source id. | ||
| * how to build. `branch` maps to the workspace source; `session` needs the | ||
| * active session id embedded in the source id (the context id is a worktree | ||
| * or `local`, not a session). | ||
| */ | ||
| export function scopeToSourceId(scope: DiffScope, ctx: string): string { | ||
| export function scopeToSourceId(scope: DiffScope, ctx: string, sessionId?: string): string { | ||
| if (scope === "staged") return "staged" | ||
| if (scope === "unstaged") return "unstaged" | ||
| if (scope === "session") return `session:${ctx}` | ||
| if (scope === "session") return `session:${sessionId ?? ctx}` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: the When The only thing preventing that id from being sent today is that the reset-to-Branch effect ( Reply with |
||
| return "workspace" | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,11 @@ export class WorktreeDiffController { | |
| sessionId: source.descriptor.id, | ||
| loading, | ||
| }), | ||
| notice: (source, notice) => ({ | ||
| type: "agentManager.worktreeDiffNotice", | ||
| sessionId: source.descriptor.id, | ||
| notice, | ||
| }), | ||
| diffs: (source, diffs) => ({ | ||
| type: "agentManager.worktreeDiff", | ||
| sessionId: source.descriptor.id, | ||
|
|
@@ -81,8 +86,8 @@ export class WorktreeDiffController { | |
| } | ||
|
|
||
| public shouldStopForWorktree(path: string, sessions: ManagedSession[]): boolean { | ||
| // Pass the parsed context id, not the composite id, so the orphaned-session | ||
| // check matches real session ids. | ||
| // The parsed context id is a worktree id (or `local`), so the | ||
| // orphaned-session check matches sessions of the deleted worktree. | ||
| const current = this.controller.currentId | ||
| const ctxId = current ? parseDiffId(current).ctx : undefined | ||
| return shouldStopDiffPolling(path, sessions, this.target, ctxId) | ||
|
|
@@ -225,6 +230,9 @@ export class WorktreeDiffController { | |
| const { ctx } = parseDiffId(id) | ||
| const resolved = await this.resolve(ctx) | ||
| 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 }) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Reply with |
||
| this.controller.setContext({ | ||
| workspaceRoot: this.ctx.getRoot(), | ||
| dir: resolved?.directory, | ||
|
|
@@ -250,21 +258,11 @@ export class WorktreeDiffController { | |
| return undefined | ||
| } | ||
|
|
||
| const session = state.getSession(ctxId) | ||
| if (!session) { | ||
| this.ctx.log( | ||
| `resolveDiffTarget: session ${ctxId} not found in state (${state.getSessions().length} total sessions)`, | ||
| ) | ||
| return undefined | ||
| } | ||
| if (!session.worktreeId) { | ||
| this.ctx.log(`resolveDiffTarget: session ${ctxId} has no worktreeId (local session)`) | ||
| return undefined | ||
| } | ||
|
|
||
| const worktree = state.getWorktree(session.worktreeId) | ||
| // The context is the worktree itself (the sidebar selection), not one of | ||
| // its sessions — resolution survives session churn inside the worktree. | ||
| const worktree = state.getWorktree(ctxId) | ||
| if (!worktree) { | ||
| this.ctx.log(`resolveDiffTarget: worktree ${session.worktreeId} not found for session ${ctxId}`) | ||
| this.ctx.log(`resolveDiffTarget: worktree ${ctxId} not found`) | ||
| return undefined | ||
| } | ||
| const base = this.baseOverrides.get(ctxId) ?? remoteRef(worktree) | ||
|
|
@@ -287,13 +285,14 @@ export class WorktreeDiffController { | |
|
|
||
| /** | ||
| * Build the active source for a composite id by delegating to the catalog. | ||
| * The composite id (ctx#scope) is preserved as the descriptor id so the | ||
| * webview keys diff data by context+scope. Context resolution (dir/base) | ||
| * already happened in activate() and is carried by the PanelContext. | ||
| * The composite id (`ctx#scope`, or `ctx#session:<sid>` for the session | ||
| * scope) is preserved as the descriptor id so the webview keys diff data by | ||
| * context+scope. Context resolution (dir/base) already happened in | ||
| * activate() and is carried by the PanelContext. | ||
| */ | ||
| private source(id: string, panelCtx: PanelContext): DiffSource { | ||
| const { ctx, scope } = parseDiffId(id) | ||
| const built = this.ctx.catalog.build(scopeToSourceId(scope, ctx), panelCtx) | ||
| const { ctx, scope, sessionId } = parseDiffId(id) | ||
| const built = this.ctx.catalog.build(scopeToSourceId(scope, ctx, sessionId), panelCtx) | ||
| return { | ||
| ...built, | ||
| descriptor: { ...built.descriptor, id }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import { describe, it, expect } from "bun:test" | ||
| import { composeDiffId, parseDiffId, scopeDescriptors } from "../../webview-ui/agent-manager/diff-scope-state" | ||
|
|
||
| describe("agent-manager webview diff scope descriptors", () => { | ||
| it("offers the three git scopes without an active session", () => { | ||
| const descriptors = scopeDescriptors("wt_1") | ||
| expect(descriptors.map((d) => d.type)).toEqual(["workspace", "staged", "unstaged"]) | ||
| expect(descriptors.map((d) => d.id)).toEqual(["wt_1#branch", "wt_1#staged", "wt_1#unstaged"]) | ||
| }) | ||
|
|
||
| it("adds the session scope with the active session embedded", () => { | ||
| const descriptors = scopeDescriptors("wt_1", "ses_abc") | ||
| expect(descriptors.map((d) => d.type)).toEqual(["workspace", "staged", "unstaged", "session"]) | ||
| const session = descriptors[3]! | ||
| expect(session.id).toBe("wt_1#session:ses_abc") | ||
| expect(session.group).toBe("Session") | ||
| expect(session.capabilities.revert).toBe(false) | ||
| }) | ||
|
|
||
| it("embeds the active local session for the local context", () => { | ||
| const descriptors = scopeDescriptors("local", "ses_abc") | ||
| expect(descriptors[3]!.id).toBe("local#session:ses_abc") | ||
| }) | ||
|
|
||
| it("round-trips the session descriptor id", () => { | ||
| expect(parseDiffId(composeDiffId("wt_1", "session", "ses_abc"))).toEqual({ | ||
| ctx: "wt_1", | ||
| scope: "session", | ||
| sessionId: "ses_abc", | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
diffCtxis 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()clearsthis.targetfor the currently active id (reachable from the Apply dialog, which now requests the very samectx#branchid the panel is watching). SodiffTargetcan beundefinedwhileorphanedis empty, this returnsfalse,diffs.stop()is skipped, and the poll interval keeps running git in the removed directory. Passing the worktree id in and comparing it directly todiffCtxwould close that gap.Reply with
@kilocode-bot fix itto have Kilo Code address this issue.