From 5d01c4f6a277fac66e998e0a41205fd64c75e9bc Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 26 Jul 2026 22:49:03 +0000 Subject: [PATCH] fix(web): lane inline-create opens the split pane on desktop, stays put on mobile (IDEA-2298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The board lane `+` opens a Trello-style draft card (TASK-1676) whose Enter handler hardcoded a full-page `goto(.../{item}?new=1)`. TASK-1676 predates the split pane (PLAN-2105), so nothing revisited that destination, leaving the one create gesture that already knows its title as the only card-open path that bypasses the pane: - Clicking an EXISTING card opens the split pane (`onItemOpen` → `?item=`); creating one navigated the whole page away from the board. - `?new=1` exists to drop you into the title editor of a fresh "Untitled" item (`createNewItem`). On this path the title was just typed, so it re-opened the title editor with that title select-alled. - There was no viewport branching at all, so mobile — where the lane `+` is fully present — got ejected off the board too, making a second add a Back navigation. `quickCreateInColumn`'s third param becomes `reveal` (caller INTENT) rather than `navigate` (a destination): the local-index upsert is now unconditional so the card always lands in its lane, and revealing means `openItemPane(item)` on desktop and nothing on mobile. The page owns what revealing means, so BoardView stays unaware the pane exists. The composer closes on submit on every viewport — on desktop the pane takes focus, so keeping it open for rapid entry would fight it. Revisit if feedback asks for mobile rapid-add. The nav-guard's Save-all keeps passing `reveal: false`; saving drafts on the way out must never open anything. New e2e pins both destinations and, on both viewports, that the pathname never changes and `?new=1` is never set. Two traps worth recording: the created card renders off the SYNCHRONOUS local-index upsert, so it is not a sync point for the navigation that follows — the first draft of both tests passed against the reverted fix because the URL assertions raced an unresolved `goto`. Desktop now waits on `?item=`; mobile can't poll an absence, so it proves the negative positively by re-opening the lane composer (only possible if the board is still mounted, and awaiting it gives a would-be navigation time to land). Verified by mutating the fix back out: both fail for the right reasons, pass on the fix. Gates: npm run check 0 errors, npm run test 490 passed, new spec 2/2, pane e2e 64/65 (the one failure is the pre-existing BUG-2334 SSE-toast flake, confirmed by screenshot and passing in isolation). Codex review CLEAN (CONVE-735). Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT --- web/e2e/lane-inline-create.spec.ts | 164 ++++++++++++++++++ .../components/collections/BoardView.svelte | 26 ++- .../[workspace]/[collection]/+page.svelte | 33 +++- 3 files changed, 207 insertions(+), 16 deletions(-) create mode 100644 web/e2e/lane-inline-create.spec.ts diff --git a/web/e2e/lane-inline-create.spec.ts b/web/e2e/lane-inline-create.spec.ts new file mode 100644 index 00000000..b712f2da --- /dev/null +++ b/web/e2e/lane-inline-create.spec.ts @@ -0,0 +1,164 @@ +import { test, expect } from './fixtures'; +import { browserLogin, seedDoc } from './lib/collab-helpers'; +import type { APIRequestContext, Page } from '@playwright/test'; +import type { SuiteFixture } from './fixtures'; + +/** + * Board lane inline-create destination e2e (IDEA-2298). + * + * The lane `+` opens a Trello-style draft card (TASK-1676); Enter creates the + * item. TASK-1676 predates the split pane (PLAN-2105), so it hardcoded a + * FULL-PAGE `goto(.../{item}?new=1)` — which meant the one gesture that already + * knows the title ejected you off the board and re-opened the title editor on + * it, while clicking any EXISTING card opened the same item in the pane. This + * spec pins the corrected destinations: + * + * - desktop → the split pane (`?item=`), same entry point a card click uses + * - mobile → nothing opens; the card just appears in its lane + * - both → the pathname NEVER changes, and `?new=1` is never set + * + * The pathname assertion is the load-bearing one on both viewports: a + * regression back to a full-page navigate changes the pathname, and no amount + * of pane/lane styling can hide that. + * + * Viewport is driven explicitly with `setViewportSize` (the widths straddle the + * inclusive 768px breakpoint), so both cases are project-independent and we run + * them on one project rather than doubling the work — matching + * `pane-mobile-overlay.spec.ts`. + */ + +const SPLIT = { width: 1280, height: 900 }; // over the breakpoint → split pane +const OVERLAY = { width: 768, height: 1024 }; // == breakpoint (max-width is inclusive) → mobile + +/** The docs board groups by `status`; 'draft' is its first lane. */ +const LANE = 'Draft'; + +function boardUrl(fixture: { adminUsername: string; workspaceSlug: string }): string { + return `/${fixture.adminUsername}/${fixture.workspaceSlug}/docs?view=board`; +} + +function collectionPathname(fixture: { adminUsername: string; workspaceSlug: string }): string { + return `/${fixture.adminUsername}/${fixture.workspaceSlug}/docs`; +} + +/** + * Open the docs board with at least one item in the target lane. + * + * An EMPTY collection renders a "No docs yet" empty state instead of the + * lanes, so there'd be no lane `+` to click at all — seed one `status=draft` + * doc first so the Draft lane exists. Waits for that card so the board (fed by + * the local index) is known-rendered before the draft flow starts. + */ +async function openBoardWithLane( + page: Page, + fixture: SuiteFixture, + request: APIRequestContext, +): Promise { + await seedDoc(fixture, request, 'Lane anchor', { status: 'draft' }); + await page.goto(boardUrl(fixture)); + await expect(page.locator('.item-card', { hasText: 'Lane anchor' }).first()).toBeVisible(); +} + +/** + * Drive the lane `+` → type → Enter flow, returning the title used. Waits for + * the created card to render in the lane, which is the shared postcondition on + * both viewports (the local-index upsert) and the sync point for the + * destination assertions that follow. + */ +async function createViaLaneDraft(page: Page, lane: string): Promise { + const title = `Lane draft ${Date.now()}`; + await page.getByRole('button', { name: `Add item to ${lane}` }).click(); + + const input = page.locator('.lane-draft-input'); + await expect(input).toBeFocused(); + await input.fill(title); + await input.press('Enter'); + + // The item is upserted into the local index on BOTH viewports, so the card + // appears in the lane either way. + await expect(page.locator('.item-card', { hasText: title })).toBeVisible(); + return title; +} + +test.describe('board lane inline-create destination (IDEA-2298)', () => { + test('desktop: Enter opens the new item in the split pane without leaving the board', async ({ + page, + fixture, + request, + }, testInfo) => { + test.skip( + testInfo.project.name !== 'desktop-chromium', + 'viewport is driven explicitly; one project is enough', + ); + await page.setViewportSize(SPLIT); + await browserLogin(page); + await openBoardWithLane(page, fixture, request); + + const title = await createViaLaneDraft(page, LANE); + + // Wait for the pane open to actually land before reading the URL. The + // created card renders off the SYNCHRONOUS local-index upsert, so it is + // NOT a sync point for the navigation that follows it — asserting + // straight after it would race, and a pathname assertion in particular + // would pass vacuously against a full-page `goto` that simply hadn't + // resolved yet (caught by mutating the fix back out). + await page.waitForURL((u) => new URL(u).searchParams.has('item')); + + // Opened in the pane — NOT a full-page navigate. `?item=` is the pane's + // single source of truth, and the pathname is still the collection route. + const url = new URL(page.url()); + expect(url.pathname).toBe(collectionPathname(fixture)); + expect(url.searchParams.get('item')).toBeTruthy(); + // `?new=1` drives ItemDetail's auto-edit-title flow. The lane draft + // already captured the title, so re-opening that editor is the exact + // inconsistency IDEA-2298 filed — it must never be set on this path. + expect(url.searchParams.get('new')).toBeNull(); + + const pane = page.locator('.item-pane'); + await expect(pane).toBeVisible(); + await expect(pane).toContainText(title); + + // The board stayed mounted behind the pane (it's a split, not a + // navigation), and the composer closed on submit — uniform on every + // viewport, since here the pane takes focus anyway. + await expect(page.locator('.list-column')).toBeVisible(); + await expect(page.locator('.lane-draft-input')).toHaveCount(0); + }); + + test('mobile: Enter creates the card in the lane and opens nothing', async ({ + page, + fixture, + request, + }, testInfo) => { + test.skip( + testInfo.project.name !== 'desktop-chromium', + 'viewport is driven explicitly; one project is enough', + ); + await page.setViewportSize(OVERLAY); + await browserLogin(page); + await openBoardWithLane(page, fixture, request); + + await createViaLaneDraft(page, LANE); + + // Composer closed on submit here too (uniform close, IDEA-2298). + await expect(page.locator('.lane-draft-input')).toHaveCount(0); + + // "Nothing opened" is an ABSENCE, and absences can't be polled — a bare + // URL assertion here would pass against a full-page `goto` that just + // hadn't resolved yet. So prove it POSITIVELY instead: re-open the lane + // composer. That click can only succeed if the board is still mounted and + // interactive, and awaiting it gives any would-be navigation a real chance + // to land first. It also encodes the actual UX win — on mobile you can add + // a second card immediately instead of navigating back. + await page.getByRole('button', { name: `Add item to ${LANE}` }).click(); + await expect(page.locator('.lane-draft-input')).toBeVisible(); + await page.locator('.lane-draft-input').press('Escape'); + + // Still on the board, with nothing opened. + const url = new URL(page.url()); + expect(url.pathname).toBe(collectionPathname(fixture)); + expect(url.searchParams.get('item')).toBeNull(); + expect(url.searchParams.get('new')).toBeNull(); + await expect(page.locator('.item-pane')).toHaveCount(0); + }); +}); diff --git a/web/src/lib/components/collections/BoardView.svelte b/web/src/lib/components/collections/BoardView.svelte index 519cc4ea..b3718521 100644 --- a/web/src/lib/components/collections/BoardView.svelte +++ b/web/src/lib/components/collections/BoardView.svelte @@ -26,12 +26,17 @@ oncreate?: () => void; /** * Create an item in this lane from the inline draft card - * (TASK-1676), pre-filling the lane's group value. `navigate` true - * (Enter) opens the new item; false (nav-guard Save) just lands it - * in the lane. Throws on failure so the draft can be restored. - * Gated behind `canEdit`. When wired, the `+`/menu open a draft. + * (TASK-1676), pre-filling the lane's group value. Throws on failure + * so the draft can be restored. Gated behind `canEdit`. When wired, + * the `+`/menu open a draft. + * + * `reveal` reports INTENT, not a destination: true means "the user + * submitted this draft, show them the item", false means "create it + * quietly" (the page's nav-guard Save-all). What revealing does — and + * whether the viewport gets one at all — is the page's call, so this + * component stays unaware of the split pane (IDEA-2298). */ - onCreateInColumn?: (groupValue: string, title: string, navigate: boolean) => Promise | void; + onCreateInColumn?: (groupValue: string, title: string, reveal: boolean) => Promise | void; /** * Inline draft state (TASK-1676) — owned by the page (which holds * the leave guard + dialog) so a draft survives a board↔list view @@ -149,9 +154,14 @@ const title = (draftText[col] ?? '').trim(); if (!title || savingDraft || !onCreateInColumn) return; savingDraft = true; - // Clear optimistically BEFORE the create navigates, so the page's - // leave guard (fires when onCreateInColumn opens the new item) - // doesn't re-flag this lane. + // The composer closes on submit on EVERY viewport (IDEA-2298) — on + // desktop the pane takes focus anyway, so leaving it open for + // Trello-style rapid entry would just fight the pane for the cursor. + // + // Clear the text optimistically, BEFORE the await, so the page's + // unsaved-draft leave guard can't see this lane as dirty while the + // create is in flight. The desktop reveal is a same-pathname `?item=` + // push, which that guard skips regardless — belt and suspenders. const prior = draftText[col]; delete draftText[col]; draftOpen[col] = false; diff --git a/web/src/routes/[username]/[workspace]/[collection]/+page.svelte b/web/src/routes/[username]/[workspace]/[collection]/+page.svelte index cfc69c8d..847e570c 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/+page.svelte +++ b/web/src/routes/[username]/[workspace]/[collection]/+page.svelte @@ -1800,14 +1800,29 @@ // Create an item in a board lane from the inline draft card // (TASK-1676, refines TASK-1671 / IDEA-1159). Pre-fills the lane's - // group field so the item lands in that lane. `navigate` true (Enter - // in the draft) opens the new item; false (nav-guard "Save") just - // upserts it into the local index and stays put. Throws on failure so + // group field so the item lands in that lane. Throws on failure so // BoardView can restore the draft. Returns the created item. + // + // `reveal` is the caller's INTENT — "the user just submitted this draft, + // show them the item" (Enter / "Add card") vs. "create it quietly" (the + // nav-guard's Save-all, which must never open anything on the way out). + // This page owns what revealing MEANS, so BoardView doesn't have to know + // the pane exists (IDEA-2298): + // - desktop → open the split pane, exactly like clicking an existing card + // - mobile → nothing; the card appears in the lane and the user taps it + // if they want it + // Either way the item is upserted into the local index so it renders in + // its lane immediately, and either way the draft composer closes + // (BoardView.submitDraft) — on desktop the pane takes focus. + // + // Deliberately NOT the `?new=1` full-page goto this used to do. That flow + // exists to drop you into the title editor of a fresh "Untitled" item + // (`createNewItem`), and re-opening the title editor on a title the user + // just finished typing is the inconsistency IDEA-2298 filed. async function quickCreateInColumn( groupValue: string, title: string, - navigate: boolean + reveal: boolean ): Promise { if (!wsSlug || !collSlug) return null; const trimmed = title.trim(); @@ -1831,10 +1846,12 @@ fields: JSON.stringify(defaultFields), source: 'web' }); - if (navigate) { - goto(`/${username}/${wsSlug}/${collSlug}/${itemUrlId(item)}?new=1`); - } else { - localIndex.upsert(wsSlug, item, epoch); + localIndex.upsert(wsSlug, item, epoch); + // Desktop reveal only — `openItemPane` is the same entry point a + // plain left-click on an existing card uses, so a created card and + // a clicked card land in the same place. + if (reveal && !viewport.isMobile) { + openItemPane(item); } return item; } catch (err: any) {