From eaeeb09ab1f257abceafc712609f651c42beb58f Mon Sep 17 00:00:00 2001 From: xarmian Date: Sat, 18 Jul 2026 16:51:45 -0400 Subject: [PATCH] =?UTF-8?q?feat(web):=20pane-navigation=20controller=20?= =?UTF-8?q?=E2=80=94=20depth/ownership=20state=20machine=20(TASK-2157)=20(?= =?UTF-8?q?#962)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(web): pane-navigation controller — depth/ownership state machine (TASK-2157) Turn the collection page's split pane (PLAN-2105) into a navigable mini-browser per PLAN-2154 Architecture A. Depth + session ownership are stamped in SvelteKit page.state (never raw history.state), so they follow opaque Back/Forward, survive history.go, and reconstruct on cold-load. - New pure controller ($lib/collections/paneController.ts): planPaneDrill (same-ref guard + soft depth cap + ownership INHERITANCE), planLateralOpen (first-open mints ownership / depth-0 re-target / depth>0 stack reset), planPaneClose (three-way staged unwind). Fully unit-tested (26 cases). - navigatePaneTo(target) drill added beside openItemPane; ownership created only by first-open, inherited by drills (cold-load base = unowned). - Three-way ownership-aware staged close: OWNED -> go(-(depth+1)); UNOWNED & depth>0 -> go(-depth) then afterNavigate-latched replaceState-delete; UNOWNED & depth 0 -> replaceState-delete. - R14 fence-on-continuation baked in: controllerActionSeq + a one-shot afterNavigate latch (seq-fenced, state-rechecked), schedulePaneFollow made inert at depth>0 (schedule + fired callback) and cancelled on drill/close, an in-flight guard so a rapid gesture can't stack a second history.go. - depth+ownership preserved through every ?item=-preserving nav: updateUrlFilters, the ?graph toggle (ItemDetail), and the collection rename onNavigateAway (now replaceState, not push). - navigatePaneTo exported onto the pane ItemDetail seam for TASK-2158 and reachable now via a localStorage-gated __padPaneController test hook. Tests: 26 unit + 5 Playwright e2e (open/close/j-k, drill/back/same-ref, detach j/k inertness, cold-load close, detached-row reset). Existing pane e2e suite (13) still green. Closes TASK-2157 Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): rebase pane ownership on collection rename + tie latch to its popstate Codex review of the pane controller (TASK-2157): - P1 (rename ownership): a collection rename replaceState's /old?item=X -> /new?item=X, but every predecessor history entry still points at the now- dead OLD slug — carrying paneOwned=true forward made an owned close history.go back onto a 404. Ownership means "a live pre-pane entry exists to unwind to", which is false after a rename, so onNavigateAway now stamps a fresh {paneDepth:0, paneOwned:false} base on the new slug: close drops ?item= in place, staying on the valid new route. New e2e covers it. - P2 (latch): gate the afterNavigate latch on nav.type==='popstate' so only its own history.go can consume it; a competing goto/link/form leaves it armed until the go settles. - P1 (owned close discards mid-pane filter changes): documented as the plan-mandated R8 behavior — an explicit close is now identical to the browser Back that already closed the pane in PLAN-2105 (no deviation). Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): harden pane latch consumption + bypass draft guard on rename Second Codex round (TASK-2157): - Latch is no longer dropped by a competing history traversal: run() now RETURNS whether it reached its destination (depth collapsed to the base); the afterNavigate handler consumes the latch only when run() fires, so an unrelated browser Back/Forward during the go's in-flight window leaves the latch armed for its own popstate instead of clearing it against the wrong entry. - Collection rename now bypasses the unsaved-draft beforeNavigate guard (navigatePaneAfterRename): the server-side rename already committed and the route component is reused across the same-route pathname change (drafts survive), so a "Stay" prompt could otherwise strand the user on the dead old slug with a stale owned stamp. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): bound pane latch with a fallback timer + stronger reset correlation Third Codex round (TASK-2157): - P1 lockup: the "leave the latch armed until run() reaches its destination" rule could leave paneNavInFlight() stuck forever if the arming history.go was superseded (its own popstate never lands). Add a bounded fallback timer (PANE_LATCH_FALLBACK_MS) that best-effort-fires then UNCONDITIONALLY clears the latch, so the in-flight guard can never stick. clearPaneLatch() also tears down the timer (onDestroy + on consume). - P2 reset correlation: the detached-open reset now requires the landing entry to carry ?item= (the pane base), not just depth 0 — rejecting a competing browser Back that landed on the pre-pane (no-?item=) entry. - P2 rename + browser Back: documented that Back to the old-slug predecessor is an inherent rename-in-history limitation (past entries can't be rewritten), out of the controller's reach; the imperative close is fixed. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): fence every controller history.go + split rename vs move nav-away Fourth Codex round (TASK-2157): - P1 (move vs rename): onNavigateAway is fired by ItemDetail for BOTH a collection rename (/user/ws/NEWSLUG?item=X — pane preserved) AND a cross-collection item move (/user/ws/coll/slug — full-page route, no ?item=). handlePaneNavigateAway now branches on whether the target keeps the pane (?item=): rename gets the rebase-to-unowned + draft-guard bypass; a move keeps the ORIGINAL guarded push so its unsaved-draft prompt still fires (the collection page unmounts and would lose drafts). - P2 (duplicate close): the production owned-go close is a one-phase history.go(-1) that wasn't fenced, so a double-click ✕ / ESC+click could stack a second traversal and overshoot the pre-pane entry. Unify all controller traversals (owned close, cold-base close, reset) through paneHistoryGo(), which marks navigation in-flight (paneNavInFlight blocks a duplicate gesture) until the traversal's own popstate settles or a bounded fallback. New e2e asserts a double close lands exactly on the pre-pane URL. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra --- web/e2e/pane-controller.spec.ts | 417 ++++++++++++++++++ web/src/app.d.ts | 10 +- .../lib/collections/paneController.test.ts | 244 ++++++++++ web/src/lib/collections/paneController.ts | 185 ++++++++ .../lib/components/items/ItemDetail.svelte | 7 +- .../[workspace]/[collection]/+page.svelte | 330 +++++++++++++- 6 files changed, 1176 insertions(+), 17 deletions(-) create mode 100644 web/e2e/pane-controller.spec.ts create mode 100644 web/src/lib/collections/paneController.test.ts create mode 100644 web/src/lib/collections/paneController.ts diff --git a/web/e2e/pane-controller.spec.ts b/web/e2e/pane-controller.spec.ts new file mode 100644 index 00000000..0536fece --- /dev/null +++ b/web/e2e/pane-controller.spec.ts @@ -0,0 +1,417 @@ +import { test, expect } from './fixtures'; +import { browserLogin, seedDoc } from './lib/collab-helpers'; +import type { APIRequestContext, Page } from '@playwright/test'; +import type { SuiteFixture } from './fixtures'; + +/** + * Pane-navigation controller — depth/ownership state machine + * (PLAN-2154 Architecture A / TASK-2157). + * + * The collection page's split pane (PLAN-2105) becomes a navigable + * mini-browser: `openItemPane` handles lateral/list opens; `navigatePaneTo` + * handles in-pane DRILLS; `closeItemPane` does a three-way, ownership-aware, + * staged unwind. Depth + ownership are stamped in SvelteKit `page.state` so + * they follow opaque Back/Forward, survive `history.go`, and reconstruct on + * cold-load. + * + * `navigatePaneTo` has NO in-pane content-link caller yet (those land in + * TASK-2158/2159/2160). We exercise it — and read back the depth/ownership + * stamp — through the localStorage-gated `window.__padPaneController` test hook + * this task ships, so the drill / reset / three-way-close arithmetic is + * verified end-to-end in a REAL browser before the UI callers exist. jsdom has + * no history/navigation model, so the pure arithmetic is unit-tested in + * paneController.test.ts and the runtime wiring is verified here. + * + * Viewport is driven explicitly (desktop split), so one project is enough. + */ + +const DESKTOP = { width: 1200, height: 900 }; + +function docsUrl(fixture: SuiteFixture, query = ''): string { + return `/${fixture.adminUsername}/${fixture.workspaceSlug}/docs${query}`; +} + +function openItemParam(page: Page): string | null { + return new URL(page.url()).searchParams.get('item'); +} + +function pathname(page: Page): string { + return new URL(page.url()).pathname; +} + +interface HookState { + paneDepth: number; + paneOwned: boolean; +} + +/** Read the controller's live {paneDepth, paneOwned} via the test hook. */ +function paneState(page: Page): Promise { + return page.evaluate(() => { + const c = (window as unknown as { __padPaneController?: { getPaneState(): HookState } }) + .__padPaneController; + return c ? c.getPaneState() : null; + }); +} + +/** Drive an in-pane DRILL (`navigatePaneTo`) via the test hook. */ +async function drillTo(page: Page, ref: string): Promise { + await page.evaluate((r) => { + ( + window as unknown as { __padPaneController?: { navigatePaneTo(ref: string): void } } + ).__padPaneController?.navigatePaneTo(r); + }, ref); +} + +/** Imperative close via the test hook (exercises the three-way close). */ +async function hookClose(page: Page): Promise { + await page.evaluate(() => { + ( + window as unknown as { __padPaneController?: { closeItemPane(): void } } + ).__padPaneController?.closeItemPane(); + }); +} + +function historyLength(page: Page): Promise { + return page.evaluate(() => history.length); +} + +/** Enable the controller test hook for all navigations in this context. */ +async function enableHook(page: Page): Promise { + await page.addInitScript(() => { + try { + localStorage.setItem('pad:pane-test-hook', '1'); + } catch { + /* private mode / disabled storage — hook simply won't install */ + } + }); +} + +function authHeaders(fixture: SuiteFixture) { + return { Authorization: `Bearer ${fixture.apiToken}`, 'Content-Type': 'application/json' }; +} + +/** Create a fresh, test-scoped renamable collection (letters-only prefix, name + * unique — see pane-collection-migration-race.spec.ts for the slug/prefix + * gotchas). */ +async function seedCollection( + fixture: SuiteFixture, + request: APIRequestContext, + namePrefix: string, + itemPrefix: string, +): Promise<{ id: string; slug: string; name: string }> { + const name = `${namePrefix} ${Date.now()}`; + const schema = JSON.stringify({ fields: [{ key: 'note', label: 'Note', type: 'text' }] }); + const resp = await request.post(`/api/v1/workspaces/${fixture.workspaceSlug}/collections`, { + headers: authHeaders(fixture), + data: { name, prefix: itemPrefix, schema }, + }); + if (!resp.ok()) throw new Error(`collection create failed (${resp.status()}): ${await resp.text()}`); + return (await resp.json()) as { id: string; slug: string; name: string }; +} + +async function seedItemIn( + fixture: SuiteFixture, + request: APIRequestContext, + collSlug: string, + title: string, +): Promise<{ id: string; slug: string }> { + const resp = await request.post( + `/api/v1/workspaces/${fixture.workspaceSlug}/collections/${collSlug}/items`, + { headers: authHeaders(fixture), data: { title, fields: JSON.stringify({}), content: '' } }, + ); + if (!resp.ok()) throw new Error(`item create failed (${resp.status()}): ${await resp.text()}`); + return (await resp.json()) as { id: string; slug: string }; +} + +test.describe('pane controller: depth/ownership state machine (PLAN-2154 / TASK-2157)', () => { + // The controller is viewport-agnostic; the desktop split project is enough. + test.beforeEach(({}, testInfo) => { + test.skip( + testInfo.project.name !== 'desktop-chromium', + 'controller is viewport-agnostic; the desktop split project is enough', + ); + }); + + test('no PLAN-2105 regression: click-open (owned depth 0), j/k re-target (still depth 0), close returns to pre-pane URL', async ({ + page, + fixture, + request, + }) => { + await page.setViewportSize(DESKTOP); + await enableHook(page); + await browserLogin(page); + await seedDoc(fixture, request, 'Ctrl regress alpha'); + await seedDoc(fixture, request, 'Ctrl regress bravo'); + await page.goto(docsUrl(fixture)); + + const prePaneUrl = page.url(); + const row = page.locator('.item-card', { hasText: 'Ctrl regress alpha' }).first(); + await expect(row).toBeVisible(); + await row.click(); + + const pane = page.locator('.item-pane'); + await expect(pane).toBeVisible(); + const refA = openItemParam(page); + expect(refA).not.toBeNull(); + // First-open MINTS ownership at depth 0. + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 0, paneOwned: true }); + + // j moves the list cursor; the pane FOLLOWS (re-target replace) — still + // depth 0, still owned, and NO new history entry (the PLAN-2105 fix). + const lenAfterOpen = await historyLength(page); + await page.keyboard.press('j'); + await expect.poll(() => openItemParam(page)).not.toBe(refA); + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 0, paneOwned: true }); + expect(await historyLength(page)).toBe(lenAfterOpen); // replace, not push + await expect(pane).toBeVisible(); + + // Close via the pane's real ✕ (aria-label is unique across the loaded and + // minimal headers, which are mutually exclusive) — OWNED depth 0 → + // history.go(-1) back to the exact pre-pane URL (pane gone, `?item=` gone). + await pane.locator('button[aria-label="Close pane"]').click(); + await expect.poll(() => openItemParam(page)).toBeNull(); + await expect(pane).toBeHidden(); + await expect.poll(() => page.url()).toBe(prePaneUrl); + }); + + test('a duplicate close gesture cannot stack a second history.go and overshoot the pre-pane URL', async ({ + page, + fixture, + request, + }) => { + await page.setViewportSize(DESKTOP); + await enableHook(page); + await browserLogin(page); + await seedDoc(fixture, request, 'Ctrl dblclose alpha'); + await page.goto(docsUrl(fixture)); + + const prePaneUrl = page.url(); + await page.locator('.item-card', { hasText: 'Ctrl dblclose alpha' }).first().click(); + const pane = page.locator('.item-pane'); + await expect(pane).toBeVisible(); + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 0, paneOwned: true }); + + // Fire closeItemPane TWICE synchronously (a double-click ✕ / ESC+click + // race). The owned close is a one-phase history.go(-1); without the + // in-flight fence the second call would read the still-stale state and + // stack a SECOND go(-1), overshooting PAST the pre-pane entry (here, back + // to /login). The fence must make the second call a no-op. + await page.evaluate(() => { + const c = ( + window as unknown as { __padPaneController?: { closeItemPane(): void } } + ).__padPaneController; + c?.closeItemPane(); + c?.closeItemPane(); + }); + + await expect.poll(() => openItemParam(page)).toBeNull(); + await expect(pane).toBeHidden(); + // Landed EXACTLY on the pre-pane URL — not overshot to /login. + await expect.poll(() => page.url()).toBe(prePaneUrl); + expect(page.url()).not.toContain('/login'); + }); + + test('drill A→B pushes a depth-1 owned entry; browser Back returns to A at depth 0', async ({ + page, + fixture, + request, + }) => { + await page.setViewportSize(DESKTOP); + await enableHook(page); + await browserLogin(page); + await seedDoc(fixture, request, 'Ctrl drill alpha'); + const b = await seedDoc(fixture, request, 'Ctrl drill bravo'); + await page.goto(docsUrl(fixture)); + + await page.locator('.item-card', { hasText: 'Ctrl drill alpha' }).first().click(); + const pane = page.locator('.item-pane'); + await expect(pane).toBeVisible(); + const refA = openItemParam(page); + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 0, paneOwned: true }); + const lenBeforeDrill = await historyLength(page); + + // DRILL A→B (no UI caller yet → test hook). Pushes depth 1, INHERITING + // ownership (owned base → owned drill). + await drillTo(page, b.slug); + await expect.poll(() => openItemParam(page)).toBe(b.slug); + await expect(pane.locator('.title', { hasText: /Ctrl drill bravo/ })).toBeVisible(); + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 1, paneOwned: true }); + expect(await historyLength(page)).toBe(lenBeforeDrill + 1); // push + + // Same-ref guard (D4): re-drilling to the CURRENT item is a no-op — no + // new history entry, depth unchanged. + const lenAtB = await historyLength(page); + await drillTo(page, b.slug); + await page.waitForTimeout(50); + expect(await historyLength(page)).toBe(lenAtB); + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 1, paneOwned: true }); + + // Browser Back unwinds ONE hop → back to A at depth 0. + await page.goBack(); + await expect.poll(() => openItemParam(page)).toBe(refA); + await expect(pane.locator('.title', { hasText: /Ctrl drill alpha/ })).toBeVisible(); + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 0, paneOwned: true }); + }); + + test('detach: j/k is INERT at depth>0 (a list follow cannot re-target a drilled stack)', async ({ + page, + fixture, + request, + }) => { + await page.setViewportSize(DESKTOP); + await enableHook(page); + await browserLogin(page); + await seedDoc(fixture, request, 'Ctrl detach alpha'); + const b = await seedDoc(fixture, request, 'Ctrl detach bravo'); + await seedDoc(fixture, request, 'Ctrl detach charlie'); + await page.goto(docsUrl(fixture)); + + await page.locator('.item-card', { hasText: 'Ctrl detach alpha' }).first().click(); + const pane = page.locator('.item-pane'); + await expect(pane).toBeVisible(); + + // Drill to B → depth 1 (detached). + await drillTo(page, b.slug); + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 1, paneOwned: true }); + const refAtB = openItemParam(page); + + // Move focus to the list, then press j. schedulePaneFollow must BAIL at + // depth>0 (both at schedule time and in the fired callback), so the pane + // stays on B — j/k does NOT laterally re-target a drilled stack. + await page.evaluate(() => document.querySelector('.list-column')?.focus()); + await page.keyboard.press('j'); + await page.waitForTimeout(250); // > PANE_FOLLOW_DEBOUNCE_MS (140ms) + expect(openItemParam(page)).toBe(refAtB); + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 1, paneOwned: true }); + }); + + test('cold-load close: a cold `?item=` then drill closes back to the cold base — never go(-2) off it', async ({ + page, + fixture, + request, + }) => { + await page.setViewportSize(DESKTOP); + await enableHook(page); + await browserLogin(page); + const a = await seedDoc(fixture, request, 'Ctrl cold alpha'); + const b = await seedDoc(fixture, request, 'Ctrl cold bravo'); + + // COLD LOAD: deep-link straight into an open pane. No history stamp → + // UNOWNED base. + await page.goto(docsUrl(fixture, `?item=${a.slug}`)); + const pane = page.locator('.item-pane'); + await expect(pane).toBeVisible(); + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 0, paneOwned: false }); + const coldBasePath = pathname(page); + + // Drill A→B: INHERITS the cold base's unowned stamp (depth 1, UNOWNED) — + // this is what keeps the cold-base close branch reachable. + await drillTo(page, b.slug); + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 1, paneOwned: false }); + + // Imperative close: UNOWNED depth>0 → history.go(-1) to the cold base, + // THEN a latched replaceState-delete of `?item=`. Must NOT go(-2) off the + // base into whatever preceded the cold load (the /login page here). + await hookClose(page); + await expect.poll(() => openItemParam(page)).toBeNull(); + await expect(pane).toBeHidden(); + // Landed on the SAME collection page (the cold base), not an earlier page. + expect(pathname(page)).toBe(coldBasePath); + expect(page.url()).not.toContain('/login'); + }); + + test('detached row click RESETS the stack: a new top-level open, closing cleanly to the pre-pane URL', async ({ + page, + fixture, + request, + }) => { + await page.setViewportSize(DESKTOP); + await enableHook(page); + await browserLogin(page); + await seedDoc(fixture, request, 'Ctrl reset alpha'); + const b = await seedDoc(fixture, request, 'Ctrl reset bravo'); + await seedDoc(fixture, request, 'Ctrl reset charlie'); + await page.goto(docsUrl(fixture)); + + const prePaneUrl = page.url(); + await page.locator('.item-card', { hasText: 'Ctrl reset alpha' }).first().click(); + const pane = page.locator('.item-pane'); + await expect(pane).toBeVisible(); + + // Drill A→B (depth 1). + await drillTo(page, b.slug); + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 1, paneOwned: true }); + + // Now a DIRECT list-row click on a THIRD item (charlie) while detached: + // resets the stack (go(-depth) to base, then a latched re-target), + // landing at depth 0 (the base ownership preserved → owned). + await page.locator('.item-card', { hasText: 'Ctrl reset charlie' }).first().click(); + await expect(pane.locator('.title', { hasText: /Ctrl reset charlie/ })).toBeVisible(); + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 0, paneOwned: true }); + + // The reset collapsed the stack, so closing now returns straight to the + // pre-pane URL in a single unwind (owned go(-1)). + await pane.locator('button[aria-label="Close pane"]').click(); + await expect.poll(() => openItemParam(page)).toBeNull(); + await expect.poll(() => page.url()).toBe(prePaneUrl); + }); + + test('collection rename keeps the pane on the NEW slug and closes cleanly (ownership rebased, not 404)', async ({ + page, + fixture, + request, + }) => { + await page.setViewportSize(DESKTOP); + await enableHook(page); + await browserLogin(page); + const coll = await seedCollection(fixture, request, 'Ctrl rename', 'CTRN'); + await seedItemIn(fixture, request, coll.slug, 'Rename target item'); + + await page.goto(`/${fixture.adminUsername}/${fixture.workspaceSlug}/${coll.slug}`); + await page.locator('.item-card', { hasText: 'Rename target item' }).first().click(); + const pane = page.locator('.item-pane'); + await expect(pane).toBeVisible(); + // Owned first-open at depth 0. + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 0, paneOwned: true }); + // The `?item=` value (canonical ref or slug) that must survive the rename. + const itemParam = openItemParam(page); + expect(itemParam).not.toBeNull(); + + // Rename the collection via the pane's Quick Actions → Manage actions → + // Edit Collection modal (General tab). The rename changes the pathname + // (old→new slug) while preserving `?item=` — routed through + // onNavigateAway (must replaceState + rebase ownership). + await pane.locator('button.trigger-btn[title="Quick actions"]').click(); + await pane.locator('button.action-item.footer-row', { hasText: 'Manage actions' }).click(); + await expect(page.locator('#edit-collection-title')).toBeVisible(); + // "Manage actions" opens the modal on the Actions tab — switch to General + // to reach the collection-name field. + await page.locator('button.tab', { hasText: 'General' }).click(); + const newName = `Ctrl renamed ${Date.now()}`; + await page.locator('input.name-input').fill(newName); + await page.locator('button.btn-save', { hasText: 'Save Changes' }).click(); + + // The pane survives on the NEW slug with `?item=` intact and the pane not + // remounted away. + await expect.poll(() => new URL(page.url()).pathname).not.toContain(`/${coll.slug}`); + const newPath = new URL(page.url()).pathname; + expect(newPath).toMatch(new RegExp(`/${fixture.workspaceSlug}/[^/]+$`)); + expect(openItemParam(page)).toBe(itemParam); + await expect(pane).toBeVisible(); + // Ownership REBASED to a fresh unowned depth-0 base on the new slug (the + // pre-pane entry now points at the dead old slug). + await expect.poll(() => paneState(page)).toEqual({ paneDepth: 0, paneOwned: false }); + + // Closing must drop `?item=` IN PLACE (staying on the valid new slug), + // never `history.go` back onto the now-404 old slug. + await pane.locator('button[aria-label="Close pane"]').click(); + await expect.poll(() => openItemParam(page)).toBeNull(); + await expect(pane).toBeHidden(); + expect(new URL(page.url()).pathname).toBe(newPath); + // Still a LIVE collection page — the renamed collection renders its own + // heading, not a "Collection not found" error. (The item list re-hydrates + // the renamed collection asynchronously via the local index, so we assert + // page liveness via the heading rather than a specific row.) + await expect(page.getByRole('heading', { level: 1, name: new RegExp(newName) })).toBeVisible(); + }); +}); diff --git a/web/src/app.d.ts b/web/src/app.d.ts index da08e6da..768ed731 100644 --- a/web/src/app.d.ts +++ b/web/src/app.d.ts @@ -5,7 +5,15 @@ declare global { // interface Error {} // interface Locals {} // interface PageData {} - // interface PageState {} + // Split-pane mini-browser depth/ownership stamp (PLAN-2154 / + // TASK-2157). Carried in SvelteKit `page.state` (NOT raw + // `history.state`) so it follows opaque Back/Forward, survives + // `history.go`, and reconstructs on cold-load. See + // `$lib/collections/paneController.ts`. + interface PageState { + paneDepth?: number; + paneOwned?: boolean; + } // interface Platform {} } } diff --git a/web/src/lib/collections/paneController.test.ts b/web/src/lib/collections/paneController.test.ts new file mode 100644 index 00000000..0114040c --- /dev/null +++ b/web/src/lib/collections/paneController.test.ts @@ -0,0 +1,244 @@ +import { describe, it, expect } from 'vitest'; +import { + readPaneState, + planPaneDrill, + planLateralOpen, + planPaneClose, + PANE_DEPTH_SOFT_CAP, + type ResolvedPaneState, +} from './paneController'; + +const base: ResolvedPaneState = { paneDepth: 0, paneOwned: false }; +const ownedBase: ResolvedPaneState = { paneDepth: 0, paneOwned: true }; + +describe('readPaneState', () => { + it('defaults an unstamped (cold-loaded) entry to depth 0, UNOWNED', () => { + expect(readPaneState(undefined)).toEqual({ paneDepth: 0, paneOwned: false }); + expect(readPaneState(null)).toEqual({ paneDepth: 0, paneOwned: false }); + expect(readPaneState({})).toEqual({ paneDepth: 0, paneOwned: false }); + }); + + it('reads a full stamp verbatim', () => { + expect(readPaneState({ paneDepth: 3, paneOwned: true })).toEqual({ + paneDepth: 3, + paneOwned: true, + }); + }); + + it('coerces owned to a strict boolean (only literal true owns)', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect(readPaneState({ paneOwned: 1 as any }).paneOwned).toBe(false); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect(readPaneState({ paneOwned: 'yes' as any }).paneOwned).toBe(false); + expect(readPaneState({ paneOwned: true }).paneOwned).toBe(true); + }); + + it('floors negative / fractional / non-numeric depths to 0', () => { + expect(readPaneState({ paneDepth: -4 }).paneDepth).toBe(0); + expect(readPaneState({ paneDepth: 2.9 }).paneDepth).toBe(2); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect(readPaneState({ paneDepth: 'x' as any }).paneDepth).toBe(0); + expect(readPaneState({ paneDepth: NaN }).paneDepth).toBe(0); + expect(readPaneState({ paneDepth: Infinity }).paneDepth).toBe(0); + }); +}); + +describe('planPaneDrill — same-ref guard (D4)', () => { + it('is a no-op when the target equals the currently-shown item', () => { + expect(planPaneDrill('TASK-5', 'TASK-5', ownedBase)).toEqual({ kind: 'noop' }); + expect(planPaneDrill('TASK-5', 'TASK-5', { paneDepth: 4, paneOwned: true })).toEqual({ + kind: 'noop', + }); + }); + + it('is a no-op for a falsy / empty target', () => { + expect(planPaneDrill('TASK-5', null, ownedBase)).toEqual({ kind: 'noop' }); + expect(planPaneDrill('TASK-5', undefined, ownedBase)).toEqual({ kind: 'noop' }); + expect(planPaneDrill('TASK-5', '', ownedBase)).toEqual({ kind: 'noop' }); + }); + + it('drills when the target differs, even if currentRef is null (cold base)', () => { + expect(planPaneDrill(null, 'TASK-9', base)).toEqual({ + kind: 'push', + state: { paneDepth: 1, paneOwned: false }, + }); + }); +}); + +describe('planPaneDrill — ownership INHERITANCE', () => { + it('a drill from an OWNED base inherits owned=true', () => { + expect(planPaneDrill('TASK-1', 'TASK-2', ownedBase)).toEqual({ + kind: 'push', + state: { paneDepth: 1, paneOwned: true }, + }); + }); + + it('a drill from a COLD (unowned) base inherits owned=false — keeps the cold-close branch reachable', () => { + expect(planPaneDrill('TASK-1', 'TASK-2', base)).toEqual({ + kind: 'push', + state: { paneDepth: 1, paneOwned: false }, + }); + }); + + it('inheritance carries through multiple hops', () => { + expect(planPaneDrill('TASK-2', 'TASK-3', { paneDepth: 1, paneOwned: false })).toEqual({ + kind: 'push', + state: { paneDepth: 2, paneOwned: false }, + }); + expect(planPaneDrill('TASK-3', 'TASK-4', { paneDepth: 2, paneOwned: true })).toEqual({ + kind: 'push', + state: { paneDepth: 3, paneOwned: true }, + }); + }); +}); + +describe('planPaneDrill — soft depth cap (D4)', () => { + it('pushes below the cap', () => { + const belowCap = { paneDepth: PANE_DEPTH_SOFT_CAP - 1, paneOwned: true }; + expect(planPaneDrill('A', 'B', belowCap)).toEqual({ + kind: 'push', + state: { paneDepth: PANE_DEPTH_SOFT_CAP, paneOwned: true }, + }); + }); + + it('REPLACES at the cap — holds depth + ownership steady', () => { + const atCap = { paneDepth: PANE_DEPTH_SOFT_CAP, paneOwned: true }; + expect(planPaneDrill('A', 'B', atCap)).toEqual({ + kind: 'replace', + state: { paneDepth: PANE_DEPTH_SOFT_CAP, paneOwned: true }, + }); + }); + + it('REPLACES past the cap too (no unbounded growth)', () => { + const pastCap = { paneDepth: PANE_DEPTH_SOFT_CAP + 5, paneOwned: false }; + expect(planPaneDrill('A', 'B', pastCap)).toEqual({ + kind: 'replace', + state: { paneDepth: PANE_DEPTH_SOFT_CAP + 5, paneOwned: false }, + }); + }); + + it('honours a custom cap', () => { + expect(planPaneDrill('A', 'B', { paneDepth: 2, paneOwned: true }, 2)).toEqual({ + kind: 'replace', + state: { paneDepth: 2, paneOwned: true }, + }); + expect(planPaneDrill('A', 'B', { paneDepth: 1, paneOwned: true }, 2)).toEqual({ + kind: 'push', + state: { paneDepth: 2, paneOwned: true }, + }); + }); +}); + +describe('planLateralOpen', () => { + it('first-open (pane closed) PUSHES and MINTS ownership', () => { + expect(planLateralOpen(false, base)).toEqual({ + kind: 'push', + state: { paneDepth: 0, paneOwned: true }, + }); + }); + + it('re-target at depth 0 from an OWNED base REPLACES, keeping owned=true', () => { + expect(planLateralOpen(true, ownedBase)).toEqual({ + kind: 'replace', + state: { paneDepth: 0, paneOwned: true }, + }); + }); + + it('re-target at depth 0 from a COLD base REPLACES, keeping owned=false', () => { + expect(planLateralOpen(true, base)).toEqual({ + kind: 'replace', + state: { paneDepth: 0, paneOwned: false }, + }); + }); + + it('a direct row click at depth>0 (detached) RESETS the stack, preserving base ownership', () => { + expect(planLateralOpen(true, { paneDepth: 3, paneOwned: true })).toEqual({ + kind: 'reset', + goDelta: -3, + resetState: { paneDepth: 0, paneOwned: true }, + }); + expect(planLateralOpen(true, { paneDepth: 2, paneOwned: false })).toEqual({ + kind: 'reset', + goDelta: -2, + resetState: { paneDepth: 0, paneOwned: false }, + }); + }); +}); + +describe('planPaneClose — three-way staged unwind (R8)', () => { + it('OWNED depth 0 (click-opened, no drill) → go(-1) back to the pre-pane URL', () => { + expect(planPaneClose(ownedBase)).toEqual({ kind: 'owned-go', goDelta: -1 }); + }); + + it('OWNED depth N → go(-(N+1)) unwinds base + every drill', () => { + expect(planPaneClose({ paneDepth: 3, paneOwned: true })).toEqual({ + kind: 'owned-go', + goDelta: -4, + }); + }); + + it('UNOWNED depth 0 (cold load) → replaceState-delete in place (never go off the base)', () => { + expect(planPaneClose(base)).toEqual({ kind: 'replace-delete' }); + }); + + it('UNOWNED depth>0 (cold base then drilled) → go(-depth) to the cold base, then latched delete', () => { + expect(planPaneClose({ paneDepth: 2, paneOwned: false })).toEqual({ + kind: 'cold-base-go', + goDelta: -2, + }); + expect(planPaneClose({ paneDepth: 1, paneOwned: false })).toEqual({ + kind: 'cold-base-go', + goDelta: -1, + }); + }); +}); + +describe('integration — full open/drill/close round-trips', () => { + it('click-open → close returns to the pre-pane URL (owned go(-1))', () => { + const open = planLateralOpen(false, base); // push {0,true} + expect(open).toMatchObject({ kind: 'push', state: { paneDepth: 0, paneOwned: true } }); + const close = planPaneClose({ paneDepth: 0, paneOwned: true }); + expect(close).toEqual({ kind: 'owned-go', goDelta: -1 }); + }); + + it('click-open → drill A→B→C → close unwinds all four entries', () => { + // first-open A: {0,true} + let s = readPaneState({ paneDepth: 0, paneOwned: true }); + // drill A→B + const b = planPaneDrill('A', 'B', s); + expect(b).toMatchObject({ kind: 'push', state: { paneDepth: 1, paneOwned: true } }); + s = readPaneState((b as { state: ResolvedPaneState }).state); + // drill B→C + const c = planPaneDrill('B', 'C', s); + expect(c).toMatchObject({ kind: 'push', state: { paneDepth: 2, paneOwned: true } }); + s = readPaneState((c as { state: ResolvedPaneState }).state); + // close from depth 2 owned → go(-3) + expect(planPaneClose(s)).toEqual({ kind: 'owned-go', goDelta: -3 }); + }); + + it('cold-load A → drill A→B → close goes to the cold base then deletes (NOT go(-2) off the base)', () => { + // cold load: unstamped → {0,false} + let s = readPaneState(undefined); + expect(s).toEqual({ paneDepth: 0, paneOwned: false }); + const b = planPaneDrill('A', 'B', s); + expect(b).toMatchObject({ kind: 'push', state: { paneDepth: 1, paneOwned: false } }); + s = readPaneState((b as { state: ResolvedPaneState }).state); + // close from depth 1 UNOWNED → go(-1) to cold base, then latched delete + expect(planPaneClose(s)).toEqual({ kind: 'cold-base-go', goDelta: -1 }); + }); + + it('detached row click resets, and the reset base then closes correctly', () => { + // owned, drilled to depth 2 + const reset = planLateralOpen(true, { paneDepth: 2, paneOwned: true }); + expect(reset).toEqual({ + kind: 'reset', + goDelta: -2, + resetState: { paneDepth: 0, paneOwned: true }, + }); + // after reset the base is {0,true}; closing it is a clean go(-1) + expect(planPaneClose((reset as { resetState: ResolvedPaneState }).resetState)).toEqual({ + kind: 'owned-go', + goDelta: -1, + }); + }); +}); diff --git a/web/src/lib/collections/paneController.ts b/web/src/lib/collections/paneController.ts new file mode 100644 index 00000000..a922a785 --- /dev/null +++ b/web/src/lib/collections/paneController.ts @@ -0,0 +1,185 @@ +// Pane-navigation controller — the depth/ownership state machine that turns +// the collection page's split-pane detail view (PLAN-2105) into a navigable +// mini-browser with a back stack (PLAN-2154 / IDEA-2153, Architecture A). +// +// This module holds the PURE decision logic, framework-agnostic and free of +// `$state`/`page`/`goto` so it's exhaustively unit-testable without mounting +// the 4200-line route. `+page.svelte` reads the current `{paneDepth, +// paneOwned}` off SvelteKit's `page.state` (NEVER raw `history.state` — Kit +// nests app state under `sveltekit:states`), passes it here, and EXECUTES the +// returned plan via `goto(..., { state })` / `history.go`. Keeping the +// arithmetic pure is what makes the three-way close and the ownership model +// reviewable in isolation — the exact class of bug (opaque Back/Forward, +// cold-load close off the base, late-async clobbers) this controller lives in. +// +// The vocabulary: +// • depth — how many in-pane drill hops deep we are. 0 = first-open or a +// cold-loaded shared `?item=` URL (the "base"). +// • ownership — whether THIS session pushed the pane's base entry, so there +// is a pre-pane history entry to unwind to on close. Created ONLY by a +// first-open (`openItemPane` on a closed pane); INHERITED by every drill +// (`navigatePaneTo`). A cold-loaded `?item=` has no stamp → unowned, which +// is what keeps the UNOWNED-close branches reachable (a cold A→B drill +// must NOT `go(-2)` off the base). + +/** + * Depth + ownership stamp carried in SvelteKit `page.state` for the split-pane + * mini-browser. Both fields are optional at the type level because a + * cold-loaded / never-stamped history entry has no `page.state` of its own; + * {@link readPaneState} normalizes those absences to the base defaults. + */ +export interface PaneHistoryState { + /** In-pane drill depth. 0 = first-open / cold-load base. */ + paneDepth?: number; + /** + * True when this session minted the pane's base entry (first-open), so a + * pre-pane history entry exists to unwind to. False for a cold-loaded + * shared `?item=` URL. Drills copy (inherit) this from the current entry. + */ + paneOwned?: boolean; +} + +/** The normalized, always-present form after {@link readPaneState}. */ +export interface ResolvedPaneState { + paneDepth: number; + paneOwned: boolean; +} + +/** + * Soft cap on drill depth (D4). Past it a drill REPLACES instead of pushing, + * so a pathological very-deep distinct chain can't grow the browser history + * unbounded. Chosen in the plan's ~15–20 band; the exact value isn't + * load-bearing — it only bounds an extreme. + */ +export const PANE_DEPTH_SOFT_CAP = 20; + +/** + * Normalize an opaque `page.state`-shaped value into a definite + * `{paneDepth, paneOwned}`. A cold-loaded or never-stamped entry (`undefined`, + * `null`, `{}`) resolves to the base: depth 0, UNOWNED — the invariant that + * keeps a cold-load A→B drill from wrongly unwinding off the base on close. + * Negative / non-integer / non-number depths are floored to a safe 0. + */ +export function readPaneState(state: PaneHistoryState | null | undefined): ResolvedPaneState { + const s = state ?? {}; + const rawDepth = s.paneDepth; + const paneDepth = + typeof rawDepth === 'number' && Number.isFinite(rawDepth) && rawDepth > 0 + ? Math.floor(rawDepth) + : 0; + return { paneDepth, paneOwned: s.paneOwned === true }; +} + +/** + * A single-entry history write: `push` mints a new entry, `replace` rewrites + * the current one. Both carry the `page.state` stamp to emit. + */ +export type PaneWritePlan = { kind: 'push' | 'replace'; state: ResolvedPaneState }; + +/** + * Detached lateral open (a direct list-row click at depth>0): a NEW top-level + * open, not a replace of the drilled entry. Unwind the drill stack back to the + * base with `history.go(goDelta)`, THEN (from a one-shot `afterNavigate` latch + * — `history.go` has no completion promise) re-target the base to the new item, + * PRESERVING the base's ownership (`resetState`). + */ +export type PaneResetPlan = { kind: 'reset'; goDelta: number; resetState: ResolvedPaneState }; + +/** What a DRILL (`navigatePaneTo`) decides: a same-ref/cycle no-op, or a write. */ +export type PaneDrillPlan = { kind: 'noop' } | PaneWritePlan; + +/** What a LATERAL open (`openItemPane`) decides: a write, or a detached reset. */ +export type PaneLateralPlan = PaneWritePlan | PaneResetPlan; + +/** The staged close plan (R8). Three ownership-aware branches. */ +export type PaneClosePlan = + /** UNOWNED & depth 0 (cold-loaded base, no pre-pane entry): drop `?item=` + * in place with `replaceState`. Never `go` off the base. */ + | { kind: 'replace-delete' } + /** OWNED: unwind the pushed base entry + every drill back to the pre-pane + * URL in one `history.go(goDelta)`. No follow-up write needed — the + * destination has no `?item=`, so the pane closes naturally. */ + | { kind: 'owned-go'; goDelta: number } + /** UNOWNED & depth>0 (cold base, then drilled): `history.go(goDelta)` back + * to the cold base, THEN a latched `replaceState`-delete of `?item=` (the + * base still carries it). Two-phase because `history.go` can't be awaited. */ + | { kind: 'cold-base-go'; goDelta: number }; + +/** + * Plan a DRILL (`navigatePaneTo`) — re-target the pane in place, deeper into + * the stack. Ownership is INHERITED from the current entry (drills copy it), + * so a cold-loaded base stays unowned all the way down. + * + * - same-ref guard (D4): a drill to the item already shown is a no-op — kills + * the common `A→B→A` oscillation with zero stack; + * - soft depth cap (D4): at/above the cap, REPLACE (bound history growth); + * - otherwise PUSH at depth+1, carrying the inherited ownership. + * + * `targetRef` is the already-resolved canonical `?item=` value; a falsy target + * is a no-op (nothing to open). + */ +export function planPaneDrill( + currentRef: string | null, + targetRef: string | null | undefined, + current: ResolvedPaneState, + softCap: number = PANE_DEPTH_SOFT_CAP, +): PaneDrillPlan { + if (!targetRef) return { kind: 'noop' }; + // D4 same-ref guard — skip the push when re-targeting the current item. + if (currentRef !== null && targetRef === currentRef) return { kind: 'noop' }; + if (current.paneDepth >= softCap) { + // At the cap: replace, holding depth + inherited ownership steady. + return { kind: 'replace', state: { paneDepth: current.paneDepth, paneOwned: current.paneOwned } }; + } + // Drill: push one level deeper, INHERITING ownership from the current entry. + return { kind: 'push', state: { paneDepth: current.paneDepth + 1, paneOwned: current.paneOwned } }; +} + +/** + * Plan a LATERAL open (`openItemPane` — a list/board/table row click, Enter, + * or a j/k pane-follow settle). Three cases: + * + * - pane CLOSED → first-open: PUSH `{depth:0, owned:true}`. Only a first-open + * mints ownership (there's now a pre-pane entry to unwind to); + * - pane open at depth 0 → re-target: REPLACE, preserving the current + * ownership (an owned first-open stays owned; a cold-loaded base stays + * unowned) — the PLAN-2105 "hold j/k settles with one history entry" fix; + * - pane open at depth>0 (detached) → RESET: a direct row click is a NEW + * top-level open, not a replace of the drilled entry — collapse the stack + * (`go(-depth)`) then re-target the base, preserving the base's ownership. + * (`j`/`k` never reaches here: it's gated inert at depth>0.) + */ +export function planLateralOpen(paneOpen: boolean, current: ResolvedPaneState): PaneLateralPlan { + if (!paneOpen) { + // First-open mints ownership. + return { kind: 'push', state: { paneDepth: 0, paneOwned: true } }; + } + if (current.paneDepth === 0) { + // Re-target at the base: replace, preserving ownership. + return { kind: 'replace', state: { paneDepth: 0, paneOwned: current.paneOwned } }; + } + // Detached: collapse the stack, then re-open the base (ownership preserved — + // drills inherited it from the base, so current.paneOwned === base ownership). + return { + kind: 'reset', + goDelta: -current.paneDepth, + resetState: { paneDepth: 0, paneOwned: current.paneOwned }, + }; +} + +/** + * Plan the three-way, ownership-aware, staged CLOSE (R8). See + * {@link PaneClosePlan} for the branch semantics. + */ +export function planPaneClose(current: ResolvedPaneState): PaneClosePlan { + if (current.paneOwned) { + // Unwind the pushed base + every drill back to the pre-pane URL. + return { kind: 'owned-go', goDelta: -(current.paneDepth + 1) }; + } + if (current.paneDepth > 0) { + // Cold base, then drilled: go back to the cold base, latch a delete. + return { kind: 'cold-base-go', goDelta: -current.paneDepth }; + } + // Cold-loaded base with no drills: drop `?item=` in place. + return { kind: 'replace-delete' }; +} diff --git a/web/src/lib/components/items/ItemDetail.svelte b/web/src/lib/components/items/ItemDetail.svelte index d23e141b..87d0aa55 100644 --- a/web/src/lib/components/items/ItemDetail.svelte +++ b/web/src/lib/components/items/ItemDetail.svelte @@ -343,7 +343,12 @@ const url = new URL(page.url); if (open) url.searchParams.set('graph', '1'); else url.searchParams.delete('graph'); - goto(url, { replaceState: true, noScroll: true, keepFocus: true }); + // This same-page toggle can PRESERVE an open pane's `?item=` (on the + // full-page host, Q1/Phase 2), so re-emit the current `page.state` to + // keep the pane depth+ownership stamp intact — a bare replaceState would + // blank it and desync the close arithmetic (PLAN-2154 R13). A no-op when + // there's no pane state to carry. + goto(url, { replaceState: true, noScroll: true, keepFocus: true, state: page.state }); } function openGraph() { setGraphParam(true); diff --git a/web/src/routes/[username]/[workspace]/[collection]/+page.svelte b/web/src/routes/[username]/[workspace]/[collection]/+page.svelte index 6730a68a..89bb7285 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/+page.svelte +++ b/web/src/routes/[username]/[workspace]/[collection]/+page.svelte @@ -1,7 +1,7 @@