You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Joob1n (via GitHub)" <gi...@apache.org> on 2026/08/22 07:42:54 UTC
[PR] feat(core): search the External Session catalog by title and path [maka]
Joob1n opened a new pull request, #3471:
URL: https://github.com/apache/maka/pull/3471
## 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.
`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 whatever `listSessions` returns, 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.** `externalSessionMatchesQuery` lives in `@maka/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 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 `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.
**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**. `ExternalSessionCatalogQueryInput` gains an optional `text`. `scripts/protocol-epoch-check.mjs --base origin/main` confirms: `Protocol changed and the epoch moved: 36 -> 37.`
## Verification
**Tests — 26 new, across every layer the term crosses:**
| layer | what is pinned |
| --- | --- |
| `@maka/core` (12) | title/path match, blank-is-not-a-filter, truncation, archived gate, cwd+text both apply, path normalization |
| Claude Code adapter (2) | filters the source not the page; trailing separator tolerated |
| Codex adapter | the same term, same results — the "silently works for one source" case |
| protocol (4) | a term is carried; a non-string, empty, or over-long one is refused at the frame |
| storybook `play` | typing `worktree` narrows three rows to one |
The storybook stub honours `text` on 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/1092**
- typecheck (preload + main + renderer + storybook): clean
- `npm run format:check`: 1571 files, clean
- `biome lint .`: 2510 files, clean
- `build-storybook` + `smoke:storybook`: **162 stories**, AX audit included
**Not 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:
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
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-by` trailer is on the commit.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(core): search the External Session catalog by title and path [maka]
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on code in PR #3471:
URL: https://github.com/apache/maka/pull/3471#discussion_r3836086657
##########
apps/desktop/src/preload/preload.ts:
##########
@@ -1807,6 +1807,7 @@ const makaBridge = {
adapterId: string;
includeArchived?: boolean;
cursor?: string;
+ text?: string;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(core): search the External Session catalog by title and path [maka]
Posted by "likun666661 (via GitHub)" <gi...@apache.org>.
likun666661 merged PR #3471:
URL: https://github.com/apache/maka/pull/3471
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(core): search the External Session catalog by title and path [maka]
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on code in PR #3471:
URL: https://github.com/apache/maka/pull/3471#discussion_r3836087400
##########
apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx:
##########
@@ -170,18 +180,29 @@ export function ImportTasksSettingsPage(props: {
// source's rows under the new source's label.
const requestGeneration = useRef(0);
const recoveryGeneration = useRef(0);
- const catalogSelectionRef = useRef({ adapterId, includeArchived, generation: 0 });
+ const catalogSelectionRef = useRef({ adapterId, includeArchived, search, generation: 0 });
if (
catalogSelectionRef.current.adapterId !== adapterId ||
- catalogSelectionRef.current.includeArchived !== includeArchived
+ catalogSelectionRef.current.includeArchived !== includeArchived ||
+ catalogSelectionRef.current.search !== search
) {
catalogSelectionRef.current = {
adapterId,
includeArchived,
+ search,
generation: catalogSelectionRef.current.generation + 1,
};
}
+ // 250ms after typing stops, not per keystroke. The term reaches the adapter,
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(core): search the External Session catalog by title and path [maka]
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on code in PR #3471:
URL: https://github.com/apache/maka/pull/3471#discussion_r3836087035
##########
packages/storage/src/claude-code-session-adapter.ts:
##########
@@ -76,26 +94,64 @@ export class ClaudeCodeSessionAdapter implements ExternalSessionAdapter {
async listSessions(query?: ExternalSessionQuery): Promise<readonly ExternalSessionSummary[]> {
const summaries: ExternalSessionSummary[] = [];
+ const live = new Set<string>();
for (const file of await this.#transcriptFiles()) {
- const parsed = await this.#parse(file.path, file.sessionId);
- if (!parsed) continue;
- // Sub-agent transcripts are whole files, never records interleaved into
- // a parent — so exclusion is per file. Importing one would present a
- // fragment of a conversation as a conversation.
- if (parsed.isSidechain) continue;
- if (query?.cwd !== undefined && parsed.cwd !== query.cwd) continue;
- summaries.push({
- id: file.sessionId,
- name: parsed.title || file.sessionId,
- cwd: parsed.cwd,
- ...(parsed.createdAt !== undefined ? { createdAt: parsed.createdAt } : {}),
- ...(parsed.updatedAt !== undefined ? { updatedAt: parsed.updatedAt } : {}),
- });
+ live.add(file.path);
+ const summary = await this.#summaryOf(file.path, file.sessionId);
+ if (!summary) continue;
+ // The shared matcher, not a local cwd comparison: filtering happens here
+ // rather than after paging, and every source has to answer a query the
+ // same way or the catalog lies about which one dropped the term.
+ if (!externalSessionMatchesQuery(summary, query)) continue;
+ summaries.push(summary);
+ }
+ // A transcript the source no longer lists must not keep its entry alive,
+ // or a long-lived Host grows one per deleted session.
+ for (const path of this.#summaries.keys()) {
+ if (!live.has(path)) this.#summaries.delete(path);
}
summaries.sort((left, right) => (right.updatedAt ?? 0) - (left.updatedAt ?? 0));
return summaries;
}
+ /**
+ * The summary for one transcript, parsed only when the file has changed.
+ *
+ * `undefined` is cached too: a sidechain transcript or an unreadable one is
+ * a stable answer, and re-deriving it every list would defeat the point.
+ */
+ async #summaryOf(path: string, sessionId: string): Promise<ExternalSessionSummary | undefined> {
+ let mtimeMs: number;
+ let size: number;
+ try {
+ const info = await stat(path);
+ mtimeMs = info.mtimeMs;
+ size = info.size;
+ } catch {
+ this.#summaries.delete(path);
+ return undefined;
+ }
+ const cached = this.#summaries.get(path);
+ if (cached && cached.mtimeMs === mtimeMs && cached.size === size) return cached.summary;
+
+ const parsed = await this.#parse(path, sessionId);
+ // Sub-agent transcripts are whole files, never records interleaved into a
+ // parent — so exclusion is per file. Importing one would present a
+ // fragment of a conversation as a conversation.
+ const summary =
+ parsed && !parsed.isSidechain
+ ? {
+ id: sessionId,
+ name: parsed.title || sessionId,
+ cwd: parsed.cwd,
+ ...(parsed.createdAt !== undefined ? { createdAt: parsed.createdAt } : {}),
+ ...(parsed.updatedAt !== undefined ? { updatedAt: parsed.updatedAt } : {}),
+ }
+ : undefined;
+ this.#summaries.set(path, { mtimeMs, size, ...(summary ? { summary } : {}) });
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(core): search the External Session catalog by title and path [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3471:
URL: https://github.com/apache/maka/pull/3471#discussion_r3835640586
##########
packages/core/src/external-session.ts:
##########
@@ -18,6 +29,58 @@ export interface ExternalSessionSummary {
archived?: boolean;
}
+/**
+ * Whether one summary answers a query.
+ *
+ * Shared by every adapter on purpose. The catalog is one surface over several
+ * sources, so a filter that quietly worked for Codex and not for Claude Code
+ * would be worse than no filter — the user cannot see which source dropped
+ * their term. Keeping the decision here means a new adapter inherits the
+ * behaviour instead of reimplementing it.
+ */
+export function externalSessionMatchesQuery(
+ summary: ExternalSessionSummary,
+ query: ExternalSessionQuery = {},
+): boolean {
+ if (!query.includeArchived && summary.archived) return false;
+ if (query.cwd !== undefined && !sameExternalSessionPath(summary.cwd, query.cwd)) return false;
+ const text = normalizeExternalSessionQueryText(query.text);
+ if (text === undefined) return true;
+ // Title and path, because those are the two things a user remembers about a
+ // 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);
Review 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.
##########
packages/storage/src/codex-session-adapter.ts:
##########
@@ -750,8 +751,9 @@ function cwdSqlVariants(cwd: string): string[] {
}
function matchesQuery(entry: ExternalSessionSummary, query: ExternalSessionQuery): boolean {
- if (!query.includeArchived && entry.archived) return false;
- 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);
Review 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.
##########
apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx:
##########
@@ -609,7 +645,14 @@ export function ImportTasksSettingsPage(props: {
</div>
)}
- {catalogEmpty && (
+ {/* A search that found nothing is not the same as a source with
+ 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 !== '' && (
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(core): search the External Session catalog by title and path [maka]
Posted by "Joob1n (via GitHub)" <gi...@apache.org>.
Joob1n commented on PR #3471:
URL: https://github.com/apache/maka/pull/3471#issuecomment-5379919324
All three addressed in `4df38eb42`. Your root-cause read was the useful part — I had built two path rules and would have patched them into two slightly different path rules.
**The two P3s were one bug in opposite directions.** Reproduced both first:
```
same('/', '') -> true (should be false)
cwd 'C:/Repo/App' term 'C:\Repo\App\' -> false (should be true)
```
`normalizeExternalSessionPath` stripped trailing separators unconditionally, so `/` and `''` folded together. `foldExternalSessionPathSeparators` folded separators but stripped nothing, so a Windows Explorer paste missed. The second now delegates to the first, and the first stops stripping at one separator — the POSIX root *is* its separator.
Boundaries pinned, including the ones neither rule handled before:
```
'/' vs '' false '//' vs '/' true '///' vs '/' true
'/' vs '/' true '' vs '' true '/' vs '/repo' false
'C:/' vs 'C:\' true 'C:/Repo/' vs 'c:\repo' true
```
## On the P2 — I did not thread an `AbortSignal`
Your measurement of the problem is right, and I reproduced it: three consecutive listings over 1128 transcripts took **948 ms · 1128 ms · 958 ms**. Nothing is reused, and `requestGeneration` discards the result rather than the work.
But there is no request cancellation anywhere in this stack — no signal in the Host protocol, no cancel/abort operation, nothing in the preload bridge. Threading one means inventing cancellation for the protocol, with an epoch bump and a cancel operation of its own. That is a feature, and it should not arrive inside a search PR where it would be reviewed as a footnote.
**So I removed the cost that cancellation would have been avoiding.** Summaries are now cached per transcript and invalidated by the file's own mtime and size. Same three calls:
| | 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/core` **598/598** · `@maka/storage` **897/897** · `@maka/runtime-host` **1066/1066**
- typecheck (preload + main + renderer + storybook): clean
- `format:check` 1571 files · `biome lint .` 2510 files: clean
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(core): search the External Session catalog by title and path [maka]
Posted by "Joob1n (via GitHub)" <gi...@apache.org>.
Joob1n commented on PR #3471:
URL: https://github.com/apache/maka/pull/3471#issuecomment-5379716874
All four addressed. `66ed545a4` for the P1, `9778cb715` for the rest.
**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:
```
stored C:/Repo/App searched C:\Repo\App -> false
stored NFC searched NFD -> false
```
Both sides now pass through one normalizer — NFC, lowercase, separators — and `sameExternalSessionPath` folds in the same NFC step, so the two rules cannot disagree about the same pair. Adapter-level cases pin both, as you asked.
**P2 — the SQL prefilter.** Also right, and the sharper framing is yours: it was a second, weaker rule sitting in front of the authority. `cwd IN (<variants>)` enumerated spellings of the query, but SQLite compares them exactly, so a row stored `C:\Repo\App` never reached `matchesQuery` to be told that `c:/repo/app` is the same project.
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 `LIMIT`, so dropping the clause widens the read instead of truncating it, and the archived clause stays — an exact boolean that agrees with the matcher by construction. `normalizePath` and `cwdSqlVariants` were only reachable from that clause and went with it.
Per your note, **the regression drives the real state database**, not the helper: it seeds a row with `cwd: 'C:\Repo\App'` and asserts all four spellings find it. Restoring the prefilter fails it — checked by restoring it.
**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
- `@maka/core` **595/595** · `@maka/storage` **896/896** · `@maka/runtime-host` **1066/1066** · `@maka/desktop` **1092/1092**
- typecheck (preload + main + renderer + storybook): clean
- `format:check` 1571 files · `biome lint .` 2510 files: clean
- all seven generated-artifact checks: clean
- `build-storybook` + `smoke:storybook`: 162 stories, AX audit included
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(core): search the External Session catalog by title and path [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3471:
URL: https://github.com/apache/maka/pull/3471#discussion_r3835844441
##########
apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx:
##########
@@ -170,18 +180,29 @@ export function ImportTasksSettingsPage(props: {
// source's rows under the new source's label.
const requestGeneration = useRef(0);
const recoveryGeneration = useRef(0);
- const catalogSelectionRef = useRef({ adapterId, includeArchived, generation: 0 });
+ const catalogSelectionRef = useRef({ adapterId, includeArchived, search, generation: 0 });
if (
catalogSelectionRef.current.adapterId !== adapterId ||
- catalogSelectionRef.current.includeArchived !== includeArchived
+ catalogSelectionRef.current.includeArchived !== includeArchived ||
+ catalogSelectionRef.current.search !== search
) {
catalogSelectionRef.current = {
adapterId,
includeArchived,
+ search,
generation: catalogSelectionRef.current.generation + 1,
};
}
+ // 250ms after typing stops, not per keystroke. The term reaches the adapter,
Review 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, `readFile`s 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.
##########
packages/core/src/external-session.ts:
##########
@@ -18,6 +29,103 @@ export interface ExternalSessionSummary {
archived?: boolean;
}
+/**
+ * Whether one summary answers a query.
+ *
+ * Shared by every adapter on purpose. The catalog is one surface over several
+ * sources, so a filter that quietly worked for Codex and not for Claude Code
+ * would be worse than no filter — the user cannot see which source dropped
+ * their term. Keeping the decision here means a new adapter inherits the
+ * behaviour instead of reimplementing it.
+ */
+export function externalSessionMatchesQuery(
+ summary: ExternalSessionSummary,
+ query: ExternalSessionQuery = {},
+): boolean {
+ if (!query.includeArchived && summary.archived) return false;
+ if (query.cwd !== undefined && !sameExternalSessionPath(summary.cwd, query.cwd)) return false;
+ const text = normalizeExternalSessionQueryText(query.text);
+ if (text === undefined) return true;
+ // Title and path, because those are the two things a user remembers about a
+ // 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.
+ //
+ // Candidate and term pass through the same normalizer. Without it a term
+ // pasted from a Windows path missed a stored forward-slash path that
+ // `sameExternalSessionPath` already calls the same project, and a title
+ // typed in NFC missed one macOS recorded in NFD.
+ // Separator folding is applied to the path pair only, never to the title.
+ // Folding a title would make a search for `/n` match a title containing a
+ // literal backslash-n, and folding the term without the title would stop
+ // `\\n` from finding the very title it names. The path pair has no such
+ // ambiguity: a separator there is a separator.
+ return (
+ normalizeExternalSessionMatchText(summary.name).includes(text) ||
+ foldExternalSessionPathSeparators(normalizeExternalSessionMatchText(summary.cwd)).includes(
+ foldExternalSessionPathSeparators(text),
+ )
+ );
+}
+
+/**
+ * The comparable form of one side of a text match.
+ *
+ * Three normalizations, each for a difference that is not a difference to the
+ * person searching:
+ *
+ * - **NFC** — macOS records decomposed filenames, so the same visible name can
+ * arrive composed or decomposed depending on where it was typed.
+ * - **case** — nobody searching for a project remembers its capitalisation.
+ * - **separators** — a term pasted from a Windows path should still find the
+ * project the summary stored with forward slashes, matching the equivalence
+ * `sameExternalSessionPath` already applies to the `cwd` filter.
+ *
+ * Applied to the title as well as the path. A title rarely holds a separator,
+ * but running one normalizer over both is what keeps this a single authority
+ * rather than two rules free to drift.
+ */
+function normalizeExternalSessionMatchText(value: string): string {
+ return value.normalize('NFC').toLowerCase();
+}
+
+/**
+ * Windows separators folded to the POSIX form, so a term pasted from one
+ * 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 {
+ return value.split(String.fromCharCode(92)).join('/');
+}
+
+/**
+ * The comparable form of a search term, or `undefined` when it selects
+ * nothing — an empty or whitespace-only box is not a filter, and treating it
+ * as one would hide every session behind a stray space.
+ */
+export function normalizeExternalSessionQueryText(value: string | undefined): string | undefined {
+ if (value === undefined) return undefined;
+ const trimmed = value.trim().slice(0, EXTERNAL_SESSION_QUERY_TEXT_MAX_CHARS);
+ return trimmed.length > 0 ? normalizeExternalSessionMatchText(trimmed) : undefined;
+}
+
+/**
+ * Path equality across the shapes different sources record.
+ *
+ * Codex normalizes separators and lowercases a Windows drive prefix before
+ * comparing; the Claude Code adapter compared raw strings, so the same project
+ * reached through a different separator answered "no such project". One rule
+ * for both.
+ */
+export function sameExternalSessionPath(left: string, right: string): boolean {
+ return normalizeExternalSessionPath(left) === normalizeExternalSessionPath(right);
+}
+
+function normalizeExternalSessionPath(value: string): string {
Review 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.
##########
packages/core/src/external-session.ts:
##########
@@ -18,6 +29,103 @@ export interface ExternalSessionSummary {
archived?: boolean;
}
+/**
+ * Whether one summary answers a query.
+ *
+ * Shared by every adapter on purpose. The catalog is one surface over several
+ * sources, so a filter that quietly worked for Codex and not for Claude Code
+ * would be worse than no filter — the user cannot see which source dropped
+ * their term. Keeping the decision here means a new adapter inherits the
+ * behaviour instead of reimplementing it.
+ */
+export function externalSessionMatchesQuery(
+ summary: ExternalSessionSummary,
+ query: ExternalSessionQuery = {},
+): boolean {
+ if (!query.includeArchived && summary.archived) return false;
+ if (query.cwd !== undefined && !sameExternalSessionPath(summary.cwd, query.cwd)) return false;
+ const text = normalizeExternalSessionQueryText(query.text);
+ if (text === undefined) return true;
+ // Title and path, because those are the two things a user remembers about a
+ // 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.
+ //
+ // Candidate and term pass through the same normalizer. Without it a term
+ // pasted from a Windows path missed a stored forward-slash path that
+ // `sameExternalSessionPath` already calls the same project, and a title
+ // typed in NFC missed one macOS recorded in NFD.
+ // Separator folding is applied to the path pair only, never to the title.
+ // Folding a title would make a search for `/n` match a title containing a
+ // literal backslash-n, and folding the term without the title would stop
+ // `\\n` from finding the very title it names. The path pair has no such
+ // ambiguity: a separator there is a separator.
+ return (
+ normalizeExternalSessionMatchText(summary.name).includes(text) ||
+ foldExternalSessionPathSeparators(normalizeExternalSessionMatchText(summary.cwd)).includes(
+ foldExternalSessionPathSeparators(text),
+ )
+ );
+}
+
+/**
+ * The comparable form of one side of a text match.
+ *
+ * Three normalizations, each for a difference that is not a difference to the
+ * person searching:
+ *
+ * - **NFC** — macOS records decomposed filenames, so the same visible name can
+ * arrive composed or decomposed depending on where it was typed.
+ * - **case** — nobody searching for a project remembers its capitalisation.
+ * - **separators** — a term pasted from a Windows path should still find the
+ * project the summary stored with forward slashes, matching the equivalence
+ * `sameExternalSessionPath` already applies to the `cwd` filter.
+ *
+ * Applied to the title as well as the path. A title rarely holds a separator,
+ * but running one normalizer over both is what keeps this a single authority
+ * rather than two rules free to drift.
+ */
+function normalizeExternalSessionMatchText(value: string): string {
+ return value.normalize('NFC').toLowerCase();
+}
+
+/**
+ * Windows separators folded to the POSIX form, so a term pasted from one
+ * 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 {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(core): search the External Session catalog by title and path [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3471:
URL: https://github.com/apache/maka/pull/3471#discussion_r3835650054
##########
apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx:
##########
@@ -5,6 +5,7 @@ import { CheckboxInput } from '@astryxdesign/core/CheckboxInput';
import { EmptyState } from '@astryxdesign/core/EmptyState';
import { List, ListItem } from '@astryxdesign/core/List';
import { SegmentedControl, SegmentedControlItem } from '@astryxdesign/core/SegmentedControl';
+import { TextInput } from '@astryxdesign/core/TextInput';
Review Comment:
[P1] Adding this Astryx component changes the committed surface inventory, but the generated Markdown was not updated. The current exact head deterministically fails the required `Astryx surface inventory` CI step because `docs/astryx-surface-file-inventory.md` still omits `TextInput` for this file. Please run `npm run astryx:surface-inventory:write` and commit the generated Markdown update; the `.paths` inventory is already unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org