feat(core): search the External Session catalog by title and path - #3471
Conversation
The import catalog pages 16 at a time with one filter, `includeArchived`. That was proportionate to Codex's 19 sessions here. Claude Code brought 1128 — 71 pages of cursor paging with no way to say which session you want. `ExternalSessionQuery` gains `text`, matched against the summary's title and cwd. Both already sit on `ExternalSessionSummary`, so neither adapter reads more than it did. **The term reaches the adapter, before paging.** The coordinator slices a page out of what `listSessions` returns, so a filter applied afterwards would search the 16 rows already fetched — worse than offering no search at all, because it looks like it worked. **One matcher, not one per adapter.** `externalSessionMatchesQuery` lives in core and both adapters call it. The catalog is one surface over several sources; a filter that quietly worked for Codex and not for Claude Code would be undebuggable from the UI, which cannot say which source dropped the term. A third adapter inherits the behaviour instead of reimplementing it. That consolidation fixes a real inconsistency apache#3435 introduced: Codex compared cwd through `normalizePath`, the Claude Code adapter compared raw strings, so the same project reached with a trailing separator answered "no such project" on one source and not the other. Both now share one path rule. A blank box is not a filter — `''` and `' '` normalize to "no term", so a stray space cannot hide every session. An over-long term is truncated to its 200-character prefix rather than throwing at the user; the protocol bounds it at the frame because it reaches every adapter. Search is a dimension of the catalog *selection*, so it joins `catalogSelectionRef` and the `ImportAttempt` record beside `includeArchived` — import recovery re-reads the window a row came from, and a cleared box would look at a different list. Typing is debounced at 250ms: the term walks every transcript on the source, so a request per keystroke would queue a thousand-file scan behind each character. A search that finds nothing gets its own empty state. Reusing "no conversations" would tell a user their transcripts are missing when the term is simply wrong. Protocol epoch 36 → 37: `ExternalSessionCatalogQueryInput` gains a field. Generated-by: Claude Opus 5
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for tackling a real usability problem here — filtering before pagination and keeping the shared matcher in Core are both the right direction. I reviewed this exact head through two independent passes and kept only the concrete cross-platform and UI consistency gaps inline. The clean final state is one normalized text/path authority used by both adapters and the renderer, without an earlier storage filter narrowing the candidate set differently.
AI-assisted review disclosure: OpenAI Codex coordinated two independent review passes. I verified the retained findings against this exact head and the live PR state, and I made the final review decision.
| // conversation they are looking for. Both already sit on the summary, so | ||
| // matching costs no extra reads. Message content is deliberately excluded: | ||
| // it would mean opening every transcript on every keystroke. | ||
| return summary.name.toLowerCase().includes(text) || summary.cwd.toLowerCase().includes(text); |
There was a problem hiding this comment.
[P2] Could the shared matcher normalize both the query and candidate strings before matching? On Windows, a stored C:/Repo/App does not match pasted C:\\Repo\\App, even though sameExternalSessionPath() already treats those paths as equivalent. On macOS, the same visible name/path can also arrive in NFC or NFD form and miss here. Reusing one text normalizer plus the existing separator rule for the cwd field would keep the shared matcher as the authority; adapter-level cases for Windows separators and composed/decomposed Unicode would pin it.
| return query.cwd === undefined || normalizePath(entry.cwd) === normalizePath(query.cwd); | ||
| // Delegated so both sources answer a query identically; the local | ||
| // `normalizePath` below is still used for the SQL cwd variants. | ||
| return externalSessionMatchesQuery(entry, query); |
There was a problem hiding this comment.
[P2] This shared matcher is reached only after the existing SQLite cwd IN (...) prefilter has already narrowed the rows. That prefilter cannot express the same Windows path equivalence: for example, a DB row stored as C:\\Repo\\App and a query c:/repo/app can be discarded before this function sees it. Could we remove the lossy SQL prefilter and let this function be the single authority, or store/query one canonical path representation? Please pin the real SQLite adapter path rather than only the helper.
| nothing in it. Reusing the "no conversations" copy would tell a | ||
| user their transcripts are missing when the term is simply | ||
| wrong, and hide the one control that would fix it. */} | ||
| {catalogEmpty && search !== '' && ( |
There was a problem hiding this comment.
[P3] Whitespace-only input is normalized to “no filter” by the shared helper, but this branch still treats it as an active search. On an empty source, entering spaces therefore shows No conversation has " " instead of the ordinary empty-source state. Could this condition use the same normalized query value so matching and empty-state copy cannot diverge?
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the current-head CI has now run, and it exposed one deterministic generated-contract failure. I verified the generator diff before adding the inline note; this is a small mechanical fix rather than a broader implementation change.
AI-assisted review disclosure: OpenAI Codex delegated the failed-CI diagnosis. I verified the run SHA, failed step, generator output, and minimal fix against this exact head.
…ield The import page gained a `TextInput`, and the inventory is generated from what each surface imports. CI regenerates and diffs it, so the row went stale the moment the field landed. I ran lint, format, typecheck and the affected suites before pushing, but not the generated-artifact checks. Running all of them now — surface inventory, Windows inventory, third-party notices, ASF source, release identity, Windows cargo notices — leaves this the only stale one, on both open branches. Generated-by: Claude Opus 5
Three review findings, one root cause: the matcher compared strings the cwd rule already considered equal. **Windows separators.** `sameExternalSessionPath` treats `C:/Repo/App` and `C:\Repo\App` as the same project, but the text match did not, so a term pasted from a Windows path missed a summary stored with forward slashes — the same two strings, two answers. **Composed and decomposed Unicode.** macOS records decomposed filenames, so a title typed in NFC missed one recorded in NFD. Nothing visible distinguishes them. Both sides now pass through one normalizer — NFC, lowercase, separators — and the cwd rule folds in the same NFC step, so the two can no longer disagree about the same pair. **The SQL prefilter was a second, weaker rule.** `cwd IN (<spelling variants>)` enumerated forms of the query, but SQLite compares them exactly: a row stored `C:\Repo\App` was discarded before the shared matcher could see that `c:/repo/app` names the same project. A prefilter that cannot express the matcher's own equivalence is not an optimization. It is gone, and the matcher is the only authority on which project a row belongs to; the statement has no LIMIT, so this widens the read rather than truncating it. The archived clause stays — an exact boolean that agrees with the matcher by construction. `normalizePath` and `cwdSqlVariants` were only reachable from that clause and are removed with it. The regression drives the real state database rather than the matcher in isolation, because the database is where the row was being dropped. Restoring the prefilter fails it. **A whitespace-only box is no longer half a filter.** The matcher already normalized `' '` to "no term", but the empty state compared the raw string, so spaces on an empty source answered `No conversation has " "` instead of the ordinary empty-source copy. Both now read the same normalized value. Generated-by: Claude Opus 5
|
All four addressed. P1 — inventory. Regenerated and committed. The root cause was mine: I ran lint, format, typecheck and the affected suites before pushing, but not the generated-artifact checks. I have since run all seven — surface inventory, Windows inventory, third-party notices, CLI notices, ASF source, release identity, Windows cargo notices — on both of my open branches, and that was the only stale one. P2 — normalize both sides. You were right, and the two cases you named turned out to be the same root cause: the matcher compared strings the cwd rule already considered equal. Reproduced both before fixing: Both sides now pass through one normalizer — NFC, lowercase, separators — and P2 — the SQL prefilter. Also right, and the sharper framing is yours: it was a second, weaker rule sitting in front of the authority. I removed it rather than teaching it the equivalence. A prefilter that cannot express the matcher's semantics is not an optimization. The statement has no Per your note, the regression drives the real state database, not the helper: it seeds a row with P3 — whitespace empty state. Fixed by keying the branch on the normalized value the matcher uses, so the copy and the match cannot diverge. Verification
|
Self-review after the last round. My fix applied separator folding to both
fields, which over-matches: a search for `/n` found a title holding a
literal `\n`.
The review had actually asked for the narrower thing — "one text
normalizer plus the existing separator rule for the cwd field" — and I
widened it without saying so.
Folding only one side is not an option either. With the term folded and the
title not, `\n` stops finding the very title it names, which trades an
over-match for a miss. So the fold now applies to the *pair*: term and cwd
together, title untouched.
title `regex: \n handling` term `\n` -> found
title `regex: \n handling` term `/n` -> not found
cwd `C:/Repo/App` term `C:\Repo\App` -> found
Both directions are pinned, because a one-sided fold trades one bug for the
other and a test on one side alone would not notice.
Generated-by: Claude Opus 5
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — routing the filter into the adapters so it runs before pagination is the right call, and collapsing Codex's SQL cwd IN (...) prefilter into the shared matcher rather than patching it was the right instinct too: it was a second, weaker rule for the same fact.
What this solves / how: the External Session catalog could only be filtered by cwd/archived, so finding one session among ~1128 meant paging until it appeared. This adds a shared externalSessionMatchesQuery in core that both adapters apply before assembling a page.
One root cause behind all three findings below. The PR now has two path-equivalence rules where it means to have one: normalizeExternalSessionPath (used by sameExternalSessionPath) strips trailing separators, and foldExternalSessionPathSeparators (used by the new matcher) does not. One strips too much and swallows the POSIX root; the other strips too little and misses pasted paths. Worth fixing as a single rule rather than three separate patches — otherwise you're likely to still have two rules afterwards.
Details inline.
AI-assisted review. Two reviewers worked this PR independently and reached different conclusions; the facts behind each finding were then verified directly against this head, and one finding was downgraded during that check.
| }; | ||
| } | ||
|
|
||
| // 250ms after typing stops, not per keystroke. The term reaches the adapter, |
There was a problem hiding this comment.
[P2, ordinary user path] Each typing pause over 250ms can start a full catalog scan, and the previous one is never cancelled.
listSessions walks every transcript file, readFiles it whole, and split('\n')-parses all of it — no cache, no header-only path. That cost already existed; what changes here is how often it runs. It used to happen when the page opened. Now it happens per search.
The debounce is a standard trailing-edge one, so typing fast collapses into a single request. But pausing longer than 250ms mid-word is ordinary typing, and each pause can start another full scan while requestGeneration only discards the result — the earlier scan still runs to completion. At the ~1128-session scale this PR itself cites, those overlap and stack.
Not P1: nothing is lost or corrupted and closing the page recovers. But the user has no way to make it stop.
Suggestion: thread an AbortSignal through the catalog read and abort superseded scans. Discarding results isn't cancellation.
There was a problem hiding this comment.
Rechecked on current head 4df38eb42: the cache makes completed repeat listings cheap, but it does not cover the overlapping cold-request case in this thread. The 948ms / 28ms / 26ms measurement is sequential; during the first ~948ms scan, a second 250ms-debounced catalog request can still be dispatched concurrently. connection-session.ts only serializes session.transcript.page, and #summaries stores settled summaries rather than sharing an in-flight parse, so once the second walk catches the first it can still duplicate the remaining reads/parses. A narrow fix can coalesce the in-flight summary/listing work without adding protocol cancellation; cancellation can remain separate scope.
| return normalizeExternalSessionPath(left) === normalizeExternalSessionPath(right); | ||
| } | ||
|
|
||
| function normalizeExternalSessionPath(value: string): string { |
There was a problem hiding this comment.
[P3] normalizeExternalSessionPath folds the POSIX root and an unknown cwd into the same value.
The unconditional .replace(/\/+$/u, '') makes normalizeExternalSessionPath('/') and normalizeExternalSessionPath('') both '', so sameExternalSessionPath('/', '') is true. Verified directly:
'/' -> ''
'' -> ''
same('/', '') === true
Both ends of the path are reachable: the workspace resolver accepts / (it's absolute and resolves to a directory), and the Claude adapter leaves cwd: '' when no record carries one. So viewing the import catalog with the workspace at filesystem root pulls in sessions whose cwd is simply unknown.
Only P3: everything involved is the same user's own sessions on the same machine, so this is filter accuracy rather than a boundary being crossed. Fix: keep / as /, and a sameExternalSessionPath('/', '') === false test to pin it.
| * spelling of a path finds the project the summary stored in the other. The | ||
| * same equivalence `sameExternalSessionPath` applies to the `cwd` filter. | ||
| */ | ||
| function foldExternalSessionPathSeparators(value: string): string { |
There was a problem hiding this comment.
[P3] A search term with a trailing separator doesn't match the same project.
foldExternalSessionPathSeparators folds separators but doesn't strip a trailing one. Paste C:\Repo\App\ from Windows Explorer (which routinely includes the trailing backslash) against a stored cwd of C:/Repo/App: the term folds to c:/repo/app/, and includes('c:/repo/app/') is false.
The PR's own Windows-paste test case has no trailing separator, so this sits right next to covered ground.
This is the other half of the root cause above — here the rule strips too little, where normalizeExternalSessionPath strips too much.
Review found I had built two path-equivalence rules where the PR means to
have one, and was right that patching them separately would leave two.
They are now the same function.
`normalizeExternalSessionPath` stripped trailing separators
unconditionally, folding `/` and `''` to the same value — so a workspace at
filesystem root matched every session whose cwd the adapter could not
determine. The root is its own separator, so stripping now stops at one:
`/`, `//` and `///` are the same directory; `/` and `''` are not.
`foldExternalSessionPathSeparators` did the opposite — folded separators
without stripping a trailing one, so `C:\Repo\App\` as Windows Explorer
copies it missed a stored `C:/Repo/App`. It now delegates to the same rule,
which strips and folds together.
Both directions and the boundaries are pinned, including `//` and the
root-versus-unknown pair the old rule collapsed.
**On the scan cost.** The finding is real: listing parses every transcript,
and the catalog is now listed once per search term, so a pause in typing
starts another full parse while `requestGeneration` only discards the
result. Measured here, three times in a row against 1128 transcripts:
948 ms · 1128 ms · 958 ms
I did not thread an `AbortSignal`. Nothing in the stack has request
cancellation — no signal in the protocol, no cancel operation, nothing in
the preload bridge — so that means inventing cancellation for the Host
protocol, which is a feature and its own change, not a fix inside a search
PR.
What I did instead removes the cost the cancellation would have been
avoiding: summaries are cached per transcript and invalidated by the file's
own mtime and size. The same three calls now take
948 ms · 28 ms · 26 ms
A superseded scan still runs, but it re-reads nothing that has not changed,
so pauses no longer stack a second of work each. Entries for transcripts
the source stops listing are dropped, so a long-lived Host does not grow
one per deleted session.
A cache's failure mode is a stale answer, so both directions are pinned: a
rewritten transcript is re-read, a deleted one drops out. Keying on path
alone fails that test.
Generated-by: Claude Opus 5
|
All three addressed in The two P3s were one bug in opposite directions. Reproduced both first:
Boundaries pinned, including the ones neither rule handled before: On the P2 — I did not thread an
|
| 1st | 2nd | 3rd | |
|---|---|---|---|
| before | 948 ms | 1128 ms | 958 ms |
| after | 948 ms | 28 ms | 26 ms |
A superseded scan still runs to completion — that part of your finding stands — but it re-reads nothing that has not changed, so pauses no longer stack a second of work each. The first listing is unchanged; this is about repetition, which is exactly what the search box introduced.
Two things a cache has to get right, both pinned:
- staleness — a rewritten transcript is re-read, a deleted one drops out. Keying on path alone fails that test; I checked by keying on path alone.
- growth — entries for transcripts the source stops listing are dropped on each listing, so a long-lived Host does not accumulate one per deleted session.
If you would rather have real cancellation regardless, I would take that as a separate issue against the protocol — happy to file it, and it would help more surfaces than this one.
Verification
@maka/core598/598 ·@maka/storage897/897 ·@maka/runtime-host1066/1066- typecheck (preload + main + renderer + storybook): clean
format:check1571 files ·biome lint .2510 files: clean
| adapterId: string; | ||
| includeArchived?: boolean; | ||
| cursor?: string; | ||
| text?: string; |
There was a problem hiding this comment.
[P2] The implementation accepts text here, but the public renderer contract still does not: apps/desktop/src/preload/bridge-contract.d.ts declares MakaBridge.externalSessions.list with only adapterId, includeArchived, and cursor. The current page happens to put text behind an object spread, so TypeScript does not report the excess property, but a direct typed consumer of the bridge cannot pass the new field. Please add text?: string to the declared list input so the exposed contract matches the implementation.
| ...(parsed.updatedAt !== undefined ? { updatedAt: parsed.updatedAt } : {}), | ||
| } | ||
| : undefined; | ||
| this.#summaries.set(path, { mtimeMs, size, ...(summary ? { summary } : {}) }); |
There was a problem hiding this comment.
[P3] This negative cache also records transient I/O failures as if they were stable exclusions. #parse returns undefined for a failed inner stat or readFile, and this line then caches that value under the earlier mtime/size. If the file becomes readable again without its contents changing (for example, permissions are restored after a temporary read failure, or a transient EMFILE/EIO clears), later listings keep returning the cached undefined until the Host restarts or the transcript metadata changes. Sidechain/oversize results are stable, but read failures are not; please keep transient failures out of the negative cache or return a discriminated parse outcome so only stable exclusions are cached.
Summary
设置 → 导入任务 pages 16 sessions at a time with one filter,
includeArchived. That was proportionate to what Codex produces here — 19 sessions. Claude Code brought 1128, which is 71 pages of cursor paging with no way to say which one you want.ExternalSessionQuerygainstext, matched against the summary's title and cwd. Both already sit onExternalSessionSummary, so neither adapter reads more than it did.The term reaches the adapter, before paging. The coordinator slices a page out of whatever
listSessionsreturns, so a filter applied after that would search the 16 rows already fetched. That is worse than offering no search at all, because it looks like it worked.One matcher, not one per adapter.
externalSessionMatchesQuerylives in@maka/coreand both adapters call it. The catalog is one surface over several sources; a filter that quietly worked for Codex and not for Claude Code would be undebuggable from the UI, which cannot say which source dropped the term. A third adapter inherits the behaviour rather than reimplementing it.Fixes #3438
Review focus
This consolidation fixes an inconsistency I introduced in #3435. Codex compared cwd through
normalizePath; the Claude Code adapter compared raw strings. The same project reached with a trailing separator answered "no such project" on one source and not the other. Both now share one path rule, and there is a test for it.A blank box is not a filter.
''and' 'both normalize to "no term", so a stray space cannot hide every session. An over-long term is truncated to its 200-character prefix rather than throwing at the user; the protocol bounds it at the frame because it reaches every adapter.Search is a dimension of the catalog selection, not a view filter. It joins
catalogSelectionRefand theImportAttemptrecord besideincludeArchived: import recovery re-reads the window a row came from, and a cleared box would look at a different list.Debounced at 250ms. The term walks every transcript on the source, so a request per keystroke would queue a thousand-file scan behind each character.
A search that finds nothing gets its own empty state. Reusing "no conversations" would tell a user their transcripts are missing when the term is simply wrong, and hide the one control that would fix it.
Breaking change
Protocol epoch 36 → 37.
ExternalSessionCatalogQueryInputgains an optionaltext.scripts/protocol-epoch-check.mjs --base origin/mainconfirms:Protocol changed and the epoch moved: 36 -> 37.Verification
Tests — 26 new, across every layer the term crosses:
@maka/core(12)playworktreenarrows three rows to oneThe storybook stub honours
texton purpose. A stub that ignored it would render a search box that looks wired and is not, and the story would certify that. Disconnecting the term from the request fails that story — checked by removing it.Checks run locally:
@maka/core: 593/593 ·@maka/storage: 895/895 ·@maka/runtime-host: 1066/1066 ·@maka/desktop: 1092/1092npm run format:check: 1571 files, cleanbiome lint .: 2510 files, cleanbuild-storybook+smoke:storybook: 162 stories, AX audit includedNot run: the packaged-artifact and release checks;
@maka/runtime's suite has 5 unrelated environmental failures here (spawn rg ENOENT— no ripgrep binary).Not in scope
Searching message content. Titles and paths are metadata the adapters already hold; content means reading every transcript on every keystroke, which is a different problem with a different budget.
AI use
Select exactly one:
Tool(s) and scope: Claude Opus 5 (Claude Code) — wrote the implementation, tests, and story; ran the checks above. Reviewed and directed by me.
Generated-bytrailer is on the commit.Checklist
Does this PR entail a change in behavior?