feat(ui): replace thread-delete alert with a confirmation card - #135
Merged
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThread deletion now uses an accessible confirmation modal with cascade details, focus and dismissal handling, timed requests, inline errors, and expanded Playwright coverage. E2E Docker stages also trust additional host CA certificates. ChangesThread deletion modal
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ThreadMenu
participant RemoveThreadModal
participant API
User->>ThreadMenu: Select delete thread
ThreadMenu->>RemoveThreadModal: Open with cascade details
User->>RemoveThreadModal: Confirm deletion
RemoveThreadModal->>API: DELETE thread with timeout
API-->>RemoveThreadModal: Success or error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/giskard-server/static/index.html (1)
252-264: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExpose the confirmation card as an accessible dialog.
The overlay is a generic
<div>, and dynamically populated#removeThreadErris not announced to assistive technologies. Addrole="dialog",aria-modal="true", anaria-labelledbytarget for the heading, androle="alert"/aria-livefor the error slot. Verify with keyboard and screen-reader coverage.🤖 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 `@crates/giskard-server/static/index.html` around lines 252 - 264, Update the removeThreadModal dialog markup to add role="dialog", aria-modal="true", and aria-labelledby referencing its heading element; give the heading a matching unique id. Mark removeThreadErr as a live alert region with role="alert" and appropriate aria-live behavior, then verify keyboard and screen-reader accessibility for the confirmation flow.crates/giskard-server/static/app.js (1)
1394-1408: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRestore focus to the invoking control when closing.
openRemoveThreadModal()focuses the confirmation button, butcloseRemoveThreadModal()only hides the overlay. Cancel, backdrop, and Escape therefore leave keyboard focus on a hidden element or lose the user’s place. Save the active trigger before opening and restore focus after dismissal, with a connected-element fallback.🤖 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 `@crates/giskard-server/static/app.js` around lines 1394 - 1408, Update openRemoveThreadModal and closeRemoveThreadModal to capture the invoking control before focusing removeThreadConfirm, then restore focus to that control when closing if it remains connected to the document. Add a safe connected-element fallback when the original trigger is unavailable, while preserving the existing modal state cleanup.tests/e2e/tests/subagents.spec.ts (1)
102-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover modal failure and dismissal paths.
These tests verify only the successful delete flow. Add coverage for Cancel, Escape/outside-click dismissal, the disabled confirmation button during deletion, and an inline
#removeThreadErrresponse when the DELETE fails.Also applies to: 315-317
🤖 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 `@tests/e2e/tests/subagents.spec.ts` around lines 102 - 107, Add coverage around the `#removeThreadModal` deletion flow for Cancel, Escape, and outside-click dismissal, ensuring the modal closes without deleting. Verify `#removeThreadConfirm` is disabled while deletion is in progress, and mock a failed DELETE to assert the inline `#removeThreadErr` message appears; retain the existing successful deletion assertions.
🤖 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 `@crates/giskard-server/static/app.js`:
- Around line 1410-1421: Update the remove-thread modal flow around
closeRemoveThreadModal and the removeThreadConfirm handler to track whether
deletion is in flight, block Cancel, outside-click, and Escape dismissal while
deleting, and disable the corresponding dismissal controls. Keep the modal open
while the DELETE request is pending, and force-close it only after a successful
deletion; ensure failure handling can still display the current request’s error
without being overwritten.
---
Nitpick comments:
In `@crates/giskard-server/static/app.js`:
- Around line 1394-1408: Update openRemoveThreadModal and closeRemoveThreadModal
to capture the invoking control before focusing removeThreadConfirm, then
restore focus to that control when closing if it remains connected to the
document. Add a safe connected-element fallback when the original trigger is
unavailable, while preserving the existing modal state cleanup.
In `@crates/giskard-server/static/index.html`:
- Around line 252-264: Update the removeThreadModal dialog markup to add
role="dialog", aria-modal="true", and aria-labelledby referencing its heading
element; give the heading a matching unique id. Mark removeThreadErr as a live
alert region with role="alert" and appropriate aria-live behavior, then verify
keyboard and screen-reader accessibility for the confirmation flow.
In `@tests/e2e/tests/subagents.spec.ts`:
- Around line 102-107: Add coverage around the `#removeThreadModal` deletion flow
for Cancel, Escape, and outside-click dismissal, ensuring the modal closes
without deleting. Verify `#removeThreadConfirm` is disabled while deletion is in
progress, and mock a failed DELETE to assert the inline `#removeThreadErr` message
appears; retain the existing successful deletion assertions.
🪄 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: f79c178d-cacf-4f11-af7d-af56aa29090e
📒 Files selected for processing (4)
crates/giskard-server/static/app.csscrates/giskard-server/static/app.jscrates/giskard-server/static/index.htmltests/e2e/tests/subagents.spec.ts
marmeladema
force-pushed
the
feat/thread-delete-card
branch
from
July 27, 2026 10:27
19a2ddf to
157785f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/tests/subagents.spec.ts (1)
448-455: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
times: 1for this one-shot delete mock
page.route(..., { times: 1 })is cleaner than callingunroute()from inside the handler. Also reword the later note: the DELETE is still sent, but it’s intercepted before reaching the backend.🤖 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 `@tests/e2e/tests/subagents.spec.ts` around lines 448 - 455, Update the DELETE interception in the subagent test to configure the route with times: 1, removing the in-handler page.unroute call while preserving the 500 response for the single request. Reword the later note to state that the DELETE is sent but intercepted before reaching the backend.
🤖 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.
Nitpick comments:
In `@tests/e2e/tests/subagents.spec.ts`:
- Around line 448-455: Update the DELETE interception in the subagent test to
configure the route with times: 1, removing the in-handler page.unroute call
while preserving the 500 response for the single request. Reword the later note
to state that the DELETE is sent but intercepted before reaching the backend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 05dfd952-b494-4575-b240-590ae6f7c53a
📒 Files selected for processing (3)
crates/giskard-server/static/app.jscrates/giskard-server/static/index.htmltests/e2e/tests/subagents.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/giskard-server/static/app.js
- crates/giskard-server/static/index.html
marmeladema
force-pushed
the
feat/thread-delete-card
branch
2 times, most recently
from
July 27, 2026 11:03
547c574 to
90a5706
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/giskard-server/static/app.js`:
- Around line 1394-1402: Update openRemoveThreadModal and the remove-thread
modal dismissal/success paths to capture the triggering thread-menu button
before focusing removeThreadConfirm, then restore focus to that opener when the
modal closes. If the opener or deleted row no longer exists, fall back to the
appropriate remaining thread-menu button or another stable focus target, while
preserving existing deletion behavior.
- Around line 1405-1408: Update the api() request helper to support cancellation
or a timeout for fetch calls, ensuring hung requests eventually resolve or abort
and restore the remove-thread modal’s usable state. Integrate this with the
removeThreadConfirm deletion flow and closeRemoveThreadModal so the existing
protection during active deletion remains, while timeout/abort cleanup
re-enables dismissal and preserves the inline error 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: 2cb87604-d539-4c06-a358-ed2f1c15164c
📒 Files selected for processing (4)
crates/giskard-server/static/app.csscrates/giskard-server/static/app.jscrates/giskard-server/static/index.htmltests/e2e/tests/subagents.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/giskard-server/static/app.css
- crates/giskard-server/static/index.html
- tests/e2e/tests/subagents.spec.ts
marmeladema
force-pushed
the
feat/thread-delete-card
branch
2 times, most recently
from
July 27, 2026 13:38
80746a0 to
232af48
Compare
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 @.gitignore:
- Around line 2-3: Ensure the e2e Docker build receives a populated
tests/e2e/.host-ca-certificates directory: update .github/workflows/e2e.yml,
tests/e2e/run.sh, and tests/e2e/screenshots.sh to generate or copy the required
.crt files before invoking docker build, or fail fast when none are present.
Update the related tests/e2e/Dockerfile CA-copy steps at lines 16-20 and 33-36
only as needed to enforce the non-empty trust-store requirement.
🪄 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: 5a57f372-2f1e-4339-85df-7a8beb2a9905
📒 Files selected for processing (7)
.gitignorecrates/giskard-server/static/app.csscrates/giskard-server/static/app.jscrates/giskard-server/static/index.htmltests/e2e/.host-ca-certificates/.gitkeeptests/e2e/Dockerfiletests/e2e/tests/subagents.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/giskard-server/static/app.css
- crates/giskard-server/static/index.html
- tests/e2e/tests/subagents.spec.ts
marmeladema
force-pushed
the
feat/thread-delete-card
branch
from
July 27, 2026 13:49
232af48 to
3f115e0
Compare
Thread deletion used a native browser confirm() while project deletion already had a proper modal. Add a matching removeThreadModal card with the thread name, cascade details for linked sub-agents and native threads, Cancel plus danger actions, and an inline error slot. The modal now tracks delete-in-flight state explicitly. It disables both actions, blocks Cancel, Escape, and backdrop dismissal while DELETE is pending, force-closes only after success, restores focus on dismissal, and surfaces bounded request failures inline. Add accessible dialog and alert markup, update Playwright coverage for cancel, Escape, backdrop, pending failure, and focus behavior, and let the e2e Docker image trust any local certificates placed in tests/e2e/.host-ca-certificates.
marmeladema
force-pushed
the
feat/thread-delete-card
branch
from
July 27, 2026 13:53
3f115e0 to
db2ca18
Compare
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
1 participant
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
Thread deletion used a native browser
confirm()alert box, while project deletion already had a proper confirmation card. This adds a matching modal for thread deletion so the two flows are consistent, per the request that users want a proper card instead of an alert box.Changes
crates/giskard-server/static/index.html— New#removeThreadModaloverlay/card, structurally identical to the existing#removeProjectModal: heading, content with the thread name + cascade description, and a footer with Cancel + danger button + inline error slot.crates/giskard-server/static/app.css—.remove-thread-dialogrules mirroring the project dialog width/spacing.crates/giskard-server/static/app.js— Replaced theconfirm(...)call indeleteThreadwithopenRemoveThreadModal/closeRemoveThreadModal:linked sub-agent threads,all corresponding Codex threads,cannot be undone) is rendered into the card via#removeThreadCascadeinstead of a native dialog string.#removeThreadErr, and disables the button while in flight — matchingremoveProjectConfirmsemantics.tests/e2e/tests/subagents.spec.ts— Updated the two tests that drovepage.waitForEvent("dialog")to assert the new modal opens and confirm via#removeThreadConfirm.Verification
cargo fmt --check,cargo clippy -D warnings,cargo test— all green.tests/e2e/run.sh tests/subagents.spec.ts— all 4 tests pass (includes the cross-project deletion and sub-agent cascade tests that now drive the modal).Summary by CodeRabbit