Skip to content

feat: session-grouped search with sort toggle#200

Closed
tlmaloney wants to merge 1 commit intowesm:mainfrom
tlmaloney:feat/search-pane-refinement
Closed

feat: session-grouped search with sort toggle#200
tlmaloney wants to merge 1 commit intowesm:mainfrom
tlmaloney:feat/search-pane-refinement

Conversation

@tlmaloney
Copy link
Copy Markdown
Contributor

@tlmaloney tlmaloney commented Mar 19, 2026

Relates to #190.

Summary

  • Session-grouped results: FTS5 search now returns one result per session (not per message). The best-ranked matching message provides the snippet and scroll target; other matches in the same session are collapsed.
  • Relevance/Recency sort toggle: A two-button control above results lets users switch between FTS rank order and session recency (ended_at DESC). Sort is validated server-side before ORDER BY interpolation. Toggle is visible during loading and zero-results states.
  • New result row: agent color dot · session name (first line) · message snippet (second line, muted) · project · Xh ago · 8-char short ID (click to copy). Consistent styling for both search results and recent-session rows.
  • Session name in search scope: search matches display_name and first_message in addition to message content, via a UNION with a LIKE/ILIKE session-name branch. Name-only matches return ordinal = -1; the frontend navigates to the session without scrolling. Snippet correctly shows the matching field (display_name vs first_message).
  • PostgreSQL parity: pg serve path updated with DISTINCT ON (session_id) grouping, is_system column added to the PG schema (SchemaVersion bumped to 2), ILIKE session-name branch in the UNION CTE, and an actionable error message when the schema needs migration (run 'agentsview pg push').
  • FTS5 duplication fix: outer JOIN messages_fts includes WHERE messages_fts MATCH ? to prevent segment-level row duplication.
  • Stable timestamp ordering: julianday() normalization in SQLite ORDER BY avoids lexicographic misorderings from variable RFC3339Nano fractional-second precision.

Test plan

  • make test — all Go unit tests pass
  • make test-postgres — PG integration tests pass (requires Docker)
  • cd frontend && npx vitest run — all TypeScript/Svelte unit tests pass
  • Manual: make dev + make frontend-dev, open Cmd+K, search for a 3+ char term, verify session-grouped results with name/snippet/meta layout
  • Manual: switch Relevance ↔ Recency, verify results reorder; confirm toggle is visible during loading and zero-results states
  • Manual: search a term only in a session display name, verify it appears with correct snippet and ordinal=-1 behavior (navigates to session without scrolling to a message)
  • Manual: agentsview pg serve against a schema-v1 database, verify the error message tells you to run pg push

🤖 Generated with Claude Code

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 19, 2026

roborev: Combined Review (17c7374)

Summary Verdict: All reviewers agree the code is clean, with no medium, high, or critical issues
found.

No issues found.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@tlmaloney tlmaloney force-pushed the feat/search-pane-refinement branch from 17c7374 to b63f766 Compare March 19, 2026 13:04
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 19, 2026

roborev: Combined Review (b63f766)

The PR introduces search sorting and session-grouped results, but requires fixes for non-deterministic pagination and incorrect NULL sorting in database queries.

Medium

  • Location: internal/db/search.go:51-99
    Problem:
    The SQLite search path now paginates session-grouped results with OFFSET, but its ORDER BY is not deterministic: relevance uses only best.best_rank, and recency uses only COALESCE(s.ended_at, s.started_at) DESC. When multiple sessions share
    the same rank or timestamp, page boundaries can reshuffle between requests, causing duplicate or skipped sessions across pages.
    Fix: Add stable tie-breakers to both sort modes, for example best.best_rank ASC, COALESCE(s.ended_at, s.started_at) DESC , m.session_id ASC for relevance and COALESCE(s.ended_at, s.started_at) DESC, m.session_id ASC for recency.

  • Location: internal/postgres/messages.go:206-250

    Problem: The PostgreSQL outer sort uses session_ended_at DESC without NULLS LAST. In PostgreSQL, DESC puts NULL values first, so sessions missing both ended_at and started_at will incorrectly rise to the top of recency results and of relevance
    tie-breaks.
    Fix: Add NULLS LAST anywhere session_ended_at DESC is used, e.g. session_ended_at DESC NULLS LAST, session_id ASC.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 19, 2026

roborev: Combined Review (3614b66)

Verdict: The changes introduce a solid search grouping feature, but there is one Medium severity issue regarding the session ID copy behavior.

Medium

  • Location:
    frontend/src/lib/components/command-palette/CommandPalette.svelte:196
  • Problem: The new copy action strips the agent prefix before copying the session ID. That means the copied value is no longer the canonical session_id, and it can become ambiguous or unusable for
    prefixed sessions such as codex:... vs claude:....
  • Fix: Keep the stripped value only for display, but copy result.session_id to the clipboard (or explicitly relabel the action as copying a short/display ID).

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@tlmaloney tlmaloney force-pushed the feat/search-pane-refinement branch from 3614b66 to 4585a0d Compare March 20, 2026 01:18
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 20, 2026

roborev: Combined Review (4585a0d)

Summary Verdict: The PR successfully refactors search functionality and metadata, but requires fixes for an SQLite/PostgreSQL filtering inconsistency and a UI rendering issue with missing timestamps.

Medium

  • Location: internal/db /search.go (SearchSession)
    Problem: System messages are now filtered out in the PostgreSQL SearchSession path, but the SQLite SearchSession query was left unchanged. In the default local mode, in-session search will still surface is_system messages, so behavior now
    diverges between SQLite and pg serve, and the main local path still exposes results this change set is trying to hide.
    Fix: Add AND m.is_system = 0 to the SQLite SearchSession query and add a matching SQLite test.

  • Location: frontend/src /lib/components/command-palette/CommandPalette.svelte
    Problem: The new result row always calls formatRelativeTime(result.session_ended_at), but the backend explicitly returns an empty string when both ended_at and started_at are missing. Sessions with
    NULL timestamps are already covered by the new PostgreSQL tests, so these rows can reach the UI and will render an invalid relative-time value.
    Fix: Guard the call and omit the time segment (or show a fallback) when session_ended_at is empty.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 20, 2026

roborev: Combined Review (8e0a43f)

Verdict: The PR is functionally sound and secure, but introduces a medium-severity issue where system messages might be incorrectly surfaced during session-name searches.

Medium

  • Location: internal/db/search .go and internal/postgres/messages.go (name-search branches s.first_message LIKE/ILIKE ...)
  • Problem: System messages are filtered out only in the message-content branch, but the new session-name branch searches sessions.first_message unconditionally
    . If a session’s first_message came from a system message, that session can still be surfaced by search even though system messages are supposed to be excluded. The new PG test already has to avoid this by choosing a non-matching first_message, which indicates the hole is real.

Fix: Either guarantee at write time that sessions.first_message is never derived from a system message, or change the name-search branch to search only display names plus a non-system-derived title field.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@tlmaloney tlmaloney force-pushed the feat/search-pane-refinement branch from 8e0a43f to 823fed2 Compare March 20, 2026 18:11
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 20, 2026

roborev: Combined Review (823fed2)

Summary Verdict: The code review could not be fully completed because the commit diff was inaccessible in the testing environment, though no security or other structural issues were found in
the available metadata.

High

  • Location: Commit range e6100e064fbb2107f91fef34e4ed57778776c59f..823fed24c80dee8 a3aeed0bb79f8e3006d2a10b4
  • Problem: The code changes were not accessible in this environment: the diff was not included in the prompt, and local git diff inspection was blocked by the sandbox, preventing a reliable static
    review of the actual changes.
  • Fix: Provide the patch contents inline or make the commit range accessible so the review can be based on the real diff.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm
Copy link
Copy Markdown
Owner

wesm commented Mar 20, 2026

@tlmaloney do you mind pausing here so I can have an opportunity to take a look at this?

@tlmaloney
Copy link
Copy Markdown
Contributor Author

@tlmaloney do you mind pausing here so I can have an opportunity to take a look at this?

Yes, will do. Thank you!!

…earch, is_system backfill

- Remove planning documents
@tlmaloney
Copy link
Copy Markdown
Contributor Author

@tlmaloney do you mind pausing here so I can have an opportunity to take a look at this?

I've left the spec and plan docs in for context, they can be removed before merge.

@wesm wesm force-pushed the feat/search-pane-refinement branch from 3999cd0 to ce74682 Compare March 20, 2026 19:06
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 20, 2026

roborev: Combined Review (ce74682)

Verdict: All reviewers agree the code is clean and no issues were found.

Summary: The changes successfully refine search functionality to return session-grouped results with relevance/recency sorting, add name-based matching, and securely introduce is_system propagation along with safely handled PostgreSQL schema/backfill
migrations.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@tlmaloney
Copy link
Copy Markdown
Contributor Author

@wesm Flagging one thing: the backfill_pending machinery in schema.go/push.go (per-machine flags, orphaned session checks, --clear-backfill CLI, the pg serve blocking) solves v1→v2 migration for existing PG databases. Since PG push hasn't shipped yet, there are no v1 databases to migrate — every new user gets the v2 schema fresh.

I can go either way on keeping it (insurance for future migrations) vs. removing it (YAGNI, cuts ~250 lines + associated tests).

@tlmaloney tlmaloney marked this pull request as ready for review March 21, 2026 11:46
@wesm
Copy link
Copy Markdown
Owner

wesm commented Mar 21, 2026

I'll work on this this weekend — I'm current working on reducing the scope of changes in this PR since currently this is doing too many things.

@tlmaloney
Copy link
Copy Markdown
Contributor Author

I'll work on this this weekend — I'm current working on reducing the scope of changes in this PR since currently this is doing too many things.

Thank you! Cutting the v1->v2 migration code should cut this PR down by roughly a half.

@wesm
Copy link
Copy Markdown
Owner

wesm commented Mar 22, 2026

I opened a clean PR at #210 with the migration code trimmed out

@wesm wesm closed this Mar 22, 2026
wesm added a commit that referenced this pull request Mar 22, 2026
Supersedes #200.

## Summary

- **Session-grouped results**: FTS5 search returns one result per
session. The best-ranked matching message provides the snippet and
scroll target.
- **Relevance/Recency sort toggle**: two-button control above results.
Sort validated server-side before ORDER BY interpolation. Toggle visible
during loading and zero-results states.
- **Session name search**: matches `display_name` and `first_message`
via a UNION with LIKE/ILIKE. Name-only matches use `ordinal = -1`;
frontend navigates without scrolling.
- **is_system flag**: parsers tag system-injected user messages; search
excludes them via column check and prefix fallback.
- **Case-sensitive prefix matching**: `SystemPrefixSQL` uses `substr()`
instead of `LIKE` for consistent behavior across SQLite
(case-insensitive LIKE) and PostgreSQL.
- **PostgreSQL parity**: `pg serve` path updated with `DISTINCT ON`
grouping, `is_system` column, ILIKE name branch.
- **FTS5 dedup fix**: outer JOIN includes MATCH clause to prevent
segment-level row duplication.
- **Stable timestamp ordering**: `julianday()` in SQLite ORDER BY avoids
lexicographic misorderings from variable fractional-second precision.

## Test plan

- [ ] `make test` — Go unit tests pass
- [ ] `make test-postgres` — PG integration tests pass
- [ ] `cd frontend && npx vitest run` — frontend tests pass
- [ ] Manual: Cmd+K search, verify session-grouped results with
name/snippet/meta
- [ ] Manual: Relevance/Recency toggle reorders results
- [ ] Manual: search term only in session name, verify ordinal=-1
behavior

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Tom Maloney <tom@supermaloney.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants