feat(jetbrains): improve session changes and diff review - #12612
Merged
Conversation
The session UI tracked only one permission at a time, so when the CLI asked for several permissions in one turn the last PermissionAsked overwrote the previous one and replying it dropped the session to Busy while the earlier permission stayed pending forever. Maintain a controller-owned FIFO queue keyed by permission id. The model still holds one active permission; advancing is driven by the authoritative PermissionReplied event so it behaves the same whether the reply came from this client, another client, or auto-approve. Recovery and child-session permissions seed the same queue in FIFO order, and the queue is cleared on abort, auto-approve enable, and subscription reset.
# Conflicts: # packages/kilo-jetbrains/frontend/src/main/kotlin/ai/kilocode/client/session/views/TurnView.kt
Show JetBrains session changes against the base branch after restart by wiring the diff editor entry point and matching VS Code diff semantics for uncommitted and untracked files.
- Color file names with IntelliJ VCS status colors (added/modified/deleted/untracked) - Add tree context menu with Open File (F4) and Refresh actions - Show changed-file count in the toolbar before the diff stat badge - Fix keyboard selection snap-back while preserving debounced buffering
kirillk
changed the title
kirillk
marked this pull request as ready for review
July 30, 2026 15:46
marius-kilocode
approved these changes
Jul 30, 2026
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Notes and assumptions
Files Reviewed this revision (2 files)
Fix these issues in Kilo Cloud Previous Review Summaries (7 snapshots, latest commit 36fbfbc)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 36fbfbc)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Notes and assumptions
Files Reviewed this revision (3 files)
Fix these issues in Kilo Cloud Previous review (commit 56e0785)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Notes and assumptions
Files Reviewed this revision (6 files)
Fix these issues in Kilo Cloud Previous review (commit 79ac066)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Notes and assumptions
Files Reviewed this revision (6 files)
Fix these issues in Kilo Cloud Previous review (commit a0e6273)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Notes and assumptions
Files Reviewed this revision (2 files)
Fix these issues in Kilo Cloud Previous review (commit 568b21c)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Notes and assumptions
Files Reviewed this revision (32 files)
Fix these issues in Kilo Cloud Previous review (commit a103f4a)Status: 29 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)New in this revision
Still open from earlier revisions
Resolved in this revision
Notes and assumptions
Files Reviewed this revision (13 files)
Fix these issues in Kilo Cloud Previous review (commit 8a15b00)Status: 28 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)New in this revision
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by claude-opus-5 · Input: 38 · Output: 12.4K · Cached: 1.3M Review guidance: REVIEW.md from base branch |
Roll back unrelated shared changes and address the PR review on the
JetBrains session-diff work.
Shared reverts (keep the PR scoped to JetBrains):
- Restore packages/opencode/src/mcp/catalog.ts to upstream (the defensive
MCP error-shape guard was unnecessary and widened the fork diff).
- Restore the ui `spacing: 0` diff metrics in file.tsx / virtualizer.ts.
Backend:
- branchDiff: add --relative so tracked paths are project-relative and
scoped to the opened directory (matches the untracked list in monorepos).
- branchDiff: fetch patches per file and stop at DIFF_CAP instead of
materializing the whole repo's full-context diff up front; add a
patches=false stat-only path for the header badge.
- defaultBranch: resolve via origin/HEAD then origin/local main|master
using fully-qualified refs (no tag confusion, non-main/master repos).
- session diff() now uses GET /session/:id/diff (cumulative, deduped,
unquoted) instead of concatenating per-turn summaries.
Frontend:
- Permission queue: purge ghost permissions the CLI abandons on turn
interruption (TurnClose / idle) and on child untrack.
- Diff editor: bind processor listeners to the processor, constrain
"open file" to the diff directory, bind the refresh coroutine to the
view, stop swallowing CancellationException, and make the branch source
fetch authoritative (drop the store seeding side channel).
- KiloInlineDiffStore: bound with a small LRU.
- DiffLineNumbers / pureDiff: keep in-hunk header-shaped lines and mirror
trim('\n') so the gutter stays aligned; DiffPatchReconstruct treats
multi-hunk / length-mismatched patches as non-renderable.
- ToolMarkdownBody re-installs the diff gutter on applyStyle so it
survives collapse/re-expand.
- EditToolView: don't bind the file link for toggling, avoid re-parsing
diff metadata per delta, keep the file name in single-file diff titles.
- Only relayout the transcript when a turn's modified-files card changed.
- Propagate branch/session diff errors so the editor shows a retry.
- Badge: separate open vs refresh jobs, keyboard-accessible, gate on
isEnabled; remove dead history-load refresh branches.
- DiffBlocks / KiloBundle: bundle key for the diff-unavailable string,
drop unused bundle keys.
Address the follow-up review on the diff tree toolbar: - Remove the now-unused IdeActions import from KiloDiffEditorContent (its only users were the removed prev/next-diff lookups). - KiloDiffEditorContentTest now asserts on the ActionToolbar the view actually installs (via its actionGroup) instead of a freshly built detached group, so it guards the real "toolbar lost its actions" regression.
DiffPatchReconstruct counted the trailing empty element that
split('\n') produces for a newline-terminated patch (every real git
patch) as a hunk body line. That inflated oldSeen/newSeen past the
header lengths, so the renderable check failed and every modified-file
diff fell back to the empty-vs-patch view — rendering the whole file
(and its line-number gutter) as all-added green.
Drop the trailing empty split artifact before counting, mirroring the
edge trim in DiffLineNumbers. Add a newline-terminated regression test;
the existing trimIndent fixtures masked the bug by omitting the final
newline.
# Conflicts: # packages/kilo-jetbrains/frontend/src/main/kotlin/ai/kilocode/client/session/controller/SessionController.kt
…d permissions
Address review feedback on the session/branch diff work:
- branchDiff no longer keeps calling git per file after the DIFF_CAP
budget is exhausted. A single oversized full-context patch previously
left the budget open, so every remaining file still spawned a
`git diff --unified=inf` that was fetched and discarded — hundreds of
subprocesses per open/refresh on large branches. The new pure
`capDiff` accumulator stops fetching once a patch overflows the cap.
- The stats-only (badge) untracked path now streams a newline count
instead of reading each file into a String on every turn end/revert.
- Delete the now-dead `buildBranchDiff`/`splitGitPatch` helpers and
re-target the suite onto the live `capDiff` path plus `parseNumstat`/
`parseNameStatus` (renamed BranchDiffBuildTest -> BranchDiffTest).
- SessionController.status("idle") no longer clobbers a permission that
purgePending just promoted from an unpurged child session; it mirrors
idle() and leaves the promoted card in place.
Address follow-up review on the JetBrains diff/session work: - Parse message `summary` with the safe JSON-object accessor so assistant compaction summaries (`summary: true`) do not crash history/event parsing. - Route auto-approve skill-shell permission cards through the same queue as ordinary permissions, preserving FIFO order and letting purge/resolve logic handle them consistently. - Make Stop purge current root/child permission ghosts instead of clearing only the backing queue and leaving a dead card on screen. - Let capDiff skip an oversized patch and still fetch later small patches, while bounding repeated oversized misses to keep subprocess count finite. Add regression coverage for compacted assistant messages, oversized diff cap behavior, Stop ghost purge, and multi skill-shell queueing.
Follow-up review on the permission queue: - Decide the skill-shell "needs a human" case synchronously on the EDT in approve() and enqueue there, so back-to-back auto-approve asks keep arrival (FIFO) order instead of racing two independent coroutines. Only the replyPermission RPC stays in a coroutine. - Route permission enqueue/promote through a show() helper wrapped in updateModel, so cards added from approve()/abort() preserve the transcript's bottom-follow like the drain and child-recovery paths. - Queue the auto-approve error card too, so pending stays the single source of truth and Stop / TurnClose / idle purge can clear it rather than stranding a card that can only fail with NotFoundError. Add coverage for the purged auto-approve error card.
marius-kilocode
approved these changes
Jul 31, 2026
| // machine reply. Decide and enqueue synchronously on the EDT so back-to-back asks keep | ||
| // arrival (FIFO) order, matching asked()'s non-auto path; only the RPC needs a coroutine. | ||
| if (!autoApprove || restore().meta.raw["skillShell"] == "true") { | ||
| show(restore()) |
Contributor
There was a problem hiding this comment.
WARNING: making this branch synchronous lets setAutoApprove wipe the card it just queued
setAutoApprove(true) writes the setting first, then (line 392) calls approve(current.permission) for the visible card, and then does pending.clear() on line 397 before draining. With the old deferred cs.launch { edt { enqueue(...) } }, the enqueue landed after that clear(), so a visible skill-shell card stayed tracked. Now show(restore()) runs inline: it enqueues, skips promote() (state is already AwaitingPermission), and line 397 immediately drops the entry again.
The result is the exact ghost this commit is fixing elsewhere — a visible skill-shell card that is not in pending, so purgePending on Stop / TurnClose / idle can't clear it, and answering it later fails with NotFoundError. It's worse if the drain finds other permissions: drainAutoApprove excludes skip, so cards is empty, count > 0 takes the setState(Busy) branch (line 781) and the card disappears with nothing left in pending to re-promote, leaving the CLI waiting on a request the user can no longer answer.
Suggestion: move pending.clear() above the approve(current.permission) call in setAutoApprove (or have it drop only the ids not being shown), so the synchronous show() is the last writer. A regression test that toggles auto-approve on while a skill-shell card is visible and then aborts would cover it.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Contributor
Author
There was a problem hiding this comment.
Fixed in e20f5ec. setAutoApprove now runs pending.clear() before re-surfacing the visible card, so the synchronous show() inside approve() is the last writer and the preserved skill-shell card stays tracked in pending (a later Stop / TurnClose / idle purge can clear it instead of stranding a card that only NotFoundErrors). I also closed the "worse if the drain finds other permissions" case: drainAutoApprove now guards its setState(Busy) branch with current.permission.id !in skip, so it never flips a card handled synchronously by the caller to Busy and vanishes a preserved skill-shell card. Covered by test toggling auto approve on keeps a visible skill shell card queued for purge (toggle on with the card up, then abort → Idle) and test toggling auto approve on keeps skill shell card while draining other permissions (the other permission is replied while the skill-shell card stays visible); reverting either fix independently makes the corresponding test fail.
Collaborator
|
I like the tooltip, maybe we need that in vs code as well! |
Toggling auto-approve on while a skill-shell permission card is visible stranded the card: approve() synchronously re-enqueues it via show(), but the trailing pending.clear() dropped that entry, leaving a ghost card not in pending that Stop/idle purge could not clear (answering it later failed with NotFoundError). Clear the queue before re-surfacing the card so show() is the last writer. Also guard drainAutoApprove so it never flips a card that was handled synchronously (in skip) to Busy, which otherwise hid a preserved skill-shell card with no reply path left.
| // replied to it — already Busy — or re-showed a skill-shell card we must keep). | ||
| // Never transition it to Busy here or the preserved skill-shell card vanishes | ||
| // with no reply path left. | ||
| if (current is SessionState.AwaitingPermission && |
Contributor
There was a problem hiding this comment.
SUGGESTION: the skip exemption only covers cards that existed before the drain started
The new id !in skip guard fixes the card setAutoApprove preserved, but this branch still flips any visible AwaitingPermission for ids to Busy, including one that has nothing to do with what the drain replied to. Concrete window:
setAutoApprove(true)clearspending,approve()replies to the visible non-skill-shell card (stateBusy),drainAutoApprovelaunches and suspends insessions.pendingPermissions(...).- While that RPC is in flight, a new skill-shell
PermissionAskedarrives on the EDT →asked()→approve()→show()→AwaitingPermission(permX). - The drain resumes with a server snapshot taken before
permXexisted, socardsis empty whilecount > 0, and this branch setsBusy— hidingpermX.
permX stays in pending, so it isn't leaked, but nothing re-promotes it unless another permission.replied or a purge fires. If it's the only outstanding request the turn just sits on "considering" until the ask is abandoned. Consider narrowing the condition to the permissions the drain actually answered (e.g. have replyAll return the replied ids and require current.permission.id to be one of them) instead of exempting only skip.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Contributor
Author
There was a problem hiding this comment.
Acknowledged — this is a real but low-severity race, tracking it as a follow-up rather than blocking this PR. It does not leak: permX stays in pending, and the drain replied to count > 0 permissions, so the resulting permission.replied events hit replied() while state is Busy and call promote(), re-surfacing permX. Worst case is a transient "considering" spinner during the pendingPermissions RPC round-trip, not a stuck state or a NotFoundError. The suggested hardening (have replyAll return the replied ids and gate the Busy transition on current.permission.id being one of them, instead of exempting only skip) is the right narrowing and we'll do it in a follow-up.
unixcrh
pushed a commit
to unixcrh/kilocode
that referenced
this pull request
Aug 1, 2026
t7tran
pushed a commit
to t7tran/kilocode
that referenced
this pull request
Aug 14, 2026
t7tran
pushed a commit
to t7tran/kilocode
that referenced
this pull request
Aug 14, 2026
feat(jetbrains): improve session changes and diff review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
No tracked issue — batch of JetBrains session and diff improvements from dogfooding.
Features
Screenshots
Verification
./gradlew typecheck./gradlew testChecklist