diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d00bacf73..c7004091ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -193,6 +193,15 @@ 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 + 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** 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..8a0bcff669 --- /dev/null +++ b/packages/app/e2e/regression/session-delete-navigation.spec.ts @@ -0,0 +1,104 @@ +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"]') +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 +// 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 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 +// 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..c69ae20c2e 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) @@ -325,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) { @@ -374,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) }) @@ -438,6 +445,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 }) @@ -459,6 +470,28 @@ export function createHomeModel() { // be pinned too. removed.forEach((id) => pinned?.remove(ServerConnection.key(conn), id)) refreshIndex() + // 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() + 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) + } + notifySessionTabsRemoved({ server: ServerConnection.key(conn), directory: session.directory, @@ -516,6 +549,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") + }) +})