Repository navigation
Conversation
…ilter Adversarial review round on Kilo-Org#7040: - the live list answered every organization plus personal, so switching to an org still showed another scope's Remote rows; scope it like the stored list - a session another connection takes over was kept alongside its new row, rendering it twice; replace it in place - the wire 'platform' field is the client the CLI runs in, not the origin, so it must not claim a platform filter - 'other' has to mean every origin outside KNOWN_PLATFORMS, as the stored query defines it, and the search has to match only title and id - keep the cached organization attribution, where null means personal Refs Kilo-Org#7034, Kilo-Org#6813
Review round three on Kilo-Org#7040: the connection payload carries no organization id, so scoping the live list client-side leaked another scope's rows into a personal view and hid a new row in an org view until the next poll, and the scoped query key silently broke the deletion cache write. The live list stays unscoped, as before; scoping it needs an org id on the wire. Also import the live row type from the hook instead of duplicating it, and make the takeover test fail on the old merge. Refs Kilo-Org#7034, Kilo-Org#6813
|
Sweep notes: of the ten open issues authored by @maphew in Kilo-Org/cloud, eight already have a PR (6994→#6997, 6663→#6817, 6595/6167/5901/5900→#6800/#6978, 6248→#6786, 6186→#6784). #7034 and #6813 had none and are the same defect area — the Remote (live) section of the sidebar — so this PR fixes both. Adjacent gap found while working on this, left for a follow-up: the live list is not organization-scoped, so the Remote section can show another scope's rows. Fixing it needs an organization id on the connection payload from |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| /** Matches what the stored list matches: the session id and its title. */ | ||
| function matchesLiveSearch(session: LiveSidebarSession, needle: string): boolean { | ||
| return session.title.toLowerCase().includes(needle) || session.id.toLowerCase().includes(needle); |
There was a problem hiding this comment.
WARNING: The live search is narrower than the stored-list search this claims to mirror
The server search procedure matches title, session_id, git_url, git_branch, PR title, PR number, and worktree name (apps/web/src/routers/cli-sessions-v2-router.ts:1224-1247), but this client-side matcher only checks title and id. useSidebarSessions swaps to the server search results whenever the query is non-empty (useSidebarSessions.ts:650), so with a repository or branch search a not-yet-ingested Remote row is hidden here even though the stored list shows the identical session. This regresses the earlier matcher in this PR (which searched title, id, gitUrl, gitBranch) and contradicts the code/test claim that the stored list does not search the repository (see live-sidebar-sessions.test.ts:116). The server also strips a leading # before matching (cli-sessions-v2-router.ts:1215); the client does not. Consider matching gitUrl/gitBranch again, or correcting the stated parity.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe six changed web files are clean at HEAD ( The two prior warnings are resolved at HEAD. The heartbeat/list merge ( Files Reviewed (6 files)
Previous Review Summaries (6 snapshots, latest commit 1c6cc96)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 1c6cc96)Status: No Issues Found | Recommendation: Merge Executive SummaryThe six changed web files are clean at HEAD ( The prior warnings are fixed at HEAD. The heartbeat/list merge ( Files Reviewed (6 files)
Previous review (commit da2c542)Status: No Issues Found | Recommendation: Merge No new findings. The six changed web files are blob-identical to the previously reviewed revision ( Files Reviewed (6 files)
Previous review (commit 9c064b6)Status: No Issues Found | Recommendation: Merge No unresolved issues in the six changed files. The two prior warnings are fixed at HEAD: Files Reviewed (6 files)
Previous review (commit 31146bb)Status: No Issues Found | Recommendation: Merge No unresolved issues in the six changed files. The prior empty-needle warning is fixed: a query that trims/strips Known gap (not introduced here): the live list stays unscoped by organization, so the Remote section can show another scope's rows. This is pre-existing and explicitly documented in the PR. Files Reviewed (6 files)
Previous review (commit f94472f)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous review (commit 0b8c968)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud The merge logic ( Reviewed by deepseek-v4.1-flash · Input: 85.9K · Output: 15.7K · Cached: 1.4M Review guidance: REVIEW.md from base branch |
…ilter Adversarial review round on Kilo-Org#7040: - the live list answered every organization plus personal, so switching to an org still showed another scope's Remote rows; scope it like the stored list - a session another connection takes over was kept alongside its new row, rendering it twice; replace it in place - the wire 'platform' field is the client the CLI runs in, not the origin, so it must not claim a platform filter - 'other' has to mean every origin outside KNOWN_PLATFORMS, as the stored query defines it, and the search has to match only title and id - keep the cached organization attribution, where null means personal Refs Kilo-Org#7034, Kilo-Org#6813
0b8c968 to
f94472f
Compare
Review round three on Kilo-Org#7040: the connection payload carries no organization id, so scoping the live list client-side leaked another scope's rows into a personal view and hid a new row in an org view until the next poll, and the scoped query key silently broke the deletion cache write. The live list stays unscoped, as before; scoping it needs an org id on the wire. Also import the live row type from the hook instead of duplicating it, and make the takeover test fail on the old merge. Refs Kilo-Org#7034, Kilo-Org#6813
| /** Matches what the stored list matches: id, title, repository, and branch. */ | ||
| function matchesLiveSearch(session: LiveSidebarSession, needle: string): boolean { | ||
| const normalizedNeedle = needle.startsWith('#') ? needle.slice(1) : needle; | ||
| if (normalizedNeedle.length === 0) return true; |
There was a problem hiding this comment.
WARNING: A #-only search shows every Remote row instead of none
filterLiveSidebarSessions trims the query before calling this matcher, so searchQuery: '#' arrives here as needle === '#'. This line strips the leading # to an empty string and returns true, so every live row passes the search clause. The stored-list search deliberately does the opposite: cli-sessions-v2-router.ts:1220-1221 pushes a literal false predicate when the needle is empty after the same strip, matching zero stored rows. A sidebar search of # therefore shows all Remote rows while the stored list shows none. Return false for the empty needle; the outer !needle check in filterLiveSidebarSessions already covers a genuinely empty query.
| if (normalizedNeedle.length === 0) return true; | |
| if (normalizedNeedle.length === 0) return false; |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
f94472f to
31146bb
Compare
…ilter Adversarial review round on Kilo-Org#7040: - the live list answered every organization plus personal, so switching to an org still showed another scope's Remote rows; scope it like the stored list - a session another connection takes over was kept alongside its new row, rendering it twice; replace it in place - the wire 'platform' field is the client the CLI runs in, not the origin, so it must not claim a platform filter - 'other' has to mean every origin outside KNOWN_PLATFORMS, as the stored query defines it, and the search has to match only title and id - keep the cached organization attribution, where null means personal Refs Kilo-Org#7034, Kilo-Org#6813
Review round three on Kilo-Org#7040: the connection payload carries no organization id, so scoping the live list client-side leaked another scope's rows into a personal view and hid a new row in an org view until the next poll, and the scoped query key silently broke the deletion cache write. The live list stays unscoped, as before; scoping it needs an org id on the wire. Also import the live row type from the hook instead of duplicating it, and make the takeover test fail on the old merge. Refs Kilo-Org#7034, Kilo-Org#6813
maphew
left a comment
maphew
left a comment
There was a problem hiding this comment.
Rebased onto main (4 conflicts, all in ChatSidebar.tsx) and reviewed the result. Three things worth recording, plus one correction to the description.
1. Conflict resolution: main's Remote ordering is preserved, not dropped
main (#6938, 74cc4d6b8) orders the Remote section attention-first → working → id. The filter change replaced that block wholesale, so taking the incoming side would have silently dropped shipped behaviour. The two fixes compose rather than compete — heartbeats no longer move rows, and the order is then decided once in the component — so the rebase keeps both: filter first, then order (43821093b).
2. Fixed: a query that trims away to nothing showed every live row
cli-sessions-v2-router.ts strips one leading #, then resolves an empty needle to zero rows, because position('' in …) would otherwise match every stored row. filterLiveSidebarSessions matched every live row for the same input, so typing a bare # — or whitespace — emptied the stored list while the Remote section stayed fully visible: the exact class of bug this PR exists to fix. Normalization (trim, lower-case, one leading #) now happens once per call, and an empty needle matches nothing, mirroring the stored search (31146bb17).
3. Fixed: the platform selection was rebuilt per row
matchesLivePlatformFilter called platformFilterValues and allocated a Set for every row, on every filter pass — and the pass re-runs on every heartbeat. Hoisted to one Set per call, with the "no selection means no filter" short-circuit preserved (31146bb17).
Correction to the description's known gap
The org-scoping gap is real, but narrower than the body states. activeSessions.list does carry a per-row organizationId — mapEnrichedHeartbeatSession sets it, null for a never-ingested row. What is missing is that the WebSocket heartbeat row carries none and ENRICHED_FIELDS does not preserve the cached value, so the id is dropped again on the next heartbeat. Client-side scoping is therefore already possible for rows that have been ingested; the wire change from UserConnectionDO.aggregateSessions is only needed for rows that have not.
Verification
oxlintandoxfmtclean; both changed suites pass (22 tests).apps/webtypecheck cannot complete in this environment:@kilocode/trpc's build fails on a missing@simplewebauthn/serverinstall. The identical failure reproduces on an unmodifiedmain, with the same error count and the same per-file distribution, so nothing here is attributable to this branch.- The full web jest suite needs the repository Postgres, unavailable here, so only the pure-unit suites ran. The
cloud-agent-nextsuites that fail without the DB-backed setup fail identically onmain(9 suites / 6 tests, before and after).
31146bb to
9c064b6
Compare
…ilter Adversarial review round on Kilo-Org#7040: - the live list answered every organization plus personal, so switching to an org still showed another scope's Remote rows; scope it like the stored list - a session another connection takes over was kept alongside its new row, rendering it twice; replace it in place - the wire 'platform' field is the client the CLI runs in, not the origin, so it must not claim a platform filter - 'other' has to mean every origin outside KNOWN_PLATFORMS, as the stored query defines it, and the search has to match only title and id - keep the cached organization attribution, where null means personal Refs Kilo-Org#7034, Kilo-Org#6813
Review round three on Kilo-Org#7040: the connection payload carries no organization id, so scoping the live list client-side leaked another scope's rows into a personal view and hid a new row in an org view until the next poll, and the scoped query key silently broke the deletion cache write. The live list stays unscoped, as before; scoping it needs an org id on the wire. Also import the live row type from the hook instead of duplicating it, and make the takeover test fail on the old merge. Refs Kilo-Org#7034, Kilo-Org#6813
…ilter Adversarial review round on Kilo-Org#7040: - the live list answered every organization plus personal, so switching to an org still showed another scope's Remote rows; scope it like the stored list - a session another connection takes over was kept alongside its new row, rendering it twice; replace it in place - the wire 'platform' field is the client the CLI runs in, not the origin, so it must not claim a platform filter - 'other' has to mean every origin outside KNOWN_PLATFORMS, as the stored query defines it, and the search has to match only title and id - keep the cached organization attribution, where null means personal Refs Kilo-Org#7034, Kilo-Org#6813
9c064b6 to
da2c542
Compare
Review round three on Kilo-Org#7040: the connection payload carries no organization id, so scoping the live list client-side leaked another scope's rows into a personal view and hid a new row in an org view until the next poll, and the scoped query key silently broke the deletion cache write. The live list stays unscoped, as before; scoping it needs an org id on the wire. Also import the live row type from the hook instead of duplicating it, and make the takeover test fail on the old merge. Refs Kilo-Org#7034, Kilo-Org#6813
…ilter Adversarial review round on Kilo-Org#7040: - the live list answered every organization plus personal, so switching to an org still showed another scope's Remote rows; scope it like the stored list - a session another connection takes over was kept alongside its new row, rendering it twice; replace it in place - the wire 'platform' field is the client the CLI runs in, not the origin, so it must not claim a platform filter - 'other' has to mean every origin outside KNOWN_PLATFORMS, as the stored query defines it, and the search has to match only title and id - keep the cached organization attribution, where null means personal Refs Kilo-Org#7034, Kilo-Org#6813
Review round three on Kilo-Org#7040: the connection payload carries no organization id, so scoping the live list client-side leaked another scope's rows into a personal view and hid a new row in an org view until the next poll, and the scoped query key silently broke the deletion cache write. The live list stays unscoped, as before; scoping it needs an org id on the wire. Also import the live row type from the hook instead of duplicating it, and make the takeover test fail on the old merge. Refs Kilo-Org#7034, Kilo-Org#6813
8627af8 to
1c6cc96
Compare
Live sessions bypassed the sidebar's platform, project, and search filters, so the Remote section kept showing CLI rows while only Cloud was selected, and every CLI heartbeat prepended its connection's rows, so two or more live connections reshuffled the section every few seconds. Filter the live rows with the same selections the stored list gets, keep the cached order when a heartbeat merges in, and preserve the enriched fields a wire row lacks. Refs Kilo-Org#7034, Kilo-Org#6813
…ilter Adversarial review round on Kilo-Org#7040: - the live list answered every organization plus personal, so switching to an org still showed another scope's Remote rows; scope it like the stored list - a session another connection takes over was kept alongside its new row, rendering it twice; replace it in place - the wire 'platform' field is the client the CLI runs in, not the origin, so it must not claim a platform filter - 'other' has to mean every origin outside KNOWN_PLATFORMS, as the stored query defines it, and the search has to match only title and id - keep the cached organization attribution, where null means personal Refs Kilo-Org#7034, Kilo-Org#6813
Review round three on Kilo-Org#7040: the connection payload carries no organization id, so scoping the live list client-side leaked another scope's rows into a personal view and hid a new row in an org view until the next poll, and the scoped query key silently broke the deletion cache write. The live list stays unscoped, as before; scoping it needs an org id on the wire. Also import the live row type from the hook instead of duplicating it, and make the takeover test fail on the old merge. Refs Kilo-Org#7034, Kilo-Org#6813
The heartbeat merge now leaves every row where it was, so the Remote section still needs the ordering main ships: attention first, then working, then id. Apply it after the live filter instead of instead of it, so a filtered sidebar keeps the order an unfiltered one has.
A query that trims away to nothing — a bare `#`, or whitespace — leaves the stored list empty, because the search resolves an empty needle to zero rows. The live filter returned every row for it instead, so the Remote section ignored the query the rest of the sidebar had applied. Hoist the platform selection out of the row loop at the same time: it was rebuilt per row, on every heartbeat.
1c6cc96 to
1fceafc
Compare
…section # Conflicts: # apps/web/src/components/cloud-agent-next/CloudSidebarLayout.tsx
Fix the Cloud Agent sidebar's Remote (live) section: stop the rows reshuffling on every CLI heartbeat, and stop them ignoring the sidebar's platform, project, and search filters.
Two open issues, one area, one change:
applyActiveSessionsHeartbeatprepended the heartbeating connection's sessions ahead of every other connection, so two or more live connections flipped the order on alternating heartbeats. The merge now keeps each row where it was and appends only ids the heartbeat introduces. Rows a heartbeat stops reporting are still dropped, and a row another connection takes over is replaced in place instead of being kept alongside its new row.filterLiveSidebarSessions, which mirrors the stored list's query and the mobile live-session filter:cloud-agentcoverscloud-agent-web,extensioncoversvscodeandagent-manager), andothermeans every origin outsideKNOWN_PLATFORMS, asaddCreatedOnPlatformConditionsdefines itnormalizeGitUrlthe server query usessearch arms end for a live row: its remaining arms need a pull request or a worktree,
and a row that is not in the stored list has neither
Supporting changes:
activeSessions.listenriches fromcli_sessions_v2— including the origin the new filter reads. The merge now keeps the cached title, origin, and timestamps, so a filtered sidebar does not drop and re-add rows on each heartbeat.CloudSidebarLayoutintolive-sidebar-sessions.ts, so the server query and the client filter cannot drift.Known gap, deliberately left alone: the live list is not scoped by organization, so the Remote section can show another scope's rows. Scoping it client-side is not possible — the connection payload carries no organization id — and scoping the query alone breaks
session-deletion.ts, which writes to the unscoped query key. Fixing it needs an org id on the wire fromUserConnectionDO.aggregateSessions.Verification:
apps/webtypecheck, oxlint, oxfmt, and thecloud-agent-nextjest suites (62 suites, 1640 tests) pass. The full web jest suite needs the repository Postgres, which is not available in this environment, so only the pure-unit suites were run. Three adversarial review rounds ran against the diff; the findings that mattered (theplatformfield being the client OS tag, theotherbucket, ownership takeover, and the org-scoping regressions) are fixed or reverted as described above.Rebased onto
main: the Remote ordering #6938 introduced composes with the filter ratherthan competing with it, so the rebase filters first and then orders attention-first. The
post-rebase review round also fixed a query that trims to nothing (
#, whitespace)matching every live row, and a per-row rebuild of the platform selection.
Refs #7034, #6813