diff --git a/CHANGELOG.md b/CHANGELOG.md index bc159d3c..b82123bf 100644 --- a/CHANGELOG.md +++ b/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 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/application/dashboard-delete.ts b/src/application/dashboard-delete.ts new file mode 100644 index 00000000..66b06c59 --- /dev/null +++ b/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; + +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 { + const outcome = await deps.mutateWorkspace((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 { + const outcome = await deps.mutateWorkspace((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; + } +} diff --git a/src/application/dashboard-panel-metadata.ts b/src/application/dashboard-panel-metadata.ts new file mode 100644 index 00000000..b5c7bb4c --- /dev/null +++ b/src/application/dashboard-panel-metadata.ts @@ -0,0 +1,144 @@ +// Editing ONE panel's dedicated owned query metadata — name and description +// only (#494). +// +// A Dashboard panel does not own a title of its own for ordinary authoring: +// what the tile displays comes from the query it owns (`spec.name` / +// `spec.description`), and only an IMPORTED tile carries a local override. +// So the pencil on a Panel row edits the owned QUERY, through the very same +// `renameSaved` path the Library row's pencil uses — which is what keeps a +// linked Query tab in sync (clean tabs adopt the commit, dirty ones are +// conflict-flagged) instead of this growing a second, subtly different +// metadata writer. +// +// What this module adds on top is the part a Library rename does not need: +// PROOF, at dequeue time, that the query still belongs to the tile the dialog +// was opened for. A tree dialog can outlive the row it came from — another tab +// can delete the tile, or #428's assignment can re-point it — and the query id +// alone cannot say which member owns it. The `guard` runs inside the queued +// transform, so the answer comes from committed truth rather than from +// whatever the tree was painting when the pencil was clicked. +// +// Typed against a structural deps bag: `src/application/**` must never import +// `src/ui/**` (build/check-boundaries.mjs). + +import { renameSaved } from '../state.js'; +import type { AppState, MutateWorkspace, SpecValidationService } from '../state.js'; +import { ownersOfQuery } from '../dashboard/model/query-ownership.js'; +import { queryDashboardRole } from '../dashboard/model/workspace-semantics.js'; +import { findDashboardStrict } from '../workspace/workspace-dashboards.js'; +import type { SavedQueryV2, StoredWorkspaceV5 } from '../generated/json-schema.types.js'; + +/** Which owned query to edit, addressed the way #430 requires — Dashboard id + + * the member's own Dashboard-local id + the query id, never a title, a name + * or a collection position. All three are re-resolved at dequeue time. */ +export interface PanelMetadataTarget { + dashboardId: string; + tileId: string; + queryId: string; +} + +export type PanelMetadataOutcome = + | { status: 'ok'; entry: SavedQueryV2 } + /** The tile no longer owns this query (deleted, re-pointed, or now shared), + * or the query itself is gone. Nothing was committed, and no retry of the + * same edit can succeed. */ + | { status: 'stale' } + /** A linked tab is holding Spec JSON that does not parse/validate, so the + * same patch cannot be applied to both the entry and the draft. Nothing was + * committed; the tab is where this gets resolved. */ + | { status: 'invalid-draft' } + | { status: 'rejected'; message: string }; + +export interface PanelMetadataDeps { + state: AppState; + mutateWorkspace: MutateWorkspace; + specValidators: SpecValidationService; + /** + * Ask every rendered surface to re-read committed truth, so a renamed panel's + * tile heading and its tree row update without waiting for an unrelated + * repaint. Declared as a plain notification rather than as + * `onWorkspaceExternallyChanged` itself: `renameSaved` does not hand back the + * committed aggregate, and inventing a `workspace: null` for that hook would + * mean "the record was cleared", which is the opposite of what happened. + */ + refreshCommittedSurfaces(): void; +} + +/** + * The #427 exactly-one-owner rule, asked about ONE specific member. + * + * Exported because the tree model answers the same question when it decides + * whether to enable the pencil at all — the difference is only WHEN it is + * asked (paint time there, dequeue time here), and the two must agree. + * + * Dashboard and tile identity are resolved STRICTLY, the same way + * `removeDashboardPanel` resolves them, rather than by a loose `.some(...)` + * membership check: a duplicated Dashboard id or a duplicated Dashboard-local + * tile id refuses here even when the query-ownership index alone cannot see + * it. Two tiles sharing an id but referencing different queries would each + * otherwise still look, independently, like that query's sole owner — the + * `.some(...)` check never asked "how many", only "does at least one match". + */ +export function ownedByPanel( + workspace: StoredWorkspaceV5, target: PanelMetadataTarget, +): boolean { + const lookup = findDashboardStrict(workspace, target.dashboardId); + if (lookup.status !== 'ok') return false; + + const tiles = lookup.dashboard.tiles.filter((tile) => tile.id === target.tileId); + if (tiles.length !== 1 || tiles[0].queryId !== target.queryId) return false; + + // Exactly one document, and a PANEL one: an ambiguous id would make "which + // query am I editing" unanswerable, and a Setup- or other-role reference is + // malformed data this must not quietly write over. + const matching = workspace.queries.filter((query) => query.id === target.queryId); + if (matching.length !== 1 || queryDashboardRole(matching[0]) !== 'panel') return false; + + // The sole owner is never re-checked against `dashboardId`/`tileId` by hand + // — same as `removeDashboardPanel`'s own comment: whenever this reports + // exactly one owner for a query id this exact (now uniquely resolved) tile + // references, that owner IS this Dashboard and this tile. + return ownersOfQuery(workspace, target.queryId).length === 1; +} + +/** + * Commit a panel's owned-query name/description. + * + * `renameSaved` does the rest of the work and its rules are inherited + * deliberately: the name is trimmed and a blank one commits nothing, an empty + * description CLEARS the field rather than storing `''`, the SQL, the query + * id, `spec.dashboard.role`, panel configuration, variants, size hints, time + * ranges and unknown Spec fields are all carried through untouched, and no + * Dashboard document is read or written — so a metadata-only edit cannot + * increment a Dashboard revision. + * + * A tile-local imported `title`/`description` override is likewise untouched: + * this writes the QUERY, and the override keeps its established display + * precedence (the caller is responsible for saying so). + */ +export async function commitPanelQueryMetadata( + deps: PanelMetadataDeps, target: PanelMetadataTarget, name: string, description: string, +): Promise { + const result = await renameSaved( + deps.state, target.queryId, name, description, + deps.mutateWorkspace, deps.specValidators, + (latest) => ownedByPanel(latest, target), + ); + // `renameSaved` answers `undefined` for a blank name or a query missing from + // the PROJECTED collection — both mean "nothing was attempted". + if (result === undefined) return { status: 'stale' }; + if (result.ok) { + deps.refreshCommittedSurfaces(); + return { status: 'ok', entry: result.entry }; + } + // `deletedExternally` is deliberately NOT checked alongside this: it means + // the query was absent from `latest`, and `ownedByPanel` already requires it + // to be present — `patchSavedSpec` evaluates the guard BEFORE its own + // deleted-externally test, so a deleted query always refuses here first. + if (result.guardRefused) return { status: 'stale' }; + if (result.invalidTab !== null) return { status: 'invalid-draft' }; + return { + status: 'rejected', + message: result.diagnostics?.[0]?.message || 'Could not save this panel.', + }; +} diff --git a/src/application/dashboard-tree-model.ts b/src/application/dashboard-tree-model.ts index de136e39..f18a5869 100644 --- a/src/application/dashboard-tree-model.ts +++ b/src/application/dashboard-tree-model.ts @@ -30,6 +30,8 @@ import type { DashboardFocusTarget, MainSurfaceState, OpenDashboardRequest } fro import type { DashboardTreeGroup, DashboardTreeUiState } from '../core/dashboard-tree-ui-state.js'; 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 { queryDashboardRole } from '../dashboard/model/workspace-semantics.js'; import type { DashboardVariable } from '../core/dashboard-variables.js'; import type { LibraryDropTarget } from '../core/library-drag.js'; @@ -48,7 +50,7 @@ export interface TreeQuery { * Optional here for the same reason the collections are — a real * `SavedQueryV2` always carries it. */ sql?: string; - spec?: { name?: string; description?: string } | null; + spec?: { name?: string; description?: string; dashboard?: { role?: string } | null } | null; } export interface TreeTile { @@ -126,12 +128,60 @@ export type DashboardTreeCommand = * drawer; the command itself is unchanged, since the identity is the same. */ | { kind: 'open-variable'; dashboardId: string; name: string }; -export interface DashboardTreeMenuItem { +/** + * #494 — the trailing DIRECT controls a row offers, replacing the `⋯` overflow + * menu on Dashboard and Panel rows alike. + * + * A row's operations are stated here, fully resolved, for the same reason its + * commands are: `ui/dashboard-tree.ts` must not re-derive capability from row + * kind, DOM classes or — worst of all — ownership. Which panel may be edited + * or deleted is a data-integrity question about the whole workspace, and it is + * answered exactly once, here. + */ +export type DashboardTreeActionKind = + | 'edit-dashboard' + | 'delete-dashboard' + | 'edit-panel' + | 'delete-panel' + /** #447's orphaned-variable trash, now expressed as one of these rather + * than as its own `deletable` boolean. */ + | 'delete-variable-config'; + +/** What an action acts ON, by stable ids only — never a title, a label or a + * collection position (#430). A panel action carries the owned query's id as + * well as the tile's: both are re-resolved inside the write's transform, and + * the pair is what proves the write is aimed at the intended resource. */ +export type DashboardTreeActionTarget = + | { kind: 'dashboard'; dashboardId: string } + | { kind: 'panel'; dashboardId: string; tileId: string; queryId: string } + | { kind: 'variable-config'; dashboardId: string; name: string }; + +export interface DashboardTreeAction { + kind: DashboardTreeActionKind; + /** The control's accessible name. Always identifies the target RESOURCE — + * a screen-reader user hears the buttons of many rows in sequence, and + * "Edit" alone names none of them. */ label: string; - /** `null` when the operation is unavailable — a source-less or broken member's - * query-open. The item still renders, disabled, so the row's full vocabulary - * stays discoverable and keyboard-reachable. */ - command: DashboardTreeCommand | null; + /** The pointer tooltip, which may be shorter than the accessible name. */ + tooltip: string; + /** `null` when the operation belongs to this row's vocabulary but cannot be + * performed right now (#494's malformed-ownership rule). The control still + * renders — disabled semantically, not merely greyed out — so the row's + * vocabulary stays stable and discoverable. */ + target: DashboardTreeActionTarget | null; + /** Why it is unavailable, for the tooltip and the accessible description. + * Non-null exactly when `target` is `null`. */ + unavailable: string | null; + /** + * The question a DESTRUCTIVE action asks before it runs; `null` for an + * editing one, which needs no confirmation. + * + * Composed here rather than in the view because it names the resources by + * their resolved display titles — the panel's label AND its owning + * Dashboard's — and the view holds only the row it is painting. It is the + * same reason a row's `diagnostic` is composed by the model. + */ + confirm: string | null; } export interface DashboardTreeRow { @@ -174,16 +224,16 @@ export interface DashboardTreeRow { * variable this is the variable's OWN diagnostic, verbatim from * `core/dashboard-variables.ts` — never re-composed here. */ diagnostic: string | null; - /** #447: whether this row offers a trailing destructive affordance. True for - * an ORPHANED variable only — deleting it drops stored SQL that nothing else - * holds. An active or conflicted variable is inferred from the panel queries, - * so there is nothing about it a tree row could delete. */ - deletable: boolean; - /** #429 phase 3: whether this row offers a trailing rename affordance (title - * + description). True for the DASHBOARD row only — a panel row's own - * metadata pencil is phase 4's, and a variable/group row has no document of - * its own to rename. */ - renamable: boolean; + /** + * #494: the row's trailing direct controls, in paint order — edit before + * delete, so the destructive one is rightmost and never where the pointer + * lands by habit. + * + * This replaced three separate expressions of the same idea: `renamable` + * (#429 phase 3's Dashboard pencil), `deletable` (#447's orphaned-variable + * trash) and the `menu` list. A group row has none. + */ + actions: readonly DashboardTreeAction[]; dashboardId: string; /** The member this row addresses, by Dashboard-local id — never by query id. */ member: DashboardFocusTarget | null; @@ -201,14 +251,6 @@ export interface DashboardTreeRow { * press can open at once. */ double: DashboardTreeCommand | null; shift: DashboardTreeCommand | null; - /** The keyboard-reachable equivalent of every gesture this row offers that a - * pointer-only affordance would otherwise HIDE. A Dashboard row therefore - * offers *Open in Edit* only (#429/#472): plain View is its primary press, so - * it is not hidden, while Edit exists only as Shift-click/Shift+Enter. EMPTY - * for a variable row: #447 forbids the `…` menu there, and a variable row has - * no double/Shift gesture for it to expose — its single activation (open the - * option-SQL editor) is already reachable with Enter. */ - menu: readonly DashboardTreeMenuItem[]; /** * #428: what a Library-query drop on THIS row would write, or `null` when the * row rejects assignment. Resolved here for the same reason every other @@ -367,19 +409,165 @@ const variableAnnotation = ( return { invalid: null, severity: null }; }; -/** A panel row's action vocabulary. #447 removed the filter half: a variable row - * has no menu at all. */ -const PANEL_MENU_LABELS = { - open: 'Open query', - view: 'Open Dashboard in View and focus panel', - edit: 'Open Dashboard in Edit and focus panel', -}; +/** + * Why a panel's pencil and trash are rendered but unavailable (#494). + * + * Both say what is wrong with the DATA rather than "not allowed": the row is + * showing a tile whose dedicated query cannot be proven, and the user's next + * move is to look at the panel, not to try again. Repairing such a workspace + * is #429's repair-planner phase, deliberately not something these controls + * attempt — a guessed owner is how one delete becomes two. + */ +const MISSING_PANEL_QUERY_REASON = + 'This panel’s query is not in this workspace, so there is nothing to edit or remove.'; +const UNPROVEN_OWNERSHIP_REASON = + 'This panel’s query is shared with another panel, so it cannot be edited or removed here.'; +/** Two saved-query documents carry this id (`workspace-duplicate-query-id`). + * Which one the pencil would edit has no answer, so neither control is + * offered — and the commit paths refuse the same state, which is the point: + * a control must not open a dialog only to refuse at the end of it. */ +const AMBIGUOUS_QUERY_REASON = + 'Two saved queries in this workspace share this panel’s query id, so it cannot be edited or removed here.'; +/** A tile may only reference a PANEL-role query (`dashboard-setup-reference` / + * `dashboard-tile-role-incompatible` in the semantic validator). Editing or + * deleting through a tile that references, say, a Setup query would silently + * repair the workspace by destroying the evidence — fail closed instead. */ +const WRONG_ROLE_REASON = + 'This panel references a query that is not a panel query, so it cannot be edited or removed here.'; +/** Two Dashboard documents carry this id. Which one a Dashboard-level edit or + * delete would act on has no answer — the same "a control must not open a + * dialog only to refuse at the end of it" rule `AMBIGUOUS_QUERY_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 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 + * checked ahead of ownership rather than folded into it. */ +const AMBIGUOUS_TILE_REASON = + 'Two panels in this dashboard share this id, so it cannot be edited or removed here.'; +/** Same ambiguity as `AMBIGUOUS_DASHBOARD_REASON`, worded for the one control + * an orphaned-variable row offers — delete only, never edit. Deleting stored + * option SQL is addressed by Dashboard id alone (#447), so a duplicated + * Dashboard id leaves it exactly as unresolvable as a Dashboard-level delete; + * `commitVariableConfig`'s own strict-replacement guard refuses it too. */ +const AMBIGUOUS_DASHBOARD_VARIABLE_REASON = + 'Two dashboards in this workspace share this id, so this stored option SQL cannot be removed here.'; + +/** One trailing control, available. */ +const action = ( + kind: DashboardTreeActionKind, label: string, tooltip: string, + target: DashboardTreeActionTarget, confirm: string | null = null, +): DashboardTreeAction => ({ kind, label, tooltip, target, unavailable: null, confirm }); + +/** One trailing control the data does not permit right now. It still renders: + * a row whose vocabulary silently shrinks teaches the user that panels + * sometimes have no pencil, which is a worse lie than a disabled one. */ +const unavailableAction = ( + kind: DashboardTreeActionKind, label: string, reason: string, +): DashboardTreeAction => ({ + kind, label, tooltip: reason, target: null, unavailable: reason, confirm: null, +}); + +/** Quoted for a confirmation sentence, in the typographic quotes this UI uses + * everywhere else. */ +const quoted = (name: string): string => '“' + name + '”'; export function deriveDashboardTree( { workspace, surface, ui }: DashboardTreeInput, ): DashboardTree { const dashboards = workspace?.dashboards ?? []; const queries = queryMap(workspace); + // #494: ONE ownership index per paint, from the #427 module that defines the + // rule — the tree does not get its own private notion of who owns a query. + // Tiles with no `queryId` are dropped on the way in because ownership is + // exactly the set of tile→query references; a tile that references nothing + // owns nothing, and its panel row's edit/delete are unavailable anyway. + // How many DOCUMENTS carry each id — not the same question as how many + // owners reference it. `queryMap` above collapses duplicates (last wins), so + // the availability rule below cannot read cardinality off it. + const documentsById = new Map(); + for (const query of workspace?.queries ?? []) { + documentsById.set(query.id, (documentsById.get(query.id) ?? 0) + 1); + } + // How many Dashboard DOCUMENTS carry each Dashboard id — the same shape of + // count as `documentsById` above, one level up. `deleteDashboardDocument`/ + // `removeDashboardPanel` already refuse a duplicated Dashboard id + // (`dashboard-duplicate`); this is what lets the row's own pencil/trash + // agree ahead of a commit rather than opening a dialog that then refuses. + const dashboardIdCounts = new Map(); + for (const dashboard of dashboards) { + dashboardIdCounts.set(dashboard.id, (dashboardIdCounts.get(dashboard.id) ?? 0) + 1); + } + // Which OCCURRENCE of a duplicated Dashboard id this is, in document order — + // 0 for the first, 1 for the second, and so on. Availability only needs the + // COUNT above; this is what lets two rows sharing a duplicated id still get + // two DISTINCT presentation keys below, so the roving tabindex, `data-key` + // focus restoration and drag-highlight lookups (all keyed on `row.key`) each + // resolve to exactly one row instead of silently picking whichever matching + // node happens to be first in the DOM. + const dashboardOccurrenceSeen = new Map(); + const ownership = buildQueryOwnershipIndex({ + queries: workspace?.queries ?? [], + dashboards: dashboards.map((dashboard) => ({ + id: dashboard.id, + tiles: (dashboard.tiles ?? []) + .filter((tile): tile is TreeTile & { queryId: string } => typeof tile.queryId === 'string') + .map((tile) => ({ id: tile.id, queryId: tile.queryId })), + })), + }); + + /** + * The pencil + trash a panel row offers, and whether the data permits them. + * + * Availability is the #427 exactly-one-owner rule, checked against THIS + * Dashboard and THIS tile: a query with several owners, one owned by a + * different member, or a reference to a query the workspace does not carry + * all leave both controls rendered and unavailable. `facts.queryId` is + * already `null` for a reference that resolves to nothing, which is what + * separates the two reasons. + */ + const panelActions = ( + dashboardId: string, dashboardLabel: string, tileId: string, label: string, queryId: string | null, + identityReason: string | null, + ): DashboardTreeAction[] => { + // Checked BEFORE ownership, not folded into it: a duplicated Dashboard or + // Dashboard-local tile id is ambiguous on its own terms, and the #427 + // ownership index — keyed by query id — cannot see it. Two tiles sharing + // an id but referencing different queries each look, independently, like + // that query's sole owner; this is what keeps the model agreeing with + // `removeDashboardPanel`'s own `dashboard-duplicate`/`tile-duplicate` + // refusals instead of offering a pencil delete already refuses. + if (identityReason !== null) { + return [ + unavailableAction('edit-panel', 'Edit ' + label, identityReason), + unavailableAction('delete-panel', 'Remove ' + label + ' from dashboard', identityReason), + ]; + } + const owners = queryId === null ? [] : ownership.ownersByQueryId.get(queryId) ?? []; + const owned = owners.length === 1 + && owners[0].dashboardId === dashboardId && owners[0].tileId === tileId; + const rightRole = queryId !== null && queryDashboardRole(queries.get(queryId)) === 'panel'; + const unique = queryId !== null && documentsById.get(queryId) === 1; + if (queryId === null || !owned || !unique || !rightRole) { + const reason = queryId === null + ? MISSING_PANEL_QUERY_REASON + : (!owned ? UNPROVEN_OWNERSHIP_REASON + : (!unique ? AMBIGUOUS_QUERY_REASON : WRONG_ROLE_REASON)); + return [ + unavailableAction('edit-panel', 'Edit ' + label, reason), + unavailableAction('delete-panel', 'Remove ' + label + ' from dashboard', reason), + ]; + } + const target: DashboardTreeActionTarget = { kind: 'panel', dashboardId, tileId, queryId }; + return [ + action('edit-panel', 'Edit ' + label, 'Edit name & description', target), + action('delete-panel', 'Remove ' + label + ' from dashboard', 'Remove panel', target, + 'Remove panel ' + quoted(label) + ' from ' + quoted(dashboardLabel) + + '? This also deletes its dedicated query copy.'), + ]; + }; const search = ui.searchText.trim().toLowerCase(); const hits = (haystack: readonly string[]): boolean => @@ -392,12 +580,35 @@ export function deriveDashboardTree( const rows: DashboardTreeRow[] = []; for (const dashboard of dashboards) { - const dashboardKey = encodeKeyPart(workspaceId) + ':' + encodeKeyPart(dashboard.id); + const dashboardIdDuplicated = (dashboardIdCounts.get(dashboard.id) ?? 0) > 1; + const dashboardOccurrenceIndex = dashboardOccurrenceSeen.get(dashboard.id) ?? 0; + dashboardOccurrenceSeen.set(dashboard.id, dashboardOccurrenceIndex + 1); + // The PRESENTATION key, distinct from the (still ambiguous) mutation + // target below: when the id is duplicated, every row this Dashboard emits + // — this one and, through `groupKeyFor`/the variable and panel keys built + // from it, every descendant — carries a `:dup:` suffix so the + // two Dashboards' subtrees never collide on `row.key`/`data-key`, even + // though `dashboardId: dashboard.id` (what `edit-dashboard`'s target and + // `dropTarget` would name, were either not already unavailable/null below) + // still names the SAME ambiguous id. + const dashboardKey = encodeKeyPart(workspaceId) + ':' + encodeKeyPart(dashboard.id) + + (dashboardIdDuplicated ? ':dup:' + dashboardOccurrenceIndex : ''); const title = trimmed(dashboard.title); const description = trimmed(dashboard.description); const dashboardMatched = search !== '' && hits([title, description]); const tiles = dashboard.tiles ?? []; + // Dashboard-LOCAL tile-id cardinality: two tiles of the SAME Dashboard + // sharing an id, never a count across Dashboards — a tile id is only ever + // addressed together with its owning Dashboard id (#430). + const tileIdCounts = new Map(); + for (const tile of tiles) { + tileIdCounts.set(tile.id, (tileIdCounts.get(tile.id) ?? 0) + 1); + } + // Same occurrence-tracking as `dashboardOccurrenceSeen`, scoped to THIS + // Dashboard's own tiles — reset every iteration, since a tile id is only + // ever compared against siblings of the same Dashboard. + const tileOccurrenceSeen = new Map(); const tileEntries = tileEntriesOf(dashboard, queries); // #447: the variable rows come from the pure inference service, over THIS // Dashboard's panel-owned queries plus its stored option SQL. @@ -442,8 +653,30 @@ export function deriveDashboardTree( invalid: null, severity: null, diagnostic: null, - deletable: false, - renamable: true, + // #494: the Dashboard row's own two 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). + // + // A duplicated Dashboard id leaves both unavailable: `findDashboardStrict` + // already refuses `dashboard-duplicate` for both removal paths, and a + // rename has no less ambiguous a target — offering a pencil that a + // commit would only refuse is the exact bug this closes. + actions: dashboardIdDuplicated + ? [ + unavailableAction('edit-dashboard', 'Edit dashboard ' + (title || UNTITLED_DASHBOARD), + AMBIGUOUS_DASHBOARD_REASON), + unavailableAction('delete-dashboard', 'Delete dashboard ' + (title || UNTITLED_DASHBOARD), + AMBIGUOUS_DASHBOARD_REASON), + ] + : [ + 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), + 'Delete dashboard', { kind: 'dashboard', dashboardId: dashboard.id }, + 'Delete dashboard ' + quoted(title || UNTITLED_DASHBOARD) + + '? This also deletes every query its panels own.'), + ], dashboardId: dashboard.id, member: null, queryId: null, @@ -457,15 +690,11 @@ export function deriveDashboardTree( single: openDashboardCommand(dashboard.id, 'view'), double: null, shift: openDashboardCommand(dashboard.id, 'edit'), - // *Open in View* is gone: it IS the primary press now, and the menu existed - // only to keep a pointer-hidden gesture discoverable. *Open in Edit* stays - // for exactly that reason — its only forms are Shift-click and Shift+Enter, - // both hidden modifiers, so this row is where a keyboard or first-time user - // finds Edit mode at all. - menu: [ - { label: 'Open in Edit', command: openDashboardCommand(dashboard.id, 'edit') }, - ], - dropTarget: { kind: 'panel', dashboardId: dashboard.id }, + // A duplicated Dashboard id cannot be a drop destination either: an + // assignment drop resolves its target by `dashboardId` alone, and + // `dashboardId` alone has two answers here — the same ambiguity that + // already withholds this row's own pencil/trash above. + dropTarget: dashboardIdDuplicated ? null : { kind: 'panel', dashboardId: dashboard.id }, }); if (!dashboardExpanded) continue; @@ -502,8 +731,25 @@ export function deriveDashboardTree( severity, // Verbatim from the inference service — never re-composed here. diagnostic: variable.diagnostic, - deletable: variable.status === 'orphaned', - renamable: false, + // #447's trash, unchanged in behaviour: an ORPHANED configuration is + // the only variable state with anything of its own to delete — an + // active or conflicted variable is inferred from the panel SQL. + // + // A duplicated Dashboard id leaves it unavailable too: the delete is + // addressed by `dashboardId` alone, `commitVariableConfig`'s own + // strict-replacement guard refuses an ambiguous one exactly the way + // the Dashboard row's own delete does, and offering a trash the + // commit would only silently no-op is the same bug being closed + // everywhere else on this row. + actions: variable.status === 'orphaned' + ? [dashboardIdDuplicated + ? unavailableAction('delete-variable-config', + 'Delete the stored option SQL for ' + variable.name, AMBIGUOUS_DASHBOARD_VARIABLE_REASON) + : action('delete-variable-config', + 'Delete the stored option SQL for ' + variable.name, 'Delete stored option SQL', + { kind: 'variable-config', dashboardId: dashboard.id, name: variable.name }, + 'Delete the stored option SQL for ' + quoted(variable.name) + '? The SQL is lost.')] + : [], dashboardId: dashboard.id, member, queryId: null, @@ -511,12 +757,13 @@ export function deriveDashboardTree( single: { kind: 'open-variable', dashboardId: dashboard.id, name: variable.name }, double: null, shift: null, - menu: [], // #428: an ORPHAN is not an assignment destination. A configuration no // panel declares any more has nothing to bind to, though it stays // editable and deletable through its own affordances — which is why this // reads the same `status === 'orphaned'` that `deletable` above does. - dropTarget: variable.status === 'orphaned' + // A duplicated Dashboard id is withheld here too — same reasoning as + // the Dashboard row's own `dropTarget` above. + dropTarget: (variable.status === 'orphaned' || dashboardIdDuplicated) ? null : { kind: 'variable', dashboardId: dashboard.id, variableName: variable.name }, }; @@ -524,6 +771,10 @@ export function deriveDashboardTree( const panelRows = shownTiles.map(({ tile, facts }): DashboardTreeRow => { const member: DashboardFocusTarget = { kind: 'tile', id: tile.id }; + const tileIdDuplicated = (tileIdCounts.get(tile.id) ?? 0) > 1; + const tileOccurrenceIndex = tileOccurrenceSeen.get(tile.id) ?? 0; + tileOccurrenceSeen.set(tile.id, tileOccurrenceIndex + 1); + const identityAmbiguous = dashboardIdDuplicated || tileIdDuplicated; // One query can back several panels, so this is one row PER TILE — never // merged by query id or label. const openQuery: DashboardTreeCommand | null = facts.queryId === null @@ -532,7 +783,19 @@ export function deriveDashboardTree( const focusView = openDashboardCommand(dashboard.id, 'view', member); const focusEdit = openDashboardCommand(dashboard.id, 'edit', member); return { - key: tileRowKey(workspaceId, dashboard.id, tile.id), + // Disambiguated the SAME way the Dashboard row's own key is — built + // from `dashboardKey` (already carrying its own `:dup:` suffix when + // the DASHBOARD id is duplicated, never the raw `dashboard.id` + // `tileRowKey` would use) plus this tile's own suffix when the TILE + // id is duplicated. A duplicated id at either level still needs a + // UNIQUE `row.key` per row — the roving tabindex, `data-key` focus + // restoration and drag-highlight lookups downstream all resolve by + // this key alone, so two rows sharing one would let either mechanism + // silently pick the wrong (first-in-DOM) row. Equal to + // `tileRowKey(workspaceId, dashboard.id, tile.id)` whenever neither is + // duplicated, since `dashboardKey` itself is then unsuffixed. + key: dashboardKey + ':tile:' + encodeKeyPart(tile.id) + + (tileIdDuplicated ? ':dup:' + tileOccurrenceIndex : ''), kind: 'panel', level: 3, parentKey: groupKeyFor('panels'), @@ -547,23 +810,22 @@ export function deriveDashboardTree( invalid: facts.invalid, severity: facts.invalid === null ? null : 'error', diagnostic: facts.invalid === null ? null : MISSING_QUERY_DIAGNOSTIC, - deletable: false, - renamable: false, + actions: panelActions(dashboard.id, title || UNTITLED_DASHBOARD, tile.id, facts.label, facts.queryId, + dashboardIdDuplicated ? AMBIGUOUS_DASHBOARD_REASON + : (tileIdDuplicated ? AMBIGUOUS_TILE_REASON : null)), dashboardId: dashboard.id, member, queryId: facts.queryId, group: 'panels', - // Only the query-open action is withheld: Dashboard View/Edit focus - // navigation stays available so a broken panel's diagnostics remain - // reachable. + // Only the query-open action stays available under identity ambiguity + // (it is addressed by `queryId` alone, which is unaffected): View/Edit + // focus navigation is withheld instead, because both are addressed by + // `dashboard.id` + `member.id` — the Dashboard viewer's own tile-focus + // lookup is keyed by tile id alone, so an ambiguous pair could resolve + // (or highlight) a DIFFERENT tile than the one the row names. single: openQuery, - double: focusView, - shift: focusEdit, - menu: [ - { label: PANEL_MENU_LABELS.open, command: openQuery }, - { label: PANEL_MENU_LABELS.view, command: focusView }, - { label: PANEL_MENU_LABELS.edit, command: focusEdit }, - ], + double: identityAmbiguous ? null : focusView, + shift: identityAmbiguous ? null : focusEdit, // #428 rejects an individual panel row: sharing one query between panels // and moving members between Dashboards are both out of scope, so there // is no assignment a panel row could mean. @@ -609,8 +871,7 @@ export function deriveDashboardTree( invalid: null, severity: null, diagnostic: null, - deletable: false, - renamable: false, + actions: [], dashboardId: dashboard.id, member: null, queryId: null, @@ -620,11 +881,13 @@ export function deriveDashboardTree( single: groupForced ? null : { kind: 'toggle' }, double: null, shift: null, - menu: [], - // #428: Panels means the same thing as the Dashboard row itself. The - // VARIABLES group never accepts — it does not identify which variable - // would receive the SQL, and guessing is worse than rejecting. - dropTarget: group === 'panels' ? { kind: 'panel', dashboardId: dashboard.id } : null, + // #428: Panels means the same thing as the Dashboard row itself — + // including its own ambiguous-id rejection when `dashboardId` alone + // has two answers. The VARIABLES group never accepts — it does not + // identify which variable would receive the SQL, and guessing is + // worse than rejecting. + dropTarget: (group === 'panels' && !dashboardIdDuplicated) + ? { kind: 'panel', dashboardId: dashboard.id } : null, }); if (groupExpanded) rows.push(...members); } diff --git a/src/dashboard/application/dashboard-removal.ts b/src/dashboard/application/dashboard-removal.ts new file mode 100644 index 00000000..70e86a3c --- /dev/null +++ b/src/dashboard/application/dashboard-removal.ts @@ -0,0 +1,226 @@ +// Dashboard/panel DELETE, atomically (#429/#494). +// +// #427 made Dashboard membership an explicit reference a tile OWNS: a panel +// tile's `queryId` names a dedicated copy that exists nowhere else. Deleting a +// tile or a whole Dashboard therefore has to delete the queries that copy +// represents too, or the Library would silently accumulate orphaned "owned" +// copies that are no longer reachable from anywhere and no longer Library +// entries either (#427's own partition: zero owners is what MAKES a query a +// Library query). +// +// Both transforms here PROVE ownership through the #427 index +// (`ownersOfQuery`) before deleting anything, rather than assuming a tile's +// `queryId` is safe to remove because it looks like the tile's own copy. The +// index is the one place that can tell a genuinely dedicated copy apart from +// the invalid states a workspace can still be caught in mid-edit — a second +// reference to the same id, or a reference to an id nothing carries any more. +// Guessing in either state and deleting anyway would either destroy a query +// another Dashboard still renders, or silently no-op while claiming success. +// Neither transform ever cascades past what it can prove: the single-panel +// delete refuses instead of picking an owner, and the whole-Dashboard delete +// keeps a query the invalid multi-owner state also points at from elsewhere. +// +// `dashboard.variableConfigs` is left untouched by a panel delete on purpose. +// #457 made a variable's identity and type come from the `{name:Type}` +// placeholders its panel queries declare, not from a stored object — so an +// orphaned configuration (a name no panel declares any more) is designed to +// survive and keep displaying, exactly the way a panel's last declaration +// disappearing through an ordinary SQL edit already leaves it. Deleting the +// tile is not a special case of that, and #494's own non-goals say so +// explicitly: the configuration is not a side effect to clean up here. +// +// Pure — no DOM, no persistence, no globals, never mutates its input. + +import { removeTileMembership } from './tile-membership.js'; +import type { TileRemovalResult } from './tile-membership.js'; +import { findDashboardStrict, replaceDashboard } from '../../workspace/workspace-dashboards.js'; +import { buildQueryOwnershipIndex, ownersOfQuery } from '../model/query-ownership.js'; +import { queryDashboardRole } from '../model/workspace-semantics.js'; +import type { DashboardDocumentV2, StoredWorkspaceV5 } from '../../generated/json-schema.types.js'; + +/** Why a single-panel delete refused, as a value rather than a thrown error — + * every one of these is an ordinary concurrent-state or data-integrity + * outcome, never a bug, and the UI phrases each differently. */ +export type PanelRemovalRefusal = + | 'dashboard-missing' + | 'dashboard-duplicate' + | 'tile-missing' + /** Two tiles carry this id, or two query documents do. An ambiguous id is + * never resolved by picking one: `tiles.filter(id !== target)` would take + * BOTH tiles out, and the query filter would drop both documents. */ + | 'tile-duplicate' + /** The tile no longer references the query the caller captured — it was + * re-pointed while the confirmation was open. The caller confirmed removing + * a specific panel's specific query copy; this is a different one now. */ + | 'tile-retargeted' + | 'ownership-unproven'; + +export type PanelRemovalResult = + | { status: 'ok'; workspace: StoredWorkspaceV5; queryId: string } + | { status: 'refused'; reason: PanelRemovalRefusal }; + +/** + * Remove ONE Dashboard panel tile and exactly the dedicated query it owns, in + * one atomic commit. + * + * Resolution is entirely id-addressed, matching `findDashboardStrict`'s own + * rule: `dashboard-missing`/`dashboard-duplicate` refuse before anything else + * is even looked at, because an ambiguous-id workspace must never be written + * through a guess at which entry was meant. + * + * Ownership is PROVEN, not assumed: the tile's `queryId` must both name a + * query that still exists in `workspace.queries`, and have EXACTLY ONE owner + * in the #427 index. A dangling id, a query with no owners left after some + * other edit, or — the invalid state #427 forbids reaching but this cannot + * assume away — more than one owner, all refuse `ownership-unproven` rather + * than deleting the tile alone and leaving a stray query behind, or deleting a + * query some other tile still renders. The sole owner is never re-checked + * against `dashboardId`/`tileId` by hand: `ownersOfQuery` is built by scanning + * every tile of `workspace.dashboards` including this very one, so whenever it + * reports exactly one owner for this tile's own `queryId`, that owner IS this + * Dashboard and this tile — there is no second identity for a single-owner + * result to disagree with. + * + * The tile itself is removed through `removeTileMembership`, which already + * normalizes through the active layout plugin and regenerates the + * grafana-grid flow fallback — neither is reimplemented here. The target + * Dashboard's `revision` is bumped by exactly one; every other Dashboard, + * every other query, and both collections' order are preserved exactly. + */ +export function removeDashboardPanel(input: { + workspace: StoredWorkspaceV5; + dashboardId: string; + tileId: string; + /** The query the CALLER resolved when it built the confirmation. Re-checked + * against the tile's own reference below rather than trusted: it is what + * the user was told would be deleted. */ + queryId: string; +}): PanelRemovalResult { + const { workspace, dashboardId, tileId, queryId } = input; + + const lookup = findDashboardStrict(workspace, dashboardId); + if (lookup.status !== 'ok') { + return { status: 'refused', reason: lookup.status === 'missing' ? 'dashboard-missing' : 'dashboard-duplicate' }; + } + const dashboard = lookup.dashboard; + + // Exactly one match, the same rule `findDashboardStrict` applies one level + // up. The removal below is a `filter` by id, so an ambiguous id would take + // out every match — which is precisely why this refuses instead. + const tiles = dashboard.tiles.filter((candidate) => candidate.id === tileId); + if (tiles.length === 0) return { status: 'refused', reason: 'tile-missing' }; + if (tiles.length > 1) return { status: 'refused', reason: 'tile-duplicate' }; + const tile = tiles[0]; + + // The tile still has to reference the query the caller confirmed. Both the + // before and after states of a re-pointed tile are perfectly valid, so + // nothing else in this transform would notice. + if (tile.queryId !== queryId) return { status: 'refused', reason: 'tile-retargeted' }; + + const matching = workspace.queries.filter((query) => query.id === queryId); + if (matching.length > 1) return { status: 'refused', reason: 'tile-duplicate' }; + const owners = ownersOfQuery(workspace, queryId); + // Owned by exactly this tile, and a PANEL query — the role the tile + // contract requires. A Setup- or other-role reference is malformed data + // (`dashboard-setup-reference` / `dashboard-tile-role-incompatible`), and + // deleting it here would silently "repair" the workspace by destroying the + // evidence, which #494's fail-closed rule forbids. + if (matching.length === 0 || owners.length !== 1 + || queryDashboardRole(matching[0]) !== 'panel') { + return { status: 'refused', reason: 'ownership-unproven' }; + } + + // `removeTileMembership` returns `null` only for a tile id it cannot find in + // `dashboard.tiles` — already excluded by the `tile` lookup above, so this + // cast reflects a genuinely unreachable branch rather than skipping a check. + const removed = removeTileMembership(dashboard, workspace.queries, tileId) as TileRemovalResult; + const nextDashboard: DashboardDocumentV2 = { ...removed.dashboard, revision: dashboard.revision + 1 }; + const nextQueries = workspace.queries.filter((query) => query.id !== queryId); + const withQueries: StoredWorkspaceV5 = { ...workspace, queries: nextQueries }; + + // `replaceDashboard` re-checks the exactly-one-match rule at write time, but + // `withQueries` only changed `queries` — `dashboards` is the same reference + // `lookup` already resolved to `ok` above, so `null` here is unreachable too. + const next = replaceDashboard(withQueries, dashboardId, nextDashboard) as StoredWorkspaceV5; + + return { status: 'ok', workspace: next, queryId }; +} + +export type DashboardRemovalResult = + | { status: 'ok'; workspace: StoredWorkspaceV5; removedQueryIds: readonly string[] } + | { status: 'refused'; reason: 'dashboard-missing' | 'dashboard-duplicate' }; + +/** + * Remove ONE whole Dashboard document and, recursively, the queries its own + * tiles own — the document-level counterpart to `removeDashboardPanel` above. + * + * A tile's `queryId` joins the delete set only when ALL of these hold: exactly + * one query document carries it, every owner `ownersOfQuery` reports for it + * belongs to THIS Dashboard, and it is a PANEL query. The second half is what protects the + * invalid multi-owner state (a query some other Dashboard's tile also + * references) rather than trusting that this Dashboard's own reference is the + * only one — deleting a query another Dashboard still renders would trade one + * data-integrity problem for a worse one. A query with no owners at all + * (Library) never reaches this check because it is never named by one of this + * Dashboard's own tiles in the first place. Ids repeated across this + * Dashboard's own tiles are deduplicated, and the result is reported in tile + * order. + * + * There is no revision bump: the document that carried a revision is gone. + * Every other Dashboard, every other query, and both collections' order are + * preserved exactly. + */ +export function removeDashboardDocument(input: { + workspace: StoredWorkspaceV5; + dashboardId: string; +}): DashboardRemovalResult { + const { workspace, dashboardId } = input; + + const lookup = findDashboardStrict(workspace, dashboardId); + if (lookup.status !== 'ok') { + return { status: 'refused', reason: lookup.status === 'missing' ? 'dashboard-missing' : 'dashboard-duplicate' }; + } + const dashboard = lookup.dashboard; + + // One index for the whole walk: `ownersOfQuery` rebuilds it per call, and a + // Dashboard's tiles are exactly the ids being asked about. + const { ownersByQueryId, dashboardOwnedQueryIds } = buildQueryOwnershipIndex(workspace); + const removedQueryIds: string[] = []; + const considered = new Set(); + for (const tile of dashboard.tiles) { + const { queryId } = tile; + if (considered.has(queryId)) continue; + considered.add(queryId); + + // `dashboardOwnedQueryIds` holds only ids that EXIST and have an owner, so + // it answers the dangling-reference case as well. + if (!dashboardOwnedQueryIds.has(queryId)) continue; + // An id carried by two query documents is ambiguous: the removal is a + // filter, so deleting "it" would delete both. Keep them, and let the + // Dashboard go without them — an orphaned copy is recoverable, a + // destroyed one is not. + const documents = workspace.queries.filter((query) => query.id === queryId); + if (documents.length > 1) continue; + // Same rule the single-panel delete applies: only a PANEL query is a + // dedicated copy this Dashboard may take with it. A tile referencing a + // Setup- or other-role query is malformed data, and destroying that query + // as a side effect of removing the Dashboard would be the cascade #494's + // fail-closed rule forbids — it survives as a Library query instead. + if (queryDashboardRole(documents[0]) !== 'panel') continue; + + // Present by construction: `dashboardOwnedQueryIds` is exactly the ids + // `ownersByQueryId` carries an owner list for. + const ownedOnlyHere = ownersByQueryId.get(queryId)! + .every((owner) => owner.dashboardId === dashboardId); + if (ownedOnlyHere) removedQueryIds.push(queryId); + } + + const removeSet = new Set(removedQueryIds); + const next: StoredWorkspaceV5 = { + ...workspace, + dashboards: workspace.dashboards.filter((candidate) => candidate.id !== dashboardId), + queries: workspace.queries.filter((query) => !removeSet.has(query.id)), + }; + + return { status: 'ok', workspace: next, removedQueryIds }; +} diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index 3b1bdfe6..0491761b 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -560,7 +560,13 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // labels, and all of `ui/dashboard.ts`'s composed names alike). const authored = typeof tile.title === 'string' ? tile.title.trim() : ''; const title = authored || (query ? queryName(query) : tile.queryId) || tile.id; - const description = (typeof tile.description === 'string' && tile.description) + // Same TRIM-before-fallback as the title above, for the same reason: the + // tile description has no `minLength` either, so a whitespace-only + // authored value is schema-legal, and left untrimmed it wins the chain + // and masks `query.spec.description` with blank text instead of falling + // through to it. + const authoredDescription = typeof tile.description === 'string' ? tile.description.trim() : ''; + const description = authoredDescription || (typeof query?.spec?.description === 'string' ? query.spec.description : ''); const state: ViewerTileState = { tileId: tile.id, queryId: tile.queryId, title, description, isKpi, panel, diff --git a/src/dashboard/model/query-ownership.ts b/src/dashboard/model/query-ownership.ts index 4103fa53..fa30213a 100644 --- a/src/dashboard/model/query-ownership.ts +++ b/src/dashboard/model/query-ownership.ts @@ -65,8 +65,18 @@ export interface OwnershipDashboard { tiles: readonly OwnershipTile[]; } +/** All ownership reads of a query: its identity. Narrowed to this in #494 so + * the Dashboard tree's own loosened projection types (`TreeQuery`, which + * makes `sql`/`spec` optional because the tree must render stale or imported + * data) can be indexed by the ONE ownership rule instead of re-deriving + * "which member owns this query" beside it. `libraryQueries` still answers in + * the caller's own element type — see its generic below. */ +export interface OwnershipQuery { + id: string; +} + export interface OwnershipWorkspace { - queries: readonly SavedQueryV2[]; + queries: readonly OwnershipQuery[]; dashboards: readonly OwnershipDashboard[]; } @@ -120,7 +130,9 @@ export function buildQueryOwnershipIndex(workspace: OwnershipWorkspace): QueryOw * original `workspace.queries[]` relative order. The lower sidebar renders * exactly this (#427); owned copies stay serialized and stay openable by id, * they are simply not Library entries. */ -export function libraryQueries(workspace: OwnershipWorkspace): SavedQueryV2[] { +export function libraryQueries( + workspace: { queries: readonly Q[]; dashboards: readonly OwnershipDashboard[] }, +): Q[] { const { libraryQueryIds } = buildQueryOwnershipIndex(workspace); return workspace.queries.filter((query) => libraryQueryIds.has(query.id)); } diff --git a/src/state.ts b/src/state.ts index 7d78f769..ddec10be 100644 --- a/src/state.ts +++ b/src/state.ts @@ -451,7 +451,16 @@ export type PatchDraftResult = * dead Library row until the next activation refresh. */ export type PatchSavedResult = | { ok: true; invalidTab: null; entry: SavedQueryV2 } - | { ok: false; invalidTab: QueryTab | null; entry: null; diagnostics?: SpecDiagnostic[]; deletedExternally?: true }; + | { + ok: false; invalidTab: QueryTab | null; entry: null; + diagnostics?: SpecDiagnostic[]; deletedExternally?: true; + /** #494: the caller's own dequeue-time precondition no longer holds — for + * the Dashboard tree's panel pencil, the tile that owned this query was + * deleted or re-pointed while the dialog was open. Distinct from + * `deletedExternally` (the QUERY itself is gone): the query is still + * there, it is simply no longer the resource the caller meant to edit. */ + guardRefused?: true; + }; /** * A tab's complete `spec.panel` payload, cloned for safe use/persistence. The @@ -1155,9 +1164,17 @@ export async function patchSavedSpec( id: string, patch: SpecPatch, mutate: MutateWorkspace, validationService: SpecValidationService = defaultSpecValidationService, + guard?: (base: StoredWorkspaceV5) => boolean, ): Promise { const invalidTab = invalidSpecTabForSaved(state, id); if (invalidTab) return { ok: false, invalidTab, entry: null }; + // #494: a caller-supplied precondition, re-checked against DEQUEUE-TIME truth + // rather than against whatever the caller saw when it opened its dialog. The + // Dashboard tree's panel pencil is the first user: the query id alone does not + // say which tile owns it, so "still owned by the tile I was opened for" is a + // fact only `latest` can settle, and settling it out here would leave exactly + // the race the write queue exists to close. + let guardRefused = false; // The PERSISTED entry patch folds into the LATEST workspace (#343): resolve // the entry by id against `latest.queries` (not stale `state`) and preserve // every other latest query. #427 removed the Dashboard-membership half — a @@ -1175,6 +1192,7 @@ export async function patchSavedSpec( let prePatchToken: string | null = null; const outcome = await mutate((latest) => { const base = baselineWorkspace(state, latest); + if (guard !== undefined && !guard(base)) { guardRefused = true; return null; } const index = base.queries.findIndex((query) => query.id === id); if (index < 0) return null; prePatchToken = queryToken(base.queries[index]); @@ -1205,6 +1223,7 @@ export async function patchSavedSpec( // (invalidTab null); a blocking linked draft identifies its tab; an entry no // longer present in `latest` (deleted externally) aborts flagged so the // caller can refresh the tab association (#343 review). + if (guardRefused) return { ok: false, invalidTab: null, entry: null, guardRefused: true }; if (entryDiagnostics) return { ok: false, invalidTab: null, entry: null, diagnostics: entryDiagnostics }; // `blockedDraft` is assigned inside a `for` loop nested in the `mutate` // closure above; TS's control-flow narrowing loses the loop-nested closure @@ -1258,11 +1277,17 @@ export async function patchSavedSpec( * Rename a saved query, keeping any linked tab's name in sync. When * `description` is provided (not undefined) it is set/cleared too; pass * undefined to leave the existing description untouched (name-only rename). + * + * `guard` (#494) is an extra precondition evaluated against the dequeue-time + * workspace: the Dashboard tree's panel pencil uses it to re-prove that the + * tile it was opened for still owns this query. A refusal commits nothing and + * answers `guardRefused`. */ export async function renameSaved( state: AppState, id: string, name: unknown, description: string | null | undefined, mutate: MutateWorkspace, validationService: SpecValidationService = defaultSpecValidationService, + guard?: (base: StoredWorkspaceV5) => boolean, ): Promise { const nm = String(name || '').trim(); const index = state.savedQueries.findIndex((q) => q.id === id); @@ -1273,7 +1298,7 @@ export async function renameSaved( const desc = String(description || '').trim(); // match saveQuery: null/non-string → '' → cleared patch.description = desc || undefined; } - return patchSavedSpec(state, id, patch, mutate, validationService); + return patchSavedSpec(state, id, patch, mutate, validationService, guard); } /** diff --git a/src/styles.css b/src/styles.css index 0f26901d..35a098f4 100644 --- a/src/styles.css +++ b/src/styles.css @@ -703,7 +703,7 @@ h1, h2, h3, h4, h5, h6 { .fm-item:hover { background: var(--bg-hover); } /* #452: an unavailable operation still renders in its settled position — the menu's vocabulary must not change with the work surface, only its reachability. - Same treatment as `.dash-tree-menu .is-disabled`. The `:hover` override is + Same treatment as the tree's own disabled controls. The `:hover` override is required: `.fm-item:hover` above still matches a disabled