Refactor Wizard AgentSelectionScreen into directory module#1077
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplace a monolithic AgentSelectionScreen with typed contracts, utilities, five custom hooks, focused presentational components, an orchestrating screen component, barrel exports, and Vitest test coverage. ChangesAgentSelectionScreen Module Refactoring
Sequence Diagram(s)sequenceDiagram
participant Screen as AgentSelectionScreen
participant Detection as useAgentDetection
participant Ssh as useSshRemotes
participant Focus as useAgentSelectionFocus
participant Keyboard as useAgentSelectionKeyboard
participant ConfigPanel as useAgentConfigurationPanel
Screen->>Detection: provide sshRemoteConfig, selectedAgent
Detection->>Screen: isDetecting, detectedAgents, sshConnectionError, announcement
Screen->>Ssh: sessionSshRemoteConfig
Ssh->>Screen: sshRemotes, sshRemoteConfig
Screen->>Focus: isDetecting, selectedAgent, detectedAgents
Focus->>Screen: focusedTileIndex
Screen->>Keyboard: focus state, tile refs -> keydown handler
Screen->>ConfigPanel: configuringAgentId, selectedAgent
ConfigPanel->>Screen: agentConfig, availableModels, handlers (open/close/blur/refresh)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 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 |
Greptile SummaryThis PR refactors the monolithic
Confidence Score: 4/5Safe to merge; behavior is faithfully preserved across all paths including SSH remote detection, keyboard navigation, and agent configuration. The refactor is a straight extraction with no logic changes. Two minor style issues were found — a no-op key prop on a component root element and an internal setter left in a hook's return value — neither of which affects runtime behavior. useSshRemotes.ts (unnecessary setSshRemoteConfig export) and AgentTileButton.tsx (redundant key prop) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[AgentSelectionScreen] --> B[useSshRemotes]
A --> C[useAgentDetection]
A --> D[useAgentSelectionFocus]
A --> E[useAgentConfigurationPanel]
A --> F[useAgentSelectionKeyboard]
B -->|sshRemoteConfig| C
C -->|detectedAgents, announce| E
C -->|isDetecting, detectedAgents| D
A -->|viewMode=grid| G[AgentSelectionHeader]
A -->|viewMode=grid| H[AgentGrid]
A -->|viewMode=grid| I[AgentSelectionFooter]
A -->|sshConnectionError| J[SshConnectionErrorPanel]
A -->|viewMode=config| K[AgentConfigurationView]
A -->|isDetecting| L[AgentSelectionLoading]
H --> M[AgentTileButton x N]
K --> N2[AgentConfigPanel]
G --> O[AgentLocationSelect]
K --> P[AgentLocationSelect]
Reviews (1): Last reviewed commit: "refactor wizard agent selection screen" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+36
to
+39
| return ( | ||
| <button | ||
| key={tile.id} | ||
| ref={(element) => setTileRef(index, element)} |
There was a problem hiding this comment.
key prop on a component's own root element has no effect in React — keys only participate in reconciliation when React processes a list in the parent. The meaningful key is already set on <AgentTileButton key={tile.id} ...> in AgentGrid. This key is dead code and can be removed.
Suggested change
| return ( | |
| <button | |
| key={tile.id} | |
| ref={(element) => setTileRef(index, element)} | |
| return ( | |
| <button | |
| ref={(element) => setTileRef(index, element)} |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+74
to
+79
| return { | ||
| sshRemotes, | ||
| sshRemoteConfig, | ||
| setSshRemoteConfig, | ||
| handleSshRemoteChange, | ||
| }; |
There was a problem hiding this comment.
setSshRemoteConfig is returned from the hook but no caller in the current codebase destructures or uses it — handleSshRemoteChange now covers both the local state update and the wizard context sync. Exporting an unused setter leaks internal state mutation and invites uncoordinated updates that skip the wizard context sync.
Suggested change
| return { | |
| sshRemotes, | |
| sshRemoteConfig, | |
| setSshRemoteConfig, | |
| handleSshRemoteChange, | |
| }; | |
| return { | |
| sshRemotes, | |
| sshRemoteConfig, | |
| handleSshRemoteChange, | |
| }; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentTileButton.tsx (1)
136-157: ⚡ Quick winUnreachable keyboard handler due to tabIndex={-1}.
The "Customize" overlay has
tabIndex={-1}, which excludes it from keyboard tab navigation. This means theonKeyDownhandler at lines 143-147 will never fire during normal keyboard navigation, since the element cannot receive focus via Tab. The handler would only be invoked if something programmatically called.focus()on this element, which doesn't appear to happen.Consider either:
- Remove the
onKeyDownhandler if keyboard access isn't intended (reduces dead code).- Change to
tabIndex={0}to make the customize action keyboard-accessible for users who cannot use a mouse.- Document if this is intentionally preserving existing behavior.
The test coverage only validates click behavior, not keyboard interaction.
🤖 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/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentTileButton.tsx` around lines 136 - 157, The Customize overlay's keyboard handler (the onKeyDown in the tile.supported block) is unreachable because tabIndex={-1} prevents focus via Tab; either remove the onKeyDown if keyboard interaction isn't intended, or change tabIndex to 0 to make it focusable and thus allow the onKeyDown to call onOpenConfig(tile.id) when Enter/Space is pressed; update the element that renders the overlay (the div containing Settings and "Customize" in AgentTileButton) accordingly and ensure tab focus/ARIA behavior aligns with onOpenConfig.
🤖 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/renderer/components/Wizard/screens/AgentSelectionScreen/AgentSelectionScreen.tsx`:
- Around line 97-117: showConfigView and showGridView start setTimeouts and call
state setters which can run after the component unmounts; store the timer IDs
(e.g., via a ref like timerRef) when calling setTimeout in both functions, clear
any existing timer before starting a new one, and add a useEffect cleanup that
clears the stored timer(s) on unmount so calls to setIsTransitioning,
setViewMode, setConfiguringAgentId, setFocusedTileIndex and
tileRefs.current[index]?.focus() won't run after unmount.
In
`@src/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentConfigurationPanel.ts`:
- Around line 87-95: Replace the logger-only error handling in
useAgentConfigurationPanel's model-load try/catch (the block calling
getSshRemoteIdForDetection and window.maestro.agents.getModels, which currently
calls logger.error and setLoadingModels) to also call the Sentry utilities
(import captureException or captureMessage from src/utils/sentry.ts) with
contextual extras (agentId and whether sshRemoteId indicates remote mode) before
recovering UI state; do the same change for the other similar catch (the later
block around setAvailableModels/setLoadingModels) so both failure paths report
to Sentry with agent id + remote-mode context and then continue to call
setLoadingModels(false).
In
`@src/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentDetection.ts`:
- Around line 40-45: refreshAgentDetection currently calls
window.maestro.agents.detect() which only probes local agents and ignores
SSH/remote context; change it to call the same detection API used for initial
detection that includes the current SSH context (e.g., pass the current
detection options/context object or call the shared
detectWithContext/detectRemote helper) so it returns remote agents when SSH is
enabled, update the useCallback dependency list to include that context (e.g.,
sshEnabled or detectionContext) and keep the post-processing via
getVisibleAgents followed by setDetectedAgents and setAvailableAgents.
In
`@src/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentSelectionKeyboard.ts`:
- Around line 22-28: The Shift+Tab handler currently focuses the raw last index
(AGENT_TILES.length - 1) which can land on a non-interactive tile; update the
handler in useAgentSelectionKeyboard to find the last selectable tile instead
(iterate AGENT_TILES backwards, checking the tile's selectable/disabled/support
property used elsewhere in this module), then call
setFocusedTileIndex(foundIndex) and tileRefs.current?.[foundIndex]?.focus(); if
no selectable tile exists, avoid changing focus (or fall back to previous
behavior only if explicitly supported).
In
`@src/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useSshRemotes.ts`:
- Around line 45-47: In useSshRemotes's catch block (where logger.error('Failed
to load SSH remotes:', undefined, error) is used) add an explicit Sentry
telemetry call: import and call captureException (or captureMessage with
context) from the sentry utilities and pass the caught error plus a context
object indicating the operation (e.g. "loadSSHRemotes" or "SSH remotes load")
and any relevant state (e.g. username/path or remotes length if available) so
the failure is reported to Sentry alongside the existing logger.error recovery
handling.
In
`@src/renderer/components/Wizard/screens/AgentSelectionScreen/utils/sshConfig.ts`:
- Around line 9-19: The getInitialSshRemoteConfig function can return an empty
string for remoteId due to the nullish coalescing operator, causing
inconsistency with getSyncedSshRemoteConfig and selectSshRemoteConfig which
treat empty string as falsy; change the remoteId assignment in
getInitialSshRemoteConfig to explicitly normalize empty strings to null (e.g.,
if sessionSshRemoteConfig.remoteId is undefined or an empty string then set
remoteId to null) so the returned AgentSshRemoteConfig always uses null for
missing/empty remoteId.
---
Nitpick comments:
In
`@src/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentTileButton.tsx`:
- Around line 136-157: The Customize overlay's keyboard handler (the onKeyDown
in the tile.supported block) is unreachable because tabIndex={-1} prevents focus
via Tab; either remove the onKeyDown if keyboard interaction isn't intended, or
change tabIndex to 0 to make it focusable and thus allow the onKeyDown to call
onOpenConfig(tile.id) when Enter/Space is pressed; update the element that
renders the overlay (the div containing Settings and "Customize" in
AgentTileButton) accordingly and ensure tab focus/ARIA behavior aligns with
onOpenConfig.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42bedcd5-5963-45a3-bd59-30705cba67be
📒 Files selected for processing (28)
src/__tests__/renderer/components/Wizard/screens/AgentSelectionScreen/components.test.tsxsrc/__tests__/renderer/components/Wizard/screens/AgentSelectionScreen/hooks.test.tsxsrc/__tests__/renderer/components/Wizard/screens/AgentSelectionScreen/utils.test.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/AgentSelectionScreen.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentConfigurationView.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentGrid.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentLocationSelect.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentLogo.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentSelectionFooter.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentSelectionHeader.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentSelectionLoading.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentTileButton.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/components/SshConnectionErrorPanel.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/components/index.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/index.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentConfigurationPanel.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentDetection.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentSelectionFocus.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentSelectionKeyboard.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useSshRemotes.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/index.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/types.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/utils/agentAvailability.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/utils/agentConfigForms.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/utils/agentGrid.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/utils/agentTiles.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/utils/sshConfig.ts
💤 Files with no reviewable changes (1)
- src/renderer/components/Wizard/screens/AgentSelectionScreen.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useSshRemotes.ts (1)
42-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle non-throwing SSH config failures explicitly.
On Line 43, only the success case is handled. If
window.maestro.sshRemote.getConfigs()resolves withsuccess: false, the failure path is currently dropped with no logging/telemetry branch.Suggested fix
async function loadSshRemotes() { try { const configsResult = await window.maestro.sshRemote.getConfigs(); - if (mounted && configsResult.success && configsResult.configs) { - setSshRemotes(configsResult.configs); + if (!mounted) { + return; + } + if (configsResult.success && configsResult.configs) { + setSshRemotes(configsResult.configs); + } else { + const resolvedError = new Error( + configsResult.error ?? 'Failed to load SSH remotes' + ); + logger.error('Failed to load SSH remotes:', undefined, resolvedError); + captureException(resolvedError, { + extra: { + operation: 'agentSelection:loadSshRemotes', + sessionRemoteEnabled: Boolean(sessionSshRemoteConfig?.enabled), + sessionRemoteId: sessionSshRemoteConfig?.remoteId ?? null, + hasWorkingDirOverride: Boolean(sessionSshRemoteConfig?.workingDirOverride), + }, + }); } } catch (error) {As per coding guidelines, "Handle expected/recoverable errors explicitly (e.g., NETWORK_ERROR)." and "Do not silently swallow errors."
🤖 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/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useSshRemotes.ts` around lines 42 - 45, In useSshRemotes, the call to window.maestro.sshRemote.getConfigs() only handles the success:true path and silently ignores responses with success:false; update the logic to explicitly handle the failure case returned by getConfigs() — e.g., when configsResult.success is false, call your logging/telemetry utility and/or set an error state (or clear ssh remotes via setSshRemotes([])) so failures aren’t swallowed; use the existing variables (configsResult, setSshRemotes, mounted) and include the error/reason from configsResult in the log/telemetry call.
🤖 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/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useSshRemotes.ts`:
- Around line 64-68: The effect inside the useSshRemotes hook is refetching
remotes whenever any field on sessionSshRemoteConfig changes (caused by the
dependency array including sessionSshRemoteConfig?.workingDirOverride), turning
a mount-time load into repeated IPC fetches; update the loader effect
dependencies to only include the stable keys that should trigger a reload (e.g.,
sessionSshRemoteConfig?.remoteId and sessionSshRemoteConfig?.enabled) so changes
to transient fields like workingDirOverride do not retrigger the fetch, and
ensure the effect reads those specific properties (not the whole object) to keep
dependencies stable.
---
Outside diff comments:
In
`@src/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useSshRemotes.ts`:
- Around line 42-45: In useSshRemotes, the call to
window.maestro.sshRemote.getConfigs() only handles the success:true path and
silently ignores responses with success:false; update the logic to explicitly
handle the failure case returned by getConfigs() — e.g., when
configsResult.success is false, call your logging/telemetry utility and/or set
an error state (or clear ssh remotes via setSshRemotes([])) so failures aren’t
swallowed; use the existing variables (configsResult, setSshRemotes, mounted)
and include the error/reason from configsResult in the log/telemetry call.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e088ce1c-e019-4082-9de1-65b86f2c66b6
📒 Files selected for processing (10)
src/__tests__/renderer/components/Wizard/screens/AgentSelectionScreen/components.test.tsxsrc/__tests__/renderer/components/Wizard/screens/AgentSelectionScreen/hooks.test.tsxsrc/__tests__/renderer/components/Wizard/screens/AgentSelectionScreen/utils.test.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/AgentSelectionScreen.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentTileButton.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentConfigurationPanel.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentDetection.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentSelectionKeyboard.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useSshRemotes.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen/utils/sshConfig.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentSelectionKeyboard.ts
- src/renderer/components/Wizard/screens/AgentSelectionScreen/components/AgentTileButton.tsx
- src/renderer/components/Wizard/screens/AgentSelectionScreen/AgentSelectionScreen.tsx
- src/tests/renderer/components/Wizard/screens/AgentSelectionScreen/utils.test.ts
- src/tests/renderer/components/Wizard/screens/AgentSelectionScreen/components.test.tsx
- src/renderer/components/Wizard/screens/AgentSelectionScreen/utils/sshConfig.ts
- src/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentDetection.ts
- src/renderer/components/Wizard/screens/AgentSelectionScreen/hooks/useAgentConfigurationPanel.ts
- src/tests/renderer/components/Wizard/screens/AgentSelectionScreen/hooks.test.tsx
Collaborator
|
Thanks for this, @reachrazamair! This is a clean, well-structured refactor and the directory-module split mirrors the I verified the extraction against the original 1,349-line On the bot feedback: the substantive CodeRabbit items (setTimeout transitions without an unmount cleanup, Two genuinely-new, non-blocking nits from Greptile if you happen to touch these files again (no need to hold the merge for them):
Approving. Nice work, and thanks again for the thorough test coverage and write-up. |
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
Refactors the Wizard
AgentSelectionScreenfrom a monolithic screen file into a focused directory module matching theConversationScreenrefactor pattern.Behavior is intended to remain unchanged: same WizardContext contract, agent detection flow, SSH remote behavior, provider configuration behavior, keyboard navigation, focus handling, announcements, copy, styling, and public import path.
Changes
src/renderer/components/Wizard/screens/AgentSelectionScreen.tsxwithscreens/AgentSelectionScreen/.screens/index.tscontinue to work.AgentSelectionScreen,AGENT_TILES,AgentTile, andAgentLogo.WizardContext, agent IPC,AgentConfigPanel, prompt services, and release docs unchanged.Architecture Notes
useWizard(), refs, view mode, transition state, and composition.useAgentDetectionowns detection, hidden-agent filtering, SSH connection-error detection, Claude Code auto-select, stale-result guards, and announcements.useSshRemotesowns remote config loading, location sync, and remote selection forwarding.useAgentSelectionKeyboardowns arrow, Tab, Shift+Tab, Enter, and Space behavior without global listeners.useAgentConfigurationPanelowns config load/save, model load/refresh, custom path blur refresh, env var mutations, single-agent refresh, and unmount guards.useAgentConfigurationandSshRemoteSelectorwere not reused because their semantics differ from the onboarding wizard flow.Test Coverage
Added 25 focused renderer tests:
screens/AgentSelectionScreen/utils.test.tsscreens/AgentSelectionScreen/components.test.tsxscreens/AgentSelectionScreen/hooks.test.tsxVerification
npm run test -- src/__tests__/renderer/components/Wizard/screens/AgentSelectionScreennpm run format:checknpm run lintnpm run lint:eslintnpm run testSummary by CodeRabbit
New Features
Refactor
Tests