fix(ui): land on a draft after deleting the active thread - #141
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:
📝 WalkthroughWalkthroughDeleting the active thread now opens a fresh draft in the same project, clears ChangesActive thread deletion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DeleteHandler
participant LocalStorage
participant DraftThread
participant ProjectModel
participant Composer
User->>DeleteHandler: Confirm active thread deletion
DeleteHandler->>LocalStorage: Remove giskard.lastThread
DeleteHandler->>DraftThread: Open draft in same project
DeleteHandler->>ProjectModel: Apply project default model
DraftThread->>Composer: Focus composer input
DeleteHandler-->>User: Show New thread draft
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/tests/subagents.spec.ts (1)
414-418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert composer focus after deletion.
The new handler intentionally avoids restoring modal focus, but this test only checks visibility. Assert
#inputis focused to protect that UX contract.Proposed test addition
await expect(page.locator("`#composer`")).toBeVisible(); await expect(page.locator("`#input`")).toBeVisible(); +await expect(page.locator("`#input`")).toBeFocused(); await expect(page.locator("`#mbTitle`")).toContainText("New thread");🤖 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 414 - 418, Update the deletion-flow assertions in the end-to-end test to verify that `#input` is focused, not merely visible. Keep the existing visibility and transcript assertions unchanged while adding the focus assertion for the composer input.
🤖 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 414-418: Update the deletion-flow assertions in the end-to-end
test to verify that `#input` is focused, not merely visible. Keep the existing
visibility and transcript assertions unchanged while adding the focus assertion
for the composer input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d22fb471-8705-4293-8af9-3edf3bf2ef1e
📒 Files selected for processing (2)
crates/giskard-server/static/app.jstests/e2e/tests/subagents.spec.ts
a1e143e to
b46b3b1
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 `@crates/giskard-server/static/app.js`:
- Around line 1559-1563: Update the thread-loading flow around loadThreads and
the openedDraft condition so loadThreads returns whether refreshing succeeded.
Only evaluate knownThreadForId(pid, activeThread) when that refresh succeeded;
when it fails, rely solely on deletedIds while preserving the existing draft
behavior for confirmed missing threads.
🪄 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: f3a72283-4db3-4dda-af47-f06ed9be1771
📒 Files selected for processing (3)
crates/giskard-server/static/app.jscrates/giskard-server/tests/ui.rstests/e2e/tests/subagents.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/giskard-server/tests/ui.rs
- tests/e2e/tests/subagents.spec.ts
Deleting the currently-open thread left the view titled with the deleted
thread's name and an empty transcript ("No thread selected"). Open a fresh
draft in the same project instead, so the composer is ready for the next
conversation: tear down the WebSocket, reset the title to "New thread",
show the draft explainer, resolve the project's default model, and clear
giskard.lastThread so a reload no longer resurrects the deleted thread.
Skip the modal's focus restoration in this path so the composer input keeps
focus.
Update the Rust UI string assertion to match the new openDraftThread call
and add an #input focus assertion to the e2e test.
b46b3b1 to
2ab131a
Compare
Problem
Deleting the currently-open thread left the UI showing an empty transcript still titled with the deleted thread's name, with the composer hidden and a "No thread selected" status. The user had to manually open a new thread to keep working.
Fix
When the delete-thread confirmation handler determines the active view was the deleted thread (or a cascaded descendant), instead of
clearThreadViewit now:giskard.lastThreadfromlocalStorageso a reload no longer resurrects the deleted thread.openDraftThread(pid)— the same path the per-project "+" uses — to drop into a fresh draft in the same project: composer visible and focused, title reset to "New thread", draft explainer in the transcript, WebSocket torn down,state.threadIdcleared.applyProjectDefaultModel(pid, state.draftThread)(fire-and-forget, matchingnewThread) to resolve the project's default model in the background.The cross-project deletion guard is unchanged: deleting a thread in another project while viewing one here still leaves the active view untouched.
clearThreadViewis left intact for the archive path andclearProjectView, which legitimately leave no active thread.Test
New
active-thread deletion lands on a draftdescribe block intests/e2e/tests/subagents.spec.ts: seeds an isolated project + thread server-side, opens it, deletes it via the row menu, and asserts the view is now a draft in the same project (composer/#inputvisible,#mbTitlecontains "New thread",#transcript .draft-emptyvisible, no "No thread selected",giskard.lastThreadis null).Verification
subagents.spec.ts(8) passes — including the existing cross-project deletion and confirmation-card tests.thread.spec.ts,thread-selection.spec.ts,draft-composer.spec.ts(18) pass — no regressions in draft/thread interactions.No README/screenshots regeneration needed: no config keys, endpoints, or storage layout touched, and the screenshot generator captures a populated thread state, never the draft-after-delete state.
Summary by CodeRabbit