From f608472a25dd69c3442fd18e041f6c4976a776ee Mon Sep 17 00:00:00 2001 From: minhn4 Date: Wed, 19 Aug 2026 18:02:46 +0700 Subject: [PATCH 1/2] fix(app): land on the next session when the open one is deleted Deleting the session on screen left the route pointing at it. The tab was spliced out of the store, nothing moved, and the window rendered "This session cannot be found" over a Close Tab button -- a dead end reached by the most ordinary way there is to delete a session, its own row in the sidebar. context/tabs.tsx already had a successor branch for this and it has been dead for a while: removeSessions() decides whether the deleted session is the one on screen from `params.dir && params.id`, and `dir` is a param of the legacy /:dir/session/:id route only. Sessions route through /server/:serverKey/session/:id now, so the condition is never true, the branch returns before navigating, and no amount of deleting moves the route. Left alone rather than revived: it is an upstream file, and two places choosing a destination would race. deleteSession (fork-only pages/home-model.tsx) picks the destination instead, from the list the user is actually looking at -- the sidebar's own rows, Pinned above Recents, snapshotted before the request because the delete refreshes the index they derive from. sessionAfterRemoval scans down from the deleted row and wraps, so the bottom row lands on the top one, and it skips every id the delete took: without that the wrap comes back around to the deleted session itself, which is the only candidate left when it was the only row. That case answers undefined and opens a new session in the same project instead. Indexed by the deleted session rather than by the session on screen, because those differ when a child session is open and its parent is deleted, and a child is not a row. Nothing happens unless the route is a session, on the server the sidebar is focused on, whose id the delete took -- so deleting a session you are not in still just removes it and its tab. Both sides get this: the list, the open path and the new-session path are all the ones the sidebar already uses, and none of them are Code-specific. The in-session header's own delete (message-timeline.tsx) is untouched. It already navigates -- parent, else the next session in the directory, else a new draft -- so it never produced the not-found screen, and giving it this rule would mean reaching the sidebar's list from a component that has no model. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 8 ++ .../session-delete-navigation.spec.ts | 95 +++++++++++++++++++ packages/app/src/pages/home-model.tsx | 49 ++++++++++ .../test-browser/home-session-records.test.ts | 53 ++++++++++- 4 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 packages/app/e2e/regression/session-delete-navigation.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d00bacf73..1176efabed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -193,6 +193,14 @@ already in use` and no way forward short of finding the file in a temporary the sandbox rather than in the project on screen. The hold that keeps the strip still while a sandbox restarts no longer applies after you switch sides. +- **Deleting the session you are reading takes you to the next one** — the tab + closed but the screen stayed on the session that had just been deleted, + saying "This session cannot be found" and offering nothing but **Close Tab**. + Deleting the session on screen now moves you to the row below it in the + sidebar, and from the bottom of the list back to the top. If the delete + emptied the list, you land on a new session in the same project. Deleting a + session you are not currently in still just removes it and its tab, leaving + you where you were. Both **Code** and **Work**. - **A folder on Work is unmounted from its own menu** — the menu on a **Work** project offered **Close**, which only took the row out of the sidebar while the sandbox went on holding the folder: the folder was still mounted, still diff --git a/packages/app/e2e/regression/session-delete-navigation.spec.ts b/packages/app/e2e/regression/session-delete-navigation.spec.ts new file mode 100644 index 0000000000..cf5b5f06a3 --- /dev/null +++ b/packages/app/e2e/regression/session-delete-navigation.spec.ts @@ -0,0 +1,95 @@ +import { expect, test, type Locator, type Page } from "@playwright/test" +import { fixture, pageMessages } from "../smoke/session-timeline.fixture" +import { mockOpenCodeServer } from "../utils/mock-server" +import { expectAppVisible, expectSessionTitle } from "../utils/waits" + +// Deleting the session you are looking at used to leave the route pointing at +// it: the tab went, the screen stayed, and what it said was "This session +// cannot be found". It now lands on the row below the deleted one in the +// sidebar, wrapping from the bottom of the list back to the top. +// +// The fixture lists two sessions, newest first, so the rows read +// [target, source] and the two directions are one row apart. + +const sidebar = (page: Page) => page.locator('[data-component="app-sidebar"]') +const rows = (page: Page) => sidebar(page).locator('[data-component="sidebar-session-row"]') +// The menu is a sibling of the row button, not a child — both sit in the +// wrapper that owns the hover state. +const rowMenu = (row: Locator) => row.locator("xpath=..").locator('[data-action="session-menu"]') +const dialog = (page: Page) => page.locator('[data-slot="dialog-content"]') + +// Hover first: the trigger is pointer-events-none until its row is hovered, and +// Playwright hit-tests before it moves the mouse, so a bare click never becomes +// actionable. +async function deleteRow(page: Page, row: Locator) { + await row.hover() + await rowMenu(row).click() + await expect(rowMenu(row)).toHaveAttribute("aria-expanded", "true") + await page.getByRole("menuitem", { name: /^Delete/ }).click() + await expectAppVisible(dialog(page)) + await dialog(page).getByRole("button", { name: "Delete session" }).click() + await expect(dialog(page)).toHaveCount(0) +} + +async function boot(page: Page) { + await mockOpenCodeServer(page, { + sessions: fixture.sessions, + provider: fixture.provider, + directory: fixture.directory, + project: fixture.project, + pageMessages, + }) + await page.addInitScript((directory) => { + localStorage.setItem( + "opencode.global.dat:server", + JSON.stringify({ + projects: { local: [{ worktree: directory, expanded: true }] }, + lastProject: { local: directory }, + }), + ) + }, fixture.directory) + await page.goto("/") + await expectAppVisible(sidebar(page)) + await expectAppVisible(rows(page).first()) +} + +test("lands on the row below when the open session is deleted", async ({ page }) => { + await boot(page) + + await rows(page).first().click() + await expectSessionTitle(page, fixture.expected.targetTitle) + + await deleteRow(page, rows(page).first()) + + await expectSessionTitle(page, fixture.expected.sourceTitle) + await expect.poll(() => new URL(page.url()).pathname).toContain(`/session/${fixture.sourceID}`) +}) + +// The bottom row has nothing below it, and stopping there is what left the +// route on a deleted session in the first place. +test("wraps to the top row when the bottom one is deleted", async ({ page }) => { + await boot(page) + + await rows(page).nth(1).click() + await expectSessionTitle(page, fixture.expected.sourceTitle) + + await deleteRow(page, rows(page).nth(1)) + + await expectSessionTitle(page, fixture.expected.targetTitle) + await expect.poll(() => new URL(page.url()).pathname).toContain(`/session/${fixture.targetID}`) +}) + +// Only the session on screen moves anyone. Deleting a row you are not in takes +// its tab and leaves you where you were. +test("stays put when the deleted session is not the one on screen", async ({ page }) => { + await boot(page) + + await rows(page).nth(1).click() + await expectSessionTitle(page, fixture.expected.sourceTitle) + const before = new URL(page.url()).pathname + + await deleteRow(page, rows(page).first()) + + await expectSessionTitle(page, fixture.expected.sourceTitle) + expect(new URL(page.url()).pathname).toBe(before) +}) diff --git a/packages/app/src/pages/home-model.tsx b/packages/app/src/pages/home-model.tsx index 3beae5f32d..82ce29b4cd 100644 --- a/packages/app/src/pages/home-model.tsx +++ b/packages/app/src/pages/home-model.tsx @@ -243,6 +243,10 @@ export function createHomeModel() { }) const isPinned = (session: Session) => pinnedIDs().includes(session.id) const togglePin = (session: Session) => pinned?.toggle(selection().server, session.id) + // Fork: the sidebar's sessions in the order it renders them — Pinned above + // Recents — which is the list "the one below this" refers to when a delete + // has to pick where to land. + const listedSessions = () => [...pinnedRecords(), ...recentRecords()].map((record) => record.session) function setSelection(next: HomeProjectSelection) { layout.home.setSelection(next) @@ -438,6 +442,10 @@ export function createHomeModel() { const conn = focusedServer() const ctx = focusedServerCtx() if (!conn || !ctx) return + // Fork: snapshotted before the request, so where the delete lands is + // decided from the list the user was looking at when they confirmed it — + // the delete itself refreshes the index this is derived from. + const listed = listedSessions() const [store, setStore] = ctx.sync.child(session.directory) const deleted = await ctx.sdk.client.session .delete({ directory: session.directory, sessionID: session.id }) @@ -464,6 +472,24 @@ export function createHomeModel() { directory: session.directory, sessionIDs: [...removed], }) + + // Fork: the deleted session's tab goes with it, but the route stays where + // it was — which left the window on a session that no longer exists, + // reading "This session cannot be found" with nothing to do but close the + // tab. Land on the row below it in the sidebar instead, and on a new + // session in the same project when the delete emptied the list. Deleting a + // session that was not on screen, or one belonging to a server the route is + // not on, moves nobody. + const route = layout.route() + if (route.type !== "session") return + if (route.server !== ServerConnection.key(conn)) return + if (!removed.has(route.sessionId)) return + const next = sessionAfterRemoval(listed, session.id, removed) + if (next) { + openSession(next) + return + } + openProjectNewSession(conn, session.directory) } function chooseProject(conn: ServerConnection.Any) { @@ -516,6 +542,29 @@ export function createHomeModel() { } } +// Fork: where deleting the session on screen lands — the row below it in the +// sidebar's own list. The scan wraps, which is what turns deleting the bottom +// row into the top one rather than into nothing. +// +// Skipping the ids the delete took is what stops the wrap from coming all the +// way back around to the deleted session itself, which is the only candidate +// left when it was the only row. That is the caller's cue to open a new session +// instead, and the one case this answers undefined for. The set covers the +// whole subtree rather than the one id, which costs nothing and does not depend +// on child sessions staying out of the index the list is built from. +// +// Indexed by the deleted session rather than by the session on screen, because +// those differ whenever a child session is open and its parent is deleted, and +// a child is not a row. A deleted session the list does not hold — one past the +// rendered cap — starts the scan at the top for the same reason: it has no row +// below it, and the top of the list is the honest answer. +export function sessionAfterRemoval(listed: Session[], deletedID: string, removed: ReadonlySet) { + const index = listed.findIndex((session) => session.id === deletedID) + return listed + .map((_, step) => listed[(index + 1 + step) % listed.length]!) + .find((session) => !removed.has(session.id)) +} + // Exported for the same reason buildHomeSessionRecords is: it is the rule that // decides how much a delete takes with it, and it is worth testing on its own. export function descendantIDs(sessions: Session[], rootID: string) { diff --git a/packages/app/test-browser/home-session-records.test.ts b/packages/app/test-browser/home-session-records.test.ts index 6f0694cab3..1f6e01fe14 100644 --- a/packages/app/test-browser/home-session-records.test.ts +++ b/packages/app/test-browser/home-session-records.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "bun:test" import type { Session } from "@opencode-ai/sdk/v2/client" import type { LocalProject } from "@/context/layout" -import { buildHomeSessionRecords, descendantIDs } from "@/pages/home-model" +import { buildHomeSessionRecords, descendantIDs, sessionAfterRemoval } from "@/pages/home-model" // The recents list the persistent sidebar and the home screen both render comes // out of buildHomeSessionRecords, and the subtree a delete evicts comes out of @@ -206,3 +206,54 @@ describe("descendantIDs", () => { expect([...descendantIDs(cyclic, "a")].sort()).toEqual(["a", "b"]) }) }) + +describe("sessionAfterRemoval", () => { + // The sidebar's own order: Pinned above Recents, each newest first. The + // function is handed one flat list because that is what the user reads. + const listed = ["pinned_a", "recent_a", "recent_b", "recent_c"].map((id) => ({ id })) as unknown as Session[] + const after = (deleted: string, removed = [deleted]) => sessionAfterRemoval(listed, deleted, new Set(removed))?.id + + test("lands on the row below the deleted one", () => { + expect(after("recent_a")).toBe("recent_b") + }) + + test("crosses from Pinned into Recents", () => { + expect(after("pinned_a")).toBe("recent_a") + }) + + test("wraps to the top when the bottom row is deleted", () => { + expect(after("recent_c")).toBe("pinned_a") + }) + + // The removed set is the whole deleted subtree, not just the one id. Children + // are not rows today, so this is the defensive half of the skip. + test("skips rows the same delete took", () => { + expect(after("recent_a", ["recent_a", "recent_b"])).toBe("recent_c") + }) + + test("wraps past a removed run to the first survivor", () => { + expect(after("recent_b", ["recent_b", "recent_c", "pinned_a"])).toBe("recent_a") + }) + + // The wrap would otherwise come back around to the deleted session and hand + // the caller the very row it just removed. Undefined is what sends the user + // to a new session instead. + test("is undefined when the delete emptied the list", () => { + expect(after("recent_a", ["pinned_a", "recent_a", "recent_b", "recent_c"])).toBeUndefined() + }) + + test("is undefined for an empty list", () => { + expect(sessionAfterRemoval([], "recent_a", new Set(["recent_a"]))).toBeUndefined() + }) + + // Past the rendered cap, so it has no row below it: the top of the list is + // the only answer that is not arbitrary. + test("starts at the top for a session the list does not hold", () => { + expect(after("ses_offscreen")).toBe("pinned_a") + }) + + test("keeps the caller's order rather than re-sorting", () => { + const reversed = ["recent_c", "recent_b", "recent_a"].map((id) => ({ id })) as unknown as Session[] + expect(sessionAfterRemoval(reversed, "recent_c", new Set(["recent_c"]))?.id).toBe("recent_b") + }) +}) From 50f24e87c14babf07cee449404f9622278b2e5d1 Mon Sep 17 00:00:00 2001 From: minhn4 Date: Wed, 19 Aug 2026 18:19:55 +0700 Subject: [PATCH 2/2] fix(app): drop the deleted session's tab instead of reviving it Found by deleting an open session in the real app: it lands on the next one now, but the session just deleted is still in the tab strip, titled "Unknown Session" and closable only by hand. The strip is kept in step with the route by an effect in titlebar.tsx: if the route names a session that has no tab, it adds one. Dropping the tabs before navigating put the app in exactly that state for an instant -- the route still on the deleted session, its tab already gone -- so the effect minted it a replacement. The replacement is titleless because the removal took the remembered title with it, and it survives the navigation that follows because nothing removes it a second time. Navigate first, then drop the tabs. The route never names a session with no tab, so there is no instant for the effect to act on. openSession and openProjectNewSession return their transitions so the delete can await the move, which also means the old session stays on screen, tab and all, until the next one is ready to replace it -- rather than the tab disappearing under a screen that has not changed yet. The e2e case that covers the landing now also asserts the strip holds exactly the tab it landed on. It fails without this: the mock answers for a deleted session, so the revived tab keeps its title there and the count is what gives it away. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 13 +++--- .../session-delete-navigation.spec.ts | 9 ++++ packages/app/src/pages/home-model.tsx | 41 +++++++++++-------- 3 files changed, 40 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1176efabed..c7004091ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -193,12 +193,13 @@ already in use` and no way forward short of finding the file in a temporary the sandbox rather than in the project on screen. The hold that keeps the strip still while a sandbox restarts no longer applies after you switch sides. -- **Deleting the session you are reading takes you to the next one** — the tab - closed but the screen stayed on the session that had just been deleted, - saying "This session cannot be found" and offering nothing but **Close Tab**. - Deleting the session on screen now moves you to the row below it in the - sidebar, and from the bottom of the list back to the top. If the delete - emptied the list, you land on a new session in the same project. Deleting a +- **Deleting the session you are reading takes you to the next one** — the + screen stayed on the session that had just been deleted, saying "This session + cannot be found" and offering nothing but **Close Tab**. Deleting the session + on screen now moves you to the row below it in the sidebar, and from the + bottom of the list back to the top. If the delete emptied the list, you land + on a new session in the same project. The deleted session leaves the tab strip + with it — no leftover **Unknown Session** tab to close by hand. Deleting a session you are not currently in still just removes it and its tab, leaving you where you were. Both **Code** and **Work**. - **A folder on Work is unmounted from its own menu** — the menu on a **Work** diff --git a/packages/app/e2e/regression/session-delete-navigation.spec.ts b/packages/app/e2e/regression/session-delete-navigation.spec.ts index cf5b5f06a3..8a0bcff669 100644 --- a/packages/app/e2e/regression/session-delete-navigation.spec.ts +++ b/packages/app/e2e/regression/session-delete-navigation.spec.ts @@ -17,6 +17,7 @@ const rows = (page: Page) => sidebar(page).locator('[data-component="sidebar-ses // wrapper that owns the hover state. const rowMenu = (row: Locator) => row.locator("xpath=..").locator('[data-action="session-menu"]') const dialog = (page: Page) => page.locator('[data-slot="dialog-content"]') +const tabs = (page: Page) => page.locator("[data-titlebar-tab-slot]") // Hover first: the trigger is pointer-events-none until its row is hovered, and // Playwright hit-tests before it moves the mouse, so a bare click never becomes @@ -63,6 +64,14 @@ test("lands on the row below when the open session is deleted", async ({ page }) await expectSessionTitle(page, fixture.expected.sourceTitle) await expect.poll(() => new URL(page.url()).pathname).toContain(`/session/${fixture.sourceID}`) + // The deleted session's tab goes with it, leaving only the one landed on. + // Removing the tab and then navigating away re-added it from the route the + // instant it had no tab, and it came back as an "Unknown Session" ghost — + // titleless, because the removal took its remembered title too. The mock + // still answers for a deleted session, so here the ghost keeps its title; + // what gives it away either way is a tab that should not be in the strip. + await expect(tabs(page)).toHaveCount(1) + await expect(tabs(page).locator("a").first()).toHaveAttribute("href", new RegExp(`/session/${fixture.sourceID}$`)) }) // The bottom row has nothing below it, and stopping there is what left the diff --git a/packages/app/src/pages/home-model.tsx b/packages/app/src/pages/home-model.tsx index 82ce29b4cd..c69ae20c2e 100644 --- a/packages/app/src/pages/home-model.tsx +++ b/packages/app/src/pages/home-model.tsx @@ -329,7 +329,8 @@ export function createHomeModel() { tabs.select(existing) return } - tabs.newDraft({ server, directory }) + // Fork: returned for the same reason openSession's transition is. + return tabs.newDraft({ server, directory }) } function editProject(conn: ServerConnection.Any, project: LocalProject) { @@ -378,7 +379,9 @@ export function createHomeModel() { return } ctx.projects.touch(directory) - startTransition(() => { + // Fork: returned so a caller that must not act until the route has moved + // can await it — deleteSession drops the old tabs only once this lands. + return startTransition(() => { const tab = tabs.addSessionTab({ server: ServerConnection.key(conn), sessionId: session.id, directory }) tabs.select(tab) }) @@ -467,29 +470,33 @@ export function createHomeModel() { // be pinned too. removed.forEach((id) => pinned?.remove(ServerConnection.key(conn), id)) refreshIndex() - notifySessionTabsRemoved({ - server: ServerConnection.key(conn), - directory: session.directory, - sessionIDs: [...removed], - }) - - // Fork: the deleted session's tab goes with it, but the route stays where + // Fork: the deleted session's tab goes with it, but the route stayed where // it was — which left the window on a session that no longer exists, // reading "This session cannot be found" with nothing to do but close the // tab. Land on the row below it in the sidebar instead, and on a new // session in the same project when the delete emptied the list. Deleting a // session that was not on screen, or one belonging to a server the route is // not on, moves nobody. + // + // Ahead of dropping the tabs, and awaited, because the titlebar keeps a tab + // for whatever session the route names: leaving first and removing second + // means the route never names a session with no tab, which is the state + // that had it mint the deleted session a fresh, titleless tab — the + // "Unknown Session" left behind in the strip. const route = layout.route() - if (route.type !== "session") return - if (route.server !== ServerConnection.key(conn)) return - if (!removed.has(route.sessionId)) return - const next = sessionAfterRemoval(listed, session.id, removed) - if (next) { - openSession(next) - return + const leaving = + route.type === "session" && route.server === ServerConnection.key(conn) && removed.has(route.sessionId) + if (leaving) { + const next = sessionAfterRemoval(listed, session.id, removed) + if (next) await openSession(next) + else await openProjectNewSession(conn, session.directory) } - openProjectNewSession(conn, session.directory) + + notifySessionTabsRemoved({ + server: ServerConnection.key(conn), + directory: session.directory, + sessionIDs: [...removed], + }) } function chooseProject(conn: ServerConnection.Any) {