feat(#494): direct Dashboard/Panel row actions, replacing the ⋯ overflow menus (phase 4)

CHANGELOG.md

@@ -10,6 +10,86 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
 ## [Unreleased]
 
 ### Added
+- **Dashboard and Panel rows act directly, and the `⋯` overflow menus are gone**
+  (#494 / #429 phase 4). A Dashboard row now carries a pencil and a trash; a
+  Panel row carries a pencil that edits its dedicated query's name and
+  description, and a trash that removes the tile *and* that query in one atomic
+  commit. Both clusters follow the Library Query row's vocabulary: revealed on
+  hover and `:focus-within`, destructive action rightmost, each button
+  separately named for a screen reader (`Edit Revenue`, `Remove Revenue from
+  dashboard`), each keyboard-operable inside the row's single tab stop, and
+  each isolated from the row's own click/Shift-click/expand gestures.
+
+  Deleting anything **confirms first**, naming both the panel and the Dashboard
+  and saying that the dedicated query copy goes too — with keyboard focus on
+  Cancel, through the same `autofocus` row #501 added for the orphaned-variable
+  confirm. A panel delete removes the
+  tile, every layout placement (including the grafana-grid flow fallback) and
+  exactly its owned query, bumps that Dashboard's revision once, and moves
+  keyboard focus to the next panel row — then the previous, then the Panels
+  group. An orphaned variable configuration deliberately survives.
+
+  Edit and delete are only offered when ownership can be **proven**: the tile's
+  query must exist and be owned by that one tile. A query shared between panels,
+  or a reference to a query the workspace no longer carries, leaves both
+  controls rendered but `aria-disabled` with the reason in their tooltip —
+  nothing is ever guessed or cascade-deleted. Every dialog and confirmation
+  re-resolves its target inside the write queue, so one that went stale while it
+  was open commits nothing and says so.
+
+  Every one of those operations re-proves the **exact identity** the user
+  confirmed, inside the write queue: the Dashboard, the tile, the query the
+  confirmation named, that the tile still references it, that exactly one
+  document carries that id, and that it is a panel-role query. A tile
+  re-pointed while the confirmation was open, an ambiguous id, or a reference
+  to a non-panel query all refuse and say why, rather than deleting whatever
+  the tile happens to point at by then.
+
+  A fifth review pass found the Dashboard/tile identity check was looser than
+  delete's own: both the tree's availability rule and the metadata guard asked
+  "is there at least one match" rather than "is there exactly one", so two
+  Dashboard documents sharing an id — or two tiles of the *same* Dashboard
+  sharing an id, even when they reference different queries — could still
+  offer a pencil that delete already refused. Both now count Dashboard and
+  Dashboard-local tile ids the same pass counts query ids, and refuse
+  identically. The pencil's own rapid-reactivation race is fixed too:
+  replacing an already-open dialog force-closes it first, and that closing
+  dialog's `onClose` used to reset the *same* trigger's `aria-expanded` back to
+  `"false"` right after the new dialog had just set it `"true"` — stranding
+  the trigger effectively unfocusable for as long as the replacement stayed
+  open. And the viewer's tile-description override now trims before the
+  fallback, matching its title override (#476): a whitespace-only description
+  is schema-legal and was silently masking the query's own description with
+  nothing, since the tree's own warning note already trimmed and so never
+  fired for it.
+
+  A sixth pass found the fifth round's own fix incomplete: counting duplicated
+  ids made edit/delete agree with delete's stricter rule, but the row's
+  *presentation* identity — `row.key`, and the `data-key` every focus
+  restoration, drag highlight and the roving tabindex resolve by — still
+  collapsed two ambiguous rows onto one key. A duplicated Dashboard or tile id
+  could therefore put more than one row in the Tab order at once, and let
+  focus/highlight resolution silently pick whichever matching DOM node came
+  first. Malformed duplicates now get distinct presentation keys, and a
+  Panel row's own View/Edit Dashboard-focus navigation (addressed by
+  Dashboard id + tile id, unlike its still-available open-query gesture,
+  addressed by query id alone) is withheld under the same ambiguity — the
+  Dashboard viewer's own tile-focus lookup is keyed by tile id alone and
+  would otherwise resolve the *other* duplicate. Every drop target this
+  ambiguity could reach (the Dashboard row, its Panels group, and a Variable
+  row) is withheld too.
+
+  The same pass found the orphaned-variable trash still discarding its own
+  commit outcome (`void commitVariableConfig(...)`) — the one destructive
+  control on this row that predates this round's `reportRemoval` pattern for
+  the Dashboard and Panel trash. A concurrent Dashboard deletion, a Dashboard
+  that became a duplicate id, or a storage rejection all committed nothing
+  and said nothing: the confirmation just closed. It now awaits and reports
+  the outcome the same way, and the orphan-delete control is itself withheld
+  when the Dashboard id is already known to be ambiguous.
+
+  Not yet included: the **Open in Dashboard** focus button, which is held back
+  until #438 fixes tile focus on flow-layout KPI tiles.
 - **Closing a dirty tab, or leaving the page, now confirms first** (#466). The
   tab strip's close button asks before discarding an unsaved draft — a normal
   query tab or a Dashboard-variable's option SQL, saved-linked or never saved
@@ -29,11 +109,13 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
   after a save that clears a draft's dirty flags while navigating away from
   its surface mid-write, rather than only on the next unrelated tab repaint.
 - **A Dashboard row's pencil edits its title and description** (#429 phase 3).
-  Revealed on hover/focus-within next to the existing `⋯`, it opens a small
-  dialog prefilled from the Dashboard's own committed document — Cancel and
-  Escape commit nothing, a blank title disables Save, and a real commit bumps
-  only that Dashboard's revision and refreshes the tree and any open Dashboard
-  surface. There is still no Dashboard **trash** action.
+  It opens a small dialog prefilled from the Dashboard's own committed
+  document — Cancel and Escape commit nothing, a blank title disables Save, and
+  a real commit bumps only that Dashboard's revision and refreshes the tree and
+  any open Dashboard surface. (Phase 3 put this pencil beside the row's `⋯`
+  menu and shipped no Dashboard trash; #494 above then removed that menu and
+  added the trash, so what ships in this release is the pencil as part of the
+  direct-action cluster.)
 - **Drag a Library query onto a Dashboard to assign it** (#428). One drag from the
   lower Library list now has three destination-dependent meanings, and the row
   publishes two independent payloads to serve them:
@@ -133,6 +215,28 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
   no row appears, disappears or moves when the work surface changes.
 
 ### Fixed
+- **Enter on a Dashboard-tree action button ran the row's command instead of the
+  button's** (#495 review). The tree's keyboard handler lives on the list and its
+  Enter arm runs the focused row's action, so Enter on the rename pencil opened
+  the Dashboard — and could swallow the button's own activation on the way. Every
+  nested control now stops Enter/Space from propagating (without preventing the
+  default, so native activation still fires exactly once), and the tree handler
+  independently ignores an Enter that originated on a button. Arrow keys, Home and
+  End still walk the tree from a focused control.
+- **A failed Dashboard rename closed its dialog and discarded the edit** (#495
+  review). The dialog now waits for the write: a Dashboard deleted in another tab,
+  a duplicate id, a validation rejection or a storage failure keeps the card open
+  with the typed text intact and shows one targeted message inside it. Both
+  buttons are disabled while a write is in flight, so the same rename cannot be
+  submitted twice.
+- **Creating a Dashboard from the empty-workspace placeholder said nothing when
+  the commit was rejected** (#495 review / #481). Both entry points — File ▸ New
+  dashboard… and the placeholder — now run one creation command and report
+  identically; each keeps its own "where to go afterwards" policy.
+- **Modal dialogs were invisible to assistive technology** (#495 review). The
+  shared dialog shell now marks its card `role="dialog"` with `aria-modal="true"`
+  and an `aria-labelledby` pointing at its own heading, so a screen reader is told
+  a modal opened and which one.
 - **A Dashboard variable's Run action validates its option SQL again** (#465,
   follow-up to #457). #457 moved option SQL into a `dashboard-variable` main-editor
   tab and deleted the drawer's **Test** action along with it, but nothing replaced

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<string>;
+
+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<DashboardCreateOutcome> {
+  const created = createEmptyDashboard(deps.genId(), name);
+  return deps.mutateWorkspace<string>((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');
+}

src/application/dashboard-delete.ts

@@ -0,0 +1,126 @@
+// The `mutateWorkspace` plumbing around #494's two atomic deletes.
+//
+// Split along the same boundary every other Dashboard write in this repo
+// holds: the pure transforms are `dashboard/application/dashboard-removal.ts`
+// (beside the tile/layout code they compose), and the async, serialized,
+// read-latest-at-dequeue commit is here — modeled on
+// `dashboard-variable-config.ts` and `dashboard-title.ts`, which is what makes
+// three different destructive tree controls behave identically under
+// concurrency.
+//
+// The transform runs INSIDE the queue, so the target is re-resolved and its
+// ownership re-proven against committed truth rather than against whatever the
+// tree was painting when the confirmation opened. A row can be several seconds
+// stale by the time a user reads a confirmation and answers it.
+//
+// Typed against structural deps rather than the `App` contract:
+// `src/application/**` must never import `src/ui/**`
+// (build/check-boundaries.mjs).
+
+import {
+  removeDashboardDocument, removeDashboardPanel,
+} from '../dashboard/application/dashboard-removal.js';
+import type { PanelRemovalRefusal } from '../dashboard/application/dashboard-removal.js';
+import type {
+  MutateWorkspace, WorkspaceExternallyChangedInfo, WorkspaceMutationOutcome,
+} from '../state.js';
+
+/** Why the transform declined, threaded back through `mutateWorkspace`'s
+ *  `data` channel. `no-workspace` is the primitive's own "nothing is loaded"
+ *  case; every other value is the pure transform's refusal, verbatim. */
+export type DashboardDeleteAbort = PanelRemovalRefusal | 'dashboard-missing' | 'no-workspace';
+
+export type DashboardDeleteOutcome = WorkspaceMutationOutcome<DashboardDeleteAbort>;
+
+export interface DashboardDeleteDeps {
+  mutateWorkspace: MutateWorkspace;
+  /** Re-read committed truth: a rendered Dashboard holds its document from
+   *  construction, and a deleted panel (or Dashboard) must not stay on screen
+   *  after its removal commits. */
+  onWorkspaceExternallyChanged(info: WorkspaceExternallyChangedInfo): void;
+}
+
+/** One tile, addressed by the ids the row carried — never by label or index. */
+export interface PanelDeleteTarget {
+  dashboardId: string;
+  tileId: string;
+  /** The owned query the confirmation named. Carried all the way into the
+   *  transform so the delete can refuse a tile that was re-pointed at a
+   *  DIFFERENT query while the confirmation was open. */
+  queryId: string;
+}
+
+/**
+ * Remove one panel tile and its dedicated owned query, atomically.
+ *
+ * `queriesChanged: true` is a fact, not a default: this write deletes a query,
+ * so a rendered Dashboard has to rebuild its viewer session rather than merely
+ * re-read a document.
+ */
+export async function commitPanelRemoval(
+  deps: DashboardDeleteDeps, target: PanelDeleteTarget,
+): Promise<DashboardDeleteOutcome> {
+  const outcome = await deps.mutateWorkspace<DashboardDeleteAbort>((latest) => {
+    if (latest === null) return { candidate: null, data: 'no-workspace' };
+    const result = removeDashboardPanel({ workspace: latest, ...target });
+    return result.status === 'refused'
+      ? { candidate: null, data: result.reason }
+      : { candidate: result.workspace };
+  });
+  if (outcome.ok) {
+    deps.onWorkspaceExternallyChanged({ workspace: outcome.workspace, queriesChanged: true });
+  }
+  return outcome;
+}
+
+/** Remove one whole Dashboard document and the queries its panels own. */
+export async function commitDashboardRemoval(
+  deps: DashboardDeleteDeps, dashboardId: string,
+): Promise<DashboardDeleteOutcome> {
+  const outcome = await deps.mutateWorkspace<DashboardDeleteAbort>((latest) => {
+    if (latest === null) return { candidate: null, data: 'no-workspace' };
+    const result = removeDashboardDocument({ workspace: latest, dashboardId });
+    return result.status === 'refused'
+      ? { candidate: null, data: result.reason }
+      : { candidate: result.workspace };
+  });
+  if (outcome.ok) {
+    deps.onWorkspaceExternallyChanged({ workspace: outcome.workspace, queriesChanged: true });
+  }
+  return outcome;
+}
+
+/**
+ * What to tell the user about a delete — `null` when it succeeded and the
+ * disappearing row is its own report.
+ *
+ * Every refusal gets a sentence that says what the DATA did, because none of
+ * them is something the user did wrong and all of them mean "nothing was
+ * deleted". The missing-resource wording matches the one #429 phase 1 settled
+ * for `openSavedQuery`, so the tree speaks with one voice about resources that
+ * vanished under it.
+ */
+export function dashboardDeleteMessage(outcome: DashboardDeleteOutcome): string | null {
+  if (outcome.ok) return null;
+  if (!outcome.aborted) {
+    return '✕ ' + (outcome.diagnostics[0]?.message || 'Could not save workspace');
+  }
+  switch (outcome.data) {
+    case 'dashboard-missing':
+      return 'That dashboard is no longer part of this workspace.';
+    case 'tile-missing':
+      return 'That panel is no longer part of this dashboard.';
+    case 'ownership-unproven':
+      return 'This panel’s query is shared, missing or not a panel query, so nothing was deleted.';
+    case 'tile-retargeted':
+      return 'That panel now shows a different query, so nothing was deleted.';
+    case 'tile-duplicate':
+      return 'This workspace has two resources with the same id, so nothing was deleted.';
+    case 'dashboard-duplicate':
+      return 'This workspace has two dashboards with the same id, so nothing was deleted.';
+    // `no-workspace` — nothing is loaded, so nothing was lost and there is
+    // nothing the user can act on.
+    default:
+      return null;
+  }
+}