feat(command-palette): jump to agents and sessions by name#358
Open
kimbotai1337 wants to merge 3 commits into
Open
feat(command-palette): jump to agents and sessions by name#358kimbotai1337 wants to merge 3 commits into
kimbotai1337 wants to merge 3 commits into
Conversation
Indexes the live session list as command-palette entries so Cmd+K can navigate to top-level agents, nested subagents, and cron sessions by typing part of their label, displayName, identityName, root agent id, or session key. The active session is marked with aria-current and a visible "current" badge. Selecting a result reuses handleSessionChange, which keeps the dirty-workspace guard intact. Closes #180
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis pull request implements session-based navigation in the command palette. Users can now type ChangesSession command palette feature
Sequence DiagramsequenceDiagram
participant User
participant CommandPalette
participant filterCommands
participant createSessionCommands
participant App
participant SessionStore
App->>SessionStore: read sessions, currentSessionKey
App->>createSessionCommands: create session commands
createSessionCommands-->>App: Command[] with isActive flags
User->>CommandPalette: Cmd+K open palette
User->>CommandPalette: type session name
CommandPalette->>filterCommands: filter(allCommands, query)
filterCommands-->>CommandPalette: matching results sorted by category
CommandPalette->>CommandPalette: render Sessions group with isActive affordance
User->>CommandPalette: Enter on selected session
CommandPalette->>CommandPalette: schedule 50ms deferred action
CommandPalette->>CommandPalette: close palette immediately
CommandPalette->>App: onSelectSession(sessionKey)
App->>SessionStore: switch to session
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/features/command-palette/CommandPalette.test.tsx (1)
172-190: ⚡ Quick winThis test currently validates input-reset behavior, not the clamp logic.
Because typing triggers
setSelectedIndex(0), the assertion can pass even if the clamp effect is broken. Prefer narrowing via rerenderedcommands(or another non-typing path) to isolate the clamp behavior.Suggested adjustment
- it('clamps selectedIndex when typing narrows the filtered list below the cursor', () => { + it('clamps selectedIndex when filtered commands shrink below the cursor', () => { const first = vi.fn(); const second = vi.fn(); - const commands = [ + const onClose = vi.fn(); + const commands: Command[] = [ sessionCommand('agent:alpha:main', 'Alpha', { keywords: ['Alpha'], action: first }), sessionCommand('agent:beta:main', 'Beta', { keywords: ['Beta'], action: second }), sessionCommand('agent:gamma:main', 'Gamma', { keywords: ['Gamma'], action: vi.fn() }), ]; - renderPalette(commands); + const { rerender } = render(<CommandPalette open commands={commands} onClose={onClose} />); + act(() => { vi.advanceTimersByTime(100); }); // Move cursor to the third entry. fireEvent.keyDown(input(), { key: 'ArrowDown' }); fireEvent.keyDown(input(), { key: 'ArrowDown' }); - // Type a query that narrows results to one match (the first entry). - fireEvent.change(input(), { target: { value: 'Alpha' } }); + + // Shrink filtered list without typing (avoids reset-to-0 from input onChange). + rerender(<CommandPalette open commands={[commands[0]]} onClose={onClose} />); fireEvent.keyDown(input(), { key: 'Enter' }); act(() => { vi.advanceTimersByTime(100); }); expect(first).toHaveBeenCalledTimes(1); expect(second).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/command-palette/CommandPalette.test.tsx` around lines 172 - 190, The test is exercising input-reset behavior because typing calls setSelectedIndex(0); replace the typing-based narrowing with a rerender of the component using a filtered commands array to test the clamp logic directly: renderPalette(commands), move the cursor to the third index via fireEvent.keyDown(input(), { key: 'ArrowDown' }) twice, then call the test harness's rerender (or call renderPalette again) with a new commands array that only contains the single matching command (e.g., the 'Alpha' entry) so selectedIndex must be clamped, then simulate Enter and assert first was invoked and second was not; reference setSelectedIndex/selectedIndex, renderPalette, input(), and the command fixtures (sessionCommand) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/command-palette/CommandPalette.tsx`:
- Around line 48-51: The current clamp only applies an upper bound and allows
negative selectedIndex; update the clamp in the useEffect (the setSelectedIndex
call) to enforce both lower and upper bounds and likewise fix the ArrowDown
handler so it never sets -1. Concretely, replace the current Math.min(...)
expression with a two-sided clamp like Math.max(0, Math.min(i, filtered.length -
1)) in the useEffect and apply the same clamping logic in the ArrowDown handler
(and the similar update at lines 79-83) so selectedIndex always stays within [0,
filtered.length - 1].
---
Nitpick comments:
In `@src/features/command-palette/CommandPalette.test.tsx`:
- Around line 172-190: The test is exercising input-reset behavior because
typing calls setSelectedIndex(0); replace the typing-based narrowing with a
rerender of the component using a filtered commands array to test the clamp
logic directly: renderPalette(commands), move the cursor to the third index via
fireEvent.keyDown(input(), { key: 'ArrowDown' }) twice, then call the test
harness's rerender (or call renderPalette again) with a new commands array that
only contains the single matching command (e.g., the 'Alpha' entry) so
selectedIndex must be clamped, then simulate Enter and assert first was invoked
and second was not; reference setSelectedIndex/selectedIndex, renderPalette,
input(), and the command fixtures (sessionCommand) when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 047fc43a-0c47-4215-a5fb-3fd11485d279
📒 Files selected for processing (8)
src/App.test.tsxsrc/App.tsxsrc/features/command-palette/CommandPalette.test.tsxsrc/features/command-palette/CommandPalette.tsxsrc/features/command-palette/commands.test.tssrc/features/command-palette/commands.tssrc/features/command-palette/index.tssrc/features/command-palette/types.ts
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…test CR pass on the round-1 PR caught two related issues on top of the autofix round: - ArrowDown set selectedIndex to -1 when filtered.length was 0 (Math.min(i + 1, -1)). The clamp useEffect did not recover because Math.min(-1, 0) is -1. With selectedIndex at -1, Enter silently no-ops and the visible highlight disappears once results return. Fix: wrap both the clamp effect and the ArrowDown handler with Math.max(0, ...) so the lower bound is enforced too. - The existing clamps-on-narrow test typed a query into the input, which triggers the onChange handler's setSelectedIndex(0) reset; the assertion would have passed even if the clamp effect were removed. Rewrote the test to narrow via rerendered commands instead, so the clamp effect is the only recovery path. Added an empty-list regression test that proves ArrowDown on no results does not leave selectedIndex stuck below zero.
Collaborator
Author
|
Round 1 - pushed P1 fixed (1):
Nitpick fixed (1):
Validation: the cycle is the work @coderabbitai full review |
|
(ﾐ･ω･ﾐ)ﾉ ✅ Actions performedFull review triggered. |
Collaborator
Author
|
Bot cycle complete. No remaining P0/P1 findings after 1 round. CR's round-2 pass on speaks bot, ships code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
Cmd+Kcommand palette now indexes the live session list. Typing part of a session, agent, or sub-agent name surfaces matching sessions in a newSessionsresult group; selecting one switches the active session. The currently active session is marked witharia-currentand a visiblecurrentbadge.Closes #180.
What changed
createSessionCommands(sessions, currentSessionKey, agentName, onSelectSession)builder insrc/features/command-palette/commands.tsmaps each liveSessionto a paletteCommand. Matching surface (inkeywords): display label, rawlabel,displayName,identityName, root agent id, and session key. Loose substring, case-insensitive.Command.categorygains'sessions'.CATEGORY_ORDERplaces it first so the group renders above static commands.CATEGORY_LABELSaddsSessions.Command.isActive?: booleancarries the active-session flag. The palette renders the affordance only whencmd.category === 'sessions' && cmd.isActive === true, so a static command that incidentally setsisActivecannot render session chrome.src/App.tsxmemoizessessionCommandsoversessions,currentSession,agentName,handleSessionChangeand concatenates with the existingstaticCommands. The action reuseshandleSessionChange, which routes throughrequestWorkspaceTransitionso the dirty-workspace guard is preserved.Additional hardening from review
CommandPalettestores the close-animationsetTimeoutin a ref and clears it on nextexecuteCommandand on unmount. Rapid double-Enter no longer queues two independent transitions.selectedIndexis clamped onfiltered.lengthchanges. Typing a narrower query that shrinks the list below the cursor no longer leavesEnterpointing atundefined.filterCommandsspreads its empty-query input before sorting; the caller's array is no longer mutated.Test plan
npx vitest run src/features/command-palette/— 23 tests cover the builder, the renderer, the active-session affordance, keyboard navigation (ArrowDownadvances one step + returns viaArrowUp), thefiltered.lengthclamp, the double-Enter cancel path, and live-snapshot re-rendering.npx vitest run— full suite, 2129 tests passing.npx tsc --noEmitclean.npm run lintclean.npm run devshells without console errors. The palette itself is gated behind authenticated gateway connection, so the full E2E path is covered by the unit tests rather than a deploy-isolated browser run.Summary by CodeRabbit
New Features
Bug Fixes
Tests