Skip to content

fix(dashboard): repair policy attachment delete + use confirm modal - #25546

Closed
shivamrawat1 wants to merge 2 commits into
mainfrom
litellm_fix_delete_attachment
Closed

fix(dashboard): repair policy attachment delete + use confirm modal#25546
shivamrawat1 wants to merge 2 commits into
mainfrom
litellm_fix_delete_attachment

Conversation

@shivamrawat1

Copy link
Copy Markdown
Collaborator

Description
Policy attachments in the admin UI showed a delete (trash) control, but clicking it often did nothing. This change makes the control a proper button so clicks register, and uses the same DeleteResourceModal pattern as policy deletion so users must confirm before the attachment is removed.

Cause
The delete action was implemented as a Tremor Icon with onClick, wrapped in an Ant Design Tooltip. That combination frequently breaks: the tooltip expects a child that can hold a ref and receive DOM events, and the icon wrapper does not behave like a native interactive element, so the handler often never ran. Separately, Modal.confirm is easy to miss or misconfigure with app-wide Ant Design setup; the rest of the policies screen already uses an explicit DeleteResourceModal for deletes.

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have Added testing in the tests/test_litellm/ directory, Adding at least 1 test is a hard requirement - see details
  • My PR passes all unit tests on make test-unit
  • My PR's scope is as isolated as possible, it only solves 1 specific problem
  • I have requested a Greptile review by commenting @greptileai and received a Confidence Score of at least 4/5 before requesting a maintainer review

Delays in PR merge?

If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).

CI (LiteLLM team)

CI status guideline:

  • 50-55 passing tests: main is stable with minor issues.
  • 45-49 passing tests: acceptable but needs attention
  • <= 40 passing tests: unstable; be careful with your merges and assess the risk.
  • Branch creation CI run
    Link:

  • CI run for the last commit
    Link:

  • Merge / cherry-pick CI run
    Links:

Type

🐛 Bug Fix
✅ Test

Changes

Wrap the trash icon in a with aria-label="Delete attachment", stopPropagation() on click, and keep the tooltip on the button.
Open DeleteResourceModal with the selected attachment’s details; on confirm, call deletePolicyAttachmentCall and refresh the list.
Update attachment_table tests to target the button by its accessible name.

Use a native button for the trash action so clicks work with Tooltip.
Replace Modal.confirm with DeleteResourceModal for a consistent confirm step.
Update attachment table tests for the new accessible control.

Made-with: Cursor
@vercel

vercel Bot commented Apr 11, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
litellm Ready Ready Preview, Comment Apr 11, 2026 1:12am

Request Review

Add integration test: Attachments tab, delete button opens modal, confirm calls deletePolicyAttachmentCall.

Made-with: Cursor
@shivamrawat1

Copy link
Copy Markdown
Collaborator Author

@greptile review

@codspeed-hq

codspeed-hq Bot commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 16 untouched benchmarks


Comparing litellm_fix_delete_attachment (acd60a4) with main (4e12d3c)

Open in CodSpeed

@greptile-apps

greptile-apps Bot commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a broken delete control on the policy-attachments screen by wrapping the Tremor Icon in a native <button> with a proper aria-label and stopPropagation, then wires the confirmation flow to the existing DeleteResourceModal pattern (matching how policy deletion works) instead of Modal.confirm. New state fields (attachmentToDelete, isAttachmentDeleteModalOpen, isDeletingAttachment) follow the same shape as the policy-deletion state added earlier in the file, and an integration test covering the full click → confirm → API call path is included.

Confidence Score: 5/5

Safe to merge — targeted UI bug fix with no backend changes, proper confirmation modal, and passing unit + integration tests.

All changes are in UI-only files, the fix is minimal and correct (native button, aria-label, stopPropagation), the modal pattern matches existing policy deletion, mocks are all in-memory, and the new integration test covers the confirm path end-to-end. No P0 or P1 findings.

No files require special attention.

Important Files Changed

Filename Overview
ui/litellm-dashboard/src/components/policies/attachment_table.tsx Replaced Tremor Icon with onClick with a native button wrapper (type="button", aria-label, stopPropagation) — correct fix for the unresponsive delete control.
ui/litellm-dashboard/src/components/policies/index.tsx Migrated attachment deletion from Modal.confirm to DeleteResourceModal pattern, adding proper state (attachmentToDelete, isAttachmentDeleteModalOpen, isDeletingAttachment) and a confirm/cancel handler pair.
ui/litellm-dashboard/src/components/policies/attachment_table.test.tsx Updated two query selectors from /TrashIcon/i to /delete attachment/i to match the new aria-label on the native button; coverage is preserved.
ui/litellm-dashboard/src/components/policies/index.test.tsx New integration test covering the full delete confirmation flow: tab switch → row present → delete button → dialog → confirm → API called. All mocks are pure in-memory.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User clicks trash icon\n in AttachmentTable] --> B[button onClick fires\nstopPropagation]
    B --> C[onDeleteClick called\nwith attachment_id]
    C --> D[handleDeleteAttachmentClick\nin PoliciesPanel]
    D --> E{attachment found\nin attachmentsList?}
    E -- No --> F[silent return]
    E -- Yes --> G[setAttachmentToDelete\nsetIsAttachmentDeleteModalOpen true]
    G --> H[DeleteResourceModal renders\nwith attachment details]
    H --> I{User action}
    I -- Cancel --> J[handleAttachmentDeleteCancel\nclose modal, clear state]
    I -- Confirm --> K[handleAttachmentDeleteConfirm\nsetIsDeletingAttachment true]
    K --> L[deletePolicyAttachmentCall]
    L -- success --> M[MessageManager.success\nfetchAttachments]
    L -- error --> N[MessageManager.error]
    M --> O[finally: close modal\nclear state]
    N --> O
Loading

Reviews (1): Last reviewed commit: "test(dashboard): PoliciesPanel attachmen..." | Re-trigger Greptile

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant