Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions crates/giskard-server/static/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,7 @@ function restoreLastThread() {
}

async function loadThreads(pid) {
const box = $("threads-"+pid); if (!box) return;
const box = $("threads-"+pid); if (!box) return false;
try {
const { threads } = await api("GET",`/api/projects/${pid}/threads`);
rememberProjectThreads(pid, threads);
Expand All @@ -764,7 +764,10 @@ async function loadThreads(pid) {
// (say, catching up on a sub-agent the server just materialized) would otherwise leave the list
// with no visibly selected thread.
syncActiveThreadHighlight();
} catch {}
return true;
} catch {
return false;
}
}

function rememberProjectThreads(pid, threads) {
Expand Down Expand Up @@ -1551,19 +1554,35 @@ $("removeThreadConfirm").onclick = async () => {
// was deleted; keep the pre-request set as a fallback in case the sidebar reload fails
// (loadThreads swallows its own errors and leaves the cached list stale).
const deletedIds = new Set([String(tid), ...descendants]);
await loadThreads(pid);
const threadsRefreshed = await loadThreads(pid);
// Read the live view only after the awaits: the user may have navigated to another thread
// or another project meanwhile, and an unrelated active view must never be cleared.
const activeThread = state.threadId ? String(state.threadId) : null;
const sameProject = String(state.projectId || "") === String(pid);
let openedDraft = false;
if (
activeThread && sameProject &&
(deletedIds.has(activeThread) || !knownThreadForId(pid, activeThread))
(deletedIds.has(activeThread) ||
(threadsRefreshed && !knownThreadForId(pid, activeThread)))
) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
clearThreadView(activeThread);
// The active thread was just deleted (or cascaded away). Rather than leave the user staring
// at an empty view still titled with the deleted thread's name, drop into a fresh draft in
// the same project so the composer is ready for the next conversation. `openDraftThread`
// tears down the WebSocket, clears `state.threadId`, and resets the title to "New thread".
// The explicit `giskard.lastThread` removal above prevents a reload from resurrecting the
// deleted thread (`openDraftThread` itself never touches that key; without this, reload
// would self-heal via `restoreLastThread` failing to find the thread, but clearing it now
// avoids the transient stale entry).
try { localStorage.removeItem("giskard.lastThread"); } catch {}
openDraftThread(pid);
applyProjectDefaultModel(pid, state.draftThread);
openedDraft = true;
}
pending.deleting = false;
closeRemoveThreadModal({ force:true });
// Skip focus restoration only when we just opened a draft: `openDraftThread` already focuses
// the composer input, and restoring focus to the deleted thread's row button would yank it
// away from the input the user is now expected to type into.
closeRemoveThreadModal({ force:true, restoreFocus: !openedDraft });
} catch (e) {
if (state.pendingRemoveThread === pending && pending.requestSeq === requestSeq) {
$("removeThreadErr").textContent = "Delete thread failed: " + apiFailureMessage(e);
Expand Down
31 changes: 24 additions & 7 deletions crates/giskard-server/tests/ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,13 +494,26 @@ async fn index_page_is_served_and_public() {
&& body.contains("querySelectorAll(\".thread-menu, .project-menu\")"),
"project rows expose a remove action with source-directory-safe confirmation"
);
let remove_thread_confirm = between(
&body,
"$(\"removeThreadConfirm\").onclick = async () => {",
"function clearThreadView",
);
assert!(
body.contains("function threadDescendantIds(pid, tid)")
&& body.contains("linked sub-agent thread")
&& body.contains("all corresponding Codex threads")
&& body.contains("This cannot be undone")
&& body.contains("clearThreadView(activeThread)"),
"thread deletion warns about and clears recursively deleted sub-agent threads"
&& body.contains("linked sub-agent thread")
&& body.contains("all corresponding Codex threads")
&& body.contains("This cannot be undone")
// Deleting the active thread now drops into a fresh draft in the same project instead
// of leaving an empty view titled with the deleted thread's name.
&& remove_thread_confirm.contains("const threadsRefreshed = await loadThreads(pid)")
&& remove_thread_confirm.contains(
"threadsRefreshed && !knownThreadForId(pid, activeThread)"
)
&& remove_thread_confirm.contains("openDraftThread(pid)")
&& remove_thread_confirm.contains("applyProjectDefaultModel(pid, state.draftThread)")
&& !remove_thread_confirm.contains("clearThreadView("),
"thread deletion warns about and lands on a draft in the same project"
);
assert!(
body.contains("overflow:visible")
Expand Down Expand Up @@ -611,7 +624,9 @@ async fn index_page_is_served_and_public() {
dropping them"
);
assert!(
body.contains("deletedIds.has(activeThread) || !knownThreadForId(pid, activeThread)")
remove_thread_confirm.contains("deletedIds.has(activeThread) ||")
&& remove_thread_confirm
.contains("threadsRefreshed && !knownThreadForId(pid, activeThread)")
&& body.contains("String(state.projectId || \"\") === String(pid)"),
"cascade delete clears the active view from the refreshed thread list, scoped to the \
deleted thread's project so unrelated views survive"
Expand Down Expand Up @@ -731,7 +746,9 @@ async fn index_page_is_served_and_public() {
body.contains(
" appendThreadRows(box, pid, archived);\n }\n // Rebuilding the rows discards \
the selection highlight"
) && body.contains(" syncActiveThreadHighlight();\n } catch {}"),
) && body.contains(
" syncActiveThreadHighlight();\n return true;\n } catch {\n return false;"
),
"reloading a project's threads re-derives the selection highlight, so a reload not driven \
by opening a thread cannot leave the sidebar with nothing selected"
);
Expand Down
103 changes: 103 additions & 0 deletions tests/e2e/tests/subagents.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,109 @@ test.describe("cross-project thread deletion", () => {
});
});

// Deleting the thread that is currently open used to leave the view titled with the deleted
// thread's name and an empty transcript ("No thread selected"). It now drops into a fresh draft
// in the same project so the composer is ready for the next conversation, and clears the persisted
// last-thread so a reload no longer resurrects the deleted thread.
test.describe("active-thread deletion lands on a draft", () => {
test.beforeEach(async ({ page }) => {
await login(page);
});

// Shared seeding helpers: create an isolated project + persisted thread server-side so the
// test below drives only the deletion without depending on the shared composer/WebSocket
// timing. Mirrors the helpers in the "delete-thread confirmation card" block below.
const createProject = (page: import("@playwright/test").Page, name: string): Promise<string> =>
page.evaluate(async (projectName) => {
const res = await fetch("/api/projects", {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
name: projectName,
dir: "/tmp",
default_model: { provider: "replay", model: "replay-model" },
}),
});
if (!res.ok) {
throw new Error(`create project failed: ${res.status} ${await res.text()}`);
}
return (await res.json()).id as string;
}, name);
const startThread = (page: import("@playwright/test").Page, pid: string, text: string): Promise<string> =>
page.evaluate(
async ({ pid, text }) => {
const res = await fetch(`/api/projects/${pid}/threads/start`, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
text,
model_ref: { provider: "replay", model: "replay-model" },
mode: "build",
permission_preset: "ask_first",
}),
});
if (!res.ok) {
throw new Error(`start thread failed: ${res.status} ${await res.text()}`);
}
return (await res.json()).thread_id as string;
},
{ pid, text },
);
const deleteProject = (page: import("@playwright/test").Page, id: string): Promise<void> =>
page.evaluate(async (projectId) => {
await fetch(`/api/projects/${projectId}`, { method: "DELETE" });
}, id);

test("deleting the open thread opens a draft in the same project", async ({ page }) => {
const projectId = await createProject(page, "Draft-after-delete project");
try {
const tid = await startThread(page, projectId, "delete me while open");
await page.evaluate(() =>
(window as unknown as { loadProjects: () => Promise<void> }).loadProjects(),
);
const row = page.locator(`.thread[data-tid="${tid}"]`);
await expect(row).toBeVisible();

// Open the thread so it is unambiguously the active view.
await row.click();
await expect.poll(async () =>
page.evaluate(() =>
JSON.parse(localStorage.getItem("giskard.lastThread") || "null"),
),
).toEqual({ pid: projectId, tid });

// Delete the active thread via its row menu.
const rowContainer = row.locator("xpath=..");
await rowContainer.locator(".thread-menu-btn").click();
await rowContainer.locator(".thread-menu .danger").click();
await expect(page.locator("#removeThreadModal")).toHaveClass(/open/);
await page.locator("#removeThreadConfirm").click();
await expect(row).toHaveCount(0);

// The view is now a draft in the same project: the composer is visible and ready, the
// title bar shows "New thread" (not the deleted thread's name), and the transcript shows
// the draft explainer rather than the stale thread or an empty "No thread selected" view.
await expect(page.locator("#composer")).toBeVisible();
await expect(page.locator("#input")).toBeVisible();
// The composer keeps focus after the deletion: the modal skips focus restoration so it
// does not yank focus back to the deleted thread's row button.
await expect(page.locator("#input")).toBeFocused();
await expect(page.locator("#mbTitle")).toContainText("New thread");
await expect(page.locator("#transcript .draft-empty")).toBeVisible();
await expect(page.locator("#transcript")).not.toContainText("No thread selected");

// The deleted thread is no longer the persisted last-thread, so a reload does not
// resurrect it; the draft view is what reload would land on (no lastThread entry).
const lastAfter = await page.evaluate(() =>
JSON.parse(localStorage.getItem("giskard.lastThread") || "null"),
);
expect(lastAfter).toBeNull();
} finally {
await deleteProject(page, projectId);
}
});
});

test.describe("delete-thread confirmation card", () => {
test.beforeEach(async ({ page }) => {
await login(page);
Expand Down
Loading