From c778cba86587372b495c604d54cf01e229af288d Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Mon, 27 Jul 2026 12:17:12 +0000 Subject: [PATCH 1/8] fix(#429): handle rename outcomes, isolate button keys, unify create (#495 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge review of PR #495 raised four defects; all four are fixed here, and three of them are foundations #494 (phase 4) builds three more row buttons and two more dialogs on. 1. Enter on a nested action button ran the ROW's command. The tree's keydown handler is on the list and its Enter arm calls preventDefault() + runs the focused row's single command, so Enter on the pencil opened the Dashboard — and the preventDefault() could swallow the button's own activation on the way out. The `⋯` and the orphan-variable trash shared the bug. Fixed in two independent layers: `isolateActivationKeys` stops Enter/Space propagating from each control (without preventing the default, so native activation still fires exactly once), and `handleTreeKeydown` ignores an Enter that originated on a button. Arrow/Home/End still reach the tree from a nested control, which is what keeps the row's composite tab stop navigable. 2. Rename failures closed the dialog and discarded the outcome. `commit()` now awaits `commitDashboardRename` and keeps the card open with the typed values on every unsuccessful outcome, showing one targeted diagnostic inline (`role="alert"`): a distinct sentence for a Dashboard that no longer resolves vs. the aggregate's own rejection diagnostic. Both actions are disabled while a write is in flight, so the same mutation cannot be submitted twice, and a late answer for a force-closed dialog is dropped rather than written into a detached card. 3. Dashboard creation had two commands with divergent failure behaviour. `application/dashboard-create.ts` is now the single one: it mints, appends against dequeue-time truth (falling back to a caller-supplied baseline for a workspace with no persisted aggregate), and `dashboardCreateMessage` normalizes the report. Both entry points call it; each keeps its own reveal policy, which is genuinely different. The placeholder previously said NOTHING on a rejected commit. 4. The modal had no dialog semantics. `openDialogShell` gives the card `role="dialog"`, `aria-modal="true"` and `aria-labelledby` pointing at a per-dialog title id. The two-field metadata dialog moves from `ui/dashboard-tree.ts` to `ui/dialog-shell.ts` as `openMetadataDialog` — #494's panel pencil is the second consumer hard rule 5 asks for before extracting. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU --- src/application/dashboard-create.ts | 93 ++++++++++++++++ src/styles.css | 9 ++ src/ui/dashboard-tree.ts | 131 ++++++++++++++-------- src/ui/dashboard.ts | 34 +++--- src/ui/dialog-shell.ts | 136 ++++++++++++++++++++++- src/ui/file-menu.ts | 46 ++++---- tests/unit/dashboard-create.test.ts | 119 ++++++++++++++++++++ tests/unit/dashboard-tree.test.ts | 146 ++++++++++++++++++++++++- tests/unit/dashboard.test.ts | 40 +++++++ tests/unit/dialog-shell.test.ts | 162 +++++++++++++++++++++++++++- 10 files changed, 830 insertions(+), 86 deletions(-) create mode 100644 src/application/dashboard-create.ts create mode 100644 tests/unit/dashboard-create.test.ts diff --git a/src/application/dashboard-create.ts b/src/application/dashboard-create.ts new file mode 100644 index 00000000..6689b3cf --- /dev/null +++ b/src/application/dashboard-create.ts @@ -0,0 +1,93 @@ +// Creating ONE Dashboard document — the single command behind every entry +// point that mints one (#481, #495 review 3). +// +// #481 asked for one `createDashboard` action, and #429 phase 3 delivered half +// of it: the File menu and the empty-workspace placeholder came to share a +// dialog and the pure `appendDashboard` transform, but kept two separate +// commands around them. They disagreed about the thing that matters least +// often and hurts most when it happens — the File menu toasted a rejected +// persistence/validation outcome, the placeholder silently did nothing — and +// nothing stopped a third rule diverging later. +// +// So the MINT + APPEND + report decision lives here, once. What each caller +// still owns is its REVEAL policy, which is genuinely different: the File menu +// opens the new Dashboard in Edit mode and swaps the sidebar to the Dashboards +// tree, while the placeholder selects it in whichever mode the surface is +// already showing. Those are navigation choices about where the user was, not +// creation rules. +// +// Typed against a structural deps bag rather than `App`: `src/application/**` +// must never import `src/ui/**` (build/check-boundaries.mjs), and both the real +// `App` and `ui/dashboard.ts`'s narrower `DashboardApp` satisfy it directly. + +import { createEmptyDashboard } from '../dashboard/application/empty-dashboard.js'; +import { appendDashboard } from '../workspace/workspace-operations.js'; +import type { MutateWorkspace, WorkspaceMutationOutcome } from '../state.js'; +import type { StoredWorkspaceV5 } from '../generated/json-schema.types.js'; + +/** The created Dashboard's id, threaded back through `mutateWorkspace`'s + * `data` channel so a caller can navigate to exactly what was committed + * without reading it back out of the aggregate. */ +export type DashboardCreateOutcome = WorkspaceMutationOutcome; + +export interface DashboardCreateDeps { + /** The serialized, read-latest-at-dequeue write primitive every workspace + * producer commits through. */ + mutateWorkspace: MutateWorkspace; + /** Mints the new document's id through the injected `crypto.randomUUID` + * seam, like every other producer. */ + genId(): string; + /** + * What to append onto when NOTHING is persisted yet — `mutateWorkspace` + * hands the transform `null` for a workspace that has never been committed, + * and the very first Dashboard of a fresh workspace is created exactly + * there. Each caller answers with the freshest baseline it has (the File + * menu folds its live in-memory Dashboard in; the Dashboard surface hands + * over its projected aggregate), and `null` — no workspace at all — aborts, + * committing nothing. + */ + baseline(): StoredWorkspaceV5 | null; +} + +/** + * Append one empty Dashboard named `name`, and answer what happened. + * + * The document is minted BEFORE the commit is queued, and deliberately: an + * empty Dashboard's content does not depend on the baseline — only the APPEND + * does, and that runs inside the transform against dequeue-time truth. That is + * what lets a caller navigate to `outcome.data` without re-reading the + * aggregate. + * + * Additive by construction: `appendDashboard` preserves every existing + * Dashboard and query in place, so this can never reach `dashboards[0]` or the + * compatibility slot. It aborts — committing nothing — only when neither a + * persisted aggregate nor a caller baseline exists. + */ +export async function createDashboard( + deps: DashboardCreateDeps, name: string, +): Promise { + const created = createEmptyDashboard(deps.genId(), name); + return deps.mutateWorkspace((latest) => { + const base = latest ?? deps.baseline(); + return base === null ? null : { candidate: appendDashboard(base, created), data: created.id }; + }); +} + +/** + * What to tell the user about a creation attempt — the same sentence from + * whichever entry point ran it (#495 review 3). `null` means say nothing. + * + * Modeled on `library-assignment-service.ts`'s `libraryAssignmentMessage`: the + * pure message mapping lives beside the command, in the layer that knows the + * outcome shape, while the toast itself stays with the UI — `src/application/**` + * cannot reach `ui/toast.ts`. + * + * An abort is silent on purpose: nothing was committed and nothing was lost — + * the only reachable abort is "no workspace loaded", which is not a failure the + * user caused or can act on. + */ +export function dashboardCreateMessage(outcome: DashboardCreateOutcome): string | null { + if (outcome.ok) return 'Created dashboard'; + if (outcome.aborted) return null; + return '✕ ' + (outcome.diagnostics[0]?.message || 'Could not save workspace'); +} diff --git a/src/styles.css b/src/styles.css index c3f7a14b..5fa92c16 100644 --- a/src/styles.css +++ b/src/styles.css @@ -794,6 +794,15 @@ h1, h2, h3, h4, h5, h6 { given breathing room above the title field it follows. */ .fm-dialog-textarea { min-height: 64px; resize: vertical; font-family: inherit; } .fm-dialog-input + .fm-dialog-label { margin-top: 12px; } +/* #495 review 2: a rename/edit that could not commit reports INSIDE the dialog + it failed in, right above the actions, while the card keeps the text the + user typed — a toast fired from a dialog that simultaneously vanished was + how the entered title used to be lost. */ +.fm-dialog-error { + margin: 12px 0 0; color: var(--error-fg); + font-size: var(--text-label); line-height: var(--lh-body); +} +.fm-dialog-cancel:disabled { opacity: .45; cursor: default; } /* ------------ main row ------------ */ /* position:relative so the mobile sidebar overlay + backdrop (#126) can anchor diff --git a/src/ui/dashboard-tree.ts b/src/ui/dashboard-tree.ts index 0d0d032e..d304f731 100644 --- a/src/ui/dashboard-tree.ts +++ b/src/ui/dashboard-tree.ts @@ -41,7 +41,8 @@ import { } from '../application/dashboard-tree-model.js'; import { commitVariableConfig } from '../application/dashboard-variable-config.js'; import { commitDashboardRename } from '../application/dashboard-title.js'; -import { openDialogShell } from './dialog-shell.js'; +import type { DashboardRenameOutcome } from '../application/dashboard-title.js'; +import { openMetadataDialog } from './dialog-shell.js'; import { assignLibraryQuerySqlToVariable, assignLibraryQueryToPanel, libraryAssignmentMessage, } from '../application/library-assignment-service.js'; @@ -159,6 +160,32 @@ const rowAccessibleName = (row: DashboardTreeRow): string => [ row.invalid === null ? '' : STATUS_LABELS[row.invalid], ].filter((part) => part !== '').join(' '); +/** + * Keep Enter/Space on a nested action button from ALSO reaching the tree's own + * keyboard handler (#495 review 1). + * + * The tree's `keydown` listener lives on the LIST, and its Enter arm calls + * `preventDefault()` and runs the focused ROW's primary command. A button + * inside that row is a descendant, so without this its Enter bubbled up and + * did two wrong things at once: it ran the row's action (for a Dashboard row, + * navigating away), and the `preventDefault()` suppressed the browser's own + * key-to-click synthesis, so the button's actual job might never happen. + * + * Propagation is stopped but the default is NOT prevented, which is the whole + * point: native activation still fires — exactly once, on keydown for Enter + * and on keyup for Space — so each control keeps standard button semantics + * instead of re-implementing them. The chevron is the one exception and wires + * its own handler: it must also `preventDefault()`, because it toggles + * directly and would otherwise be re-toggled by the synthesized click. + * + * `handleTreeKeydown` independently ignores anything that did not originate on + * a row. Two layers, deliberately: this one keeps each button self-contained, + * that one holds even for a control that forgets to install this. + */ +const isolateActivationKeys = (event: KeyboardEvent): void => { + if (event.key === 'Enter' || event.key === ' ') event.stopPropagation(); +}; + /** One arbiter per app instance, surviving the repaints that replace every row. */ function arbiterFor(app: DashboardTreeApp): ClickArbiter { if (!app._dashTreeArbiter) { @@ -786,6 +813,7 @@ function buildDeleteButton(app: DashboardTreeApp, doc: Document, row: DashboardT 'aria-haspopup': 'menu', 'aria-expanded': 'false', 'aria-label': 'Delete the stored option SQL for ' + row.label, title: 'Delete stored option SQL', + onkeydown: isolateActivationKeys, onclick: (event: MouseEvent) => { event.stopPropagation(); app._dashTreeArbiter?.cancelFor(row.key); @@ -831,6 +859,7 @@ function buildRenameButton(app: DashboardTreeApp, doc: Document, row: DashboardT 'aria-haspopup': 'dialog', 'aria-expanded': 'false', 'aria-label': 'Edit dashboard ' + row.label, title: 'Edit dashboard title & description', + onkeydown: isolateActivationKeys, onclick: (event: MouseEvent) => { event.stopPropagation(); app._dashTreeArbiter?.cancelFor(row.key); @@ -840,12 +869,18 @@ function buildRenameButton(app: DashboardTreeApp, doc: Document, row: DashboardT return trigger; } -/** Two-field (title + description) metadata dialog — distinct from the - * single-field `openNameDialog` (`dialog-shell.ts`) the create flows use, so - * that shared primitive stays single-purpose. Built directly on - * `openDialogShell` (colocated here rather than in the shared module: it has - * exactly one consumer today, matching hard rule 5's "extract a shared - * primitive only when a SECOND consumer appears"). */ +/** + * The Dashboard document's own title/description dialog, on the shared + * two-field `openMetadataDialog` (`dialog-shell.ts` — the panel pencil below + * is its second consumer, which is what moved it out of this module). + * + * Every unsuccessful outcome keeps the dialog open with the typed text intact + * and reports inside it (#495 review 2). The first version closed the card + * before starting the mutation and discarded the promise, so a Dashboard + * deleted in another tab, a duplicate id, a validation rejection or a storage + * failure all read as "the dialog just disappeared" — and took the user's + * edits with it. + */ function openDashboardMetadataDialog( app: DashboardTreeApp, doc: Document, trigger: HTMLButtonElement, row: DashboardTreeRow, ): void { @@ -857,49 +892,36 @@ function openDashboardMetadataDialog( // enforces no CSS layout at all). trigger.setAttribute('aria-expanded', 'true'); const current = app.currentWorkspace?.dashboards?.find((d) => d.id === row.dashboardId); - const titleInput = h('input', { - class: 'fm-dialog-input', type: 'text', id: 'dash-rename-title', spellcheck: 'false', - value: current?.title ?? row.label, - }) as HTMLInputElement; - const descInput = h('textarea', { - class: 'fm-dialog-input fm-dialog-textarea', id: 'dash-rename-description', spellcheck: 'false', - }) as HTMLTextAreaElement; - descInput.value = current?.description ?? ''; - const confirm = h('button', { - class: 'fm-dialog-confirm', - onclick: () => commit(), - }, 'Save') as HTMLButtonElement; - const commit = (): void => { - const title = titleInput.value.trim(); - if (!title) return; - handle.close(); - // Fire-and-forget: a successful commit reprojects the workspace and - // repaints the tree/any open Dashboard surface on its own; an aborted one - // (the Dashboard was deleted/duplicated concurrently) leaves nothing here - // to undo. - void commitDashboardRename(app, row.dashboardId, title, descInput.value); - }; - const sync = (): void => { confirm.disabled = titleInput.value.trim() === ''; }; - titleInput.addEventListener('input', sync); - titleInput.addEventListener('keydown', (e) => { - // Enter commits from the TITLE field only — the description is a - // `