feat(cli): add direct delete on profile list dialog#2645
Conversation
Allow deleting a highlighted profile from /profile list with d + y/N confirmation, refresh in place, and block deletes of LB member profiles that are still referenced. Fixes vybestack#2494.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesDirect deletion is added to Profile deletion
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
LLxprt PR Review – PR #2645Issue AlignmentImplements the requested list-level delete affordance for #2494. Evidence: Side Effects
Code Quality
Tests and Coverage
VerdictReady |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Move to last item (gamma) with down arrows in 3-column wide layout: | ||
| // index 0 -> right -> 1 -> right -> 2 | ||
| await act(async () => { | ||
| stdin.write('\u001B[C'); // right |
There was a problem hiding this comment.
The comment states that down arrows are used to move to the last item, but the test actually writes right-arrow escape sequences (
\u001B[C). InProfileListDialog.tsx, right arrows move horizontally (move(1)) while down arrows move vertically (move(columns)). Please update the comment to accurately describe the right-arrow navigation so maintainers aren't misled.
| columns: number, | ||
| onClose: () => void, | ||
| isLoading: boolean, | ||
| confirmDeleteName: string | null, | ||
| setConfirmDeleteName: React.Dispatch<React.SetStateAction<string | null>>, | ||
| ) { |
There was a problem hiding this comment.
The
isLoadingparameter is accepted byuseListKeypressand included in theuseCallbackdependency array, but it is never referenced in the function body. This is dead code that causes unnecessary callback recreations wheneverisLoadingchanges, and prevents the component from using the loading state to guard against duplicate or concurrent delete operations. Please either removeisLoadingfrom the signature and dependencies, or use it to block input during loading.
| Delete profile '{confirmDeleteName}'{qualifierText}? Press y | ||
| to confirm, n or Esc to cancel. |
There was a problem hiding this comment.
The delete confirmation banner states 'Press y to confirm, n or Esc to cancel', but the implementation also accepts uppercase 'Y' and 'N' inputs. This creates a mismatch between the UI instructions and actual behavior, which may confuse users. Please update the message to reflect case-insensitivity, e.g., 'Press y/Y to confirm, n/N or Esc to cancel'.
| for (const name of names) { | ||
| if (name === profileName) { | ||
| continue; | ||
| } | ||
| try { | ||
| const filePath = path.join(this.profilesDir, `${name}.json`); | ||
| const content = await fs.readFile(filePath, 'utf8'); | ||
| const parsed: unknown = JSON.parse(content); | ||
| if ( | ||
| isPlainObject(parsed) && | ||
| parsed.type === 'loadbalancer' && | ||
| Array.isArray(parsed.profiles) && | ||
| parsed.profiles.includes(profileName) | ||
| ) { | ||
| referencing.push(name); | ||
| } | ||
| } catch { | ||
| // Skip unreadable/corrupt profiles when checking references. | ||
| } | ||
| } |
There was a problem hiding this comment.
The
findLoadBalancersReferencingmethod reads profile files sequentially in afor...ofloop withawait. Since each file read is an independent I/O operation, this should be parallelized usingPromise.allfor better performance, especially when there are many profiles.
| function useDeleteProfileFromListAction( | ||
| addMessage: AddMessageFn, | ||
| runtime: ReturnType<typeof useRuntimeApi>, | ||
| loadProfiles: (options?: { showLoading?: boolean }) => Promise<void>, | ||
| ) { | ||
| return useCallback( | ||
| async (profileName: string) => { | ||
| try { | ||
| await runtime.deleteProfileByName(profileName); | ||
| addMessage({ | ||
| type: MessageType.INFO, | ||
| content: `Profile '${profileName}' deleted`, | ||
| timestamp: new Date(), | ||
| }); | ||
| // Refresh in place without the loading flash so selection can clamp. | ||
| await loadProfiles({ showLoading: false }); | ||
| } catch (error) { | ||
| addMessage({ | ||
| type: MessageType.ERROR, | ||
| content: `Failed to delete profile: ${error instanceof Error ? error.message : String(error)}`, | ||
| timestamp: new Date(), | ||
| }); | ||
| } | ||
| }, | ||
| [addMessage, runtime, loadProfiles], | ||
| ); | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling:
useDeleteProfileFromListActioncatches deletion errors and callsaddMessage, but does not updateprofileErrorstate. This is inconsistent withuseProfileLoader, which updatesprofileErroron load failures. If the UI consumesprofileErrorfor persistent error display, deletion errors will only appear as transient messages and could be lost, causing confusing UX. Consider updatingprofileError(and optionallyclearProfileErrorif available) to match the pattern used in the profile loader.
OpenCodeReview — PR #2645
|
![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)
packages/cli/src/ui/hooks/useProfileManagement.ts (1)
339-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate delete-action boilerplate between
useDeleteProfileActionanduseDeleteProfileFromListAction.Both hooks call
runtime.deleteProfileByName, build identical success/erroraddMessagepayloads, and only diverge in what happens after success (dialog dispatch + full-loading refresh vs. no-flash refresh). Extracting a shared core reduces duplication and keeps the error-message format in sync if it ever changes.♻️ Proposed refactor sketch
+function useDeleteProfileCore( + addMessage: AddMessageFn, + runtime: ReturnType<typeof useRuntimeApi>, +) { + return useCallback( + async (profileName: string, onSuccess?: () => Promise<void> | void) => { + try { + await runtime.deleteProfileByName(profileName); + addMessage({ + type: MessageType.INFO, + content: `Profile '${profileName}' deleted`, + timestamp: new Date(), + }); + await onSuccess?.(); + } catch (error) { + addMessage({ + type: MessageType.ERROR, + content: `Failed to delete profile: ${error instanceof Error ? error.message : String(error)}`, + timestamp: new Date(), + }); + } + }, + [addMessage, runtime], + ); +}
useDeleteProfileActionanduseDeleteProfileFromListActionwould then call this core with their respectiveonSuccesscallbacks.🤖 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 `@packages/cli/src/ui/hooks/useProfileManagement.ts` around lines 339 - 395, Extract the shared delete flow from useDeleteProfileAction and useDeleteProfileFromListAction into a reusable helper or hook that performs runtime.deleteProfileByName and constructs the common success and error messages. Keep each existing action responsible only for its distinct success behavior: dialog dispatch plus loadProfiles() for useDeleteProfileAction, and loadProfiles({ showLoading: false }) for useDeleteProfileFromListAction, passing these as callbacks while preserving the current dependency handling.
🤖 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 `@packages/settings/src/profiles/ProfileManager.ts`:
- Around line 264-270: Update the load-balancer reference validation in the
surrounding profile-checking try block to call parseLoadBalancerProfile(name,
parsed) and only add name to referencing when parsing succeeds and the candidate
references profileName. Preserve the existing handling that skips malformed or
unsupported profiles so invalid load-balancer definitions cannot block deletion.
---
Nitpick comments:
In `@packages/cli/src/ui/hooks/useProfileManagement.ts`:
- Around line 339-395: Extract the shared delete flow from
useDeleteProfileAction and useDeleteProfileFromListAction into a reusable helper
or hook that performs runtime.deleteProfileByName and constructs the common
success and error messages. Keep each existing action responsible only for its
distinct success behavior: dialog dispatch plus loadProfiles() for
useDeleteProfileAction, and loadProfiles({ showLoading: false }) for
useDeleteProfileFromListAction, passing these as callbacks while preserving the
current dependency handling.
🪄 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 Plus
Run ID: eebd9796-3112-4740-82b2-9c0fedf021c9
📒 Files selected for processing (11)
packages/cli/src/ui/AppContainerRuntime.tsxpackages/cli/src/ui/components/DialogManager.tsxpackages/cli/src/ui/components/ProfileListDialog.test.tsxpackages/cli/src/ui/components/ProfileListDialog.tsxpackages/cli/src/ui/containers/AppContainer/builders/buildUIActions.test.tspackages/cli/src/ui/containers/AppContainer/builders/buildUIActions.tspackages/cli/src/ui/containers/AppContainer/hooks/useAppDialogs.tspackages/cli/src/ui/contexts/UIActionsContext.tsxpackages/cli/src/ui/hooks/useProfileManagement.tspackages/settings/src/profiles/ProfileManager.tspackages/settings/src/profiles/__tests__/ProfileManager.test.ts
| if ( | ||
| isPlainObject(parsed) && | ||
| parsed.type === 'loadbalancer' && | ||
| Array.isArray(parsed.profiles) && | ||
| parsed.profiles.includes(profileName) | ||
| ) { | ||
| referencing.push(name); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate candidate load-balancer profiles before blocking deletion.
This accepts malformed JSON with type: "loadbalancer" as a valid reference, despite the catch claiming corrupt profiles are skipped. Use parseLoadBalancerProfile(name, parsed) inside this try; otherwise an invalid profile (for example, unsupported version or mixed profiles entries) can prevent deletion of a valid member.
Proposed fix
- if (
- isPlainObject(parsed) &&
- parsed.type === 'loadbalancer' &&
- Array.isArray(parsed.profiles) &&
- parsed.profiles.includes(profileName)
- ) {
+ if (isPlainObject(parsed) && parsed.type === 'loadbalancer') {
+ const loadBalancerProfile = parseLoadBalancerProfile(name, parsed);
+ if (!loadBalancerProfile.profiles.includes(profileName)) {
+ continue;
+ }
referencing.push(name);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| isPlainObject(parsed) && | |
| parsed.type === 'loadbalancer' && | |
| Array.isArray(parsed.profiles) && | |
| parsed.profiles.includes(profileName) | |
| ) { | |
| referencing.push(name); | |
| if (isPlainObject(parsed) && parsed.type === 'loadbalancer') { | |
| const loadBalancerProfile = parseLoadBalancerProfile(name, parsed); | |
| if (!loadBalancerProfile.profiles.includes(profileName)) { | |
| continue; | |
| } | |
| referencing.push(name); | |
| } |
🤖 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 `@packages/settings/src/profiles/ProfileManager.ts` around lines 264 - 270,
Update the load-balancer reference validation in the surrounding
profile-checking try block to call parseLoadBalancerProfile(name, parsed) and
only add name to referencing when parsing succeeds and the candidate references
profileName. Preserve the existing handling that skips malformed or unsupported
profiles so invalid load-balancer definitions cannot block deletion.
Split oversized helpers, fix selection guard conditionals, consolidate delete actions, and parallelize LB reference checks for CI eslint.
|
Pushed a follow-up that clears the Lint (Javascript) failures:
CI should go green on the new head commit. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/settings/src/profiles/ProfileManager.ts`:
- Around line 254-271: Update the reference scanning in deleteProfile’s checks
map to skip only malformed or invalid profile content, while propagating
filesystem errors from fs.readFile such as EACCES or EIO. Ensure deleteProfile
aborts instead of treating unreadable profiles as having no references, and
apply the same behavior to the additional catch block around the related
scanning logic.
🪄 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 Plus
Run ID: 7cc0e605-90ea-4e7a-8329-42dd30296f23
📒 Files selected for processing (3)
packages/cli/src/ui/components/ProfileListDialog.tsxpackages/cli/src/ui/hooks/useProfileManagement.tspackages/settings/src/profiles/ProfileManager.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/src/ui/hooks/useProfileManagement.ts
- packages/cli/src/ui/components/ProfileListDialog.tsx
| const checks = names | ||
| .filter((name) => name !== profileName) | ||
| .map(async (name) => { | ||
| try { | ||
| const filePath = path.join(this.profilesDir, `${name}.json`); | ||
| const content = await fs.readFile(filePath, 'utf8'); | ||
| const parsed: unknown = JSON.parse(content); | ||
| if ( | ||
| isPlainObject(parsed) && | ||
| parsed.type === 'loadbalancer' && | ||
| Array.isArray(parsed.profiles) && | ||
| parsed.profiles.includes(profileName) | ||
| ) { | ||
| return name; | ||
| } | ||
| } catch { | ||
| // Skip unreadable/corrupt profiles when checking references. | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail closed when reference scanning fails.
listProfiles() converts directory-read failures to [], and this catch also suppresses filesystem errors such as EACCES or EIO. deleteProfile then proceeds as if no load balancer references exist, potentially leaving dangling references. Only skip known corrupt/invalid profile data; propagate filesystem failures so deletion aborts.
Also applies to: 284-293
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 258-258: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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 `@packages/settings/src/profiles/ProfileManager.ts` around lines 254 - 271,
Update the reference scanning in deleteProfile’s checks map to skip only
malformed or invalid profile content, while propagating filesystem errors from
fs.readFile such as EACCES or EIO. Ensure deleteProfile aborts instead of
treating unreadable profiles as having no references, and apply the same
behavior to the additional catch block around the related scanning logic.
|
|
||
| it('advertises the delete key in wide controls', () => { | ||
| const { lastFrame } = renderList(); | ||
| expect(lastFrame() ?? '').toContain('[d] Delete'); |
There was a problem hiding this comment.
This assertion tightly couples the test to exact control text. If the component copy changes (e.g., reordering, punctuation, or keybinding labels), this test will break even if the behavior is correct. Consider asserting on a more stable identifier, or accept the fragility if this is intentional snapshot-style testing.
| stdin.write('d'); | ||
| }); | ||
|
|
||
| expect(lastFrame() ?? '').toMatch(/Delete profile 'alpha'/); |
There was a problem hiding this comment.
This assertion is tightly coupled to exact confirmation copy. Any change to the wording or formatting of the delete confirmation message will cause this test to fail, even if the confirmation logic remains correct.
|
|
||
| expect(onDelete).not.toHaveBeenCalled(); | ||
| expect(onClose).not.toHaveBeenCalled(); | ||
| expect(lastFrame() ?? '').toContain('Profile List'); |
There was a problem hiding this comment.
This assertion depends on exact UI copy ('Profile List'). If the component title changes, this test breaks despite the cancel-on-Esc behavior still working. Consider asserting on a behavior-specific indicator instead of the title text.
| isStandard: !mockIsNarrow.value, | ||
| isWide: !mockIsNarrow.value, |
There was a problem hiding this comment.
The mock sets both
isStandardandisWideto!isNarrow, so they are simultaneouslytruein non-narrow mode. The realuseResponsivehook returns mutually exclusive breakpoints (isStandard: breakpoint === 'STANDARD',isWide: breakpoint === 'WIDE'). If the component ever relies onisStandardhaving distinct behavior fromisWide, this mock will not catch regressions.
| expect(lastFrame() ?? '').toMatch(/active/); | ||
| expect(lastFrame() ?? '').toMatch(/default/); |
There was a problem hiding this comment.
This test only verifies that the confirmation text includes 'active' and 'default', but does not assert any behavioral difference for active/default profiles (e.g., additional warnings, disabled actions, or different callback arguments). If the component is supposed to handle these profiles specially, this test gives false confidence.
| stdin.write(TerminalKeys.TAB); | ||
| }); | ||
|
|
||
| // Move to last item (gamma) with down arrows in 3-column wide layout: |
There was a problem hiding this comment.
This comment says 'down arrows' but the test uses right arrow escape sequences (
\u001B[C). In a 3-column wide layout, right arrows move horizontally, not vertically. Update the comment to avoid misleading future readers.
| }); | ||
|
|
||
| // Index should clamp to last remaining item (beta at index 1). | ||
| expect(lastFrame() ?? '').toContain('Selected: beta'); |
There was a problem hiding this comment.
This assertion verifies the final rendered state but does not explicitly confirm that the selection index was clamped from an out-of-bounds value to the new boundary. If the component incorrectly resets selection to index 0 or applies another fallback, this test could still pass. Consider adding an explicit assertion about the index transition or selection state if the component exposes it.
| if (key.sequence === 'y' || key.sequence === 'Y') { | ||
| setConfirmDeleteName(null); | ||
| onDelete(confirmDeleteName); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Directly invoking
onDeletehere means the delete confirmation banner disappears before the async operation completes. IfonDeletefails, the user gets no in-dialog feedback and may think the profile was already removed. Consider tracking a pending-delete or delete-error state and showing it in the banner/list while awaitingonDelete.
| if (!fromList && appDispatch !== null) { | ||
| appDispatch({ type: 'CLOSE_DIALOG', payload: 'profileDetail' }); | ||
| appDispatch({ type: 'OPEN_DIALOG', payload: 'profileList' }); | ||
| await loadProfiles(); | ||
| } else { | ||
| // Refresh in place without the loading flash so selection can clamp. | ||
| await loadProfiles({ showLoading: false }); | ||
| } |
There was a problem hiding this comment.
Misleading error message when
loadProfiles()fails after successful deletion. The success message is shown beforeloadProfiles()is called, so ifloadProfiles()throws, the catch block displays 'Failed to delete profile' even though the profile was already deleted. This results in users seeing both a success and an error message for the same operation. Consider moving the success message afterloadProfiles()succeeds, or decoupling the success notification from the refresh error handling.
| function buildProfileManagementResult( | ||
| dialogStates: ReturnType<typeof useProfileDialogStates>, | ||
| dataStates: ReturnType<typeof useProfileDataStates>, | ||
| actions: Record<string, unknown>, | ||
| ) { |
There was a problem hiding this comment.
The
buildProfileManagementResulthelper usesRecord<string, unknown>for theactionsparameter, which erases all type safety for the returned object. TypeScript cannot provide autocompletion or catch misspelled property names. Consider using a generic type parameter or defining a specific interface for the actions to preserve type safety.
Use a generic for buildProfileManagementResult actions so openListDialog and related methods stay on the returned type and the CLI build passes.
| // Move to last item (gamma) with down arrows in 3-column wide layout: | ||
| // index 0 -> right -> 1 -> right -> 2 | ||
| await act(async () => { | ||
| stdin.write('\u001B[C'); // right | ||
| }); |
There was a problem hiding this comment.
The comment says "Move to last item (gamma) with down arrows" but the code uses right arrow keycodes (
\u001B[C). In a 3-column wide layout, right arrows move horizontally (0→1→2), which also reaches gamma, but the comment is misleading. Please update the comment to accurately describe the actual key presses used.
| function handleDeleteConfirmKeys( | ||
| key: { name?: string; sequence?: string }, | ||
| confirmDeleteName: string, | ||
| setConfirmDeleteName: React.Dispatch<React.SetStateAction<string | null>>, | ||
| onDelete: (name: string) => void, | ||
| ): void { | ||
| if (key.name === 'escape') { | ||
| setConfirmDeleteName(null); | ||
| return; | ||
| } | ||
| if (key.sequence === 'y' || key.sequence === 'Y') { | ||
| setConfirmDeleteName(null); | ||
| onDelete(confirmDeleteName); | ||
| return; | ||
| } | ||
| if (key.sequence === 'n' || key.sequence === 'N') { | ||
| setConfirmDeleteName(null); | ||
| } | ||
| } |
There was a problem hiding this comment.
The delete confirmation flow clears
confirmDeleteNameimmediately when invokingonDelete, butonDeleteis an async parent callback that can fail. If deletion fails, the confirmation banner disappears and the profile remains visible with no error feedback in this dialog. Guard the callback: only clear the confirmation afteronDeleteresolves successfully, and on rejection keep the banner so the user can retry or cancel.
| it('refuses to delete a profile still referenced by a load balancer', async () => { | ||
| const member = { | ||
| version: 1, | ||
| provider: 'openai', | ||
| model: 'gpt-4', | ||
| modelParams: {}, | ||
| ephemeralSettings: {}, | ||
| }; | ||
| const lb = { | ||
| version: 1, | ||
| type: 'loadbalancer' as const, | ||
| policy: 'roundrobin', | ||
| profiles: ['member-a'], | ||
| provider: '', | ||
| model: '', | ||
| modelParams: {}, | ||
| ephemeralSettings: {}, | ||
| }; | ||
| await pm.saveProfile('member-a', member); | ||
| await pm.saveLoadBalancerProfile('lb-main', lb); | ||
|
|
||
| await expect(pm.deleteProfile('member-a')).rejects.toThrow( | ||
| /referenced by load balancer profile\(s\): lb-main/, | ||
| ); | ||
| await expect( | ||
| fs.access(path.join(tempDir, 'member-a.json')), | ||
| ).resolves.toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
Avoid duplicate disk reads:
findLoadBalancersReferencingreads every profile file from disk even thoughProfileManageralready keeps in-memory profile caches elsewhere. If those caches are authoritative and kept in sync, reuse them here instead of introducing extra I/O on everydeleteProfilecall.
| @@ -111,15 +111,11 @@ function handleSearchModeKeys( | |||
| ): void { | |||
| if (key.name === 'return') { | |||
| if (filteredProfiles.length > 0) { | |||
| if (isNarrow) { | |||
| onViewDetail(filteredProfiles[selectedIndex].name); | |||
| return; | |||
| } | |||
| setIsSearching(false); | |||
| onViewDetail(filteredProfiles[selectedIndex].name); | |||
| } | |||
| return; | |||
| } | |||
| if (key.name === 'tab' && !isNarrow) { | |||
| if (key.name === 'tab') { | |||
| setIsSearching(false); | |||
| return; | |||
| } | |||
There was a problem hiding this comment.
The keyboard behavior in search mode has changed: Enter now always triggers onViewDetail directly, whereas previously in wide layouts it would only exit search mode (setIsSearching(false)) without opening details. Similarly, Tab previously only exited search in wide mode (key.name === 'tab' && !isNarrow) but now exits search in all layouts. These behavioral changes alter user expectations for keyboard navigation and should be verified as intentional.
| if (key.sequence === 'y' || key.sequence === 'Y') { | ||
| setConfirmDeleteName(null); | ||
| onDelete(confirmDeleteName); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When the user confirms deletion by pressing 'y', the confirmation banner disappears immediately and onDelete is invoked asynchronously. If the deletion operation fails or takes time, the dialog provides no visual feedback (no loading spinner or error banner). The parent handles errors via addMessage, but the dialog should ideally show some feedback or disable further input during the async operation.
| return { | ||
| isNarrow, | ||
| width, | ||
| isSearching, | ||
| searchTerm, | ||
| filteredProfiles, | ||
| rows, | ||
| columns, | ||
| index, | ||
| isSearching, | ||
| isNarrow, | ||
| isWide, | ||
| activeProfileName, | ||
| defaultProfileName, | ||
| colWidth, | ||
| ); | ||
| confirmDeleteName, | ||
| grid: buildGrid( | ||
| filteredProfiles, | ||
| rows, | ||
| columns, | ||
| index, | ||
| isSearching, | ||
| isNarrow, | ||
| isWide, | ||
| opts.activeProfileName, | ||
| opts.defaultProfileName, | ||
| colWidth, | ||
| ), | ||
| }; |
There was a problem hiding this comment.
buildGrid is called directly inside useProfileListController's return object, causing it to execute and create new React element references on every render. For large profile lists, this creates unnecessary computational overhead since grid construction involves mapping over filteredProfiles. Consider memoizing the grid with useMemo keyed on filteredProfiles, rows, columns, index, and other layout values.
| function useConfirmDeleteClear( | ||
| confirmDeleteName: string | null, | ||
| filteredProfiles: ProfileListItem[], | ||
| setConfirmDeleteName: React.Dispatch<React.SetStateAction<string | null>>, | ||
| ): void { | ||
| useEffect(() => { | ||
| if ( | ||
| confirmDeleteName !== null && | ||
| !filteredProfiles.some((profile) => profile.name === confirmDeleteName) | ||
| ) { | ||
| setConfirmDeleteName(null); | ||
| } | ||
| }, [confirmDeleteName, filteredProfiles, setConfirmDeleteName]); | ||
| } |
There was a problem hiding this comment.
setConfirmDeleteName is included in the useEffect dependency array of useConfirmDeleteClear. React guarantees that state setters from useState are stable references and never change, so including them in dependency arrays is unnecessary and may trigger ESLint warnings. Consider removing setConfirmDeleteName from the dependency array.
| ESCAPE = '\u001B', | ||
| } | ||
|
|
||
| const mockIsNarrow = vi.hoisted(() => ({ value: false })); |
There was a problem hiding this comment.
Tests rely on a mutable hoisted variable
mockIsNarrowthat is modified within individual tests (set totruein the narrow layout test). WhilebeforeEachresets it tofalse, this global mutable state pattern is fragile and could cause test interference in parallel execution environments or if tests are reordered. Consider using a dedicated mock factory per test or scoping the mock state to individual test cases.
|
|
||
| it('advertises the delete key in wide controls', () => { | ||
| const { lastFrame } = renderList(); | ||
| expect(lastFrame() ?? '').toContain('[d] Delete'); |
There was a problem hiding this comment.
Tests are tightly coupled to exact UI string literals (e.g.,
[d] Delete,Delete profile 'alpha',Profile List) and specific terminal key sequences. While appropriate for CLI component testing, any copy changes or keyboard shortcut remapping in the component will cause test failures. Consider extracting expected strings into constants shared with the component, or using regex patterns that focus on structural elements rather than exact copy.
| closeProfileDetailDialog: () => void; | ||
| loadProfileFromDetail: (profileName: string) => void; | ||
| deleteProfileFromDetail: (profileName: string) => void; | ||
| deleteProfileFromList: (profileName: string) => void; |
There was a problem hiding this comment.
In the list delete path,
loadProfiles({ showLoading: false })is called withoutawaitinuseProfileManagement.ts. If the caller expects the profile list to refresh before proceeding (e.g., before another navigation action or selection), this can lead to race conditions and stale UI state. Addawaithere for consistency with the detail path above.
| function useDeleteProfileAction( | ||
| addMessage: AddMessageFn, | ||
| appDispatch: ReturnType<typeof useAppDispatch>, | ||
| appDispatch: ReturnType<typeof useAppDispatch> | null, | ||
| runtime: ReturnType<typeof useRuntimeApi>, | ||
| loadProfiles: () => Promise<void>, | ||
| loadProfiles: (options?: { showLoading?: boolean }) => Promise<void>, | ||
| options?: { fromList?: boolean }, | ||
| ) { |
There was a problem hiding this comment.
The type signature allows
fromList: falsecombined withappDispatch: null, which would skip dialog navigation and fall through toloadProfiles({ showLoading: false }), potentially leaving the UI in an inconsistent state (detail dialog open while list refreshes). Consider using a discriminated union to enforce thatappDispatchis non-null whenfromListis false:type DeleteProfileOptions = { fromList: true }; type DeleteProfileAction = ( addMessage: AddMessageFn, appDispatch: ReturnType<typeof useAppDispatch>, runtime: ReturnType<typeof useRuntimeApi>, loadProfiles: (options?: { showLoading?: boolean }) => Promise<void>, options?: DeleteProfileOptions ) => ReturnType<typeof useCallback>; // Or overload the function signature to make the contract explicit.
| const deleteProfileFromList = useDeleteProfileAction( | ||
| addMessage, | ||
| null, | ||
| runtime, | ||
| loadProfiles, | ||
| { fromList: true }, | ||
| ); |
There was a problem hiding this comment.
The inline options object
{ fromList: true }is recreated on every render. While only the derived booleanfromListis used in theuseCallbackdependency array, extracting this to a stable reference improves readability and avoids unnecessary object allocation.
|
@Ayush7614 my agent is backed up will merge in a bit |
2 participants
Summary
[d] Deleteon/profile list(wide + narrow via Tab→nav) with inliney/Nconfirmation, including active/default callouts.Fixes #2494.
Maintainer notes addressed
Test plan
bunx vitest run packages/cli/src/ui/components/ProfileListDialog.test.tsx(7/7)bunx vitest run packages/settings/src/profiles/__tests__/ProfileManager.test.ts -t deleteProfile(3/3)bunx vitest run packages/cli/src/ui/containers/AppContainer/builders/buildUIActions.test.ts/profile list→ Tab →d→ydeletes without opening detailSummary by CodeRabbit
dkey, including an on-screen confirmation banner.y/n/Escconfirm or cancel.[d] Delete, including narrow layouts.Esc), and deletion safeguards.