feat: find threads by PR number in the command palette - #2294
jonathanKingston merged 5 commits into
Conversation
Cmd/Ctrl+Shift+K matched threads on title and project name only, so the thread that opened a PR was unfindable by the number everyone actually refers to it by. The data was already on disk — `Thread.prRefs` caches every GitHub PR a thread touched — but it never reached the palette, because the palette reads `catalog.jsonl` and `ThreadCatalogEntry` dropped the field. So carry `prRefs` on the catalog entry: `catalogEntryFromDisk` already reads the `meta.json` that holds it, making this free of extra I/O and of any transcript fold. PR keys are `owner/repo#number`, so `2262`, `#2262` and `#2262` all match as plain substrings with no query parsing. A pasted PR URL is the one spelling that would not, so `queryTerms` folds it through the existing `parseGithubPrUrl` to that same shape — a link copied out of the PR pane and the bare number from the same PR should not disagree. Thread rows now show a `#2262` chip: without it a bare-number query returns a title with no visible reason for the hit. The new field is required rather than optional so that `readCatalog` rejects every line of a pre-`prRefs` catalog. Those lines then look like missing threads to `catalogMissingIndexedThreads`, which reuses the existing `rebuildCatalogFromDisk` to backfill — otherwise every thread predating the field would stay silently unsearchable by PR number. That rejection has one hazard: `upsertCatalogEntry` and `refreshCatalogLine` read the catalog and write the whole map back, so the first message append against an old file would have persisted a one-line index and dropped every other thread. Both now read through `ensureCatalogMap`, which rebuilds from thread dirs first. Threads are still matched by title, so a title containing `2262` hits too. The `@`-thread mention picker shares `loadProjectCatalog` and picks up PR matching as well. Issue numbers are deliberately not covered: nothing stores a GitHub issue against a thread today (the only issue refs live in roadmap note frontmatter), so that needs a new persisted field and a decision on ambiguous bare `#123`, which GitHub itself resolves to either kind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reference screenshots ready for reviewReview GitHub’s image diffs in screenshot PR #2339. Rendered for If this source branch moves, a later successful render closes the stale review PR and replaces this link. |
jonathanKingston
left a comment
There was a problem hiding this comment.
Review: find threads by PR number
Verdict: changes needed before merge. The migration design holds up under probing, but the branch now conflicts with main and the visible change has no screenshot evidence.
Validation
| Check | Result |
|---|---|
pnpm run typecheck |
pass |
oxfmt --check on the 7 changed files |
pass |
eslint on changed .ts files |
pass |
pnpm test -- thread-store command-palette mention-picker |
102/102 pass |
pnpm run check:dead-code |
pass |
Ad-hoc probe: pre-prRefs catalog with 3 threads, then deleteProjectThread, updateMeta({archivedAt}), updateMeta({title}), a mixed old/new file, and a malformed prRefs line |
no entry lost in any case; rebuild persisted once |
Dry merge with origin/main |
CONFLICT in src/main/services/thread-store.ts and src/shared/types/thread.ts |
Blocking
-
Rebase onto
main. #2317 moved the store intopackages/thread-store:src/main/services/thread-store.tsis now a 5-line re-export andThreadCatalogEntrylives atpackages/thread-store/src/thread-types.ts:296. TheexistsSync ? readCatalog : rebuildpattern this PR fixes is still present in the package copy (upsertCatalogEntryatpackages/thread-store/src/thread-store.ts:876,refreshCatalogLineat:923), so the fix has to be re-applied there rather than in the shim. -
Visual evidence. The
#2262chip (command-palette.ts:229-237,command-palette.css:72-80) and the new placeholder (command-palette.ts:118) are user-visible, buttests/is untouched.tests/e2e/command-palette.e2e.tsseeds four threads with noprRefsand typeslogin, socommand-palette.pngcannot show either. #2296 touches 12 screenshots and none iscommand-palette.png, so it is unrelated rendering drift, not evidence for this PR.seedThreadDirwritesmeta.jsonfrom the seed object verbatim, so addingprRefsto one seeded thread, searching by number, asserting.command-palette-pr, and saving a screenshot is a small addition.
Migration analysis
Every catalog reader-then-writer on the head:
upsertCatalogEntry(:904) andrefreshCatalogLine(:954) go throughensureCatalogMap. Safe.saveProjectThreads(:1331) writes wholesale from its argument by design. Safe.deleteProjectThread(:1631-1637) is the only remaining directreadCatalog+writeCatalog. Against an all-stale filereadCatalogreturns an empty map, the delete returns false and nothing is written; against a mixed file it writes only the new-format lines. Both self-heal becausecatalogMissingIndexedThreadschecks every non-archived thread dir against the map. No data loss, but routing it throughensureCatalogMapwould make the invariant uniform.- A malformed
prRefsline drops that line, which triggers rebuild frommeta.json. Rebuild cost ismeta.jsonplusparseSpineofevents.jsonlfor the first user message, pre-existing and one-off.
Required rather than optional is defensible: the catalog is derived and rebuildable, and optional-with-[] would leave every existing thread silently unsearchable by PR with no backfill. Older builds reading the newer catalog ignore the key and drop it on write; the newer build then rebuilds.
Non-blocking
upsertCatalogEntryandrefreshCatalogLinenow runcatalogMissingIndexedThreadson every append and meta patch: areaddirSyncplus ameta.jsonread per archived thread. Projects with many archived threads pay that per write. HavingreadCatalogreport rejected lines would keep the rebuild trigger precise without the scan.- Substring matching means
2262also hits #22620 and any title containing 2262, and rows sort byupdatedAtwith no preference for the exact PR. Anchoring numeric terms to the end of the key would fix it. - Main-side
loadProjectCatalog(:1899) does not fold PR URLs; only the rendererqueryTermsdoes, soapi.threads.catalog(id, url)from the mention picker returns nothing for a pasted link. queryTermsdoes not strip trailing punctuation from a pasted URL the wayextractGithubPrUrlsdoes, so.../pull/2262.stays a raw URL and matches nothing.
Generated by Claude Code
…fi-08afd2 Resolves conflicts from #2317 (thread store extracted to @copse/thread-store): take main's re-export shims for src/main/services/thread-store.ts and src/shared/types/thread.ts, and re-apply this branch's catalog prRefs changes inside packages/thread-store/src/.
`deleteProjectThread` was the last catalog writer reading the index with `readCatalog` directly. Against a pre-`prRefs` catalog — every line of which this build rejects — that read is empty, so the delete found nothing to remove and, had it written, would have replaced the index with nothing. Route it through `ensureCatalogMap` like `upsertCatalogEntry` and `refreshCatalogLine`, so all three writers share one rebuild-on-read invariant, and add the matching regression test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jaQbed96h8aSZLijfiHNB
…with punctuation Two follow-ups to PR-number thread search: - A PR URL pasted out of prose keeps its sentence punctuation (`…/pull/2262.`), which `parseGithubPrUrl` rejects. `queryTerms` now strips it the same way the transcript scraper does, via a shared `stripUrlTrailingPunctuation`, so the pasted link and the bare number agree. - A numeric term (`2262`, `#2262`) matched PR keys as a substring and so also surfaced #22620. `githubPrKeyMatchesTerm` pins a numeric term to the whole number while non-numeric terms (`owner/repo#2262`, `agent-pane#`) still match as substrings. Title/digest/project-name substring matching is unchanged. Applied in both the palette and `loadProjectCatalog` so the two agree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jaQbed96h8aSZLijfiHNB
Seed the target thread with a `prRefs` entry in the shape `gh_pr_create` records on `meta.json`, query its bare PR number, assert the single matching row and its `.command-palette-pr` chip, and save `command-palette-pr-number.png` (a new reference; `command-palette.png` is untouched). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jaQbed96h8aSZLijfiHNB
|
Pushed four commits. e19a22a merges Generated by Claude Code |
🖥️ PR preview
|
Summary
Cmd/Ctrl+Shift+K now finds a thread by the GitHub PR it touched. Searching
2262,#2262,copse-dev/agent-pane#2262, or a pasted PR URL all narrow to the thread that opened #2262.Why
The palette matched threads on title and project name only, so the thread behind a PR was unfindable by the number everyone actually refers to it by.
The data was already on disk.
Thread.prRefscaches every PR a thread touched — scraped from message prose, recorded directly bygh_pr_create, and filled in fromremoteAgentLink.prUrlfor cloud-agent runs. It just never reached the palette: the palette readscatalog.jsonl, andThreadCatalogEntrydropped the field.What changed
Index the refs.
ThreadCatalogEntrycarriesprRefs.catalogEntryFromDiskalready reads themeta.jsonthat holds them, so this costs no extra I/O and no transcript fold.Match without a parser. PR keys are
owner/repo#number, so2262,#2262andcopse-dev/agent-pane#2262all hit as plain substrings. A pasted PR URL is the one spelling that would not, soqueryTermsfolds it through the existingparseGithubPrUrlto that same shape — a link copied out of the PR pane and the bare number from the same PR should not disagree.Show why a row matched. Thread rows render a
#2262chip; without it a bare-number query returns a title with no visible reason for the hit.Migration, and the hazard it created
The new field is required, not optional, so
readCatalogrejects every line of a pre-prRefscatalog. Those lines then look like missing threads tocatalogMissingIndexedThreads, which reuses the existingrebuildCatalogFromDiskto backfill. Without that, every thread predating the field would stay silently unsearchable by PR number.That rejection has a sharp edge worth reviewing closely:
upsertCatalogEntryandrefreshCatalogLineread the catalog and write the whole map back, so the first message append against an old file would have persisted a one-line index and dropped every other thread. Both now read throughensureCatalogMap, which rebuilds from thread dirs first. There is a regression test that appends a message against a stale two-thread catalog and asserts both survive.Notes for reviewers
2262hits too. That is existing substring behaviour, not a regression.@-thread mention picker sharesloadProjectCatalogand picks up PR matching as well.matches()no longer re-splits the query per candidate row; terms are computed once per keystroke.refreshCatalogLinenow costs one extrareaddirper message append (viaensureCatalogMap), alongside the catalog read and write it already did.Issue numbers are deliberately out of scope
Nothing stores a GitHub issue against a thread today — the only issue refs in the codebase live in roadmap note frontmatter (
parseIssueRef), never on thread metadata. Covering issues needs a new persisted field, a scraper widened to/issues/, and a decision on bare#123, which GitHub itself resolves to either a PR or an issue. That is its own piece of work.Validation
npm run check— typecheck, lint, format, dead-code, oracle and e2e-syntax all clean. Unit suite: 8240/8262 passed. The 13 failures are the known host-flaky SSH/askpass/socket-path and workspace-indexing set (ssh-workspace/openssh-transport,ssh-workspace/ssh-paths,ssh-workspace/askpass,search/workspace-indexing); none of those files reference anything in this diff, and they reproduce identically in isolation.npm test -- thread-store— 83 passednpm test -- command-palette mention-picker— 19 passed🤖 Generated with Claude Code
Co-Authored-By: Copse noreply@copse.dev
Copse-Models: acp:claude-acp#opus[1m]