fix(jetbrains): make gh/PR focus sync responsive without overwhelming the backend - #13628
Merged
Conversation
… the backend A focus or tab-switch sync that could not run right away was dropped outright, leaving a stale gh/PR verdict standing until the next scheduled poll (up to 30s for gh, 120s for PR). Coalesce instead of drop: a sync that cannot run now is held as a single trailing probe, so a burst of events costs at most one extra probe per throttle window instead of losing the request. Scale the work to the length of the absence via a new Away helper. A dialog or popup that never really took focus out of the IDE costs nothing; a quick window switch takes the ordinary throttled path; an absence long enough to have contained a `gh auth login` or a merged PR bypasses the throttle and asks the backend for a fresher answer. Thread a maxAge ceiling through ghStatus/prStatus/branchStatus so a caller returning from a long absence can bypass the backend's own cache too - previously prStatus had no bypass at all, so even a forced frontend refresh could still serve a PR list up to 90s stale. maxAge only tightens the TTL, never extends it, so no caller can pin stale data past the backend's own TTL. Also narrow tab-switch syncing to the Agent Manager tab (Chat has no gh-dependent UI to refresh) and pass maxAge=0 when a worktree is freshly created so its PR badge doesn't wait out the cache.
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (24 files)
Previous Review Summaries (13 snapshots, latest commit cad7d81)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit cad7d81)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous review (commit 52c8af5)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit 3e4f957)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous review (commit 59c68e1)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit 3f13f5f)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit 12c0aff)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Previous review (commit 6d7939f)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Previous review (commit aebd040)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (17 files)
Fix these issues in Kilo Cloud Previous review (commit 7e5ddc0)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (25 files)
Fix these issues in Kilo Cloud Previous review (commit c9a01dc)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit c35dc1c)Status: No Issues Found | Recommendation: Merge Files Reviewed (16 files)
Previous review (commit 23007e1)Status: No Issues Found | Recommendation: Merge Files Reviewed (15 files)
Previous review (commit af2c45a)Status: No Issues Found | Recommendation: Merge Files Reviewed (15 files)
Reviewed by grok-4.6 · Input: 293.4K · Output: 19.1K · Cached: 683.4K Review guidance: REVIEW.md from base branch |
refreshPr(force = true) bypasses PR_THROTTLE, and returning from a long absence now takes that path. loadPr() replaced prJob without checking whether one was still running, so a user leaving and coming back faster than a lookup completes could stack them without bound. Each lookup fans out to several concurrent `gh pr view` calls, and the semaphore bounding that fan-out is created per call - so N overlapping lookups multiply the subprocess count rather than answer sooner, and each one pins a Dispatchers.IO thread waiting on the backend's gh lock. The existing generation guard prevented stale publishes, so this was wasted work rather than wrong data. Skip when a lookup is already in flight. Skipping rather than cancelling keeps the work already spawned; the poll and the next focus correct whatever the running lookup began too early to observe. Both new tests fail without the guard and pass with it.
marius-kilocode
approved these changes
Aug 31, 2026
`test update hooks run on EDT around history and recovery` failed intermittently on CI at the final `state is Busy` assertion, and reproduced locally about once in six runs. Recovery seeds session state from the status flow KiloSessionService collects off the fake RPC, and the test asserted immediately after a single `flush()`. 56205ea replaced settle()'s five 100ms sleeps with a bounded five-round drain, removing the wall-clock slack that had been letting that collection land first. Five dispatcher round trips usually outrun it and under load do not, leaving state at Idle. Wait for both update cycles (history load, then recovery) to be observed via the existing waitFor helper instead of assuming a fixed round budget is enough. Assertions are otherwise unchanged, and the failure message now reports the hook log actually seen. Verified: 8 runs of the class and 3 full frontend suite runs clean.
testReleaseStopsProcessesAndForgetsClones failed intermittently on CI asserting states.value.isEmpty() straight after awaiting only handler.isProcessTerminated. Release stops the process through the platform's stopProcess, which runs off the calling thread, and the manager drops its tracked state from the resulting processTerminated notification - a separate hop after isProcessTerminated flips. Asserting immediately races that hop. Wait for the tracked state to clear, matching the existing pattern at the "dropped handler" and "no running processes" waits. Applied to testReleaseStopsAStartAlreadyInFlight too, which had the identical shape and the same latent race. The other isEmpty assertions in this file follow synchronous processTerminated publishes, or run with no process started at all, so they stay as direct assertions.
Each worktree row's title line now carries the pull request's review verdict followed by its build verdict, so the states a reviewer scans the list for are readable without hovering or opening the PR. Data: WorktreePrDto gains a review verdict and a rolled-up checks summary, both defaulted and appended so the wire stays decodable across split-mode version skew and the existing positional constructors keep compiling. GhState is left alone - it is when-exhaustive in PrBadges and asserted per-entry in UiStyleTest, and review and CI are orthogonal to it. The checks summary carries counts only: per-check names and timestamps would make every poll produce a DTO that compares unequal, and both WorktreeRow.equals and WorktreeNameCache gate refreshes on whole-DTO equality. Backend asks gh for reviewDecision and statusCheckRollup, which are GraphQL sub-queries rather than scalars. An older gh rejects the field names and a restricted token is refused the data, and prError treats everything non-auth as "no PR here" - so without a fallback those users would lose a PR badge they have always had. PrResolver now detects both error shapes, retries once with the scalar list, and latches the downgrade so it costs one extra call in total rather than one per checkout per poll. UI: ActiveListBadge gains an optional icon that replaces the pill, which is what lets a status glyph use the existing badges slot with its hit-testing, tooltip and click handling instead of a parallel type and a second renderer path. Review verdicts are bare stroke glyphs and CI verdicts are filled circle badges so the two never read as the same indicator - an approved review and a green build would otherwise both be a green check. Pending review gets no glyph at all: nearly every open PR sits in that state, so it would mark almost every row and say nothing. Icons follow the icon-jetbrains skill: 16x16 action canvas, light/dark pairs with identical geometry, palette colors only, and muted-dark glyphs inside dark badges rather than white. "Running" uses the palette orange, the nearest entry to the brown that was asked for - the New UI palette has no brown and inventing one breaks theme recoloring.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Hovering a worktree row now opens a popup beside it with the full pull request picture: the shared PrHeaderView in FULL mode (title, number, state, committed and uncommitted diff counts, ahead/behind) plus a line each for the review and CI verdicts, whose row glyphs have room for a colour and nothing else. The placement rules the chat transcript already had are what this needed, so they moved rather than being rewritten. HeaderPopupGeometry becomes SidePopupGeometry in ui/popup: it picks the side with more room, breaks ties right, and only ever answers atLeft or atRight - never above or below - subtracting balloon chrome from the width budget precisely so BalloonImpl cannot silently re-point it there. The dwell, balloon and lifetime rules become SidePopupController, and HeaderPopupController is now a thin adapter that supplies chat placement, so its public API and SessionUi are untouched. ActiveList gains three seams, since a row is a renderer stamp rather than a component and none of this was reachable: - onHover, notified from the single setHovered funnel, so the paths that clear hover (mouse exit, setBusy, model rebuild, drag) all report it and cannot leave a popup pointing at a row that is gone - hoveredBounds/visibleBounds, because placement needs the row's edges and the list's visible extent; point() answers one anchor at a fixed inset for balloons that hang below a cell - onScroll, because trackBalloon only keeps action cells painted and does not close anything ActiveListBadge is unchanged here; the glyphs it renders landed earlier. AgentManagerPanel now binds onDirty, which nothing did before, so uncommitted counts reach the panel at all. It does not sync() on that callback: nothing on the row itself shows the number, so rebuilding every row per poll would be churn for an invisible change. Selection change deliberately does not hide the popup. The row does not move, so the balloon still points at the right place, and hiding would flicker it away on click only to reopen after the dwell.
gh, GitHub Apps, fine-grained PATs, and older GHE each word a refused reviewDecision/statusCheckRollup query differently, and everything the resolver does not recognise reads as "no PR here". Match the wordings the VS Code poller already handles so the scalar retry runs instead of a row losing a PR it has always shown. Latch the downgrade only for an unknown field: one resolver serves the whole backend, so latching a per-repository permission refusal stripped review and CI state from every other checkout until the IDE restarted.
The row height key snapshotted whole badges, and badge equality includes the click handler. The new review and CI badges build their handler while answering badges, so the key never matched and a sectioned list remeasured and relayouted every row on each status poll. Snapshot only the badge parts that can change a row's height.
display() builds the body before asking for a spot, and a null spot dropped the reference without disposing it. Chat bodies register editors on that disposable and a task card reparents its live view into it, so a popup that found no room leaked editors and could orphan the card's own body. Hand the body to hideAll so the existing teardown releases it.
…rows
Four things the indicators got wrong once several rows carried them:
- The failed and running CI badges used the saturated red and orange, which
turned a list of worktrees into a traffic light in both themes. Both now
use the muted tone of the same hue.
- The changes summary and the PR pill repeated in a tooltip what the row
already prints beside it. Both now say only what a click does.
- Title-line badges trailed each title's own width, so the review and CI
glyphs sat at a different x on every row. A list can now pin them to the
trailing edge the changes summary and PR pill below them end at, and the
worktree list does. Pills that label a title ('builtin', 'env') keep
hugging it, where the hover actions cannot cover them.
- The PR header showed only the state pill, so an editor tab and its row
disagreed about what a PR was waiting on. It now carries the same review
and CI glyphs, between the pill and the title.
The verdict glyphs move to a neutral ui/PrIcons so the chat session header
can reach them without depending on the Agent Manager package.
The popup put state, verdict glyphs, title, and both diff summaries on a single line, so the title was squeezed by counters that had nothing to do with it. PrHeaderView can now stack: state, verdicts and title on the first line, then the full changes row under a rule. The review and CI lines the popup already had follow it, so every fact owns a line. Double the width cap while stacking. It is only a cap -- the popup asks for the width its content needs and the side geometry trims that to the room beside the row, so a narrow window still gets a narrow popup rather than one re-pointed above the list.
The popup title was a SimpleColoredComponent, which cannot wrap, so a conventional commit subject was cut off mid-word — in the one place that exists to show it in full. The stacked header renders the title in a text area that wraps, giving up the grayed number fragment for it, and the state pill and verdict glyphs pin to the top of the line so they stay on the first row of a title that now spans several. The running dot also read as heavier than the passed and failed badges it sits in a column with, since it is a bare disc with no glyph inside it to lighten the shape. It loses a third of its radius; passed and failed keep theirs.
Reverts the wrapped title. The row popup opts into the horizontal scrollbar HeaderPopupBody already supports, so a title past the width cap is reached by scrolling sideways and the header stays one line. The state pill and verdict glyphs keep their top alignment.
…width cap A sideways-scrolling popup body now keeps a band above and below the viewport and claims a row for the bar when the content is actually wider than the width the popup settled on. Without the reservation the bar ate into the viewport and the body grew a vertical scrollbar it did not need. Bodies that never scroll sideways keep their exact content height. The row popup's width cap doubles again, which is still only a cap: the side geometry trims it to the room beside the row.
A popup asked for the width of its widest child, which is right for a column and wrong for a row: the PR header carries a state pill, verdict glyphs, a title, and a toolbar side by side, so the popup settled well short of the room it had and clipped its own title. Take whichever is wider, the widest child or the width the body's own layout reports; the max width is still the ceiling either way. Double the band a sideways-scrolling body keeps above and below itself, so the first and last line no longer sit against the balloon edge.
# Conflicts: # packages/kilo-jetbrains/frontend/src/main/kotlin/ai/kilocode/client/agentManager/AgentManagerPanel.kt
Neither the version validator nor build-version.sh accepted anything past -rc.N, so a local build off an unreleased commit had to borrow a release version and produce a ZIP indistinguishable from it. Both now allow SemVer build metadata (7.1.3-rc.1+8d0c4147). Release builds are unaffected: they take the version from a jetbrains/v tag, which never carries a suffix, and the changelog lookup already ignores a value it cannot parse as a release.
…badges A rate-limited gh was classified as OK, which the resolver reads as "this checkout has no pull request". That was the worst of both: every badge went blank for up to an hour with no reason given, and it cost the most calls to do it, because a lookup that should stop at the first refusal instead walked its whole strategy ladder against a limit that would refuse the rest too. GhAvailability gains RATE_LIMITED, recognised from both the primary and the secondary wordings in the pull-request path and in the auth probe. From there: - the resolver stops at the first refusal, and skips the scalar-field retry, which the same limit would refuse anyway - prStatus already returns before its per-worktree fan-out on any non-OK verdict, so the expensive part is skipped entirely - the backend holds the verdict for a minute rather than three seconds, so several panels asking at once cannot turn it into a probe each - the coordinator polls every five minutes while it stands, and picks the recovery up on its own - the rows and the chat dock keep the pull requests they already resolved, since a refusal is not evidence that a PR went away, and the banner says why they stopped updating A return from an absence now has to clear the throttle it bypasses before it forces a lookup, because that path spends a call per worktree. The gh availability probe keeps the shorter bar -- it costs one command.
Rows are dense, and neighbours are only ever passed over on the way to the one the pointer wants, so the transcript card's dwell flashed a popup for each row crossed. The dwell is now a constructor option and the worktree list takes twice the default; transcript cards, which the pointer goes to on purpose, keep theirs.
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
Fixes #
No tracked issue; this follows up on a code-review discussion about the gh/PR focus-sync feature (
GhStatusCoordinator,WorktreeStatusService) landed in a prior PR.Context
Focusing the IDE frame or switching tool-window tabs triggers an out-of-band gh-availability/PR-status sync so authorizing
ghor merging a PR elsewhere shows up without waiting for the poll. That sync was drop-on-conflict: if a probe was already running or the last one finished within the last 3s, the event was discarded outright — leaving a stale verdict standing until the next scheduled poll (up to 30s for gh availability, 120s for PR state). Separately,prStatushad no way to bypass the backend's own 90s cache, so even a forced frontend refresh could still serve PR data from before the event that triggered it.This PR makes focus/tab-switch syncs responsive to real absences without letting a burst of events (rapid alt-tabbing, clicking between tabs) overwhelm the backend.
Implementation
GhStatusCoordinator): a sync that can't run right now (busy, or inside the throttle window) is folded into a single heldpendingsync and answered by one trailing probe at the end of the window, rather than discarded. A burst of N events now costs at most one extra probe per throttle window instead of losing the request. A sync with no freshness requirement is discarded if a probe just succeeded (that answer already covers it); a failed probe never counts as satisfying one.Awayhelper, shared byGhStatusCoordinatorandWorktreeStatusService): work scales with how long the IDE was actually unfocused — nothing for a transient dialog/popup (< 1.5s, mirrors the platform's ownapplication.deactivation.timeoutpredicate), the ordinary throttled sync for a quick window switch, and a cache-bypassing probe once the absence is long enough (>= 10s) to plausibly contain an external change (gh auth login, a merged PR).applicationDeactivatedcallback rather than the platform's debounceddelayedApplicationDeactivated, because the latter takes ajava.awt.Windowthat can't be constructed under the headless JVM property IntelliJ platform tests run with.Away.REAL(1.5s) reapplies the same debounce predicate lazily at return time instead, giving the same filtering with noWindowdependency and no proactive timer.maxAgeceiling on the RPC (KiloWorktreeRpcApi→ghStatus/prStatus/branchStatus, backed by a new top-levelusable(time, now, ttl, maxAge)in the backend impl): lets an event-driven caller demand a fresher answer than the backend's cache would otherwise serve.maxAgeonly tightens the existing TTL, never extends it, so no caller can pin stale data past the backend's own limit. Passing the absence length (not0) is deliberate — a lookup that happened during the absence (e.g. another panel's poll) is still valid; only entries predating the departure are rejected.force = truereachable on every return from a long absence exposed a pre-existing gap —loadPr()replacedprJobwithout checking whether one was still running. SinceforcebypassesPR_THROTTLE, a user leaving and returning faster than a lookup completes could stack them without bound, and the semaphore bounding each lookup'sghfan-out is created per call, so overlapping lookups multiply the subprocess count rather than answering sooner (each also pinning aDispatchers.IOthread on the backend'sghLock). Now skipped while one is in flight. The pre-existinggenerationguard already prevented stale publishes, so this was wasted work rather than wrong data.KiloToolWindowFactory) — Chat has no gh-dependent UI to refresh, so syncing on the way out was wasted work.AgentManagerPanel's worktree-create path now passesmaxAge = 0, fixing a latent bug where a freshly created worktree's PR badge could be served from a cache entry that predated the worktree's existence.Concurrency invariants worth a reviewer's attention: the gh path checks
busyfirst, unconditionally — before themaxAgethrottle-bypass — so at most one probe is ever in flight and cost is bounded by probe duration rather than event rate. The PR path now has the equivalent guard. Neither path uses a blocking primitive on the frontend (noinvokeAndWait,runBlocking, orsynchronizedanywhere in the touched package), and all I/O runs insidecs.launchoff the EDT.No visual changes — this is internal event-coalescing and cache-freshness plumbing.
Screenshots / Video
N/A — no UI changes.
How to Test
Manual/local verification
./gradlew typecheckfrompackages/kilo-jetbrains/— clean.bun turbo typecheckfrom repo root (runs on every push via pre-push hook) — clean across all packages../gradlew test) run repeatedly: 292 classes / 4422 tests / 0 failures.bun run script/check-opencode-annotations.ts --worktree— no shared upstream files touched, nothing to check.bun run script/check-md-table-padding.ts— clean.KiloBackendChatManagerTestmock-HTTP-server test,WorktreeSessionEditorManagerTesteditor-lifecycle test) — both pre-existing and unrelated: each passes in isolation, and the affected package was re-run three consecutive times clean.(All verification above performed by the agent; no manual sandbox/IDE run was done for this change since it's event-timing logic covered by deterministic unit tests using
TestUiTimersand fake activation events, not something a visual sandbox pass would add confidence to.)Reviewer test steps
packages/kilo-jetbrains/, run./gradlew :frontend:test --tests 'ai.kilocode.client.agentManager.worktree.AwayTest' --tests 'ai.kilocode.client.agentManager.worktree.GhStatusCoordinatorTest' --tests 'ai.kilocode.client.agentManager.worktree.WorktreeStatusServiceTest'and confirm all pass../gradlew :backend:test --tests 'ai.kilocode.backend.rpc.KiloWorktreeRpcApiImplTest'and confirm all pass, including the newusable/cache-freshness cases.prJob?.isActiveearly return inWorktreeStatusService.refreshPrand re-run step 1 —test a forced refresh does not stack a second lookup on a running oneandtest activation while a lookup runs does not stack a second oneshould both fail../gradlew test) to confirm no regressions elsewhere.Blocked checks and substitute verification
Checklist
Get in Touch