diff --git a/CHANGELOG.md b/CHANGELOG.md index 74f5ff09..59558382 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,13 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] ### Added +- **A Dashboard row can create a blank Panel directly from the Dashboards + tree** (#515). Its new plus action opens the shared name/description dialog, + then atomically commits one empty panel-role query and one canonically laid + out tile—without selecting or copying a Library query. The action is + keyboard-accessible, unavailable for ambiguous or 100-panel Dashboards, and + a successful Add reveals the new Panel and opens its clean linked tab with + focus in the SQL editor. - **Every Library query now has a keyboard-accessible “Add to dashboard…” action** (#483). Its Dashboard → panel chooser uses the same assignment command as drag/drop, creates the same independent owned query and tile, and diff --git a/README.md b/README.md index 5a905618..19e869be 100644 --- a/README.md +++ b/README.md @@ -445,6 +445,14 @@ preference — it never adds or removes a Dashboard panel. Saving or editing a q form with both a name and a description field; the description shows under the row and is included in Markdown/SQL exports. +Each unambiguous Dashboard row also has a direct **+ Add panel** action. It asks +for a required Panel name and optional description, atomically creates an empty +Dashboard-owned query plus its tile, and opens that clean linked query in the +SQL editor. This is intentionally separate from **Add to dashboard…** on a +Library row: creating a blank Panel never selects, copies, or modifies a Library +query. A Dashboard at the 100-panel schema limit exposes the action as +unavailable instead of opening a dialog that cannot commit. + The queries plus the workspace's Dashboard collection form a workspace (`StoredWorkspaceV3`; records written before that contract are read and migrated automatically). A browser profile can keep multiple workspaces in diff --git a/docs/ADR-0003-dashboard-viewing.md b/docs/ADR-0003-dashboard-viewing.md index fab4f579..b1b429fd 100644 --- a/docs/ADR-0003-dashboard-viewing.md +++ b/docs/ADR-0003-dashboard-viewing.md @@ -774,6 +774,44 @@ specific: mobile is out of scope for the gesture as a whole, no `isMobile` branches were added, and `draggable` stays unconditional on Library and History rows so the shipped editor drop is unchanged there. +## Addendum (#515, 2026-07-28): blank Panel creation is one owned aggregate mutation + +A Dashboard row now places **Add panel** immediately before its edit and delete +actions. This is not Library assignment: the two-field dialog supplies the new +query's name and optional description, and the command constructs a blank +`spec.dashboard.role: "panel"` query directly. No Library source is resolved, +copied, favorited, or modified. + +The pure candidate builder lives in `dashboard/application`, while its +read-latest-at-dequeue wrapper and injected id generation live in +`application`. Query and tile ids are minted once per Add attempt. Inside the +queued transform the command strictly re-resolves the Dashboard, re-checks the +100-tile limit, rejects workspace-wide query-id and Dashboard-local tile-id +collisions, appends the query, and delegates tile creation to +`add-query-instance`. That canonical path remains the sole owner of initial +placement, layout normalization, and grid-fallback regeneration. The target +Dashboard revision advances once; every unrelated query and Dashboard is +retained unchanged. Aggregate validation and persistence see one candidate, so +an orphan query or dangling tile is never published on failure. + +Success settlement is deliberately a post-dialog-close callback. The dialog +first tears down and performs its ordinary return-focus step; only then does +the tree reveal the new Panel and call the existing saved-query open path. +The creation service does not invoke the active Dashboard's workspace-refresh +hook before returning: `mutateWorkspace` has already projected the committed +aggregate, while an eager Dashboard rerender would force-close the dialog and +discard that post-close navigation. +That path switches to the Query surface, seeds the linked tab's committed-token +baseline, and focuses the SQL editor, so dialog teardown cannot steal focus back +to the hidden plus trigger. Cancel, Escape, backdrop close, stale targets, +collisions, validation errors, and persistence failures perform no navigation; +failed writes keep the entered values and diagnostic in the open dialog. +An unexpected rejected write is converted to the same recoverable dialog state: +the fields remain, a generic diagnostic is announced, and every dismissal path +is re-enabled. If persistence succeeds only after the route has moved on, +settlement instead closes silently without claiming failure or running the +old route's reveal/open/focus callback. + ## Addendum (#465, 2026-07-27): Test's shape check is re-hosted on Run, not re-invented The #457 addendum above ("Losing Test is accepted...") deferred re-hosting the diff --git a/src/application/dashboard-tree-model.ts b/src/application/dashboard-tree-model.ts index 307fc06b..71f976cd 100644 --- a/src/application/dashboard-tree-model.ts +++ b/src/application/dashboard-tree-model.ts @@ -31,6 +31,7 @@ import type { DashboardTreeGroup, DashboardTreeUiState } from '../core/dashboard import { encodeKeyPart, groupStateKey } from '../core/dashboard-tree-ui-state.js'; import { inferDashboardVariables } from '../core/dashboard-variables.js'; import { buildQueryOwnershipIndex } from '../dashboard/model/query-ownership.js'; +import { PORTABLE_LIMITS } from '../dashboard/model/portable-limits.js'; import { queryDashboardRole } from '../dashboard/model/workspace-semantics.js'; import type { DashboardVariable } from '../core/dashboard-variables.js'; import type { LibraryDropTarget } from '../core/library-drag.js'; @@ -139,6 +140,7 @@ export type DashboardTreeCommand = * answered exactly once, here. */ export type DashboardTreeActionKind = + | 'add-panel' | 'edit-dashboard' | 'delete-dashboard' | 'edit-panel' @@ -440,7 +442,9 @@ const WRONG_ROLE_REASON = * states, applied to the Dashboard identity delete already refuses on * (`dashboard-duplicate` in `removeDashboardDocument`/`removeDashboardPanel`). */ const AMBIGUOUS_DASHBOARD_REASON = - 'Two dashboards in this workspace share this id, so it cannot be edited or removed here.'; + 'Two dashboards in this workspace share this id, so neither can be changed here.'; +const DASHBOARD_TILE_LIMIT_REASON = + 'This dashboard already has the maximum of 100 panels, so another panel cannot be added.'; /** Two tiles in the SAME Dashboard carry this id — even when they reference * different queries. Each would otherwise look, independently, like that * query's sole owner (`ownersOfQuery` cannot tell them apart), so this is @@ -653,7 +657,7 @@ export function deriveDashboardTree( invalid: null, severity: null, diagnostic: null, - // #494: the Dashboard row's own two direct controls. Its `⋯` menu is + // #494/#515: the Dashboard row's own three direct controls. Its `⋯` menu is // gone — *Open in Edit* was its last item, and a menu button that opens // a one-item menu beside two real controls is chrome, not vocabulary. // Shift-click / Shift+Enter remain the Edit gesture (`shift` below). @@ -664,12 +668,19 @@ export function deriveDashboardTree( // commit would only refuse is the exact bug this closes. actions: dashboardIdDuplicated ? [ + unavailableAction('add-panel', 'Add panel to ' + (title || UNTITLED_DASHBOARD), + AMBIGUOUS_DASHBOARD_REASON), unavailableAction('edit-dashboard', 'Edit dashboard ' + (title || UNTITLED_DASHBOARD), AMBIGUOUS_DASHBOARD_REASON), unavailableAction('delete-dashboard', 'Delete dashboard ' + (title || UNTITLED_DASHBOARD), AMBIGUOUS_DASHBOARD_REASON), ] : [ + ...(tiles.length >= PORTABLE_LIMITS.maxTilesPerDashboard + ? [unavailableAction('add-panel', 'Add panel to ' + (title || UNTITLED_DASHBOARD), + DASHBOARD_TILE_LIMIT_REASON)] + : [action('add-panel', 'Add panel to ' + (title || UNTITLED_DASHBOARD), + 'Add panel', { kind: 'dashboard', dashboardId: dashboard.id })]), action('edit-dashboard', 'Edit dashboard ' + (title || UNTITLED_DASHBOARD), 'Edit dashboard title & description', { kind: 'dashboard', dashboardId: dashboard.id }), action('delete-dashboard', 'Delete dashboard ' + (title || UNTITLED_DASHBOARD), diff --git a/src/application/panel-creation-service.ts b/src/application/panel-creation-service.ts new file mode 100644 index 00000000..ab4429cb --- /dev/null +++ b/src/application/panel-creation-service.ts @@ -0,0 +1,65 @@ +// The async application command around the pure blank-Panel candidate (#515). +// IDs are minted exactly once per Add attempt, then the target and all limits +// are re-resolved inside `mutateWorkspace` against committed dequeue-time truth. + +import { createPanelCandidate } from '../dashboard/application/panel-creation.js'; +import type { PanelCreationAbort } from '../dashboard/application/panel-creation.js'; +import type { MutateWorkspace, WorkspaceMutationOutcome } from '../state.js'; + +export type PanelCreationData = { status: 'ok'; queryId: string; tileId: string }; +export type PanelCreationDeclined = { status: 'declined'; reason: PanelCreationAbort }; +export type PanelCreationOutcome = + WorkspaceMutationOutcome; + +export interface PanelCreationDeps { + mutateWorkspace: MutateWorkspace; + genId(): string; +} + +const declined = ( + reason: PanelCreationAbort, +): { candidate: null; data: PanelCreationDeclined } => ({ + candidate: null, + data: { status: 'declined', reason }, +}); + +export async function createDashboardPanel( + deps: PanelCreationDeps, + dashboardId: string, + name: string, + description: string, +): Promise { + const queryId = deps.genId(); + const tileId = deps.genId(); + const outcome = await deps.mutateWorkspace((latest) => { + if (latest === null) return declined('dashboard-missing'); + const result = createPanelCandidate({ + latest, dashboardId, queryId, tileId, name, description, + }); + if (!result.ok) return declined(result.reason); + return { candidate: result.workspace, data: { status: 'ok', ...result.data } }; + }); + return outcome; +} + +const DECLINE_MESSAGES: Record = { + 'dashboard-missing': 'That dashboard is no longer part of this workspace.', + 'dashboard-ambiguous': 'That dashboard is ambiguous and cannot be changed.', + 'tile-limit': 'That dashboard already has the maximum of 100 panels.', + 'id-collision': 'Could not create this panel because an id already exists. Please try again.', + 'blank-name': 'Enter a panel name.', +}; + +/** + * A dialog diagnostic, `null` for a route-current committed creation, or + * `undefined` when the route became stale and the dialog should dismiss + * without claiming failure or running route-local success settlement. + */ +export function panelCreationMessage(outcome: PanelCreationOutcome): string | null | undefined { + if (outcome.ok) return null; + if (!outcome.aborted) { + return outcome.diagnostics[0]?.message || 'Could not save this panel.'; + } + const data = outcome.data; + return data?.status === 'declined' ? DECLINE_MESSAGES[data.reason] : undefined; +} diff --git a/src/dashboard/application/panel-creation.ts b/src/dashboard/application/panel-creation.ts new file mode 100644 index 00000000..3ddaacd4 --- /dev/null +++ b/src/dashboard/application/panel-creation.ts @@ -0,0 +1,90 @@ +// Creating one blank Dashboard-owned Panel (#515). Pure — no DOM, +// persistence, or id generation. +// +// The query and tile are one aggregate candidate: callers either validate and +// commit both or observe neither. Placement deliberately goes through the +// canonical `add-query-instance` command so this path inherits the same +// size-hint defaults, layout normalization, and grid-fallback regeneration as +// every other panel add. + +import { SPEC_VERSION } from '../../core/saved-query.js'; +import { PORTABLE_LIMITS } from '../model/portable-limits.js'; +import { findDashboardStrict, replaceDashboard } from '../../workspace/workspace-dashboards.js'; +import { applyCommand } from './dashboard-commands.js'; +import type { ApplyCommandResult } from './dashboard-commands.js'; +import { createQueryResolver } from './dashboard-query-resolver.js'; +import { resolveLayoutPluginSync } from '../layouts/layout-registry.js'; +import type { SavedQueryV2, StoredWorkspaceV5 } from '../../generated/json-schema.types.js'; + +export type PanelCreationAbort = + | 'dashboard-missing' + | 'dashboard-ambiguous' + | 'tile-limit' + | 'id-collision' + | 'blank-name'; + +export type PanelCreationResult = + | { + ok: true; + workspace: StoredWorkspaceV5; + data: { queryId: string; tileId: string }; + } + | { ok: false; reason: PanelCreationAbort }; + +export interface CreatePanelCandidateInput { + latest: StoredWorkspaceV5; + dashboardId: string; + queryId: string; + tileId: string; + name: string; + description: string; +} + +const fail = (reason: PanelCreationAbort): PanelCreationResult => ({ ok: false, reason }); + +/** + * Build the complete candidate for one new empty panel. + * + * Existing query and Dashboard objects are retained byte-for-byte except for + * the one target Dashboard, whose revision advances exactly once. + */ +export function createPanelCandidate(input: CreatePanelCandidateInput): PanelCreationResult { + const { latest, dashboardId, queryId, tileId } = input; + const lookup = findDashboardStrict(latest, dashboardId); + if (lookup.status !== 'ok') { + return fail(lookup.status === 'missing' ? 'dashboard-missing' : 'dashboard-ambiguous'); + } + const base = lookup.dashboard; + if (base.tiles.length >= PORTABLE_LIMITS.maxTilesPerDashboard) return fail('tile-limit'); + if (latest.queries.some((query) => query.id === queryId) + || base.tiles.some((tile) => tile.id === tileId)) { + return fail('id-collision'); + } + + const name = input.name.trim(); + if (name === '') return fail('blank-name'); + const description = input.description.trim(); + const query: SavedQueryV2 = { + id: queryId, + sql: '', + specVersion: SPEC_VERSION, + spec: { + name, + ...(description === '' ? {} : { description }), + dashboard: { role: 'panel' }, + }, + }; + const queries = [...latest.queries, query]; + const applied = applyCommand(base, { type: 'add-query-instance', queryId }, { + resolver: createQueryResolver(queries), + genTileId: () => tileId, + plugin: resolveLayoutPluginSync(base.layout), + }) as Extract; + const normalized = resolveLayoutPluginSync(applied.dashboard.layout).normalize(applied.dashboard); + const workspace = replaceDashboard({ ...latest, queries }, dashboardId, { + ...normalized, + revision: base.revision + 1, + }) as StoredWorkspaceV5; + + return { ok: true, workspace, data: { queryId, tileId } }; +} diff --git a/src/ui/dashboard-tree.ts b/src/ui/dashboard-tree.ts index c4c84963..fade4c26 100644 --- a/src/ui/dashboard-tree.ts +++ b/src/ui/dashboard-tree.ts @@ -52,6 +52,10 @@ import { commitPanelQueryMetadata } from '../application/dashboard-panel-metadat import type { PanelMetadataDeps, PanelMetadataOutcome, } from '../application/dashboard-panel-metadata.js'; +import { + createDashboardPanel, panelCreationMessage, +} from '../application/panel-creation-service.js'; +import type { PanelCreationData } from '../application/panel-creation-service.js'; import { closeOpenDialogShell, openMetadataDialog } from './dialog-shell.js'; import { assignLibraryQuerySqlToVariable, assignLibraryQueryToPanel, libraryAssignmentMessage, @@ -841,7 +845,8 @@ function buildActionButton( // to look and announce like a delete (#494 — a row's vocabulary must not // change with availability), and its `confirm` is null precisely because it // will never be asked. - const destructive = act.kind !== 'edit-dashboard' && act.kind !== 'edit-panel'; + const destructive = act.kind !== 'add-panel' + && act.kind !== 'edit-dashboard' && act.kind !== 'edit-panel'; const trigger: HTMLButtonElement = h('button', { class: 'dash-tree-act' + (destructive ? ' dash-tree-act-danger' : '') @@ -866,7 +871,9 @@ function buildActionButton( app._dashTreeArbiter?.cancelFor(row.key); runAction(app, doc, row, act, trigger); }, - }, act.kind === 'edit-dashboard' || act.kind === 'edit-panel' ? Icon.pencil() : Icon.trash()); + }, act.kind === 'add-panel' + ? Icon.plus() + : (act.kind === 'edit-dashboard' || act.kind === 'edit-panel' ? Icon.pencil() : Icon.trash())); return trigger; } @@ -904,6 +911,7 @@ const returnFocusAfterDialog = ( * question above it already named the resource, and "OK" next to a sentence * full of names is where a destructive click gets made by accident. */ const CONFIRM_LABELS: Record = { + 'add-panel': '', 'edit-dashboard': '', 'edit-panel': '', 'delete-dashboard': 'Delete dashboard', @@ -927,6 +935,10 @@ function runAction( ): void { const target = act.target; if (target === null) return; + if (act.kind === 'add-panel') { + openPanelCreationDialog(app, doc, trigger, row); + return; + } if (act.kind === 'edit-dashboard') { openDashboardMetadataDialog(app, doc, trigger, row); return; } if (act.kind === 'edit-panel') { openPanelMetadataDialog(app, doc, trigger, row, target as PanelActionTarget); @@ -940,6 +952,40 @@ function runAction( * each use: the model pairs kind and target by construction. */ type PanelActionTarget = Extract; +/** Create a blank owned query + tile, then reveal and open it only after the + * dialog has closed so its return-focus step cannot steal focus from SQL. */ +function openPanelCreationDialog( + app: DashboardTreeApp, doc: Document, trigger: HTMLButtonElement, row: DashboardTreeRow, +): void { + closeOpenDialogShell(); + trigger.setAttribute('aria-expanded', 'true'); + let created: PanelCreationData | null = null; + openMetadataDialog({ document: doc, acquireKeyboardOwner: app.acquireKeyboardOwner }, { + title: 'Add panel', + nameLabel: 'Panel name', + descriptionLabel: 'Panel description', + name: '', + description: '', + confirmLabel: 'Add', + idPrefix: 'panel-create', + lockCloseWhileConfirming: true, + returnFocusTo: returnFocusAfterDialog(app, trigger, row.key), + onClose: () => trigger.setAttribute('aria-expanded', 'false'), + onSuccessAfterClose: () => { + revealAssignedPanel(app, row.dashboardId, created!.tileId); + app.openSavedQuery(created!.queryId); + }, + onConfirm: async ({ name, description }) => { + const outcome = await createDashboardPanel({ + mutateWorkspace: app.mutateWorkspace, + genId: app.genId, + }, row.dashboardId, name, description); + if (outcome.ok) created = outcome.data as PanelCreationData; + return panelCreationMessage(outcome); + }, + }); +} + /** Ask before destroying something, anchored on the control that asked. */ function confirmDestructive( doc: Document, trigger: HTMLButtonElement, act: DashboardTreeAction, go: () => void, diff --git a/src/ui/dialog-shell.ts b/src/ui/dialog-shell.ts index ee836207..d7a16105 100644 --- a/src/ui/dialog-shell.ts +++ b/src/ui/dialog-shell.ts @@ -32,6 +32,9 @@ export interface DialogHostApp { export interface OpenDialogShellOpts { extraCardClass?: string; + /** Refuse user-initiated close paths while this returns false. Application + * teardown still force-closes through `closeOpenDialogShell()`. */ + canClose?: () => boolean; /** * Where focus goes when the dialog closes — `null` when there is nothing to * remember (the trigger is already gone by the time the dialog mounts, as @@ -63,7 +66,7 @@ export interface OpenDialogShellOpts { onClose?: () => void; } -let openHandle: { backdrop: Element; close: () => void } | null = null; +let openHandle: { backdrop: Element; forceClose: () => void } | null = null; /** Distinguishes one dialog's `aria-labelledby` target from the next one's. * Module-local and monotonic — never reset — so a stale dialog being torn @@ -74,7 +77,7 @@ let dialogSeq = 0; * the same teardown every surface transition already runs for anchored * popovers and the doc pane. A no-op when nothing is open. */ export function closeOpenDialogShell(): void { - openHandle?.close(); + openHandle?.forceClose(); } export function openDialogShell( @@ -98,8 +101,9 @@ export function openDialogShell( // that second call would restore focus (stealing it from behind the modal // that replaced it) and run the caller's `onClose` a second time. let torndown = false; - const close = (): void => { + const close = (force = false): void => { if (torndown) return; + if (!force && opts.canClose?.() === false) return; torndown = true; doc.removeEventListener('keydown', onKey, true); detachBackdrop(); @@ -147,9 +151,9 @@ export function openDialogShell( 'aria-labelledby': titleId, }, h('div', { class: 'fm-dialog-title', id: titleId }, title), content); backdrop = h('div', { class: 'fm-dialog-backdrop' }, card); - const detachBackdrop = attachBackdropClose(backdrop, close); - const handle: DialogHandle = { close }; - openHandle = { backdrop, close }; + const detachBackdrop = attachBackdropClose(backdrop, () => close()); + const handle: DialogHandle = { close: () => close() }; + openHandle = { backdrop, forceClose: () => close(true) }; doc.body.appendChild(backdrop); doc.addEventListener('keydown', onKey, true); return handle; @@ -164,6 +168,14 @@ export interface MetadataDialogValues { description: string; } +/** + * `null` is a committed success, so the dialog closes and runs + * `onSuccessAfterClose`. `undefined` is a neutral/stale settlement: close + * without reporting failure or running route-local success work. A string is + * a visible diagnostic and keeps the entered values in the open dialog. + */ +export type MetadataDialogConfirmResult = string | null | undefined; + export interface MetadataDialogOpts { /** Heading, and the dialog's accessible name (`Edit dashboard`). */ title: string; @@ -183,10 +195,23 @@ export interface MetadataDialogOpts { returnFocusTo: OpenDialogShellOpts['returnFocusTo']; onClose?: () => void; /** - * Commit the edit. Resolve `null` when it succeeded — the dialog closes — - * or a message to show, keeping the dialog open with the user's text intact. + * Runs only after a successful confirm has closed and fully torn down the + * dialog. Navigation that focuses a new destination belongs here, otherwise + * the dialog's return-focus step can steal focus back to its old trigger. + */ + onSuccessAfterClose?: () => void; + /** + * Disable every user close path while confirm is in flight. Use this when + * the commit creates a durable resource, so closing cannot imply that an + * already-started creation was cancelled. + */ + lockCloseWhileConfirming?: boolean; + /** + * Commit the edit. Resolve `null` when it succeeded, `undefined` when the + * caller became stale and should dismiss without success settlement, or a + * message to show while keeping the user's text intact. */ - onConfirm(values: MetadataDialogValues): Promise; + onConfirm(values: MetadataDialogValues): Promise; } /** @@ -203,8 +228,9 @@ export interface MetadataDialogOpts { * unsuccessful outcome keeps the card open, restores its controls and shows * ONE diagnostic; only a real commit closes it. * - * While a commit is in flight both buttons are disabled, so the same mutation - * cannot be submitted twice by an impatient second Enter. If the shell was + * While a commit is in flight confirm is disabled, so the same mutation + * cannot be submitted twice by an impatient second Enter. Creation callers + * may also lock Cancel, Escape and backdrop dismissal. If the shell was * force-closed underneath us (a surface transition runs * `closeOpenDialogShell()`), the late resolution is dropped rather than * writing a diagnostic into a detached card. @@ -233,26 +259,48 @@ export function openMetadataDialog(app: DialogHostApp, opts: MetadataDialogOpts) let closed = false; let inFlight = false; const sync = (): void => { - // Only the CONFIRM is barred while a write is in flight — that is what - // stops the same mutation being submitted twice. Cancel stays operable, so - // the visible way out agrees with Escape and the backdrop, which were never - // gated; a dialog whose only escape hatches are invisible reads as wedged. - // Closing mid-write is safe: the late answer is dropped below rather than - // written into a detached card. + // Confirm is always barred while a write is in flight. Most metadata edits + // still allow dismissal and drop a late answer; durable creation opts into + // locking every user close path until the write answers. confirm.disabled = inFlight || nameInput.value.trim() === ''; + cancel.disabled = opts.lockCloseWhileConfirming === true && inFlight; }; const commit = async (): Promise => { const name = nameInput.value.trim(); if (!name || inFlight) return; inFlight = true; sync(); - const message = await opts.onConfirm({ name, description: descInput.value }); + let message: MetadataDialogConfirmResult; + try { + message = await opts.onConfirm({ name, description: descInput.value }); + } catch { + // Rejections are part of the injected mutation contract (store access, + // transforms and persistence may all throw). A locked creation dialog + // must recover just like a diagnostic outcome rather than becoming a + // modal the user cannot dismiss. + if (closed) return; + error.textContent = 'Could not save changes. Please try again.'; + error.hidden = false; + nameInput.focus(); + return; + } finally { + if (!closed) { + inFlight = false; + sync(); + } + } // Force-closed while the write was queued: whatever it answered belongs to // a dialog that is no longer on screen, and its own commit path reports. if (closed) return; - inFlight = false; - sync(); - if (message === null) { handle.close(); return; } + if (message === null) { + handle.close(); + opts.onSuccessAfterClose?.(); + return; + } + if (message === undefined) { + handle.close(); + return; + } error.textContent = message; error.hidden = false; nameInput.focus(); @@ -275,6 +323,7 @@ export function openMetadataDialog(app: DialogHostApp, opts: MetadataDialogOpts) h('div', { class: 'fm-dialog-actions' }, cancel, confirm), ], { returnFocusTo: opts.returnFocusTo, + canClose: () => opts.lockCloseWhileConfirming !== true || !inFlight, onClose: () => { closed = true; opts.onClose?.(); }, }); sync(); diff --git a/tests/e2e/dashboard-tree.spec.js b/tests/e2e/dashboard-tree.spec.js index 1a456608..c5e2ddfb 100644 --- a/tests/e2e/dashboard-tree.spec.js +++ b/tests/e2e/dashboard-tree.spec.js @@ -310,8 +310,8 @@ test.describe('Dashboard hierarchy tree', () => { // #472: "focus styling must visibly distinguish the disclosure control from the // navigation target from the trailing action". happy-dom can see none of this, and // `:focus-visible` only applies under real keyboard modality — so it has to be a - // real Tab walk in a real browser. Which doubles as proof that all four targets - // (#429 phase 3 added the rename pencil) are keyboard-reachable, in row order, + // real Tab walk in a real browser. Which doubles as proof that all five targets + // (#515 added the plus before the rename pencil) are keyboard-reachable, in row order, // within ONE composite tab stop. test('Tab walks the row, its chevron and its trailing actions, each ringed differently', async ({ page }) => { await open(page); @@ -336,15 +336,17 @@ test.describe('Dashboard hierarchy tree', () => { await page.keyboard.press('Tab'); const chevron = await ring(); await page.keyboard.press('Tab'); + const plus = await ring(); + await page.keyboard.press('Tab'); const pencil = await ring(); await page.keyboard.press('Tab'); const trash = await ring(); - // #494: the trailing target became a CLUSTER — pencil then trash, the - // destructive one last — and Tab reaches both inside the one composite - // tab stop, in paint order. - expect([row.what, chevron.what, pencil.what, trash.what]).toEqual([ - 'workspace:sales', 'chevron', 'Edit dashboard Sales revenue', 'Delete dashboard Sales revenue', + // #515: the cluster is plus, pencil, trash — destructive last — and Tab + // reaches all three inside the one composite tab stop, in paint order. + expect([row.what, chevron.what, plus.what, pencil.what, trash.what]).toEqual([ + 'workspace:sales', 'chevron', 'Add panel to Sales revenue', + 'Edit dashboard Sales revenue', 'Delete dashboard Sales revenue', ]); // The row rings with a box-shadow and no outline; the chevron with an outline and // no shadow. Different channels, so neither reads as the other — and neither @@ -353,6 +355,7 @@ test.describe('Dashboard hierarchy tree', () => { expect(row.outline).toBe('none'); expect(chevron.outline).toContain('solid'); expect(chevron.shadow).toBe('none'); + expect(plus.outline).not.toBe(chevron.outline); expect(pencil.outline).not.toBe(chevron.outline); expect(trash.outline).not.toBe(chevron.outline); // Nothing was opened or expanded by walking the row. @@ -854,12 +857,13 @@ test.describe('direct row actions (#494)', () => { await open(page); await roleTab(page, 'Dashboards').click(); // Tab to the pencil the way a keyboard user reaches it — row, chevron, - // pencil — rather than calling `.click()`, which is what let the #495 + // plus, pencil — rather than calling `.click()`, which is what let the #495 // review defect through: the tree's own Enter handler runs on the LIST and // would otherwise navigate instead. await treeRow(page, 'workspace:sales').focus(); await page.keyboard.press('Tab'); await page.keyboard.press('Tab'); + await page.keyboard.press('Tab'); await expect(treeRow(page, 'workspace:sales') .getByRole('button', { name: 'Edit dashboard Sales revenue' })).toBeFocused(); await page.keyboard.press('Enter'); @@ -875,8 +879,9 @@ test.describe('direct row actions (#494)', () => { await treeRow(page, 'workspace:sales').focus(); await page.keyboard.press('Tab'); await page.keyboard.press('Tab'); + await page.keyboard.press('Tab'); await page.keyboard.press('Space'); - await expect(page.locator('.fm-dialog-card')).toHaveCount(1); + await expect(page.getByRole('dialog', { name: 'Edit dashboard' })).toBeVisible(); expect(await page.evaluate(() => window.__opened)).toEqual([]); await page.keyboard.press('Escape'); }); @@ -984,7 +989,7 @@ test.describe('direct row actions (#494)', () => { // #494: "narrow sidebar layout must preserve the label's usable width and // ellipsis rather than overlapping or wrapping the controls". Two revealed - // buttons per row is one more than #429 phase 3 had, and happy-dom can see + // buttons per row is two more than #429 phase 3 had, and happy-dom can see // none of this — only a real layout can. test('a long title still ellipsizes in a narrow pane with the cluster revealed', async ({ page }) => { await open(page, 900); @@ -1007,13 +1012,13 @@ test.describe('direct row actions (#494)', () => { // No control sits on top of the label… overlap: acts.some((act) => act.getBoundingClientRect().left < labelBox.right - 0.5), // …and, the assertion that actually bites: the label SHRINKS to make - // room, so both controls stay inside the visible pane instead of being + // room, so all controls stay inside the visible pane instead of being // pushed off its right edge where nothing can reach them. spilled: acts.filter((act) => act.getBoundingClientRect().right > listBox.right + 0.5).length, rowOverflow: el.scrollWidth - list.clientWidth, }; }); - expect(box.actCount).toBe(2); + expect(box.actCount).toBe(3); expect(box.lines).toBeLessThanOrEqual(24); expect(box.clipped).toBe(true); expect(box.overlap).toBe(false); diff --git a/tests/e2e/tile-open-workbench.html b/tests/e2e/tile-open-workbench.html index 1416e51d..cc58ea52 100644 --- a/tests/e2e/tile-open-workbench.html +++ b/tests/e2e/tile-open-workbench.html @@ -47,6 +47,7 @@ import { createCodeMirrorEditor } from '/src/editor/codemirror-adapter.js'; import { createSpecEditor } from '/src/editor/spec-editor.js'; import { createCodeViewer } from '/src/editor/code-viewer.js'; + import { libraryQueries } from '/src/dashboard/model/query-ownership.js'; // `table` rather than a chart panel: the Chart.js seam is deliberately NOT // injected here (this fixture is about a tile's chrome, not its rendering), and @@ -145,6 +146,9 @@ window.__tabs = () => app.state.tabs.value.map((tab) => ({ name: tab.name, savedId: tab.savedId, docKind: tab.doc?.kind ?? null, active: tab.id === app.state.activeTabId.value, + sql: tab.sqlDraft, spec: tab.specParsed, + dirtySql: tab.dirtySql, dirtySpec: tab.dirtySpec, + committedToken: tab.lastCommittedQueryToken, })); window.__surface = () => app.mainSurface.kind; /** Committed truth for every query, so a Save can be checked against the @@ -155,6 +159,10 @@ ? read.workspace.queries.map((query) => ({ id: query.id, name: query.spec.name, sql: query.sql })) : null; }; + window.__libraryIds = async () => { + const read = await app.workspace.loadById(seed.id); + return read.status === 'ok' ? libraryQueries(read.workspace).map((query) => query.id) : null; + }; window.__app = app; window.__ready = true; diff --git a/tests/e2e/tile-open-workbench.spec.js b/tests/e2e/tile-open-workbench.spec.js index 795bfdfa..e1645007 100644 --- a/tests/e2e/tile-open-workbench.spec.js +++ b/tests/e2e/tile-open-workbench.spec.js @@ -103,6 +103,70 @@ test('the action is keyboard reachable and activates on Enter', async ({ page }) expect((await tabs(page)).at(-1)).toMatchObject({ savedId: 'q-sales', active: true }); }); +test('a Dashboard-row plus creates a blank linked Panel and focuses its SQL editor', async ({ page }) => { + await open(page); + // Begin on the actual Dashboard surface. A successful mutation must not + // rerender that surface (which force-closes overlays) before the dialog can + // close and perform its reveal/open/focus settlement. + await openDashboard(page, 'sales'); + await expect.poll(() => surface(page)).toBe('dashboard'); + const dashboard = treeRow(page, 'workspace:sales'); + const plus = dashboard.locator('.dash-tree-act[aria-label="Add panel to Sales"]'); + + // The direct action is a real keyboard target, revealed by focus just like + // the adjacent pencil and trash. Start from the row's disclosure control: + // the next Tab must be the plus in the declared within-row order. + await dashboard.locator('.dash-tree-chev').focus(); + await page.keyboard.press('Tab'); + await expect(plus).toHaveAttribute('aria-haspopup', 'dialog'); + await expect(plus).toBeFocused(); + await expect.poll(() => plus.evaluate((el) => getComputedStyle(el).display)).not.toBe('none'); + await page.keyboard.press('Enter'); + + const dialog = page.getByRole('dialog', { name: 'Add panel' }); + await expect(dialog).toBeVisible(); + const name = dialog.getByRole('textbox', { name: 'Panel name' }); + const description = dialog.getByRole('textbox', { name: 'Panel description' }); + await name.fill('New revenue panel'); + await description.fill('Created from the tree'); + await name.focus(); + await page.keyboard.press('Enter'); + + // Settlement happens only after the dialog closes: the linked tab is active, + // blank and clean, and focus belongs to the real CodeMirror SQL editor. + await expect(dialog).toBeHidden(); + await expect.poll(() => surface(page)).toBe('query'); + const active = (await tabs(page)).find((tab) => tab.active); + expect(active).toMatchObject({ + name: 'New revenue panel', + sql: '', + spec: { + name: 'New revenue panel', + description: 'Created from the tree', + dashboard: { role: 'panel' }, + }, + dirtySql: false, + dirtySpec: false, + }); + expect(active.savedId).toBeTruthy(); + expect(active.committedToken).toBeTruthy(); + await expect(page.locator('.cm-content[data-language="sql"]')).toBeFocused(); + // The row is addressed by TILE id, not the query id. Its visible existence is + // asserted by name after reveal expanded the Panels group. + await expect(page.locator('.dash-tree-row', { hasText: 'New revenue panel' })).toHaveCount(1); + expect(await page.evaluate(() => window.__libraryIds())).not.toContain(active.savedId); + + // Ordinary linked-query lifecycle from here: typing dirties the tab; Save + // updates the owned query and returns it to clean without moving it to Library. + await page.keyboard.type('SELECT 42'); + await expect.poll(async () => (await tabs(page)).find((tab) => tab.active).dirtySql).toBe(true); + await page.locator('.save-btn').click(); + await expect.poll(async () => (await page.evaluate(() => window.__queries())) + .find((query) => query.id === active.savedId).sql).toBe('SELECT 42'); + await expect.poll(async () => (await tabs(page)).find((tab) => tab.active).dirtySql).toBe(false); + expect(await page.evaluate(() => window.__libraryIds())).not.toContain(active.savedId); +}); + test('a KPI tile in VIEW mode still exposes a reachable action', async ({ page }) => { // The hard case, and the reason this spec exists: a grafana-grid KPI tile is // frameless, and its head is an absolutely-positioned `pointer-events: none` diff --git a/tests/unit/dashboard-tree-model.test.ts b/tests/unit/dashboard-tree-model.test.ts index 49ac5a68..29754ab8 100644 --- a/tests/unit/dashboard-tree-model.test.ts +++ b/tests/unit/dashboard-tree-model.test.ts @@ -603,7 +603,8 @@ describe('deriveDashboardTree — search', () => { expect(dash.single).toMatchObject({ kind: 'open-dashboard', request: { mode: 'view' } }); expect(dash.shift).toMatchObject({ kind: 'open-dashboard', request: { mode: 'edit' } }); // The search forcing expansion open does not touch the row's OWN actions. - expect(dash.actions.map((a) => a.kind)).toEqual(['edit-dashboard', 'delete-dashboard']); + expect(dash.actions.map((a) => a.kind)) + .toEqual(['add-panel', 'edit-dashboard', 'delete-dashboard']); }); it('every row is toggleable again once the search clears', () => { @@ -705,8 +706,9 @@ describe('deriveDashboardTree — command sets', () => { expect('focus' in (dash.single as { request: object }).request).toBe(false); // #494 removed the `⋯` menu — *Open in Edit* was its last item, and Shift // (asserted above via `shift`) is still how Edit is reached. The row's - // vocabulary is now its two direct actions, and nothing else mirrors a menu. - expect(dash.actions.map((a) => a.kind)).toEqual(['edit-dashboard', 'delete-dashboard']); + // vocabulary is now its three direct actions, and nothing else mirrors a menu. + expect(dash.actions.map((a) => a.kind)) + .toEqual(['add-panel', 'edit-dashboard', 'delete-dashboard']); }); it('gives a group row ONLY a toggle — no double, no Shift, no actions', () => { @@ -804,12 +806,33 @@ describe('deriveDashboardTree — direct actions (#494)', () => { ]); }); - it('names the Dashboard row\'s own controls "Edit dashboard " / "Delete dashboard <title>"', () => { + it('orders and names the Dashboard row add / edit / delete controls', () => { const tree = derive(ws({ dashboards: [dashboard({ id: 'd', title: 'D' })] })); const dash = row(tree.rows, 'w1:d'); - expect(dash.actions.map((a) => a.kind)).toEqual(['edit-dashboard', 'delete-dashboard']); - expect(dash.actions.map((a) => a.label)).toEqual(['Edit dashboard D', 'Delete dashboard D']); - expect(dash.actions[1].confirm).toBe('Delete dashboard “D”? This also deletes every query its panels own.'); + expect(dash.actions.map((a) => a.kind)) + .toEqual(['add-panel', 'edit-dashboard', 'delete-dashboard']); + expect(dash.actions.map((a) => a.label)) + .toEqual(['Add panel to D', 'Edit dashboard D', 'Delete dashboard D']); + expect(dash.actions[0]).toMatchObject({ + tooltip: 'Add panel', target: { kind: 'dashboard', dashboardId: 'd' }, + unavailable: null, confirm: null, + }); + expect(dash.actions[2].confirm).toBe('Delete dashboard “D”? This also deletes every query its panels own.'); + }); + + it('keeps Add panel visible but unavailable at the 100-tile limit', () => { + const tiles = Array.from({ length: 100 }, (_, i) => ({ id: 't' + i, queryId: 'q' + i })); + const dash = row(derive(ws({ + dashboards: [dashboard({ id: 'd', title: 'Full', tiles })], + })).rows, 'w1:d'); + + expect(dash.actions.map((action) => action.kind)) + .toEqual(['add-panel', 'edit-dashboard', 'delete-dashboard']); + expect(dash.actions[0]).toMatchObject({ + label: 'Add panel to Full', + target: null, + unavailable: 'This dashboard already has the maximum of 100 panels, so another panel cannot be added.', + }); }); it('falls back to the row\'s own Untitled label in an action name, on both row kinds', () => { @@ -823,7 +846,9 @@ describe('deriveDashboardTree — direct actions (#494)', () => { expect(dash.label).toBe(UNTITLED_DASHBOARD); expect(panel.label).toBe(UNTITLED_PANEL); expect(dash.actions.map((a) => a.label)).toEqual([ - 'Edit dashboard ' + UNTITLED_DASHBOARD, 'Delete dashboard ' + UNTITLED_DASHBOARD, + 'Add panel to ' + UNTITLED_DASHBOARD, + 'Edit dashboard ' + UNTITLED_DASHBOARD, + 'Delete dashboard ' + UNTITLED_DASHBOARD, ]); expect(panel.actions.map((a) => a.label)).toEqual([ 'Edit ' + UNTITLED_PANEL, 'Remove ' + UNTITLED_PANEL + ' from dashboard', @@ -950,7 +975,8 @@ describe('deriveDashboardTree — direct actions (#494)', () => { const dash0 = row(tree.rows, 'w1:d:dup:0'); const dash1 = row(tree.rows, 'w1:d:dup:1'); for (const dash of [dash0, dash1]) { - expect(dash.actions.map((a) => a.kind)).toEqual(['edit-dashboard', 'delete-dashboard']); + expect(dash.actions.map((a) => a.kind)) + .toEqual(['add-panel', 'edit-dashboard', 'delete-dashboard']); for (const a of dash.actions) { expect(a.target).toBeNull(); expect(a.confirm).toBeNull(); diff --git a/tests/unit/dashboard-tree.test.ts b/tests/unit/dashboard-tree.test.ts index 9c222da6..6514a1d0 100644 --- a/tests/unit/dashboard-tree.test.ts +++ b/tests/unit/dashboard-tree.test.ts @@ -708,10 +708,10 @@ describe('renderDashboardTree — the disclosure control (#472)', () => { // The chevron and every trailing control keep their own, distinct names // (`openAll` expanded this row, so its verb is Collapse). expect(chevron(list, 'w1:sales').getAttribute('aria-label')).toBe('Collapse Sales'); - expect(actionNames(list, 'w1:sales')).toEqual(['Edit dashboard Sales', 'Delete dashboard Sales']); - // …and none of those four names leaks into the row's own (#494 adds three - // more labelled buttons per row than #472 measured this against). - for (const label of ['Edit dashboard', 'Delete dashboard', 'Collapse']) { + expect(actionNames(list, 'w1:sales')) + .toEqual(['Add panel to Sales', 'Edit dashboard Sales', 'Delete dashboard Sales']); + // …and none of those names leaks into the row's own. + for (const label of ['Add panel', 'Edit dashboard', 'Delete dashboard', 'Collapse']) { expect(name('w1:sales')).not.toContain(label); } expect(name('w1:sales:tile:t1')).toBe('Revenue'); @@ -727,9 +727,9 @@ describe('renderDashboardTree — the disclosure control (#472)', () => { vi.advanceTimersByTime(400); expect(readTreeUi(app.state.dashboardTreeUi, 'w1').expandedDashboardIds.size).toBe(0); expect(app.openDashboard).not.toHaveBeenCalled(); - // #472's three targets became four (#494): chevron, row, pencil, trash. + // Chevron, row, plus, pencil, trash are independent targets. expect(row.querySelectorAll('.dash-tree-chev')).toHaveLength(1); - expect(row.querySelectorAll('.dash-tree-act')).toHaveLength(2); + expect(row.querySelectorAll('.dash-tree-act')).toHaveLength(3); vi.useRealTimers(); }); }); @@ -759,10 +759,11 @@ describe('renderDashboardTree — direct row actions (#494)', () => { expect(list.querySelectorAll('[aria-label^="Actions for"]')).toHaveLength(0); }); - it('gives the Dashboard row a pencil and a trash, in that order', () => { + it('gives the Dashboard row plus, pencil and trash, in that order', () => { const { list } = open(); // Destructive rightmost — never where the pointer lands by habit. - expect(actionNames(list, 'w1:sales')).toEqual(['Edit dashboard Sales', 'Delete dashboard Sales']); + expect(actionNames(list, 'w1:sales')) + .toEqual(['Add panel to Sales', 'Edit dashboard Sales', 'Delete dashboard Sales']); }); it('gives a Panel row a pencil and a trash that name the panel', () => { @@ -798,6 +799,8 @@ describe('renderDashboardTree — direct row actions (#494)', () => { it('marks the destructive one so it can be styled apart from the pencil', () => { const { list } = open(); + expect(actionBtn(list, 'w1:sales', 'Add panel to Sales')!.classList.contains('dash-tree-act-danger')) + .toBe(false); expect(actionBtn(list, 'w1:sales', 'Edit dashboard Sales')!.classList.contains('dash-tree-act-danger')) .toBe(false); expect(actionBtn(list, 'w1:sales', 'Delete dashboard Sales')!.classList.contains('dash-tree-act-danger')) @@ -813,9 +816,11 @@ describe('renderDashboardTree — direct row actions (#494)', () => { expect(trash.getAttribute('aria-haspopup')).toBe('menu'); }); - it('paints the pencil glyph on edit and the trash glyph on delete', () => { + it('paints the plus, pencil and trash glyphs on their actions', () => { const { list } = open(); // Swapping the two icons would otherwise pass every other assertion here. + expect(actionBtn(list, 'w1:sales', 'Add panel to Sales')!.innerHTML) + .toBe(Icon.plus().outerHTML); expect(actionBtn(list, 'w1:sales', 'Edit dashboard Sales')!.innerHTML) .toBe(Icon.pencil().outerHTML); expect(actionBtn(list, 'w1:sales', 'Delete dashboard Sales')!.innerHTML) @@ -1239,6 +1244,200 @@ describe('renderDashboardTree — panel metadata pencil (#494)', () => { }); }); +describe('renderDashboardTree — Add panel (#515)', () => { + const storedWorkspace = (): StoredWorkspaceV5 => ({ + storageVersion: 5, id: 'w1', key: 'w1', name: 'Workspace', + queries: [query('q1', 'Revenue')], + dashboards: [{ + documentVersion: 2, id: 'sales', title: 'Sales', revision: 4, + layout: { type: 'flow', version: 1, preset: 'report', items: {} }, + tiles: [{ id: 't1', queryId: 'q1' }], + }], + }); + const open = (over: Partial<DashboardTreeApp> = {}) => { + let id = 0; + const fixture = treeApp({ + currentWorkspace: storedWorkspace(), + genId: vi.fn(() => 'new-' + ++id), + ...over, + }); + renderDashboardTree(fixture.app); + return fixture; + }; + const plus = (list: HTMLElement): HTMLButtonElement => + actionBtn(list, 'w1:sales', 'Add panel to Sales')!; + const nameInput = (): HTMLInputElement => + document.querySelector<HTMLInputElement>('#panel-create-name')!; + const descInput = (): HTMLTextAreaElement => + document.querySelector<HTMLTextAreaElement>('#panel-create-description')!; + const add = (): HTMLButtonElement => + document.querySelector<HTMLButtonElement>('.fm-dialog-confirm')!; + const settle = (): Promise<void> => new Promise((resolve) => { setTimeout(resolve, 0); }); + + it('renders a real dialog button immediately before the pencil', () => { + const { list } = open(); + expect(actionNames(list, 'w1:sales')) + .toEqual(['Add panel to Sales', 'Edit dashboard Sales', 'Delete dashboard Sales']); + expect(plus(list).getAttribute('aria-haspopup')).toBe('dialog'); + expect(plus(list).getAttribute('data-act')).toBe('add-panel'); + expect(plus(list).getAttribute('title')).toBe('Add panel'); + expect(plus(list).innerHTML).toBe(Icon.plus().outerHTML); + }); + + it('opens the shared two-field dialog with Add disabled until name is nonblank', () => { + const { list } = open(); + click(plus(list)); + expect(document.querySelector('.fm-dialog-title')!.textContent).toBe('Add panel'); + expect(document.querySelector('label[for="panel-create-name"]')!.textContent).toBe('Panel name'); + expect(document.querySelector('label[for="panel-create-description"]')!.textContent) + .toBe('Panel description'); + expect(nameInput().value).toBe(''); + expect(descInput().value).toBe(''); + expect(add().textContent).toBe('Add'); + expect(add().disabled).toBe(true); + }); + + it('Cancel changes neither the workspace nor tabs', async () => { + const { app, list, committed } = open(); + const beforeWorkspace = structuredClone(app.currentWorkspace); + const beforeTabs = structuredClone(app.state.tabs.value); + click(plus(list)); + nameInput().value = 'Discarded'; + nameInput().dispatchEvent(new Event('input', { bubbles: true })); + click(document.querySelector<HTMLButtonElement>('.fm-dialog-cancel')!); + await settle(); + expect(committed).toEqual([]); + expect(app.currentWorkspace).toEqual(beforeWorkspace); + expect(app.state.tabs.value).toEqual(beforeTabs); + expect(app.openSavedQuery).not.toHaveBeenCalled(); + }); + + it('commits, closes, reveals the new row, then opens its linked query', async () => { + const prematureRefresh = vi.fn(() => { + throw new Error('Panel creation must settle before a Dashboard refresh'); + }); + const { app, list, committed } = open({ + onWorkspaceExternallyChanged: prematureRefresh, + mainSurface: { + kind: 'dashboard', dashboardId: 'sales', mode: 'view', + currentMember: null, pendingFocus: null, pendingScrollTop: null, + }, + }); + const openSavedQuery = vi.mocked(app.openSavedQuery); + openSavedQuery.mockImplementation(() => { + expect(document.querySelector('.fm-dialog-card')).toBeNull(); + expect(document.activeElement?.getAttribute('data-key')).toBe('w1:sales:tile:new-2'); + }); + click(plus(list)); + nameInput().value = ' New panel '; + nameInput().dispatchEvent(new Event('input', { bubbles: true })); + descInput().value = ' Description '; + add().click(); + await settle(); + + expect(committed).toHaveLength(1); + expect(committed[0]!.queries.at(-1)).toMatchObject({ + id: 'new-1', sql: '', spec: { + name: 'New panel', description: 'Description', dashboard: { role: 'panel' }, + }, + }); + expect(committed[0]!.dashboards[0].tiles.at(-1)).toEqual({ + id: 'new-2', queryId: 'new-1', + }); + expect(app.state.upperRole.value).toBe('dashboards'); + expect(readTreeUi(app.state.dashboardTreeUi, 'w1').keyboardRowKey) + .toBe('w1:sales:tile:new-2'); + expect(openSavedQuery).toHaveBeenCalledExactlyOnceWith('new-1'); + expect(prematureRefresh).not.toHaveBeenCalled(); + }); + + it('keeps every dismiss path locked while Add is committing', async () => { + let release = (_outcome: Awaited<ReturnType<App['mutateWorkspace']>>): void => {}; + const mutateWorkspace = vi.fn(() => new Promise<Awaited<ReturnType<App['mutateWorkspace']>>>( + (resolve) => { release = resolve; }, + )) as App['mutateWorkspace']; + const { app, list } = open({ mutateWorkspace }); + click(plus(list)); + nameInput().value = 'Pending panel'; + nameInput().dispatchEvent(new Event('input', { bubbles: true })); + add().click(); + + const cancel = document.querySelector<HTMLButtonElement>('.fm-dialog-cancel')!; + const backdrop = document.querySelector<HTMLElement>('.fm-dialog-backdrop')!; + expect(cancel.disabled).toBe(true); + document.dispatchEvent(new KeyboardEvent('keydown', { + key: 'Escape', bubbles: true, cancelable: true, + })); + click(backdrop); + expect(document.querySelector('.fm-dialog-card')).not.toBeNull(); + + release({ + ok: false, + diagnostics: [{ path: [], code: 'storage', message: 'Storage is full', severity: 'error' }], + }); + await settle(); + expect(cancel.disabled).toBe(false); + expect(document.querySelector('.fm-dialog-error')!.textContent).toBe('Storage is full'); + expect(app.openSavedQuery).not.toHaveBeenCalled(); + }); + + it('keeps values and reports a stale target without navigating', async () => { + const latest = storedWorkspace(); + const mutateWorkspace = (async (transform) => { + latest.dashboards = []; + const transformed = await transform(latest); + return { ok: false, aborted: true, data: transformed?.data }; + }) as App['mutateWorkspace']; + const { app, list } = open({ mutateWorkspace }); + click(plus(list)); + nameInput().value = 'Kept'; + nameInput().dispatchEvent(new Event('input', { bubbles: true })); + descInput().value = 'Still here'; + add().click(); + await settle(); + + expect(document.querySelector('.fm-dialog-card')).not.toBeNull(); + expect(nameInput().value).toBe('Kept'); + expect(descInput().value).toBe('Still here'); + expect(document.querySelector('.fm-dialog-error')!.textContent) + .toBe('That dashboard is no longer part of this workspace.'); + expect(app.openSavedQuery).not.toHaveBeenCalled(); + }); + + it('silently dismisses a route-stale durable commit without success navigation', async () => { + const onWorkspaceExternallyChanged = vi.fn(); + const mutateWorkspace = (async (transform) => { + const transformed = await transform(storedWorkspace()); + return { ok: false, aborted: true, data: transformed!.data }; + }) as App['mutateWorkspace']; + const { app, list } = open({ mutateWorkspace, onWorkspaceExternallyChanged }); + click(plus(list)); + nameInput().value = 'Committed elsewhere'; + nameInput().dispatchEvent(new Event('input', { bubbles: true })); + add().click(); + await settle(); + + expect(document.querySelector('.fm-dialog-card')).toBeNull(); + expect(app.openSavedQuery).not.toHaveBeenCalled(); + expect(onWorkspaceExternallyChanged).not.toHaveBeenCalled(); + }); + + it('renders the plus unavailable with its reason at the tile limit', () => { + const full = storedWorkspace(); + full.dashboards[0].tiles = Array.from({ length: 100 }, (_, i) => ({ + id: 't' + i, queryId: 'q' + i, + })); + const fixture = treeApp({ currentWorkspace: full }); + renderDashboardTree(fixture.app); + const button = plus(fixture.list); + expect(button.getAttribute('aria-disabled')).toBe('true'); + expect(button.getAttribute('title')) + .toBe('This dashboard already has the maximum of 100 panels, so another panel cannot be added.'); + click(button); + expect(document.querySelector('.fm-dialog-card')).toBeNull(); + }); +}); + /** #494 — removing a whole Dashboard, with the queries its panels own. */ describe('renderDashboardTree — Dashboard trash (#494)', () => { const confirmItems = (): HTMLElement[] => diff --git a/tests/unit/dialog-shell.test.ts b/tests/unit/dialog-shell.test.ts index 89d14ccc..52d6dc96 100644 --- a/tests/unit/dialog-shell.test.ts +++ b/tests/unit/dialog-shell.test.ts @@ -10,7 +10,9 @@ import { describe, it, expect, vi, afterEach } from 'vitest'; import { closeOpenDialogShell, openDialogShell, openMetadataDialog, openNameDialog, } from '../../src/ui/dialog-shell.js'; -import type { DialogHandle } from '../../src/ui/dialog-shell.js'; +import type { + DialogHandle, MetadataDialogConfirmResult, +} from '../../src/ui/dialog-shell.js'; import { h } from '../../src/ui/dom.js'; import { makeApp } from '../helpers/fake-app.js'; @@ -222,7 +224,9 @@ describe('openMetadataDialog', () => { const settle = (): Promise<void> => new Promise((resolve) => { setTimeout(resolve, 0); }); const open = ( - onConfirm: (values: { name: string; description: string }) => Promise<string | null>, + onConfirm: ( + values: { name: string; description: string }, + ) => Promise<MetadataDialogConfirmResult>, over: Partial<Parameters<typeof openMetadataDialog>[1]> = {}, ) => track(openMetadataDialog(makeApp(), { title: 'Edit panel', nameLabel: 'Name', descriptionLabel: 'Description', @@ -254,6 +258,24 @@ describe('openMetadataDialog', () => { expect(backdropOf()).toBeNull(); }); + it('runs success settlement only after close teardown and return-focus', async () => { + const trigger = h('button', {}, 'Plus') as HTMLButtonElement; + document.body.appendChild(trigger); + const order: string[] = []; + trigger.addEventListener('focus', () => order.push('focus')); + open(async () => null, { + returnFocusTo: trigger, + onClose: () => order.push('close'), + onSuccessAfterClose: () => { + order.push('success'); + expect(backdropOf()).toBeNull(); + }, + }); + save().click(); + await settle(); + expect(order).toEqual(['focus', 'close', 'success']); + }); + it('Enter in the name field commits; Enter in the description does not', async () => { const onConfirm = vi.fn(async () => null); open(onConfirm); @@ -277,7 +299,8 @@ describe('openMetadataDialog', () => { }); it('keeps the card, the typed values and the controls when the commit reports a failure', async () => { - open(async () => 'That dashboard is no longer part of this workspace.'); + const onSuccessAfterClose = vi.fn(); + open(async () => 'That dashboard is no longer part of this workspace.', { onSuccessAfterClose }); nameInput().value = 'Kept'; nameInput().dispatchEvent(new Event('input', { bubbles: true })); save().click(); @@ -290,6 +313,7 @@ describe('openMetadataDialog', () => { expect(save().disabled).toBe(false); expect(cancelBtn().disabled).toBe(false); expect(document.activeElement).toBe(nameInput()); + expect(onSuccessAfterClose).not.toHaveBeenCalled(); }); it('bars a SECOND submit while the first is in flight, but leaves the way out open', async () => { @@ -309,6 +333,60 @@ describe('openMetadataDialog', () => { expect(backdropOf()).toBeNull(); }); + it('can lock Cancel, Escape and backdrop dismissal until a creation settles', async () => { + let release = (): void => {}; + const onSuccessAfterClose = vi.fn(); + open(() => new Promise<string | null>((resolve) => { release = () => resolve(null); }), { + lockCloseWhileConfirming: true, + onSuccessAfterClose, + }); + save().click(); + expect(cancelBtn().disabled).toBe(true); + + key(document, 'Escape'); + expect(backdropOf()).not.toBeNull(); + mousedown(backdropOf()!); + click(backdropOf()!); + expect(backdropOf()).not.toBeNull(); + + release(); + await settle(); + expect(backdropOf()).toBeNull(); + expect(onSuccessAfterClose).toHaveBeenCalledTimes(1); + }); + + it('unlocks a creation after a rejected commit and shows a dismissible diagnostic', async () => { + const openRejected = () => open(async () => { + throw new Error('IndexedDB transaction aborted'); + }, { lockCloseWhileConfirming: true }); + + openRejected(); + nameInput().value = 'Kept after rejection'; + save().click(); + await settle(); + expect(nameInput().value).toBe('Kept after rejection'); + expect(cancelBtn().disabled).toBe(false); + expect(errorEl().hidden).toBe(false); + expect(errorEl().textContent).toBe('Could not save changes. Please try again.'); + key(document, 'Escape'); + expect(backdropOf()).toBeNull(); + + openRejected(); + save().click(); + await settle(); + cancelBtn().click(); + expect(backdropOf()).toBeNull(); + }); + + it('dismisses a stale settlement without running success navigation', async () => { + const onSuccessAfterClose = vi.fn(); + open(async () => undefined, { onSuccessAfterClose }); + save().click(); + await settle(); + expect(backdropOf()).toBeNull(); + expect(onSuccessAfterClose).not.toHaveBeenCalled(); + }); + it('a Cancel taken mid-write closes, and the late answer is dropped', async () => { let release = (message: string | null): void => { void message; }; open(() => new Promise<string | null>((resolve) => { release = resolve; })); @@ -333,6 +411,18 @@ describe('openMetadataDialog', () => { expect(backdropOf()).toBeNull(); }); + it('drops a late rejection for a dialog that was force-closed while the write was queued', async () => { + let reject = (_reason: unknown): void => {}; + open(() => new Promise<string | null>((_resolve, rejectPromise) => { reject = rejectPromise; }), { + lockCloseWhileConfirming: true, + }); + save().click(); + closeOpenDialogShell(); + reject(new Error('Store unavailable')); + await settle(); + expect(backdropOf()).toBeNull(); + }); + it('runs the caller\'s onClose and restores focus to the trigger', () => { const trigger = h('button', {}, 'Pencil') as HTMLButtonElement; document.body.appendChild(trigger); @@ -342,6 +432,13 @@ describe('openMetadataDialog', () => { expect(onClose).toHaveBeenCalledTimes(1); expect(document.activeElement).toBe(trigger); }); + + it('does not run success settlement for Cancel', () => { + const onSuccessAfterClose = vi.fn(); + open(async () => null, { onSuccessAfterClose }); + cancelBtn().click(); + expect(onSuccessAfterClose).not.toHaveBeenCalled(); + }); }); describe('openDialogShell — Tab trap', () => { diff --git a/tests/unit/panel-creation-service.test.ts b/tests/unit/panel-creation-service.test.ts new file mode 100644 index 00000000..52ab2943 --- /dev/null +++ b/tests/unit/panel-creation-service.test.ts @@ -0,0 +1,111 @@ +import { describe, expect, it, vi } from 'vitest'; +import { + createDashboardPanel, panelCreationMessage, +} from '../../src/application/panel-creation-service.js'; +import type { MutateWorkspace } from '../../src/state.js'; +import type { DashboardDocumentV2, StoredWorkspaceV5 } from '../../src/generated/json-schema.types.js'; + +const dash = (id: string): DashboardDocumentV2 => ({ + documentVersion: 2, id, title: id, revision: 1, + layout: { type: 'flow', version: 1, preset: 'report', items: {} }, + tiles: [], +}); +const ws = (): StoredWorkspaceV5 => ({ + storageVersion: 5, id: 'w', key: 'w', name: 'W', queries: [], dashboards: [dash('d1')], +}); + +const deps = ( + latest: StoredWorkspaceV5 | null, + beforeTransform?: () => void, +) => { + let current = latest; + const candidates: (StoredWorkspaceV5 | null)[] = []; + const mutateWorkspace = (async (transform) => { + beforeTransform?.(); + const transformed = await transform(current); + const candidate = transformed?.candidate ?? null; + candidates.push(candidate); + if (candidate === null) { + return { ok: false, aborted: true, data: transformed?.data }; + } + current = candidate; + return { + ok: true, workspace: candidate, dashboardRevision: null, data: transformed!.data, + }; + }) as MutateWorkspace; + let seq = 0; + const genId = vi.fn(() => 'id-' + ++seq); + return { mutateWorkspace, genId, candidates }; +}; + +describe('createDashboardPanel', () => { + it('mints both ids once, commits, and reports them without route-local settlement', async () => { + const app = deps(ws()); + const outcome = await createDashboardPanel(app, 'd1', 'Panel', 'Description'); + + expect(app.genId).toHaveBeenCalledTimes(2); + expect(outcome).toMatchObject({ + ok: true, data: { status: 'ok', queryId: 'id-1', tileId: 'id-2' }, + }); + if (!outcome.ok) throw new Error('expected ok'); + }); + + it('re-reads the target at dequeue time and leaves no orphan on a stale target', async () => { + const latest = ws(); + const app = deps(latest, () => { latest.dashboards = []; }); + const outcome = await createDashboardPanel(app, 'd1', 'Panel', ''); + + expect(outcome).toMatchObject({ + ok: false, aborted: true, + data: { status: 'declined', reason: 'dashboard-missing' }, + }); + expect(app.candidates).toEqual([null]); + }); + + it('declines when no workspace is committed', async () => { + const outcome = await createDashboardPanel(deps(null), 'd1', 'Panel', ''); + expect(outcome).toMatchObject({ + data: { status: 'declined', reason: 'dashboard-missing' }, + }); + }); +}); + +describe('panelCreationMessage', () => { + it('returns null on success and maps every declined reason', () => { + const success = { + ok: true, workspace: ws(), dashboardRevision: null, + data: { status: 'ok', queryId: 'q', tileId: 't' }, + } as const; + expect(panelCreationMessage(success)).toBeNull(); + + const reasons = { + 'dashboard-missing': 'That dashboard is no longer part of this workspace.', + 'dashboard-ambiguous': 'That dashboard is ambiguous and cannot be changed.', + 'tile-limit': 'That dashboard already has the maximum of 100 panels.', + 'id-collision': 'Could not create this panel because an id already exists. Please try again.', + 'blank-name': 'Enter a panel name.', + } as const; + for (const [reason, message] of Object.entries(reasons)) { + expect(panelCreationMessage({ + ok: false, aborted: true, + data: { status: 'declined', reason }, + } as never)).toBe(message); + } + }); + + it('surfaces validation/persistence diagnostics and silently dismisses stale aborts', () => { + expect(panelCreationMessage({ + ok: false, aborted: false, diagnostics: [{ message: 'Rejected value' }], + } as never)).toBe('Rejected value'); + expect(panelCreationMessage({ + ok: false, aborted: false, diagnostics: [], + } as never)).toBe('Could not save this panel.'); + expect(panelCreationMessage({ + ok: false, aborted: true, + } as never)).toBeUndefined(); + expect(panelCreationMessage({ + ok: false, aborted: true, + data: { status: 'ok', queryId: 'q', tileId: 't' }, + } as never)).toBeUndefined(); + }); +}); diff --git a/tests/unit/panel-creation.test.ts b/tests/unit/panel-creation.test.ts new file mode 100644 index 00000000..0be90a31 --- /dev/null +++ b/tests/unit/panel-creation.test.ts @@ -0,0 +1,126 @@ +import { describe, expect, it } from 'vitest'; +import { createPanelCandidate } from '../../src/dashboard/application/panel-creation.js'; +import { libraryQueries } from '../../src/dashboard/model/query-ownership.js'; +import { PORTABLE_LIMITS } from '../../src/dashboard/model/portable-limits.js'; +import { savedQuery } from '../helpers/saved-query.js'; +import type { DashboardDocumentV2, StoredWorkspaceV5 } from '../../src/generated/json-schema.types.js'; + +const dash = (id: string, over: Partial<DashboardDocumentV2> = {}): DashboardDocumentV2 => ({ + documentVersion: 2, id, title: id.toUpperCase(), revision: 1, + layout: { type: 'flow', version: 1, preset: 'report', items: {} }, + tiles: [], ...over, +}); + +const ws = (over: Partial<StoredWorkspaceV5> = {}): StoredWorkspaceV5 => ({ + storageVersion: 5, id: 'w', key: 'workspace', name: 'Workspace', + queries: [], dashboards: [dash('d1')], ...over, +}); + +const input = { + dashboardId: 'd1', queryId: 'q-new', tileId: 't-new', + name: ' Revenue ', description: ' Daily revenue ', +}; + +describe('createPanelCandidate', () => { + it('atomically appends one blank panel-role query and one identity-only tile', () => { + const result = createPanelCandidate({ latest: ws(), ...input }); + if (!result.ok) throw new Error(result.reason); + + expect(result.data).toEqual({ queryId: 'q-new', tileId: 't-new' }); + expect(result.workspace.queries).toEqual([{ + id: 'q-new', + sql: '', + specVersion: 1, + spec: { + name: 'Revenue', + description: 'Daily revenue', + dashboard: { role: 'panel' }, + }, + }]); + expect(result.workspace.dashboards[0].tiles).toEqual([{ id: 't-new', queryId: 'q-new' }]); + expect(Object.keys(result.workspace.dashboards[0].tiles[0]).sort()).toEqual(['id', 'queryId']); + expect(result.workspace.queries[0].spec.favorite).toBeUndefined(); + }); + + it('omits an empty optional description and is Dashboard-owned immediately', () => { + const result = createPanelCandidate({ + latest: ws(), ...input, description: ' ', + }); + if (!result.ok) throw new Error(result.reason); + + expect(result.workspace.queries[0].spec.description).toBeUndefined(); + expect(libraryQueries(result.workspace)).toEqual([]); + }); + + it('uses the canonical add path for flow placement and normalization', () => { + const result = createPanelCandidate({ latest: ws(), ...input }); + if (!result.ok) throw new Error(result.reason); + expect(result.workspace.dashboards[0].layout.items).toEqual({}); + }); + + it('uses the canonical add path for grid placement and flow-fallback regeneration', () => { + const latest = ws({ + dashboards: [dash('d1', { + layout: { + type: 'grafana-grid', version: 1, items: {}, + fallback: { type: 'flow', version: 1, preset: 'report', items: {} }, + }, + })], + }); + const result = createPanelCandidate({ latest, ...input }); + if (!result.ok) throw new Error(result.reason); + const layout = result.workspace.dashboards[0].layout as { + items: Record<string, unknown>; + fallback: { items: Record<string, unknown> }; + }; + expect(layout.items['t-new']).toBeDefined(); + expect(layout.fallback.items['t-new']).toBeDefined(); + }); + + it('increments only the target revision and preserves unrelated content', () => { + const existing = savedQuery({ id: 'q-old', sql: 'SELECT 1', name: 'Existing', favorite: true }); + const other = dash('d2', { + revision: 9, + tiles: [{ id: 't-old', queryId: 'q-old' }], + variableConfigs: { region: { sql: 'SELECT region' } }, + }); + const latest = ws({ + queries: [existing], + dashboards: [dash('d1', { revision: 4 }), other], + }); + const result = createPanelCandidate({ latest, ...input }); + if (!result.ok) throw new Error(result.reason); + + expect(result.workspace.dashboards[0].revision).toBe(5); + expect(result.workspace.dashboards[1]).toBe(other); + expect(result.workspace.queries[0]).toBe(existing); + expect(result.workspace.dashboards[1]).toEqual(other); + expect(result.workspace.queries[0]).toEqual(existing); + }); + + it.each([ + ['missing Dashboard', ws({ dashboards: [] }), 'dashboard-missing'], + ['ambiguous Dashboard', ws({ dashboards: [dash('d1'), dash('d1')] }), 'dashboard-ambiguous'], + ['query-id collision', ws({ queries: [savedQuery({ id: 'q-new' })] }), 'id-collision'], + ['tile-id collision', ws({ dashboards: [dash('d1', { tiles: [{ id: 't-new', queryId: 'old' }] })] }), 'id-collision'], + ] as const)('rejects a %s without mutating the input', (_label, latest, reason) => { + const before = structuredClone(latest); + expect(createPanelCandidate({ latest, ...input })).toEqual({ ok: false, reason }); + expect(latest).toEqual(before); + }); + + it('re-checks the 100-tile limit against the latest Dashboard', () => { + const tiles = Array.from({ length: PORTABLE_LIMITS.maxTilesPerDashboard }, (_, i) => ({ + id: 't' + i, queryId: 'q' + i, + })); + expect(createPanelCandidate({ + latest: ws({ dashboards: [dash('d1', { tiles })] }), ...input, + })).toEqual({ ok: false, reason: 'tile-limit' }); + }); + + it('rejects a blank trimmed name', () => { + expect(createPanelCandidate({ + latest: ws(), ...input, name: ' ', + })).toEqual({ ok: false, reason: 'blank-name' }); + }); +});