Repository navigation
feat: session ID lookup in CommandPalette (Cmd+K) - #153
Conversation
- Add sessionIdValidator shared utility (isUUID, isSessionIdFragment) - Add ProjectScanner.findSessionById and findSessionsByPartialId - Add find-session-by-id and find-sessions-by-partial-id IPC handlers - Add HTTP routes /api/sessions/:id/locate and /api/sessions/search-by-id/:fragment - Wire preload bridge and HttpAPIClient methods - Update CommandPalette to detect UUID and hex fragments, show session match UI - Add tests for sessionIdValidator and IPC guard contracts (27 cases) Closes matt1398#151 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the ability to search for sessions by their full UUID or a partial ID fragment across all projects. Key changes include new scanning logic in ProjectScanner using batched directory lookups, the addition of corresponding IPC and HTTP endpoints, and a UI update to the CommandPalette to detect and display these matches. Feedback focuses on optimizing the findSessionById method to terminate early once a match is found and simplifying the project name resolution logic in the renderer for better readability.
| const matches = await this.collectFulfilledInBatches( | ||
| projectDirs, | ||
| this.fsProvider.type === 'ssh' ? 8 : 24, | ||
| async (dir) => { | ||
| const sessionPath = buildSessionPath(this.projectsDir, dir.name, sessionId); | ||
| if (await this.fsProvider.exists(sessionPath)) { | ||
| return dir.name; | ||
| } | ||
| return null; | ||
| } | ||
| ); | ||
|
|
||
| const matchedProjectId = matches.find((m) => m !== null); |
There was a problem hiding this comment.
The current implementation of findSessionById uses collectFulfilledInBatches to check for the session file in all project directories. This is inefficient as it continues to scan all directories even after a match has been found. For a 'find first' operation like this, it would be more performant to process directories in batches and stop as soon as the first match is located.
| projectName={ | ||
| (sessionIdMatch.projectId | ||
| ? projectNameByWorktreeId.get(sessionIdMatch.projectId) | ||
| : undefined) ?? | ||
| sessionIdMatch.projectId ?? | ||
| 'Unknown' | ||
| } |
There was a problem hiding this comment.
The logic to determine the projectName is a bit complex and can be simplified for better readability. The current implementation is correct, but a more direct approach would be easier to maintain.
| projectName={ | |
| (sessionIdMatch.projectId | |
| ? projectNameByWorktreeId.get(sessionIdMatch.projectId) | |
| : undefined) ?? | |
| sessionIdMatch.projectId ?? | |
| 'Unknown' | |
| } | |
| projectName={ | |
| sessionIdMatch.projectId | |
| ? projectNameByWorktreeId.get(sessionIdMatch.projectId) ?? | |
| sessionIdMatch.projectId | |
| : 'Unknown' | |
| } |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds session-ID lookup support across the app: exact UUID and partial-fragment search. Implements ProjectScanner methods, IPC and HTTP endpoints, preload and renderer bridges, validation utilities, CommandPalette UI changes, and unit tests; also updates .gitignore. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (9)
.gitignore (1)
1-2: Unrelated change in this PR.This .gitignore addition for
claude-code-transcripts/is not mentioned in the PR summary or objectives, which focus exclusively on session ID lookup in CommandPalette. While the change itself is harmless, including unrelated modifications can make PR history harder to trace and review.Consider moving this to a separate commit or PR to keep this PR focused on its stated objective (issue
#151).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 1 - 2, The .gitignore addition for claude-code-transcripts/ is an unrelated change to this PR; remove that line from the current commit and instead create a separate commit (or separate PR) that adds "claude-code-transcripts/" to .gitignore so this PR only contains the CommandPalette session ID lookup changes; locate the .gitignore entry "claude-code-transcripts/" in the diff and revert it from this branch, then commit the .gitignore change separately with a descriptive message referencing the separate change.src/preload/index.ts (1)
142-144: Consider using IPC channel constants for consistency.The new methods use hardcoded channel strings directly (
'find-session-by-id','find-sessions-by-partial-id'). While this follows the existing pattern in this file, the coding guidelines recommend defining channel constants inconstants/ipcChannels.tsfor maintainability.This is optional since the existing code follows the same pattern, but centralizing channel names helps prevent typos and makes refactoring easier.
💡 Example using constants
In
src/preload/constants/ipcChannels.ts:export const FIND_SESSION_BY_ID = 'find-session-by-id'; export const FIND_SESSIONS_BY_PARTIAL_ID = 'find-sessions-by-partial-id';In
src/preload/index.ts:+import { + FIND_SESSION_BY_ID, + FIND_SESSIONS_BY_PARTIAL_ID, +} from './constants/ipcChannels'; - findSessionById: (sessionId: string) => ipcRenderer.invoke('find-session-by-id', sessionId), - findSessionsByPartialId: (fragment: string) => - ipcRenderer.invoke('find-sessions-by-partial-id', fragment), + findSessionById: (sessionId: string) => ipcRenderer.invoke(FIND_SESSION_BY_ID, sessionId), + findSessionsByPartialId: (fragment: string) => + ipcRenderer.invoke(FIND_SESSIONS_BY_PARTIAL_ID, fragment),Based on learnings: "Define channel constant in
constants/ipcChannels.ts"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/preload/index.ts` around lines 142 - 144, Replace the hardcoded IPC channel strings in the preload methods with channel constants: add exports (e.g. FIND_SESSION_BY_ID and FIND_SESSIONS_BY_PARTIAL_ID) to src/preload/constants/ipcChannels.ts and import them into src/preload/index.ts, then update the findSessionById and findSessionsByPartialId methods to call ipcRenderer.invoke(FIND_SESSION_BY_ID, sessionId) and ipcRenderer.invoke(FIND_SESSIONS_BY_PARTIAL_ID, fragment) respectively so channel names are centralized and typo-safe.test/shared/utils/sessionIdValidator.test.ts (1)
38-62: Consider adding a whitespace trimming test forisSessionIdFragment.The
isUUIDtests include a whitespace trimming test (line 21-23), butisSessionIdFragmentdoesn't have an equivalent test even though the implementation also trims input. This would ensure consistent behavior documentation.💡 Suggested additional test case
it('accepts hex-with-dashes', () => { expect(isSessionIdFragment('abc-def')).toBe(true); }); + + it('trims surrounding whitespace', () => { + expect(isSessionIdFragment(' abc ')).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/shared/utils/sessionIdValidator.test.ts` around lines 38 - 62, Add a test in the isSessionIdFragment describe block to verify the function trims surrounding whitespace before validation: create a case that passes a valid fragment with leading/trailing spaces (e.g., " abc " or "abc-def" with spaces) and assert it returns true, and optionally a case where spaces surround an invalid fragment to assert false; target the isSessionIdFragment tests in test/shared/utils/sessionIdValidator.test.ts so behavior mirrors the existing isUUID trimming test.src/renderer/components/search/CommandPalette.tsx (3)
18-19: Import statement after variable declaration.The import on line 19 appears after the
loggerconst declaration on line 18. Imports should be grouped at the top of the file.♻️ Move import to proper location
import { createLogger } from '@shared/utils/logger'; +import { isSessionIdFragment, isUUID } from '@shared/utils/sessionIdValidator'; import { useShallow } from 'zustand/react/shallow'; const logger = createLogger('Component:CommandPalette'); -import { isSessionIdFragment, isUUID } from '@shared/utils/sessionIdValidator'; import { formatDistanceToNow } from 'date-fns';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/search/CommandPalette.tsx` around lines 18 - 19, Move the stray import so all imports are grouped at the top of the file: relocate the "isSessionIdFragment" and "isUUID" import (from '@shared/utils/sessionIdValidator') above the `const logger = createLogger('Component:CommandPalette');` declaration and with the other imports; ensure the `logger` const remains after the import block and that no other code appears before imports in CommandPalette.tsx.
336-367: UUID lookup uses a very short 50ms debounce.The 50ms debounce for
findSessionByIdmay trigger multiple API calls while the user is still pasting a UUID. Since UUIDs are typically pasted rather than typed character-by-character, this is likely fine in practice. However, if users type UUIDs manually, increasing to 150-200ms would reduce unnecessary calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/search/CommandPalette.tsx` around lines 336 - 367, In CommandPalette's useEffect that triggers findSessionById, increase the debounce delay from 50ms to a safer 150–200ms to avoid multiple rapid API calls while users paste or type UUIDs; update the setTimeout duration (in the block that increments latestSearchRequestRef and calls api.findSessionById) to 150 or 200ms and keep the existing request-cancellation logic (latestSearchRequestRef checks, setLoading finally block) intact.
83-88: Consider defensive handling for invalidcreatedAttimestamps.If
createdAtis0,NaN, or an invalid timestamp,formatDistanceToNow(new Date(createdAt))could produce unexpected output or throw. The same pattern is used on line 727. Consider adding a fallback:createdAt > 0 ? formatDistanceToNow(new Date(createdAt), { addSuffix: true }) : 'Unknown'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/search/CommandPalette.tsx` around lines 83 - 88, The timestamp rendering uses formatDistanceToNow(new Date(createdAt)) which can produce bad output for invalid values; update the JSX in CommandPalette (the span that currently renders formatDistanceToNow(new Date(createdAt), { addSuffix: true })) to defensively check createdAt (e.g., createdAt > 0 or Number.isFinite(createdAt)) and render a fallback like "Unknown" when invalid, and apply the same defensive check where the same pattern appears elsewhere (the other occurrence around the symbol on line ~727) to avoid exceptions or misleading text.test/main/ipc/searchSessionId.test.ts (1)
34-41: Shape assertion tests are tautological and provide no coverage.These tests (lines 34-41 and 77-84) construct a local object literal and then assert properties on that same literal. They don't invoke the actual handler, so they verify nothing about real behavior—just that
{ found: false }.found === false.Consider either removing these or replacing them with integration tests that call the actual handler function (e.g.,
handleFindSessionById) with mocked services to verify the early-exit shape is returned when validation fails.Also applies to: 77-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/ipc/searchSessionId.test.ts` around lines 34 - 41, The test is asserting a local literal rather than exercising real behavior; replace the tautological assertion by invoking the actual logic (call validateSessionId and then the handler function handleFindSessionById or the public entry that uses it) with an invalid session id and a mocked scanner/service to ensure the function returns the early-exit shape { found: false }; specifically, in the test use validateSessionId('') to get the invalidResult and then call handleFindSessionById (or the exported handler that uses validateSessionId) with the same input and a minimal mock for dependencies to assert the real return value equals { found: false } (or remove the redundant test entirely if coverage is provided elsewhere).src/main/ipc/search.ts (2)
8-27: Import order does not follow guidelines.As per coding guidelines, imports should be organized: external packages → path aliases (
@shared,@main, etc.) → relative imports. Currently electron is imported after@shared, and the logger import from@sharedis separated from the other@sharedimport.♻️ Suggested import order
+import { type IpcMain, type IpcMainInvokeEvent } from 'electron'; + import { createLogger } from '@shared/utils/logger'; import { isSessionIdFragment } from '@shared/utils/sessionIdValidator'; -import { type IpcMain, type IpcMainInvokeEvent } from 'electron'; import { type FindSessionByIdResult, type FindSessionsByPartialIdResult, type SearchSessionsResult, } from '../types'; +import type { ServiceContextRegistry } from '../services'; import { coerceSearchMaxResults, validateProjectId, validateSearchQuery, validateSessionId, } from './guards'; const logger = createLogger('IPC:search'); - -import type { ServiceContextRegistry } from '../services';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/search.ts` around lines 8 - 27, Reorder the imports to follow the project guideline: group external packages first (move "electron" import up), then path-alias imports together (put both "@shared" imports—createLogger and isSessionIdFragment—and any other `@shared/`@main imports together), followed by local/project imports (the types from '../types', guards from './guards', and ServiceContextRegistry). Ensure createLogger and isSessionIdFragment imports are adjacent, keep the logger const assignment after those imports, and maintain the existing named type imports (IpcMain, IpcMainInvokeEvent, FindSessionByIdResult, FindSessionsByPartialIdResult, SearchSessionsResult, ServiceContextRegistry) in their appropriate groups.
159-162: Redundanttypeofcheck.
isSessionIdFragmentshould already handle non-string inputs gracefully (returningfalse). The explicittypeof fragment !== 'string'check is defensive but redundant if the validator is properly typed and implemented.This is minor—keeping it is fine for extra safety, but worth noting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/search.ts` around lines 159 - 162, The check combines typeof fragment !== 'string' with isSessionIdFragment(fragment), which is redundant if isSessionIdFragment already returns false for non-strings; remove the redundant typeof check and rely solely on isSessionIdFragment(fragment) in the validation branch (the logger/error return around the find-sessions-by-partial-id handler) so the code reads: if (!isSessionIdFragment(fragment)) { logger.error(...); return { found: false, results: [] }; } ensuring fragment and isSessionIdFragment are the only referenced symbols to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.gitignore:
- Around line 1-2: The .gitignore addition for claude-code-transcripts/ is an
unrelated change to this PR; remove that line from the current commit and
instead create a separate commit (or separate PR) that adds
"claude-code-transcripts/" to .gitignore so this PR only contains the
CommandPalette session ID lookup changes; locate the .gitignore entry
"claude-code-transcripts/" in the diff and revert it from this branch, then
commit the .gitignore change separately with a descriptive message referencing
the separate change.
In `@src/main/ipc/search.ts`:
- Around line 8-27: Reorder the imports to follow the project guideline: group
external packages first (move "electron" import up), then path-alias imports
together (put both "@shared" imports—createLogger and isSessionIdFragment—and
any other `@shared/`@main imports together), followed by local/project imports
(the types from '../types', guards from './guards', and ServiceContextRegistry).
Ensure createLogger and isSessionIdFragment imports are adjacent, keep the
logger const assignment after those imports, and maintain the existing named
type imports (IpcMain, IpcMainInvokeEvent, FindSessionByIdResult,
FindSessionsByPartialIdResult, SearchSessionsResult, ServiceContextRegistry) in
their appropriate groups.
- Around line 159-162: The check combines typeof fragment !== 'string' with
isSessionIdFragment(fragment), which is redundant if isSessionIdFragment already
returns false for non-strings; remove the redundant typeof check and rely solely
on isSessionIdFragment(fragment) in the validation branch (the logger/error
return around the find-sessions-by-partial-id handler) so the code reads: if
(!isSessionIdFragment(fragment)) { logger.error(...); return { found: false,
results: [] }; } ensuring fragment and isSessionIdFragment are the only
referenced symbols to locate the change.
In `@src/preload/index.ts`:
- Around line 142-144: Replace the hardcoded IPC channel strings in the preload
methods with channel constants: add exports (e.g. FIND_SESSION_BY_ID and
FIND_SESSIONS_BY_PARTIAL_ID) to src/preload/constants/ipcChannels.ts and import
them into src/preload/index.ts, then update the findSessionById and
findSessionsByPartialId methods to call ipcRenderer.invoke(FIND_SESSION_BY_ID,
sessionId) and ipcRenderer.invoke(FIND_SESSIONS_BY_PARTIAL_ID, fragment)
respectively so channel names are centralized and typo-safe.
In `@src/renderer/components/search/CommandPalette.tsx`:
- Around line 18-19: Move the stray import so all imports are grouped at the top
of the file: relocate the "isSessionIdFragment" and "isUUID" import (from
'@shared/utils/sessionIdValidator') above the `const logger =
createLogger('Component:CommandPalette');` declaration and with the other
imports; ensure the `logger` const remains after the import block and that no
other code appears before imports in CommandPalette.tsx.
- Around line 336-367: In CommandPalette's useEffect that triggers
findSessionById, increase the debounce delay from 50ms to a safer 150–200ms to
avoid multiple rapid API calls while users paste or type UUIDs; update the
setTimeout duration (in the block that increments latestSearchRequestRef and
calls api.findSessionById) to 150 or 200ms and keep the existing
request-cancellation logic (latestSearchRequestRef checks, setLoading finally
block) intact.
- Around line 83-88: The timestamp rendering uses formatDistanceToNow(new
Date(createdAt)) which can produce bad output for invalid values; update the JSX
in CommandPalette (the span that currently renders formatDistanceToNow(new
Date(createdAt), { addSuffix: true })) to defensively check createdAt (e.g.,
createdAt > 0 or Number.isFinite(createdAt)) and render a fallback like
"Unknown" when invalid, and apply the same defensive check where the same
pattern appears elsewhere (the other occurrence around the symbol on line ~727)
to avoid exceptions or misleading text.
In `@test/main/ipc/searchSessionId.test.ts`:
- Around line 34-41: The test is asserting a local literal rather than
exercising real behavior; replace the tautological assertion by invoking the
actual logic (call validateSessionId and then the handler function
handleFindSessionById or the public entry that uses it) with an invalid session
id and a mocked scanner/service to ensure the function returns the early-exit
shape { found: false }; specifically, in the test use validateSessionId('') to
get the invalidResult and then call handleFindSessionById (or the exported
handler that uses validateSessionId) with the same input and a minimal mock for
dependencies to assert the real return value equals { found: false } (or remove
the redundant test entirely if coverage is provided elsewhere).
In `@test/shared/utils/sessionIdValidator.test.ts`:
- Around line 38-62: Add a test in the isSessionIdFragment describe block to
verify the function trims surrounding whitespace before validation: create a
case that passes a valid fragment with leading/trailing spaces (e.g., " abc " or
"abc-def" with spaces) and assert it returns true, and optionally a case where
spaces surround an invalid fragment to assert false; target the
isSessionIdFragment tests in test/shared/utils/sessionIdValidator.test.ts so
behavior mirrors the existing isUUID trimming test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ad2de15-371f-452b-aa7d-5af2d4b7961d
📒 Files selected for processing (12)
.gitignoresrc/main/http/search.tssrc/main/ipc/search.tssrc/main/services/discovery/ProjectScanner.tssrc/main/types/domain.tssrc/preload/index.tssrc/renderer/api/httpClient.tssrc/renderer/components/search/CommandPalette.tsxsrc/shared/types/api.tssrc/shared/utils/sessionIdValidator.tstest/main/ipc/searchSessionId.test.tstest/shared/utils/sessionIdValidator.test.ts
7c75e3a to
fc63231
Compare
fix: address code review feedback from #153 (session ID lookup)
Summary
sessionIdValidatorshared utility (isUUID,isSessionIdFragment) used by both IPC/HTTP validation layers and the rendererProjectScanner.findSessionById/findSessionsByPartialIdmethods scan all projectsfind-session-by-id,find-sessions-by-partial-idGET /api/sessions/:sessionId/locate,GET /api/sessions/search-by-id/:fragmentCloses #151
Test plan
pnpm typecheck— cleanpnpm lint— clean (pre-existing unrelated warning only)pnpm test— 680/680 passpnpm build— clean🤖 Generated with Claude Code
Summary by CodeRabbit