Repository navigation
feat(mobile): search input also filters the Active now tray - #4823
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummarySince the last review, the only change is a new workflow learnings doc (no functional code changes); the previously reviewed search-narrowing logic in the pinned tray and body-model guard remains correct with no high-confidence issues found in changed lines. Files Reviewed (6 files)
Previous Review Summary (commit a1a31c1)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a1a31c1)Status: No Issues Found | Recommendation: Merge Executive SummaryReviewed the client-side search-narrowing changes to the pinned "Active now" tray and the body-model guard; the substring-match semantics, conjunction with platform/project filters, and the new body-model branch are correctly implemented and covered by matching tests, with no high-confidence bugs found in changed lines. Files Reviewed (5 files)
Reviewed by claude-sonnet-5 · Input: 20 · Output: 4.3K · Cached: 369.7K Review guidance: REVIEW.md from base branch |
|
(bot) @kilocode-bot please review |
There was a problem hiding this comment.
Review — tray search filtering
Reviewed the five changed mobile files plus the surrounding consumers (session-list-content.tsx, session-list-content-surface.ts, use-session-search-input.ts, use-agent-sessions.ts) and the server search (cliSessionsV2.search). The core implementation is clean and the wiring is correct; below are three substantive findings and some coverage nits.
What checks out
selectPinnedActiveSessions— needle is trimmed/lowercased once outside the predicate,includesontitle/idis a literal substring test (so%,_,\match literally, matching the router'sposition()approach), the earlyreturn falsesits before the platform/project logic so the conjunction semantics are unchanged, andActiveSession.titleisz.string()(non-nullable) so.toLowerCase()can't blow up on a missing title. Empty/whitespace-only ⇒needle.length === 0⇒ no narrowing. Making the param required is the right call: it forces every call site through typecheck, which is a stronger guarantee than the@ts-expect-errorprobe it replaces.- Screen wiring —
effectiveSearchQueryis exactly the value thesectionsmemo already consumes, so tray and history read the same committed query; theuseMemodep list (activeSessions,effectiveSearchQuery,platformFilter,projectFilter) is complete. hasPinnedActiveis now search-dependent, so I checked the downstream consumers for fallout and found none:selectSessionListContentSurface'sfull-screen-errorbranch is gated on!hasAnySessions, andhasAnySessionsis still computed from the unfilteredactiveSessions/storedSessions, so a search failure whose text filters the tray to empty can't blank the screen.showInlineErrorcan now go false in that state, but the error signal is still carried byquery-error-empty, so nothing is lost.- Body-model guard — ordered after the
isErrorbranch (Retry/clear-searchpreserved on failure), returns the same shape as the otherrender-listsites, andsession-list-content.tsxalready suppressesListEmptyComponentforrender-list, so the tray-only state renders the tray with no contradictory copy and no stray footer. "Clear search" is still reachable via the in-field X.
1. The trim is a divergence from the server, not parity (minor, but the JSDoc claims parity)
buildAgentSessionSearchInput passes search_string: options.searchQuery verbatim, and the router does search_string.toLowerCase() — no trim on either side. The helper trims. Consequences:
" login"→ tray matches a title starting withLogin…; the server search does not. The tray is strictly more permissive than history for any query with surrounding whitespace (easy to produce on mobile: leading space from the space bar, trailing space from autocorrect).- Whitespace-only (
" ") →isSearchingis true (searchQuery.length > 0, untrimmed), so the server runs a literal three-space substring search (probably empty history) while the tray is not narrowed at all — and the new guard then suppresses the "No sessions match" copy, so the user sees the full tray with no feedback that a query is active.
Neither is a hard bug, but the cheapest fix is one trim at the commit boundary (the debounce controller, or buildAgentSessionSearchInput) so isSearching, the server needle, and the tray needle all derive from the same trimmed string; keep the helper's trim as defense in depth. Otherwise I'd soften the JSDoc's "same semantics as the server-side history search" to call out the trim.
2. A search-matching active session can now become invisible (real, narrow regression)
sections is built from excludeActiveFromGroups(storedGroups, activeSessionIds), and activeSessionIds covers all active sessions regardless of the tray filter. So history hides an active session by id, while the tray now decides by the live heartbeat title. Those two titles can drift: activeSessions.list takes the title straight from the session-ingest worker and enriches only platform/timestamps, while renameSession writes the DB row (and the stored cache) — the live tray title is never updated. Sequence:
- Session is active; user renames it to "Payments refactor" from mobile/web. Tray keeps showing the old heartbeat title.
- User searches
payments. The server matches the stored row → it is dropped from history because its id is active. The tray drops it because the live title doesn't contain the needle. - Net result: a session that the search matched renders nowhere — and with an empty tray the body falls to
filtered-empty, so the user is told "No sessions match".
Before this PR the tray always showed it, so the session was at least reachable. Same shape applies whenever the live title is empty/placeholder while the stored title carries the searchable text (RemoteSessionRow renders "Untitled session" for title === '', which is itself unsearchable — consistent with the server's COALESCE(title, ''), but worth pinning in a test). Two ways out, both small: exclude only the pinned ids from history (excludeActiveFromGroups(storedGroups, new Set(pinnedActive.map(s => s.id)))), or match the tray against the stored title as well when one is known for that id (the screen already builds organizationIdBySessionId from storedSessions, so the lookup is free). The first is a one-liner and also self-heals any future tray-narrowing rule.
3. Guard asymmetry vs the filter-only branch
The PR is explicit that filter-only narrowing keeps filtered-empty under a populated tray, and I agree that's out of scope for the behavior change — but the justification in the new comment ("tray matches are matches, so the body never claims 'No sessions match' beneath them") applies verbatim to platform/project filters, since selectPinnedActiveSessions applies those filters to the tray too. The tray under a filter is, by construction, a set of matches. So the codebase now encodes two contradictory answers to the same question, and the comment explaining the search side reads as an argument for changing the filter side. Fine to leave, but I'd track it as a follow-up rather than let the comment imply the filter behavior is principled.
Test coverage
The 8 new helper tests are well chosen — in particular the 100% case genuinely discriminates literal-vs-wildcard ('1000 done' would match under LIKE semantics). Gaps:
- No test for whitespace around a real term (
' login '), which is the trim behavior that actually diverges from the server (finding 1). Whitespace-only is covered; the interesting case isn't. - No test for
title: ''(tray shows "Untitled session"; searchinguntitledmust not match). - No test that the tray consumes
effectiveSearchQueryrather than the rawsearchQuery. The "unfiltered tray while the first fetch is pending" semantic is only covered by the E2E run, and it's exactly the kind of one-token detail a future refactor can silently flip. A tiny derived-value helper (selectEffectiveSearchQuery(isSearching, isPending, searchQuery)) would make it unit-assertable, if you think it's worth the indirection. - Body-model tests cover the new guard plus both non-firing neighbours (filter-only, query-error) — good; nothing missing there.
Nits
- The unrelated
.kilo_workflow/learnings/…doc rides along in this PR; history also carries twomainmerges. Not blocking if that matches your workflow, but a rebase would keep the diff to the feature. session-list-helpers.test.tsline ~117 comment now runs past the file's wrapping width used by its neighbours — cosmetic.
Summary: finding 2 is the only one I'd want addressed before merge (one-line fix available); finding 1 is a small semantics/doc mismatch; finding 3 is a follow-up. Everything else looks solid.
|
The review's already posted on the PR. Key takeaway to flag to the author:
Nothing further to do unless you want me to spawn a follow-up Cloud Agent session to actually fix #1/#2. |
|
(bot) Triage of the four takeaways, each verified against the code on the current head (
No code changes result from this round. The posted summary (No Issues Found | Recommendation: Merge, covering all 6 PR files including the learnings doc) reviewed exactly the diff still on this PR — the head moves since ( |
|
(bot) @kilocode-bot please review the current head 01b9434 — since your last summary the branch only merged latest main (the PR diff is unchanged: the same 6 files you approved). |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe PR diff is unchanged since the last review. Verified at head The single main commit absorbed since the last review ( Prior-round discussion items (trim divergence, renamed-active-session staleness, filter-only branch symmetry, test gaps) were triaged by the author and are unaffected by the merge; they remain either unreachable given the trim-at-commit-boundary design or pre-existing/out-of-scope follow-ups. No new issues found. Files Reviewed (6 files — identical to previous review)
|
|
Review already posted: no issues found, diff unchanged from the previously approved head, verified byte-identical via patch-id and blob hashes. Recommendation stands: Merge. No further action needed. |
Summary
What: The Agents session-list search input now also filters the pinned "Active now" tray, client-side, with the same semantics as the server-side history search: a case-insensitive literal substring match on the session's
titleORid, in conjunction with the existing platform/project filters.Why: The tray previously ignored the search text by deliberate design (PR #4626). A user explicitly asked to reverse that decision: when they search, they expect every session list on the screen — including the pinned active tray — to narrow. The code's own comments anticipated this reversal.
How:
selectPinnedActiveSessionsgains a requiredsearchQuery: stringparam. The needle is trimmed/lowercased once, then applied as the first check inside the existing filter (String.includesontitle/id— literal substring, LIKE wildcards match literally, mirroring the server'sposition()search). The platform/project logic below is unchanged.AgentSessionListScreenpasseseffectiveSearchQuery(the same committed, debounced value the history data source consumes), so the tray and history narrow together.selectSessionListBodyModelgains one guard: while searching with an empty history but a populated tray, it returnsrender-listinstead offiltered-empty— tray matches are matches, so the body never claims "No sessions match" beneath a populated tray (that copy would be factually wrong on the feature's primary happy path: searching an active session's own title empties history by construction, since its stored row is excluded). The query-error branch (Retry CTA) and the filter-only branch (pre-existing deliberate decision) are unchanged.@ts-expect-errorassertion is replaced by 8 positive search-semantics tests).Verification
E2E on iOS simulator (iPhone 17 Pro Max, iOS 26.5, Maestro) against this worktree's local stack —
VERIFICATION PASSED.(round r3; run on merged headdd976cf01, and the later main-merge to the current head touched no mobile or owned files — clean merge, zero path overlap). Data: live active session "Bicycle pump haiku generation" plus stored "Zucchini haiku generation" and "New session". All five flows passed:bicycle(title substring, lowercase vs a capitalized title) → the tray row stays, ACTIVE NOW visible, and no "No sessions match" copy renders beneath the tray (tray-only-match state, AC1+AC3).qxzjkv→ the tray hides; "No sessions match / Try a different search term." with a "Clear search" CTA appears (AC2, empty state).zucchini→ the stored list narrows to "Zucchini haiku generation" only, "New session" hidden, tray hides correctly — history search unchanged (AC5, regression).0570dbf84→ the tray row stays (id-matching half of AC1).Feature-state matrix: happy PASS, tray-only-match PASS, empty PASS; retryable-unhappy skipped (pre-existing machinery untouched by this change; a deterministic search failure would require fault injection); non-retryable-unhappy N/A (structurally impossible — a pure client-side filter over an already-loaded list has no terminal failure mode). Post-run the worktree was byte-identical to baseline and the simulator/slot were released.
Automated (rerun on the final merged head):
pnpm format,pnpm typecheck,pnpm lint(0 warnings / 0 errors),pnpm check:unused, and the mobile vitest suite (277 files / 2346 tests, incl. the newsearch ruleshelper tests and the new body-model guard tests) all pass inapps/mobile.Visual Changes
No visual-design changes — tray, rows, and empty states reuse existing components; only which sessions appear changes. Simulator screenshots from the passing E2E run (tray before search, tray with a matching query, no-match empty state, restored tray) were captured and verified during verification.
Reviewer Notes
selectPinnedActiveSessions(parity with the server's case-insensitive substring search), and the single new body-model guard (isSearching && hasPinnedActive→render-list).keepPreviousData— consistent with the history list's own stale-while-refetching design; true displayed-row lockstep was rejected as unjustified state for a sub-second window. Unicode case-folding beyond ASCII parity with Postgreslower()is out of scope.cliSessionsV2.renameupdates only the stored row — so renaming an active session never updates the title shown in the tray while it stays active. Consequence under this PR's tray filtering: searching the new name matches nothing visible (the stored row is excluded from history while active; the tray still shows the old heartbeat title). The root fix is backend: propagate the renamed title into the live session state, or overlay the stored title in theactiveSessions.listenrichment. Full triage in the PR comments.