Skip to content

feat(cli): add direct delete on profile list dialog#2645

Open
Ayush7614 wants to merge 3 commits into
vybestack:mainfrom
Ayush7614:feat/2494-profile-list-delete
Open

feat(cli): add direct delete on profile list dialog#2645
Ayush7614 wants to merge 3 commits into
vybestack:mainfrom
Ayush7614:feat/2494-profile-list-delete

Conversation

@Ayush7614

@Ayush7614 Ayush7614 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add [d] Delete on /profile list (wide + narrow via Tab→nav) with inline y/N confirmation, including active/default callouts.
  • Refresh the list in place after delete (no loading flash) and clamp selection safely.
  • Block deleting profiles still referenced by a load-balancer profile, surfacing the same error path used by detail delete.

Fixes #2494.

Maintainer notes addressed

  • Kept this PR focused on the list delete affordance (no broad UI inheritance refactor from the consistency aside).

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
  • Manual: /profile list → Tab → dy deletes without opening detail
  • Manual: try deleting an LB member still referenced and confirm error message

Summary by CodeRabbit

  • New Features
    • Added profile deletion directly from the profile list using the d key, with an on-screen confirmation banner.
    • Confirmation messaging indicates whether the target profile is active and/or default; y/n/Esc confirm or cancel.
    • Updated keyboard help to include [d] Delete (including for narrow layouts).
  • Bug Fixes
    • Blocked deletion of profiles still referenced by load-balancer profiles.
    • Improved list behavior when the selected profile is removed (selection clamps to the last remaining profile).
  • Tests
    • Expanded coverage for key navigation, confirmation/cancel flows, selection clamping, and deletion safeguards.

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.
@Ayush7614
Ayush7614 requested a review from acoliver as a code owner July 22, 2026 16:05
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 97aedabf-ff76-4302-ae05-aa268b08c1b7

📥 Commits

Reviewing files that changed from the base of the PR and between 0fe556b and 4045d8a.

📒 Files selected for processing (1)
  • packages/cli/src/ui/hooks/useProfileManagement.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/cli/src/ui/hooks/useProfileManagement.ts

📝 Walkthrough

Walkthrough

Changes

Direct deletion is added to ProfileListDialog with confirmation, cancellation, layout controls, list refresh, and selection clamping. The action is wired through profile management and UI action providers, while ProfileManager blocks deletion of referenced load-balancer members.

Profile deletion

Layer / File(s) Summary
Protect referenced profiles
packages/settings/src/profiles/ProfileManager.ts, packages/settings/src/profiles/__tests__/ProfileManager.test.ts
Deletion now rejects profiles referenced by load-balancer profiles and verifies the target remains undeleted.
Add list deletion action
packages/cli/src/ui/hooks/useProfileManagement.ts
Profile management adds a deletion action and refreshes the list without showing a loading spinner.
Implement confirmed list deletion
packages/cli/src/ui/components/ProfileListDialog.tsx, packages/cli/src/ui/components/ProfileListDialog.test.tsx
The list supports d/D, confirmation with y/n/Esc, narrow and wide controls, active/default context, and selection clamping after removal.
Wire deletion through UI actions
packages/cli/src/ui/contexts/UIActionsContext.tsx, packages/cli/src/ui/containers/AppContainer/..., packages/cli/src/ui/AppContainerRuntime.tsx, packages/cli/src/ui/components/DialogManager.tsx
The new callback is added to UI action contracts and passed from profile management to the profile list dialog, with builder coverage updated.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested reviewers: acoliver

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: adding direct delete support to the profile list dialog.
Description check ✅ Passed The description covers the summary, follow-up notes, and test plan, with only some template sections left unfilled.
Linked Issues check ✅ Passed The PR implements direct delete, confirmation, list refresh, selection clamping, narrow-layout access, and referenced-member error handling for #2494.
Out of Scope Changes check ✅ Passed The settings-side delete guard and its test support the listed delete workflow and edge cases, so no clear out-of-scope changes stand out.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

LLxprt PR Review – PR #2645

Issue Alignment

Implements #2494: adds [d] Delete from /profile list with inline y/N confirmation, refreshes the list in place, and prevents deleting profiles referenced by load-balancer profiles. File-level evidence: ProfileListDialog.tsx adds delete key handling and confirmation banner; useProfileManagement.ts adds deleteProfileFromList and loadProfiles({ showLoading: false }); ProfileManager.ts adds findLoadBalancersReferencing guard. All requirements are addressed.

Side Effects

  • loadProfiles signature changed to accept options?: { showLoading?: boolean }; all internal callers updated.
  • Narrow layout behavior changed: Enter in search mode now exits search instead of opening detail view. This is consistent with the new “Tab→nav” flow but alters existing UX.
  • New deleteProfileFromList action added to UIActions interface and propagated through buildUIActions, useAppDialogs, DialogManager, and AppContainerRuntime.

Code Quality

  • Delete confirmation flow is cleanly separated (handleDeleteConfirmKeys, DeleteConfirmationBanner).
  • Selection clamping after delete is handled via useConfirmDeleteClear and existing index-bounds logic.
  • Minor concern: useProfileListController returns a new object each render, which may cause unnecessary re-renders of ProfileListBody.
  • Minor UI nit: qualifier text renders as (active, default) with a leading space before the parenthesis.
  • Error-path gap: when showLoading is false and loadProfiles fails, isLoading is not reset to false because the finally block skips it. This could leave the UI in a stale loading state if an in-place refresh fails.

Tests and Coverage

  • Coverage impact: increase.
  • New component tests cover delete confirmation, cancel paths, active/default copy, narrow layout access, and selection clamping after prop removal.
  • New ProfileManager test covers load-balancer reference guard.
  • Missing coverage: useProfileManagement hook behavior, especially deleteProfileFromList and the loadProfiles({ showLoading: false }) error path.

Verdict

Needs Work
The implementation is largely correct and well-tested for the new feature, but the missing error-path coverage for loadProfiles({ showLoading: false }) is a meaningful gap, and the potential stale isLoading state on refresh failure should be addressed before merge.

Comment on lines +184 to +187
// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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). In ProfileListDialog.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.

Comment on lines 451 to 456
columns: number,
onClose: () => void,
isLoading: boolean,
confirmDeleteName: string | null,
setConfirmDeleteName: React.Dispatch<React.SetStateAction<string | null>>,
) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isLoading parameter is accepted by useListKeypress and included in the useCallback dependency array, but it is never referenced in the function body. This is dead code that causes unnecessary callback recreations whenever isLoading changes, and prevents the component from using the loading state to guard against duplicate or concurrent delete operations. Please either remove isLoading from the signature and dependencies, or use it to block input during loading.

Comment on lines +200 to +201
Delete profile &apos;{confirmDeleteName}&apos;{qualifierText}? Press y
to confirm, n or Esc to cancel.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

Comment on lines +256 to +275
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.
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The findLoadBalancersReferencing method reads profile files sequentially in a for...of loop with await. Since each file read is an independent I/O operation, this should be parallelized using Promise.all for better performance, especially when there are many profiles.

Comment on lines +369 to +395
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],
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent error handling: useDeleteProfileFromListAction catches deletion errors and calls addMessage, but does not update profileError state. This is inconsistent with useProfileLoader, which updates profileError on load failures. If the UI consumes profileError for persistent error display, deletion errors will only appear as transient messages and could be lost, causing confusing UX. Consider updating profileError (and optionally clearProfileError if available) to match the pattern used in the profile loader.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

OpenCodeReview — PR #2645

  • Reviewed head SHA: 4045d8ab85df6ce0f8b7fe5130dc2ecd2a787e46
  • Merge base: 83bd764ea3f3418bc918fe86faa283b21179d7c0
  • OCR version: open-code-review v1.7.9 (a32f852) linux/amd64 built at: 2026-07-14T03:41:57Z https://github.com/alibaba/open-code-review
  • Phase: review
  • Exit code: 0
  • Run: https://github.com/vybestack/llxprt-code/actions/runs/29994650964
  • 5 finding(s) (3 posted inline).
  • Artifacts: ocr-review-output contains raw JSON, stdout, stderr, preview, phase, and exit-code diagnostics.

Findings without a resolvable position

  • packages/cli/src/ui/hooks/useProfileManagement.ts: > The loadProfiles type here is stale: this hook now accepts an optional options parameter, but useListDialogActions still declares loadProfiles: () =&amp;gt; Promise&amp;lt;void&amp;gt;. This creates a type mismatch at the call site in useProfileManagement and blocks future in-place refresh callers from using the new option through this hook. (line 212-215)
  • packages/cli/src/ui/hooks/useProfileManagement.ts: > Same issue as in useListDialogActions: useDetailDialogActions still types loadProfiles as () =&amp;gt; Promise&amp;lt;void&amp;gt;, but it now accepts options?: { showLoading?: boolean }. Update this parameter type so the hook contract is consistent across the file and callers can safely pass the new option if needed. (line 231-241)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/cli/src/ui/hooks/useProfileManagement.ts (1)

339-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate delete-action boilerplate between useDeleteProfileAction and useDeleteProfileFromListAction.

Both hooks call runtime.deleteProfileByName, build identical success/error addMessage payloads, 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],
+  );
+}

useDeleteProfileAction and useDeleteProfileFromListAction would then call this core with their respective onSuccess callbacks.

🤖 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

📥 Commits

Reviewing files that changed from the base of the PR and between 83bd764 and 6d595b8.

📒 Files selected for processing (11)
  • packages/cli/src/ui/AppContainerRuntime.tsx
  • packages/cli/src/ui/components/DialogManager.tsx
  • packages/cli/src/ui/components/ProfileListDialog.test.tsx
  • packages/cli/src/ui/components/ProfileListDialog.tsx
  • packages/cli/src/ui/containers/AppContainer/builders/buildUIActions.test.ts
  • packages/cli/src/ui/containers/AppContainer/builders/buildUIActions.ts
  • packages/cli/src/ui/containers/AppContainer/hooks/useAppDialogs.ts
  • packages/cli/src/ui/contexts/UIActionsContext.tsx
  • packages/cli/src/ui/hooks/useProfileManagement.ts
  • packages/settings/src/profiles/ProfileManager.ts
  • packages/settings/src/profiles/__tests__/ProfileManager.test.ts

Comment on lines +264 to +270
if (
isPlainObject(parsed) &&
parsed.type === 'loadbalancer' &&
Array.isArray(parsed.profiles) &&
parsed.profiles.includes(profileName)
) {
referencing.push(name);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.
@Ayush7614

Copy link
Copy Markdown
Contributor Author

Pushed a follow-up that clears the Lint (Javascript) failures:

  • fixed the unnecessary selection conditional
  • split oversized helpers under the 80-line limit
  • consolidated delete actions
  • parallelized LB reference checks

CI should go green on the new head commit.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 6d595b8 and 0fe556b.

📒 Files selected for processing (3)
  • packages/cli/src/ui/components/ProfileListDialog.tsx
  • packages/cli/src/ui/hooks/useProfileManagement.ts
  • packages/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

Comment on lines +254 to +271
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.
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'/);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +30 to +31
isStandard: !mockIsNarrow.value,
isWide: !mockIsNarrow.value,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mock sets both isStandard and isWide to !isNarrow, so they are simultaneously true in non-narrow mode. The real useResponsive hook returns mutually exclusive breakpoints (isStandard: breakpoint === 'STANDARD', isWide: breakpoint === 'WIDE'). If the component ever relies on isStandard having distinct behavior from isWide, this mock will not catch regressions.

Comment on lines +153 to +154
expect(lastFrame() ?? '').toMatch(/active/);
expect(lastFrame() ?? '').toMatch(/default/);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +452 to +456
if (key.sequence === 'y' || key.sequence === 'Y') {
setConfirmDeleteName(null);
onDelete(confirmDeleteName);
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directly invoking onDelete here means the delete confirmation banner disappears before the async operation completes. If onDelete fails, 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 awaiting onDelete.

Comment on lines +356 to +363
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 });
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misleading error message when loadProfiles() fails after successful deletion. The success message is shown before loadProfiles() is called, so if loadProfiles() 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 after loadProfiles() succeeds, or decoupling the success notification from the refresh error handling.

Comment on lines +593 to +597
function buildProfileManagementResult(
dialogStates: ReturnType<typeof useProfileDialogStates>,
dataStates: ReturnType<typeof useProfileDataStates>,
actions: Record<string, unknown>,
) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The buildProfileManagementResult helper uses Record&lt;string, unknown&gt; for the actions parameter, 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.
Comment on lines +184 to +188
// 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
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +442 to +460
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);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The delete confirmation flow clears confirmDeleteName immediately when invoking onDelete, but onDelete is 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 after onDelete resolves successfully, and on rejection keep the banner so the user can retry or cancel.

Comment on lines +192 to +219
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();
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid duplicate disk reads: findLoadBalancersReferencing reads every profile file from disk even though ProfileManager already 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 every deleteProfile call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add direct delete action in /profile list menu (no drill-in required)

1 participant