close
Skip to content

fix(sidebar): preserve relevance order in active-session search results - #2299

Merged
kyleseaman merged 1 commit into
kirodotdev:mainfrom
rubencu:fix/sidebar-search-relevance-order
Aug 9, 2026
Merged

fix(sidebar): preserve relevance order in active-session search results#2299
kyleseaman merged 1 commit into
kirodotdev:mainfrom
rubencu:fix/sidebar-search-relevance-order

Conversation

@rubencu

@rubencu rubencu commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Problem

The sessions sidebar's active-session search ("Search sessions…") discards the backend's relevance ranking. /api/sessions/search deliberately ranks results — search_sessions in history.py gives title hits a 10x field boost (_TITLE_BOOST) precisely so a session named after the query outranks sessions that merely mention it in message content. But the sidebar collapsed the ranked response into a membership Set and re-sorted the matches with comparePinnedThenSort (pinned first, then date-desc by default).

Observable defect: type overl while a session titled "managing kiro crew session overload" exists. During the 250ms debounce the client-side title-substring fallback shows it as the only match — then the server response lands and the pin/date re-sort buries it below every fresher session whose body happens to contain "overl…" (overly, overlap, …). The best match visibly "falls down" the list mid-keystroke.

Fix

useDebouncedSessionSearch's slots transform now returns a Map<slotKey, rank> (first-wins on canonical-key collisions) instead of a Set, and filteredSlots orders by that rank while a content search is active — falling back to the existing pinned+sort ordering otherwise. Filtering semantics are unchanged (same membership test, same substring fallback while the response is in flight or below SEARCH_MIN_CHARS).

This is the exact companion of #2226, which fixed the same bug in the Older Sessions lane (sortedHistory re-sorting relevance-ranked results by date). The command palette's sessions provider already preserves backend order and biases title matches upward — the three surfaces now agree.

Pinned rows intentionally do NOT float above search results: pinning is a reachability promise for browsing (per sessionOrder.ts's own comment), not a ranking hint inside an explicit search — the regression test locks this with a pinned decoy.

Tests

  • New ChatSidebar.slotSearchOrder.test.tsx, mirroring ChatSidebar.historySearchOrder.test.tsx (fix(sidebar): preserve relevance order in history search results #2226): a ranked mock response whose title match is the oldest and unpinned slot, with a pinned decoy and a fresher decoy ranked below it. Asserts rendered DOM order equals backend order, plus a fixture-validity guard proving the old pin+date sort would have inverted the fixture.
  • Mutation-checked: the test fails against the unfixed ChatSidebar.tsx.
  • tsc -b clean, eslint 0 errors, full vitest suite green apart from the pre-existing catalogParity ko failure (29 missing pages.sessionStorage.* keys), which reproduces identically on clean origin/main.

@rubencu
rubencu requested a review from a team August 9, 2026 02:09
@rubencu
rubencu requested a review from a team as a code owner August 9, 2026 02:09
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 9, 2026
@rubencu
rubencu force-pushed the fix/sidebar-search-relevance-order branch from be40d13 to d216bd7 Compare August 9, 2026 11:20
@rubencu

rubencu commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

CI triage — the Frontend Tests / Coverage Gate failures on be40d13 were main-side, not from this PR. The failing test was src/i18n/catalogParity.test.ts > ko (29 missing + 36 stray pages.sessionStorage.* keys). This PR touches only ChatSidebar.tsx and one test file — no i18n catalogs. The ko catalog was resynced upstream in #2306 (841e8b4, merged ~3h after this PR's failing run), so the merge-ref build picked up the broken catalog from main. I rebased onto latest main (clean, single commit d216bd7) and re-ran the full frontend gate locally: tsc clean, eslint clean, and the full vitest suite with TZ=UTC green (858 files, 11404 tests passed — including catalogParity ko and this PR's regression test).

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 9, 2026
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Opus 5 Review (fork) — ✅ no blocking findings

Reviewed d216bd76ebd85a57297bc5400b5927e3c8a8a75f via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] d216bd7

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed d216bd76ebd85a57297bc5400b5927e3c8a8a75f via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] d216bd7

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Advisory design-level review of d216bd76ebd85a57297bc5400b5927e3c8a8a75f via the fork AI-review pipeline — updated in place on each push; does not block merge.

Design-Verdict: PASS

Targeted fix that restores an intentional backend contract, aligned with the #2226 companion and command palette — all three search surfaces now agree.

[DESIGN-REVIEWED] d216bd7

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

Advisory UX-level review of d216bd76ebd85a57297bc5400b5927e3c8a8a75f via the fork AI-review pipeline — updated in place on each push; does not block merge.

I have what I need. One real UX regression surfaced: the flat-view lane renders date-segment headers (Today / Last 7 Days) over filteredSlots whenever sortKey is a date sort (ChatSidebar.tsx:3327), with no searchActive guard — unlike the Older Sessions lane, which explicitly disables segments during search because "segments only make sense when the list is date-ordered." This PR makes the active list relevance-ordered during search, so those headers will now repeat and interleave.

UX-Verdict: CONCERNS

Relevance ordering lands, but flat view still paints date-segment headers over the now non-date-ordered results — headers repeat and mislead.

Watch

  • The new .sort((a, b) => searchRanked ? …) makes filteredSlots relevance-ordered during search, but the flat lane's segment logic (isDateSort = sortKey === 'date-desc' || …, ChatSidebar.tsx:3327) has no search guard — a flat-view user typing a query now sees interleaved/duplicate "Today / Last 7 Days / Today" headers implying a grouping the list no longer has. The history lane guards exactly this (showSegments = !searchActive && …, line 3727–3731). Every flat-view search under a date sort × misleading structure × persistent — fix by suppressing segments (or segOf) when the slot search is active, mirroring the history lane.

Suggestions

  • With ranked order active, the sort menu still shows the user's pick checked (ChatSidebar.tsx:3209) while being ignored; a muted "sorted by relevance" hint at the top of results would explain why pinned rows stopped floating.

[UX-REVIEWED] d216bd7

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 9, 2026
@kyleseaman
kyleseaman merged commit 76569c9 into kirodotdev:main Aug 9, 2026
49 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 9, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants