diff --git a/CHANGELOG.md b/CHANGELOG.md index 548a4c77..674710d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,45 @@ auto-generated per-PR notes; this file is the curated, human-readable history. an `Array(scalar T)` nobody has configured.) ### Changed +- **A Dashboard tile opens its own query in the Workbench; the Dashboard-level + `< Query` button is gone** (#471). Every query-backed tile carries an + **Open in Workbench** icon action in its own chrome, in both View and Edit mode + and in both layout engines. It opens *that tile's* query document in the main SQL + editor — or selects the tab already showing it — and leaves the Dashboard's + filters, layout, scroll position and View/Edit state untouched. + + Identity is the tile's own query id, never the displayed name. A Dashboard tile + references a dedicated saved-query copy that exactly one member owns (#427), so + two Dashboards holding same-**named** copies open two separate tabs, re-opening + one selects the tab it already has, and **Save from that tab updates the + Dashboard's copy** rather than a same-named Library query. The compact + collision *badges* that make two same-named tabs tellable apart remain #464. + + The action is deliberately not edit-mode-only — inspecting the query behind a + tile is a View-mode act first. It is subtle until the tile is hovered or the + button is focused, always visible on touch, and pressing it never starts a tile + drag, resize, selection or chart gesture. A **queryless** tile (a `text` panel) + and a tile whose query id no longer resolves show no action at all, rather than a + disabled control or one pointing at an unrelated document. + + The removed `< Query` button named no document — it was ordinary back-navigation + occupying primary toolbar space. Two follow-on consequences: a Grid-Tiles **KPI** + tile's chrome overlay now reveals and accepts clicks in View mode too (it had no + View-mode control before), and because the mobile rules hide the sidebar for a + full-bleed Dashboard, **the bottom nav no longer hides itself on the Dashboard + surface** — it shows only **Editor**, which returns to the Workbench. Without + that, a phone looking at a Dashboard with no tiles would have had no route back + at all. + + **Returning with Back now lands on the Dashboard you left, where you left it.** + The URL deliberately carries no Dashboard id, and opening a query tears the + Dashboard down, so Back used to rebuild the *first* Dashboard in the workspace at + the top of the page — tolerable while a global back button existed, and not + tolerable once per-tile actions are the way out. Each Dashboard history entry now + remembers its own id, current member and scroll offset, so several Back steps + across several Dashboards each return correctly; a remembered Dashboard that has + since been deleted falls back to the Workbench rather than to some other one. + - **The Dashboard runtime now says "variable" everywhere it meant "variable"** (#459). #447 replaced curated Dashboard filters with inferred variables but left the surviving code calling them filters, so the source read as if a model it had diff --git a/DESIGN.md b/DESIGN.md index 3a6b496f..500e1f85 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -387,8 +387,9 @@ asserted rather than reviewed. - **Error / Disabled:** semantic text/background/border tokens for errors; opacity and cursor changes for disabled controls. ### Navigation -- Table, JSON, panel types, query tabs, schema/library modes, and dashboard layout choices share one quiet tab/segmented-control vocabulary. Inactive items use muted text on transparent or subtle surfaces; active items gain ink, a tonal surface, a structural border, or the accent. Mobile replaces the split workspace with a bottom navigation for Tables, Editor, and Results at `768px` and below. -- The Dashboard surface toolbar (`[Back to query] · title · [View | Edit]`) is the **same** toolbar as the filter row stacked beneath it — one gap, one height, one padding — so the two read as one sticky top bar rather than two competing bands. Only the title differs, and only by weight. +- Table, JSON, panel types, query tabs, schema/library modes, and dashboard layout choices share one quiet tab/segmented-control vocabulary. Inactive items use muted text on transparent or subtle surfaces; active items gain ink, a tonal surface, a structural border, or the accent. Mobile replaces the split workspace with a bottom navigation for Tables, Editor, and Results at `768px` and below; on the Dashboard surface that bar keeps only **Editor**, as the route back to the Workbench. +- The Dashboard surface toolbar (`[layout · tile count · search · filters] … [freshness] · [View | Edit]`) is the **same** toolbar as the filter row stacked beneath it — one gap, one height, one padding — so the two read as one sticky top bar rather than two competing bands. +- Leaving a Dashboard is a **per-tile** act, not a global one: each query-backed tile carries a quiet `Open in Workbench` icon in its own chrome, subtle until the tile is hovered or the icon focused, always shown where there is no hover. There is no Dashboard-level back button — an action that names its document beats generic back-navigation holding primary toolbar space. ### Data Table - Sticky headers and row numbers preserve context during two-axis scrolling. diff --git a/docs/ADR-0003-dashboard-viewing.md b/docs/ADR-0003-dashboard-viewing.md index 208d0b9c..fb4068cf 100644 --- a/docs/ADR-0003-dashboard-viewing.md +++ b/docs/ADR-0003-dashboard-viewing.md @@ -558,6 +558,104 @@ unchanged. Three decisions are worth recording. one would have made the inconsistency worse, not smaller — the Dashboards tree has said "Variables" since #447 phase 1. +## Addendum (#471, 2026-07-26): leaving a Dashboard is a per-tile act + +The `Back to query` control the #426 addendum above restored is removed. It named +no document: generic back-navigation in the primary toolbar, which left the user to +find the corresponding query in the Workbench themselves. Every query-backed tile +carries its own `Open in Workbench` action instead. Four decisions are worth +recording. + +- **The tile's `queryId` IS the provenance the feature needs — no new tab model.** + #471 asks for tab identity by "stable document origin, such as `dashboardId + + dashboardQueryId`", and #464 proposes a `QueryTabOrigin` union. Neither is + required here, because the #427 addendum above already made ownership a + reference: a panel tile points at a dedicated saved-query copy that exactly one + member owns, so that copy's id already IS a per-Dashboard document identity. + `loadIntoNewTab` has always deduplicated on `savedId` and `commitSavedQuery` + resolves its write target by id, so "re-opening selects the existing tab", "two + same-named copies are two tabs" and "Save targets the Dashboard copy" all hold by + construction. What #464 still owns is the *visible* half — the collision badges + and full-origin tooltips that make two identically-named tabs tellable apart. A + `TabDocument` arm for this would have been a second, redundant identity next to + `savedId`, which is exactly the "second source of truth" #427 refused. + +- **The action is not edit-mode chrome.** The grip, delete and resize handle are all + `!readOnly`-gated and built only in Edit mode. This one is built unconditionally, + like the heading: inspecting the query behind a tile is a View-mode act first, and + the issue requires both modes. That had one consequence nothing else forced — a + Grid-Tiles KPI tile's head is an absolutely-positioned, `pointer-events: none` + overlay whose reveal rules were scoped `:not(.is-view)`, correct while every + control inside it was edit-only. View mode now reveals it and the action opts back + into pointer events. + +- **Nothing to open means no control, not a disabled one.** A `text` panel is + queryless by capability (`isQuerylessPanel`, the same predicate Save and share + already use) and an unresolvable `queryId` belongs to a tile already rendering its + own missing-query error. Both render no action. A disabled button would have + advertised an affordance that can never work, and pointing it at the Dashboard's + first query would have opened someone else's document. + +- **A flow KPI band member reaches its action through the card, not the host.** Flow + renders a KPI tile into a `.dash-kpi-member` host that carries no tile chrome of any + kind — no head, no delete, no grip, no resize — and is `display: contents`, so it + generates no box at all; that is also why the drag code derives its rect from the + host's children. Absolutely positioning the action against it put the button in the + Dashboard toolbar in a real browser, which happy-dom could not see. It is therefore + anchored INSIDE the member's first card, the same reach-through the `.is-nav-target` + ring and the `.dash-drop-target` outline already need for this host — which leaves + the drag geometry untouched, because the button sits inside one of the very child + boxes those rects are derived from. `renderKpiInto` replaces that card on every + publish, so the attachment is re-applied with each repaint rather than once. + (Giving the band a full chrome surface — it still has no delete affordance — remains + #475.) + +**Back is now a supported way home, which needed a per-entry memory.** #471's +acceptance criteria lean on ordinary history navigation precisely because the global +control is gone — and that exposed a hole #425 had left tolerable: the URL carries no +Dashboard id (by design, above), the Dashboard DOM is disposed when the Workbench +takes the work area, and `adoptRouteMainSurface` had nothing to consult once the +session said "query". It resolved the *compatibility* Dashboard, so Back out of a tile +opened the collection's FIRST Dashboard, at the top of the page, however many +Dashboards the user had moved through. + +The fix keeps the URL exactly as it was and puts the missing facts in +`history.state` instead — `{dash: {workspaceKey, dashboardId, currentMember, +scrollTop}}`, written onto the entry being LEFT (and onto a Dashboard entry as it is +created). Three properties made that the right home rather than a session-wide "last +Dashboard" memo: + +- it is **per entry**, so several Back steps across several Dashboards each restore + their own — a single memo can only ever be right about the most recent one; +- it is **invisible**, so shareable URLs are untouched and #425's "the URL is derived + from the session surface, never the other way round" still holds; +- it is **discardable**: an entry that carries none (a fresh load, or one written + before this existed) simply falls back to the old behaviour. + +It is validated like any other selection — `restoreDashboardSurface` runs the snapshot +through `reconcileMainSurface`, so a remembered Dashboard that has since been deleted +lands on **Query** rather than retargeting to a different one, and the snapshot is +rejected outright when its `workspaceKey` does not match (a Dashboard id is unique per +workspace, not globally — the #457 addendum's rule). + +The offset rides in `MainSurfaceState` as `pendingScrollTop`, a second one-shot +delivery beside `pendingFocus` and consumed with it, so no later repaint can yank a +page the user has since scrolled. Applying it is not a single write: at mount the grid +host is still empty (tiles arrive with the first publish, and grafana-grid's per-tile +px heights with them), so an offset written then clamps silently to `0`. It is +re-attempted after each publish until one sticks — which happy-dom cannot observe at +all, since it stores whatever was assigned. + +The mobile consequence is recorded here too, because it reverses part of the #425 +addendum. #426 had restored the back button specifically because the mobile rules +drop the sidebar *and* the bottom nav for a full-bleed Dashboard, and a per-tile +action cannot rescue a Dashboard with no tiles. Per the owner decision on #471, the +bottom nav stops hiding itself on this surface and shows only **Editor** — the other +two panel values still say nothing about a Dashboard — and pressing it switches +surface before selecting the panel, the same order `openSavedQuery` and +`openVariableTab` use. Full-bleed was a *width* claim; a bottom bar shortens the +Dashboard without overlapping it. + ## Alternatives considered - **Durable detached snapshots:** rejected because they silently diverge from diff --git a/src/application/main-surface.ts b/src/application/main-surface.ts index a5de9bbc..58d472ac 100644 --- a/src/application/main-surface.ts +++ b/src/application/main-surface.ts @@ -54,6 +54,19 @@ export type MainSurfaceState = mode: DashboardSurfaceMode; currentMember: DashboardFocusTarget | null; pendingFocus: DashboardFocusTarget | null; + /** + * #471 — a scroll offset still owed to the surface, consumed exactly once + * alongside `pendingFocus`. + * + * A second delivery field rather than part of `pendingFocus` because it has a + * different origin: focus comes from a navigation REQUEST, this comes from + * history. Opening a tile's query tears the Dashboard DOM down (the Workbench + * disposes it), so Back rebuilds it from scratch — and #471 removed the global + * control that used to make that round trip avoidable, which is what turns + * "scroll is lost" from a wart into a broken requirement. `null` means "start + * where a fresh render starts", i.e. the top. + */ + pendingScrollTop: number | null; }; /** The one application-level Dashboard navigation request (#425). */ @@ -61,6 +74,62 @@ export interface OpenDashboardRequest { dashboardId: string; mode: DashboardSurfaceMode; focus?: DashboardFocusTarget; + /** #471: only history restoration supplies this — see `restoreDashboardSurface`. */ + scrollTop?: number; +} + +/** + * What a Dashboard history entry has to remember (#471). + * + * Everything here is absent from the URL by design: #425 keeps the selected Dashboard + * id, the current member and the scroll offset as session state, so a history entry + * that records none of them cannot be returned to faithfully. Held in + * `history.state`, which is per-entry and invisible to the user, so multiple + * Back steps across several Dashboards each restore their own. + */ +export interface DashboardHistorySnapshot { + workspaceKey: string | null; + dashboardId: string; + currentMember: DashboardFocusTarget | null; + scrollTop: number; +} + +/** Read a snapshot back out of an opaque `history.state`, or `null` when the entry + * carries none (a fresh load, an entry written before this existed) or when it + * belongs to a DIFFERENT workspace — a Dashboard id is unique per workspace, not + * globally, so a coincidental match must not resolve. */ +export function readDashboardHistorySnapshot( + state: unknown, workspaceKey: string | null, +): DashboardHistorySnapshot | null { + if (typeof state !== 'object' || state === null) return null; + const dash = (state as { dash?: unknown }).dash; + if (typeof dash !== 'object' || dash === null) return null; + const candidate = dash as Partial; + if (typeof candidate.dashboardId !== 'string' || candidate.dashboardId === '') return null; + if ((candidate.workspaceKey ?? null) !== workspaceKey) return null; + return { + workspaceKey, + dashboardId: candidate.dashboardId, + currentMember: candidate.currentMember ?? null, + scrollTop: typeof candidate.scrollTop === 'number' && Number.isFinite(candidate.scrollTop) + ? candidate.scrollTop + : 0, + }; +} + +/** The snapshot for the surface currently on screen, or `null` in Query mode — + * written onto the history entry being LEFT, with the live scroll offset the DOM + * has right now. */ +export function dashboardHistorySnapshot( + surface: MainSurfaceState, workspaceKey: string | null, scrollTop: number, +): DashboardHistorySnapshot | null { + if (surface.kind !== 'dashboard') return null; + return { + workspaceKey, + dashboardId: surface.dashboardId, + currentMember: surface.currentMember, + scrollTop, + }; } /** The Query surface carries no parameters, so one frozen value serves every @@ -109,10 +178,44 @@ export function resolveOpenDashboard( mode: request.mode, currentMember: presentMember(lookup.dashboard, requested), pendingFocus: requested, + // #471: an OPEN request never restores a scroll offset — only history does + // (`restoreDashboardSurface`). Opening a Dashboard deliberately starts at the + // top; carrying a stale offset here would land the user mid-page for a + // Dashboard they navigated to fresh. + pendingScrollTop: request.scrollTop ?? null, }, }; } +/** + * Rebuild a Dashboard selection from a history entry (#471). + * + * The URL carries no Dashboard id (#425 keeps it session state), so returning to a + * Dashboard entry with Back had nothing to resolve and fell back to the collection's + * FIRST Dashboard — the wrong document whenever the user had selected another. The + * snapshot rides in `history.state` instead of the URL, which keeps the shareable URL + * unchanged and makes the memory per-history-entry rather than one global "last + * Dashboard". + * + * Validated like any other selection: a snapshot whose Dashboard is gone (or + * ambiguous) falls back to Query mode rather than retargeting, and a stale member + * reference drops itself. + */ +export function restoreDashboardSurface( + snapshot: DashboardHistorySnapshot, mode: DashboardSurfaceMode, workspace: WorkspaceDashboards | null, +): MainSurfaceState { + return reconcileMainSurface({ + kind: 'dashboard', + dashboardId: snapshot.dashboardId, + // The MODE comes from the route, not the snapshot: the URL does carry it, and it + // is the authority for what the entry describes. + mode, + currentMember: snapshot.currentMember, + pendingFocus: null, + pendingScrollTop: snapshot.scrollTop, + }, workspace); +} + /** * Re-validate a selection against committed truth. A selected Dashboard that * was removed — or whose id became ambiguous — falls back to **Query** mode @@ -205,8 +308,12 @@ export function isSameDashboardSelection( * marking the member the user navigated to after its one-shot delivery is * spent. */ export function withoutPendingFocus(surface: MainSurfaceState): MainSurfaceState { - if (surface.kind !== 'dashboard' || surface.pendingFocus === null) return surface; - return { ...surface, pendingFocus: null }; + if (surface.kind !== 'dashboard') return surface; + // #471: `pendingScrollTop` is handed to the same render pass and consumed with it, + // for the same reason — a later repaint must not yank the page back to an offset + // the user has since scrolled away from. + if (surface.pendingFocus === null && surface.pendingScrollTop === null) return surface; + return { ...surface, pendingFocus: null, pendingScrollTop: null }; } /** diff --git a/src/styles.css b/src/styles.css index 1fc99a46..4d3a46ec 100644 --- a/src/styles.css +++ b/src/styles.css @@ -2996,7 +2996,22 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } being later with equal specificity. */ .main-row[data-surface="dashboard"] .sidebar, .main-row[data-surface="dashboard"] .col-resize { display: none; } - .main-row[data-surface="dashboard"] ~ .mobile-nav { display: none; } + /* #471: the nav itself comes BACK on this surface. #471 removed the Dashboard + toolbar's generic Back-to-query button, and a per-tile Open-in-Workbench + action cannot rescue a Dashboard with no tiles — with the sidebar hidden too, + a phone would have had no route to the Workbench at all. Only Editor shows: + Tables and Results still say nothing about a Dashboard, and here Editor is a + DESTINATION (app-shell.ts switches surface first), never the panel already on + screen — so the active-tab treatment is suppressed. */ + .main-row[data-surface="dashboard"] ~ .mobile-nav .mobile-nav-btn[data-view="tables"], + .main-row[data-surface="dashboard"] ~ .mobile-nav .mobile-nav-btn[data-view="results"] { display: none; } + /* `[data-view]` is load-bearing, not decoration: the active-tab rules above are + `…[data-view="editor"]` (one attribute selector each), so a bare + `.mobile-nav-btn` here would LOSE on specificity and a Dashboard would render + Editor as the current tab. Equal specificity + later in the file wins. */ + .main-row[data-surface="dashboard"] ~ .mobile-nav .mobile-nav-btn[data-view] { color: var(--fg-faint); } + .main-row[data-surface="dashboard"] ~ .mobile-nav .mobile-nav-btn[data-view] .mnav-label { font-weight: var(--fw-medium); } + .main-row[data-surface="dashboard"] ~ .mobile-nav .mobile-nav-btn[data-view]::before { content: none; } /* Editor view: query tabs + toolbar + editor; hide the results half. */ .main-row[data-mobile-view="editor"] .results-region, .main-row[data-mobile-view="editor"] .editor-results-split { display: none; } @@ -3105,12 +3120,9 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } .dash-toolbar::-webkit-scrollbar { display: none; } .dash-toolbar > * { flex-shrink: 0; } .dash-toolbar-spacer { flex: 1 1 auto; min-width: 8px; } -/* #426: the visible route back to the Query surface, restored into the ONE compact - toolbar (#437 removed the separate row it used to live in, and #426 removed the - header pair that was covering for it). Icon-first, so the label can drop below - the breakpoint via the existing `.dash-back-label` rule. */ -.dash-back-to-query { display: inline-flex; align-items: center; gap: 5px; } -.dash-back-to-query svg { width: 12px; height: 12px; } +/* #471 removed this row's generic Back-to-query control (`.dash-back-to-query`): + leaving a Dashboard is a per-tile act now (`.dash-tile-open`), and the phone + route it protected lives in the bottom nav. */ /* The File-style layout menu in the header's top row, replacing the old `.dash-seg` segmented button group that lived in the variable toolbar. */ .dash-layout-wrap { display: inline-flex; align-items: center; flex: 0 0 auto; } @@ -3174,13 +3186,6 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } border-radius: var(--r-sm); cursor: pointer; } .dash-icobtn:hover { background: var(--bg-hover); color: var(--fg); } -.dash-back { - display: inline-flex; align-items: center; gap: 6px; color: var(--fg-mute); - text-decoration: none; font-size: var(--text-body); padding: 4px 9px; border-radius: var(--r-sm); - border: 1px solid var(--border); -} -.dash-back:hover { background: var(--bg-hover); color: var(--fg); } -.dash-back svg { transform: scaleX(-1); } .dash-title { flex: 1 1 auto; min-width: 0; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; @@ -3416,6 +3421,22 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } /* A member's stable host contributes its children (cards, or a state card) directly to the stream's flex-wrap — it is never itself a visible box. */ .dash-kpi-member { display: contents; } +/* #471: the member generates no box (`display: contents` above), so its tile action + is anchored inside the first CARD — the same reach-through `.is-nav-target` and + `.dash-drop-target` need. `position: relative` is scoped to the band, never put on + the shared `.kpi-card` base, which the Workbench KPI panel and the grid engine's + tile body also use. Revealed by hovering the CARD, since the member itself cannot + be hovered either. */ +.dash-kpi-stream .kpi-card, +.dash-kpi-stream .dash-kpi-state-card { position: relative; } +.dash-kpi-stream .kpi-card > .dash-tile-open, +.dash-kpi-stream .dash-kpi-state-card > .dash-tile-open { + position: absolute; top: 4px; right: 4px; margin: 0; +} +.dash-kpi-stream .kpi-card:hover > .dash-tile-open, +.dash-kpi-stream .kpi-card:focus-within > .dash-tile-open, +.dash-kpi-stream .dash-kpi-state-card:hover > .dash-tile-open, +.dash-kpi-stream .dash-kpi-state-card:focus-within > .dash-tile-open { opacity: 1; } .dash-grid.modkey .dash-kpi-member:hover > * { cursor: grab; } .dash-kpi-member.dash-drop-target > * { outline: 2px dashed var(--accent); outline-offset: -2px; @@ -3448,12 +3469,6 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } their fixed-position popovers. These are CSS-only overrides, so widening restores the persisted desktop layout without writing preferences. */ @media (max-width: 768px) { - .dash-back, - /* #426: icon-only at phone widths, like every other primary-toolbar control. */ - .dash-back-to-query { - width: 30px; height: 30px; padding: 0; justify-content: center; flex-shrink: 0; - } - .dash-back-label, .dash-src, .dash-updated, .dash-spacer { display: none; } @@ -3528,6 +3543,37 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } .dash-gg-grid .dash-gg-tile:hover .dash-gg-del, .dash-gg-del:focus-visible { opacity: 1; } .dash-gg-del:hover { color: var(--error-fg); background: var(--error-bg); } +/* #471 — the tile's own Open-in-Workbench action. Same subtle chrome as the delete + button beside it, with three deliberate differences: it is NOT scoped to + `.dash-gg-grid` (it exists in the flow renderer too), it is not edit-mode-only + (View mode is where inspecting a tile's query matters most), and `display` is + never used for the reveal — `opacity` is, so the button stays focusable and + `:focus-visible` can surface it for a keyboard user who cannot hover. */ +.dash-tile-open { + display: inline-flex; flex-shrink: 0; width: 20px; height: 20px; + align-items: center; justify-content: center; padding: 0; color: var(--fg-faint); + background: transparent; border: none; border-radius: var(--r-sm); cursor: pointer; + opacity: 0; transition: opacity .12s ease-out; +} +.dash-tile:hover .dash-tile-open, +.dash-tile:focus-within .dash-tile-open, +.dash-tile-open:focus-visible { opacity: 1; } +.dash-tile-open:hover { color: var(--accent-text); background: var(--bg-hover); } +.dash-tile-open svg { width: 13px; height: 13px; } +/* No `margin-left: auto` here — the heading's `flex: 1` already pushes the trailing + actions right, and a second auto margin next to `.dash-gg-del`'s would SPLIT the + free space between the two rather than pinning both to the right (visible only on + a KPI tile, whose heading is `display: none`; that head is squared up where it is + styled, below). */ +/* A touch device has no hover and no keyboard: subtlety there would mean + invisible. On a phone this action is also the primary route off a Dashboard into + the query behind a tile, so it is always shown. */ +@media (hover: none) { + .dash-tile-open { opacity: 1; } + /* The KPI tile's chrome OVERLAY needs the same treatment, but its rule has to sit + with the KPI block further down: the selector there has identical specificity, so + only source order decides, and an override here would lose. */ +} /* Corner-drag resize handle (bottom-right, edit mode only): nwse-resize cursor, hover-revealed diagonal glyph, accent highlight while resizing. */ .dash-gg-tile { position: relative; } @@ -3622,7 +3668,32 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } positioning host for the remove control. The card itself already exposes the query title through its accessible group name. */ .dash-gg-grid .dash-gg-tile.is-kpi > .dash-tile-head > .dash-tile-heading { display: none; } -.dash-gg-grid .dash-gg-tile.is-kpi > .dash-tile-head > .dash-gg-del { pointer-events: auto; } +/* With the heading gone this head has no `flex: 1` item to push its actions right, + so it squares them up itself — and `.dash-gg-del`'s own `margin-left: auto` has to + be dropped, or that one auto margin would absorb the free space and strand + #471's action at the LEFT edge instead. */ +.dash-gg-grid .dash-gg-tile.is-kpi > .dash-tile-head { justify-content: flex-end; } +.dash-gg-grid .dash-gg-tile.is-kpi > .dash-tile-head > .dash-gg-del { margin-left: 0; } +/* The head is a pointer-transparent overlay, so every control it holds has to opt + back in. #471's action is the first one that exists in View mode. */ +.dash-gg-grid .dash-gg-tile.is-kpi > .dash-tile-head > .dash-gg-del, +.dash-gg-grid .dash-gg-tile.is-kpi > .dash-tile-head > .dash-tile-open { pointer-events: auto; } +/* #471: a View-mode KPI tile used to have NO chrome to reveal, so its head stayed + hidden (`:not(.is-view)` on the reveal rules below). It has one control now, and + the issue requires it in both modes — so View mode reveals the head too. The + `::before` hover outline stays edit-only: that one advertises drag/resize. */ +.dash-gg-grid .dash-gg-tile.is-kpi.is-view:hover > .dash-tile-head, +.dash-gg-grid .dash-gg-tile.is-kpi.is-view:focus-within > .dash-tile-head { opacity: 1; } +/* #471, touch: every reveal rule for this overlay is `:hover`/`:focus-within`, and in + Edit mode additionally `:not(.is-view)` — a touch device satisfies none of them, so + the whole head (this action AND the delete button) was unreachable there. Ancestor + opacity composites, so `.dash-tile-open`'s own `opacity: 1` cannot rescue it. This + has to live HERE, next to the KPI block, rather than with that rule further up: + the base `…is-kpi > .dash-tile-head { opacity: 0 }` above has identical + specificity, so source order alone decides. */ +@media (hover: none) { + .dash-gg-grid .dash-gg-tile.is-kpi > .dash-tile-head { opacity: 1; } +} .dash-gg-grid .dash-gg-tile.is-kpi > .dash-tile-body { padding: 0; } .dash-gg-grid .dash-gg-tile.is-kpi .dash-gg-grip { display: none; } .dash-gg-grid .dash-gg-tile.is-kpi::before { diff --git a/src/ui/app-shell.ts b/src/ui/app-shell.ts index 3766c930..0cd2de38 100644 --- a/src/ui/app-shell.ts +++ b/src/ui/app-shell.ts @@ -82,8 +82,10 @@ export interface AppShellHandle { * but contributes no layout, so a Dashboard genuinely owns the whole * right-hand work area and no invisible result drawer consumes space. * - * Also mirrored onto `.main-row[data-surface]` for the mobile rules, which - * need to drop the sidebar and the bottom nav for a full-bleed Dashboard. + * Also mirrored onto `.main-row[data-surface]` for the mobile rules, which drop + * the sidebar for a full-bleed Dashboard and reduce the bottom nav to its Editor + * entry — #471's route back, now that the Dashboard toolbar carries no generic + * one. */ showHost(kind: SurfaceHostKind): void; dispose(): void; @@ -183,7 +185,18 @@ export function mountAppShell(deps: AppShellDeps): AppShellHandle { // so the badge stays here rather than duplicating the mobile-nav markup. app.dom.mobileBadge = h('span', { class: 'mnav-badge' }); const navBtn = (view: string, icon: SVGElement, label: string, extra?: HTMLElement): HTMLButtonElement => h('button', { - class: 'mobile-nav-btn', 'data-view': view, onclick: () => { state.mobileView.value = view as 'tables' | 'editor' | 'results'; }, + class: 'mobile-nav-btn', 'data-view': view, onclick: () => { + // #471: on the Dashboard surface this bar is a route OUT, not a panel + // switcher — #425 hid it here precisely because its three values say nothing + // about a Dashboard. #471 removed the Dashboard toolbar's generic + // Back-to-query button, and a per-tile action cannot rescue a Dashboard with + // no tiles, so the phone's route back lives here now: the CSS below leaves + // only Editor visible on this surface, and pressing it returns to the + // Workbench before selecting the panel — the same order + // `openSavedQuery`/`openVariableTab` use. + if (app.mainSurface.kind === 'dashboard') app.showQuerySurface(); + state.mobileView.value = view as 'tables' | 'editor' | 'results'; + }, }, h('span', { class: 'mnav-ic' }, icon, extra || null), h('span', { class: 'mnav-label' }, label)); app.dom.mobileNav = h('div', { class: 'mobile-nav' }, navBtn('tables', Icon.database(), 'Tables'), diff --git a/src/ui/app.ts b/src/ui/app.ts index a75aa101..9c1eaf8c 100644 --- a/src/ui/app.ts +++ b/src/ui/app.ts @@ -44,7 +44,7 @@ import { normalizeVariableSql } from '../core/dashboard-variables.js'; import { batch } from '@preact/signals-core'; import { renderResults } from './results.js'; import type { Result, QueryResult, ScriptResult, ScriptEntry } from './results.js'; -import { disposeDashboardSurface, renderDashboard } from './dashboard.js'; +import { dashboardScrollTop, disposeDashboardSurface, renderDashboard } from './dashboard.js'; import type { DashboardRenderTarget } from './dashboard.js'; import { toggleThemeDom } from './theme-toggle.js'; import { openSchemaView } from './explain-graph.js'; @@ -83,7 +83,8 @@ import { createAppPreferences } from '../application/app-preferences.js'; import { QUERY_SURFACE, isSameDashboardSelection, mainSurfaceRoute, reconcileMainSurface, carryCurrentMember, resolveOpenDashboard, selectedDashboardId, withCurrentMember, - withoutPendingFocus, + withoutPendingFocus, dashboardHistorySnapshot, readDashboardHistorySnapshot, + restoreDashboardSurface, } from '../application/main-surface.js'; import type { DashboardSurfaceMode, MainSurfaceState } from '../application/main-surface.js'; import { createWorkspaceRepository } from '../workspace/workspace-repository.js'; @@ -1749,6 +1750,9 @@ export function createApp(env: CreateAppEnv = {}): App { dashboardId: surface.kind === 'dashboard' ? surface.dashboardId : null, mode: surface.kind === 'dashboard' ? surface.mode : routeMode, focus: surface.kind === 'dashboard' ? surface.pendingFocus : null, + // #471: the offset a history restoration owes this render, consumed with the + // focus delivery below for the same reason. + scrollTop: surface.kind === 'dashboard' ? surface.pendingScrollTop : null, setHeader: (header) => mounted.setHeader(header), }; // #425: a focus target is delivered ONCE. Every later repaint of the same @@ -2309,9 +2313,32 @@ export function createApp(env: CreateAppEnv = {}): App { // Surface changes stay in this tab and create one useful history entry; // a View/Edit mode change replaces so presentation toggles do not pollute // Back (ADR-0003). + // #471 — write the Dashboard the CURRENT history entry is showing onto that entry, + // with the scroll offset the DOM has right now. + // + // The URL deliberately carries neither (#425 keeps the selected id and the offset as + // session state), so an entry that records nothing cannot be returned to: Back out + // of a tile's Open-in-Workbench used to land on the collection's first Dashboard, at + // the top. It has to run BEFORE the transition, because `pushState` leaves the + // outgoing entry's state exactly as it was last written — and again after writing a + // Dashboard route, so a freshly created entry carries its id immediately (Forward + // into it, or a second Back, restores the same way). + const stampDashboardHistoryEntry = (): void => { + const snapshot = dashboardHistorySnapshot( + app.mainSurface, app.sqlRoute.workspaceKey, dashboardScrollTop() ?? 0, + ); + // `null` (Query mode) is written too: it clears a snapshot this entry may carry + // from an earlier surface, so a Query entry never restores a Dashboard. + // Unguarded, exactly like `writeRoute` immediately below — a platform with no + // history API fails there on the same transition either way. + win.history.replaceState({ dash: snapshot }, '', conn.basePath + routeSearch + (loc.hash || '')); + }; + const applyMainSurface = (surface: MainSurfaceState, method: 'push' | 'replace'): void => { + stampDashboardHistoryEntry(); app.mainSurface = surface; writeRoute(mainSurfaceRoute(surface, surfaceRouteKey()), method); + if (surface.kind === 'dashboard') stampDashboardHistoryEntry(); // #426: the tree lives in the PERSISTENT shell, so a surface transition does // not repaint it as a side effect of re-rendering the work area — it needs // telling. Current Dashboard/member styling is derived from this state. @@ -2463,10 +2490,26 @@ export function createApp(env: CreateAppEnv = {}): App { app.mainSurface = reconcileMainSurface({ ...app.mainSurface, mode, pendingFocus: null }, workspace); return; } + // #471: the route says "a Dashboard" but carries no id, and the session no longer + // holds one (we are arriving from Query — typically Back out of a tile's + // Open-in-Workbench). The history ENTRY is the only thing that knows WHICH + // Dashboard this was, so it is consulted before the compatibility fallback: + // without it, Back reliably opened the collection's first Dashboard instead of + // the one the user left, at the top of the page. + const snapshot = readDashboardHistorySnapshot(win.history?.state, app.sqlRoute.workspaceKey); + if (snapshot) { + const restored = restoreDashboardSurface(snapshot, mode, workspace); + // A snapshot whose Dashboard is gone reconciles to Query; fall through to the + // compatibility entry only then, exactly as a boot with no snapshot does. + if (restored.kind === 'dashboard') { app.mainSurface = restored; return; } + } const selectedId = workspace ? resolveCompatibilityDashboard(workspace).selectedId : null; app.mainSurface = selectedId === null ? QUERY_SURFACE - : { kind: 'dashboard', dashboardId: selectedId, mode, currentMember: null, pendingFocus: null }; + : { + kind: 'dashboard', dashboardId: selectedId, mode, + currentMember: null, pendingFocus: null, pendingScrollTop: null, + }; }; app.reloadDashboardRoute = () => { diff --git a/src/ui/dashboard.ts b/src/ui/dashboard.ts index e4bbb92b..a5862328 100644 --- a/src/ui/dashboard.ts +++ b/src/ui/dashboard.ts @@ -41,7 +41,7 @@ import { movedPastThreshold, hitTestTile, resolveOverlapInsertIndex, flipDelta } import type { TileRect } from '../core/tile-reorder.js'; import { createDragAutoScroll } from '../core/dashboard-autoscroll.js'; import type { DragAutoScrollController, DragAutoScrollTarget, FrameScheduler } from '../core/dashboard-autoscroll.js'; -import { resolvePanel } from '../core/panel-cfg.js'; +import { isQuerylessPanel, resolvePanel } from '../core/panel-cfg.js'; import type { Column } from '../core/panel-cfg.js'; import { DASH_TILE_ROW_CAP, DASH_TABLE_DISPLAY_CAP } from '../core/dashboard.js'; import { @@ -96,7 +96,7 @@ import { loadJSON } from '../core/storage.js'; import { KEYS } from '../state.js'; import type { DashboardDocumentV2, DashboardFilterDefinitionV1, DashboardLayoutDocumentV1, FlowPresetV1, - SavedQueryV2, StoredWorkspaceV5, + Panel, SavedQueryV2, StoredWorkspaceV5, } from '../generated/json-schema.types.js'; import type { App, AppDom, ActionsRegistry } from './app.types.js'; import type { SqlRoute } from '../core/sql-route.js'; @@ -117,7 +117,7 @@ const Icon: { moon(): SVGElement; trash(): SVGElement; chevDown(): SVGElement; - chevLeft(): SVGElement; + code(): SVGElement; download(): SVGElement; upload(): SVGElement; search(): SVGElement; @@ -141,6 +141,10 @@ export interface DashboardRenderTarget { mode: DashboardSurfaceMode; /** Where to land navigation focus once this Dashboard's DOM exists. */ focus: DashboardFocusTarget | null; + /** #471: the scroll offset this render owes the page — a Dashboard returned to + * through history restores where the user left it, not the top. `null` for every + * ordinary render (open, mode switch, repaint), which starts at the top. */ + scrollTop: number | null; /** Install this surface's header into the shell's shared header slot. */ setHeader(header: Element): void; } @@ -163,12 +167,19 @@ export interface DashboardApp { currentWorkspace: StoredWorkspaceV5 | null; sqlRoute: SqlRoute; /** #425 — the selected-Dashboard session state this render projects, and the - * navigation API its own chrome (View/Edit, Back to query) transitions - * through. */ + * navigation API its own View/Edit control transitions through. (#471's per-tile + * Open in Workbench does not go through it: opening a document is not a surface + * transition the Dashboard performs — see `openSavedQuery` below.) */ mainSurface: App['mainSurface']; openDashboard: App['openDashboard']; showDashboardSurface: App['showDashboardSurface']; - showQuerySurface: App['showQuerySurface']; + /** #471: a tile's own `Open in Workbench` action, by the tile's `queryId` — the + * Dashboard-owned copy's stable id, which is what makes the opened tab, and + * every later Save from it, target this Dashboard's document. This surface no + * longer needs `showQuerySurface`: leaving a Dashboard used to be a generic + * toolbar act and is now always a document-opening one, which routes through + * here (`openSavedQuery` switches the surface itself). */ + openSavedQuery: App['openSavedQuery']; navigateSqlRoute(route: SqlRoute, method: 'push' | 'replace'): Promise; surfaceCommands: App['surfaceCommands']; keyboardOwner: App['keyboardOwner']; @@ -278,6 +289,19 @@ export function disposeDashboardSurface(): void { installedDashboardHost = null; } +/** + * The mounted Dashboard's live scroll offset, or `null` when none is mounted (#471). + * + * `.dash-page` is the scroll host — the grid scrolls under a sticky topbar, so the + * document itself never scrolls and `window.scrollY` would always read 0. Read at the + * moment the surface is left, because opening a tile's query disposes this DOM: the + * offset has to be recorded onto the history entry before it is gone. + */ +export function dashboardScrollTop(): number | null { + const page = installedDashboardHost?.querySelector('.dash-page'); + return page ? page.scrollTop : null; +} + /** Build the Dashboard style picker with the same trigger and dropdown * vocabulary as File. The trigger shows only the active style; `sync()` * reflects session changes without adding a second header label. */ @@ -417,32 +441,11 @@ function renderMissingDashboard( target.host.replaceChildren(h('div', { class: 'dash-page' }, h('div', { class: 'dash-topbar' }, h('div', { class: 'dash-toolbar dash-toolbar-primary' }, - buildBackToQuery(app), h('span', { class: 'dash-toolbar-spacer' }), buildDashboardModeSwitch(app, target.mode))), body)); } -/** - * #426 — the visible way back to the Query surface. - * - * #437 removed the separate Back-to-query row on the grounds that the - * application header's `SQL Browser | Dashboard` pair already did this. #426 then - * removed THAT pair (Dashboard selection moved to the sidebar tree), which - * together would have left `g w` and "click a saved query" as the only routes back - * — and neither is reachable on a phone, where the mobile rules drop the sidebar - * and the bottom nav for a full-bleed Dashboard. #426's own Header-cleanup section - * requires this control to be retained, so it returns here: icon-first and inside - * the ONE compact toolbar, per #437's design rather than as a second row. - */ -function buildBackToQuery(app: DashboardApp): HTMLElement { - return h('button', { - class: 'editor-mode-btn dash-back-to-query', type: 'button', - 'aria-label': 'Back to query', title: 'Back to query (G then W)', - onclick: () => { app.showQuerySurface(); }, - }, Icon.chevLeft(), h('span', { class: 'dash-back-label' }, 'Query')); -} - /** #425/#437: View/Edit — the other Dashboard-owned control the compact primary * toolbar carries. Switching retains the same Dashboard id (the main-surface API * keeps it — writing a route here would re-resolve the collection's first @@ -1265,13 +1268,48 @@ export async function renderDashboard( layoutMenu.sync(); } + // ── Restored scroll offset (#471) ───────────────────────────────────────── + // The offset a history entry owes this render (see `dashboardScrollTop`), applied + // after mount and again after every publish until it TAKES. + // + // A scroll offset cannot be set against a page that is not yet tall enough: at + // mount the grid host is empty — tiles are appended by the first publish, and + // grafana-grid's per-tile px heights are applied there too — so a single + // post-mount write clamped silently to 0. happy-dom cannot see that at all; it + // reports back whatever was assigned. + // + // Self-limiting: it stops on the first write that sticks, so ordinary later + // publishes (a refresh wave, a Search, a layout switch) never yank a page the user + // has since scrolled. A Dashboard that no longer reaches that offset keeps clamping + // to its own maximum, which is the closest honest answer. + // + // Declared HERE, above the publish effect, rather than beside the mount that + // supplies `scrollHost`: `effect()` runs its body immediately on creation, so a + // `let` declared after it would be in its temporal dead zone on that first run and + // abort the whole render. + let owedScrollTop = target.scrollTop; + let scrollHost: HTMLElement | null = null; + const applyOwedScroll = (): void => { + if (owedScrollTop === null || owedScrollTop <= 0 || scrollHost === null) return; + scrollHost.scrollTop = owedScrollTop; + if (scrollHost.scrollTop > 0) owedScrollTop = null; + }; + // ── Tile DOM ────────────────────────────────────────────────────────────── const tileEls = new Map(); // Flow KPI tiles do not render their cached `.dash-tile` card. Their // `.dash-kpi-member` host is the structural/movement surface instead. const flowKpiHosts = new Map(); - // #332: the origin card of a just-completed move whose synthesized click must - // be swallowed once (see wireTileDrag). Module-to-gesture, not per-card. + // #332: the origin card of a just-completed move whose synthesized click must be + // swallowed once (see wireTileDrag). Module-to-gesture, not per-card. + // + // #471: the arming now EXPIRES on its own (see `onUp`). `onUp` cannot know whether + // a click will follow — only a release back over the origin card produces one — so + // before this the flag could stay armed indefinitely after a cross-tile or + // empty-space release and eat an unrelated later click on that card. Clearing it on + // the next pointerdown was not enough: a KEYBOARD activation (Enter/Space on a tile + // action) dispatches a click with NO pointer event before it, so the very first + // keyboard press after such a drag did nothing at all. let clickSuppressCard: HTMLElement | null = null; // #332: at most one tile-drag gesture at a time — a second pointerdown while // one is armed is ignored, so two live listener sets can't cross-contaminate. @@ -1467,11 +1505,16 @@ export async function renderDashboard( const onPointerDown: EventListener = (event) => { const pe = event as PointerEvent; if (pe.button !== 0) return; // primary button only - // The resize handle (own stopPropagation) and delete button own their own - // gestures — never start a move from them. + // A fresh gesture never inherits a stale suppress. Belt and braces next to the + // timestamp window above — this covers a pointer gesture that begins inside + // the window, which the window alone would still swallow. + clickSuppressCard = null; + // The resize handle (own stopPropagation), the delete button and #471's + // Open-in-Workbench action own their own gestures — never start a move from + // them. Open-in-Workbench is the only one of the three that also exists in + // View mode, where this handler is never wired at all. const target = pe.target as Element; - if (target.closest('.dash-gg-resize, .dash-gg-del')) return; - clickSuppressCard = null; // a fresh gesture never inherits a stale suppress + if (target.closest('.dash-gg-resize, .dash-gg-del, .dash-tile-open')) return; // Start ONLY from the grip (no modifier), or from the body with ⌘/Ctrl. // A plain body press does neither → left alone for text selection. const fromGrip = !!target.closest('.dash-gg-grip'); @@ -1731,8 +1774,15 @@ export async function renderDashboard( if (!wasMoving) return; // never crossed the threshold: leave the click alone // A completed drag that releases back over its origin card synthesizes a // real click on it (same down/up target) — swallow it so no cell/link/ - // preview fires. A cross-tile release fires no origin click (harmless). + // preview fires. clickSuppressCard = card; + // #471: and a release ANYWHERE ELSE synthesizes no origin click at all, so + // nothing would ever consume this. The synthesized click, when there is one, + // is dispatched in the same input task as the release — so a zero-delay timer + // runs strictly after it, and disarms the flag before any later click can meet + // it. That later click may have no pointerdown to clear it (Enter on a focused + // tile action), which is exactly the case a pointerdown-only reset missed. + win.setTimeout(() => { if (clickSuppressCard === card) clickSuppressCard = null; }, 0); if (targetId && targetId !== tileId) { runCommand({ type: 'move-tile', tileId, toIndex: currentDoc.tiles.map((t) => t.id).indexOf(targetId) }); } @@ -1774,6 +1824,37 @@ export async function renderDashboard( }); } + /** + * #471 — the tile's own `Open in Workbench` action, or `null` when this tile has + * no query document to open. + * + * Deliberately NOT `!readOnly`-gated the way the grip and delete button are: + * inspecting the query behind a tile is a View-mode act first, and the issue + * requires the action in both modes. Built once per tile, like the heading. + * + * The tile's `queryId` IS the stable document origin this action needs. #427 made + * every panel tile reference a dedicated saved-query copy that exactly one member + * owns, so handing that id to `openSavedQuery` re-selects the tab already open on + * the SAME copy (`loadIntoNewTab` dedups on `savedId`, never on the displayed + * name) and every later Save from that tab keeps targeting this Dashboard's copy + * rather than a same-named Library query. Two Dashboards holding same-named + * copies are two ids, therefore two tabs. + * + * `null` — never a disabled-and-silent button, and never a button pointing at + * some other tile's document — when there is nothing to open: a `text` panel is + * queryless by capability (`isQuerylessPanel`, the same shared predicate Save and + * share use), and an unresolvable `queryId` belongs to a tile that is already + * rendering its own missing-query error. + */ + function tileOpenAction(ts: ViewerTileState): HTMLButtonElement | null { + if (!queryById.has(ts.queryId) || isQuerylessPanel(ts.panel as Panel | null)) return null; + return h('button', { + class: 'dash-tile-open', type: 'button', title: 'Open in Workbench', + 'aria-label': 'Open ' + ts.title + ' in Workbench', + onclick: () => { app.openSavedQuery(ts.queryId); }, + }, Icon.code()); + } + function ensureTileEl(ts: ViewerTileState): TileEl { const existing = tileEls.get(ts.tileId); if (existing) return existing; @@ -1793,12 +1874,13 @@ export async function renderDashboard( class: 'dash-gg-del', title: 'Remove tile', 'aria-label': 'Remove ' + ts.title + ' from the dashboard', onclick: () => { if (activeEngine === 'grafana-grid') runCommand({ type: 'remove-tile', tileId: ts.tileId }); }, }, Icon.trash()) : null; + const openBtn = tileOpenAction(ts); const heading = h('div', { class: 'dash-tile-heading' }, h('span', { class: 'dash-tile-name', title: ts.title }, ts.title), ts.description ? h('span', { class: 'dash-tile-desc', title: ts.description, }, ts.description) : null); - const head = h('div', { class: 'dash-tile-head' }, grip, heading, delBtn); + const head = h('div', { class: 'dash-tile-head' }, grip, heading, openBtn, delBtn); const body = h('div', { class: 'dash-tile-body' }); const foot = h('div', { class: 'dash-tile-foot' }); const resizeHandle = !readOnly @@ -1950,14 +2032,44 @@ export async function renderDashboard( // frameless view-mode tile has no visible header, so the state card is the // only surface that can announce which tile is loading/blocked/failed. function renderKpiInto(host: HTMLElement, ts: ViewerTileState): void { + host.replaceChildren(...kpiContent(ts)); + } + + /** + * #471 — give a FLOW band member the tile's Open-in-Workbench action. + * + * Flow-only, and called right after `renderKpiInto` repaints the member: the GRID + * engine renders a KPI tile as a real card whose head carries the action already, + * so doing this there would give one tile two of them. + * + * The action is anchored INSIDE the first card rather than on the member host, + * because `.dash-kpi-member` is `display: contents` and generates no box at all — + * an absolutely-positioned child of it resolves against the page instead, which put + * the button up in the Dashboard toolbar. That is the same reason the + * `.is-nav-target` ring and the `.dash-drop-target` outline already reach through to + * `> *`. Anchoring inside a card also leaves the drag geometry untouched: + * `surfaceRect`/`hitRects` still read the member's card children, and the button + * lives inside one of those boxes rather than beside them. + * + * One action per TILE, so it goes on the first card of a multi-field KPI — and on a + * state card too, because a loading or failed tile is still query-backed and its + * query is exactly what the user wants to look at. + */ + function attachFlowKpiOpenAction(host: HTMLElement, ts: ViewerTileState): void { + const anchor = host.firstElementChild; + const action = anchor ? tileOpenAction(ts) : null; + if (action) anchor!.appendChild(action); + } + + /** The KPI cards, or the one state card standing in for them (#316). */ + function kpiContent(ts: ViewerTileState): HTMLElement[] { if (ts.status !== 'ready') { const kind = ts.status === 'error' ? 'error' : ts.status === 'unfilled' ? 'unfilled' : 'loading'; const message = ts.status === 'error' ? (ts.error || 'Error') : ts.status === 'unfilled' ? 'Enter a value for: ' + ts.unfilled.join(', ') : 'Loading…'; - host.replaceChildren(h('div', { + return [h('div', { class: 'dash-kpi-state-card', role: kpiStateRole(kind), 'aria-label': `${ts.title}: ${message}`, - }, message)); - return; + }, message)]; } const panel = (ts.panel || {}) as Record; const resolved = resolvePanel(panel as Parameters[0], { @@ -1966,10 +2078,10 @@ export async function renderDashboard( serverVersion: state.serverVersion, }); const { cards, errors } = renderKpiCards(resolved.kpi); - host.replaceChildren(...(errors.length ? errors.map((e) => h('div', { + return errors.length ? errors.map((e) => h('div', { class: 'dash-kpi-state-card', role: kpiStateRole(e.code === 'kpi-no-data' ? 'zero-data' : 'error'), 'aria-label': `${ts.title}: ${e.message}`, - }, e.message)) : cards)); + }, e.message)) : cards; } // ── Grid reconciliation from the flow model ─────────────────────────────── @@ -2003,6 +2115,9 @@ export async function renderDashboard( 'aria-label': ts.title, title: !readOnly ? 'Command/Ctrl-drag to move' : undefined, }); + // #471's action is attached by `attachFlowKpiOpenAction` after the + // content paint below, not here: this host is `display: contents`, so the + // button has to live inside a CARD, and the cards do not exist yet. flowKpiHosts.set(member.tileId, host); if (!readOnly) wireTileDrag(member.tileId, host); stream.appendChild(host); @@ -2024,10 +2139,11 @@ export async function renderDashboard( } // A KPI band member's CONTENT (cards / state) is refreshed on every publish // — cheap, KPI cards carry no charts — so a member reaching ready repaints - // without a structural rebuild. + // without a structural rebuild. #471's action is re-attached with it, because + // that repaint replaces the very card it is anchored inside. for (const host of grid.querySelectorAll('.dash-kpi-member')) { const ts = byId.get((host as HTMLElement).dataset.tile || ''); - if (ts) renderKpiInto(host as HTMLElement, ts); + if (ts) { renderKpiInto(host as HTMLElement, ts); attachFlowKpiOpenAction(host as HTMLElement, ts); } } } @@ -2276,6 +2392,9 @@ export async function renderDashboard( } if (sview.layout.engine === 'grafana-grid') reconcileGrafanaGrid(sview, sview.layout.grid); else reconcileGrid(sview, sview.layout); + // #471: the tiles this publish just placed are what finally make the page tall + // enough to hold a restored offset. + applyOwedScroll(); // #437: the freshness control's icon-only refresh swaps in the spinner // while running, and its tooltip/aria-label carry the last-updated time // the visible `.dash-updated` span shows — `aria-busy` covers the running @@ -2309,14 +2428,14 @@ export async function renderDashboard( // #437: one compact toolbar row — style/count/search/time variables, then the // freshness control, then View/Edit last. The separate #425 surface row (a // Back-to-query button plus a title) is gone. - // #426: Back to query returns as the row's FIRST control — icon-first, inside - // this same one row rather than as a second band. #437 could drop it because the - // application header still carried `SQL Browser | Dashboard`; #426 removed that - // pair, and without this the only routes back would be `g w` and clicking a - // saved query — neither reachable on a phone, where the mobile rules drop the - // sidebar and bottom nav for a full-bleed Dashboard. + // #471: so is the generic Back-to-query control #426 had put back here. It named + // no document — it was ordinary back-navigation occupying primary toolbar space — + // and leaving a Dashboard is now either a per-tile act (`Open in Workbench`, + // which says WHICH query it opens) or ordinary history/tree navigation. The + // phone route #426 was protecting moved to the bottom nav, which no longer hides + // itself on this surface (app-shell.ts + the `[data-surface="dashboard"]` mobile + // rules): a Dashboard with no tiles at all would otherwise be a dead end. const primaryToolbar = h('div', { class: 'dash-toolbar dash-toolbar-primary' }, - buildBackToQuery(app), layoutWrap, tileCount, tileSearch, @@ -2331,10 +2450,13 @@ export async function renderDashboard( }, ordinaryVariableHost, clearVariablesBtn); installedDashboardHost = target.host; - target.host.replaceChildren(h('div', { class: 'dash-page' }, + const page = h('div', { class: 'dash-page' }, h('div', { class: 'dash-topbar' }, primaryToolbar, variableToolbar, variableRefreshLiveEl), - variableDiagnosticsHost, empty, searchEmpty, grid)); + variableDiagnosticsHost, empty, searchEmpty, grid); + target.host.replaceChildren(page); + scrollHost = page; + applyOwedScroll(); // Own every route-scoped resource in one teardown. An in-place Dashboard // rebuild must not leave Chart.js observers, signal effects, popovers, or diff --git a/tests/e2e/dashboard-mobile.html b/tests/e2e/dashboard-mobile.html index 05900638..2d2b152c 100644 --- a/tests/e2e/dashboard-mobile.html +++ b/tests/e2e/dashboard-mobile.html @@ -47,7 +47,14 @@ -
+ +
+ + + +
+ + + diff --git a/tests/e2e/tile-open-workbench.spec.js b/tests/e2e/tile-open-workbench.spec.js new file mode 100644 index 00000000..0db8295d --- /dev/null +++ b/tests/e2e/tile-open-workbench.spec.js @@ -0,0 +1,332 @@ +import { test, expect } from '@playwright/test'; + +// #471 — a Dashboard tile's own `Open in Workbench` action, replacing the +// Dashboard-level `< Query` button. +// +// Everything here is something the unit suite cannot reach: a subtle action that +// only a real browser reveals on hover/focus, a KPI tile whose head is a +// pointer-transparent overlay, the real pointer-drag engine the action must not +// trigger, the shipped tab strip the opened document lands in, and the mobile +// bottom nav that is a phone's route off a Dashboard now that the toolbar button +// is gone. + +const open = async (page, { width = 1280, height = 800 } = {}) => { + await page.setViewportSize({ width, height }); + await page.goto('/tests/e2e/tile-open-workbench.html'); + await page.waitForFunction(() => window.__ready === true); +}; + +const roleTab = (page, name) => page.locator('.upper-role-tabs .side-tab', { hasText: name }); +const treeRow = (page, key) => page.locator(`.dash-tree-row[data-key="${key}"]`); +const tabNames = (page) => page.locator('.qtab .name'); +const tabs = (page) => page.evaluate(() => window.__tabs()); +const surface = (page) => page.evaluate(() => window.__surface()); +/** The tile card whose name is `title`, and the action inside it. */ +const tileCard = (page, title) => page + .locator('.dash-tile', { has: page.locator('.dash-tile-name', { hasText: title }) }); +const tileAction = (page, title) => tileCard(page, title).locator('.dash-tile-open'); +const tileNames = (page) => page.locator('.dash-tile .dash-tile-name'); + +/** Open a Dashboard from the tree the way the shipped gestures do (#426): a single + * click only expands the row — double-click opens it in View, Shift-click in Edit. */ +const openDashboard = async (page, dashboardId, mode = 'view') => { + await roleTab(page, 'Dashboards').click(); + const row = treeRow(page, `workspace:${dashboardId}`); + if (mode === 'edit') await row.click({ modifiers: ['Shift'] }); + else await row.dblclick(); + await expect(page.locator('.dash-page')).toBeVisible(); +}; + +test('a tile action opens that tile\'s own document in the shipped tab strip', async ({ page }) => { + const pageErrors = []; + page.on('pageerror', (error) => pageErrors.push(error.message)); + await open(page); + await openDashboard(page, 'sales'); + + const action = tileAction(page, 'Live KPIs'); + // Subtle until the tile is hovered — but present in the DOM and named, not + // conjured on hover. + await expect(action).toHaveAttribute('aria-label', 'Open Live KPIs in Workbench'); + await expect(action).toHaveAttribute('title', 'Open in Workbench'); + expect(await action.evaluate((el) => getComputedStyle(el).opacity)).toBe('0'); + await tileCard(page, 'Live KPIs').hover(); + await expect.poll(() => action.evaluate((el) => getComputedStyle(el).opacity)).toBe('1'); + + await action.click(); + + // We are on the Workbench, and the tile's document is the ACTIVE tab. + await expect.poll(() => surface(page)).toBe('query'); + await expect(tabNames(page)).toHaveText(['Untitled', 'Live KPIs']); + const open2 = await tabs(page); + expect(open2.at(-1)).toMatchObject({ name: 'Live KPIs', savedId: 'q-sales', active: true }); + // The shipped SQL editor holds that document. + await expect.poll(() => page.evaluate(() => window.__app.sqlEditor.getValue())).toBe('SELECT 1 AS v'); + expect(pageErrors).toEqual([]); +}); + +test('two Dashboard copies with the SAME name open two tabs; re-opening selects the existing one', async ({ page }) => { + await open(page); + + // Dashboard `sales` → its copy of `Live KPIs`. + await openDashboard(page, 'sales'); + await tileAction(page, 'Live KPIs').click(); + await expect(tabNames(page)).toHaveText(['Untitled', 'Live KPIs']); + + // Dashboard `ops` → a DIFFERENT document that happens to share the name. + await openDashboard(page, 'ops'); + await tileAction(page, 'Live KPIs').click(); + await expect(tabNames(page)).toHaveText(['Untitled', 'Live KPIs', 'Live KPIs']); + expect((await tabs(page)).map((t) => t.savedId)).toEqual([null, 'q-sales', 'q-ops']); + + // Re-opening `sales`'s tile selects the tab it already has — no third copy, and + // the identity it selects by is the id, not the name it shares with `ops`. + await page.locator('.qtab').first().click(); + await openDashboard(page, 'sales'); + await tileAction(page, 'Live KPIs').click(); + await expect(tabNames(page)).toHaveText(['Untitled', 'Live KPIs', 'Live KPIs']); + const after = await tabs(page); + expect(after.filter((t) => t.active).map((t) => t.savedId)).toEqual(['q-sales']); +}); + +test('the action is keyboard reachable and activates on Enter', async ({ page }) => { + await open(page); + await openDashboard(page, 'sales'); + + const action = tileAction(page, 'Live KPIs'); + await action.focus(); + // Focus alone reveals it — a keyboard user never hovers. + await expect.poll(() => action.evaluate((el) => getComputedStyle(el).opacity)).toBe('1'); + await expect(action).toBeFocused(); + await page.keyboard.press('Enter'); + + await expect.poll(() => surface(page)).toBe('query'); + expect((await tabs(page)).at(-1)).toMatchObject({ savedId: 'q-sales', active: true }); +}); + +test('a KPI tile in VIEW mode still exposes a reachable action', async ({ page }) => { + // The hard case, and the reason this spec exists: a grafana-grid KPI tile is + // frameless, and its head is an absolutely-positioned `pointer-events: none` + // overlay that View mode used to keep hidden forever (nothing else in it exists + // outside Edit mode). Both the reveal and the pointer-events opt-in are pure CSS, + // so only a real browser can show the action is genuinely clickable rather than + // merely present in the DOM. + await open(page); + await openDashboard(page, 'sales'); + + const kpi = page.locator('.dash-gg-tile.is-kpi'); + await expect(kpi).toHaveCount(1); + const action = kpi.locator('.dash-tile-open'); + expect(await action.evaluate((el) => getComputedStyle(el.parentElement).opacity)).toBe('0'); + await kpi.hover(); + await expect.poll(() => action.evaluate((el) => getComputedStyle(el.parentElement).opacity)).toBe('1'); + await expect.poll(() => action.evaluate((el) => getComputedStyle(el).pointerEvents)).toBe('auto'); + // The KPI value is still what the tile shows — the chrome is an overlay. + await expect(kpi.locator('.kpi-card, .dash-kpi-state-card')).toHaveCount(1); + + // A real click, hit-tested through the overlay chain. + await action.click(); + await expect.poll(() => surface(page)).toBe('query'); + expect((await tabs(page)).at(-1)).toMatchObject({ savedId: 'q-kpi', active: true }); +}); + +test('a KPI tile in EDIT mode puts the action top-right, beside the delete button', async ({ page }) => { + // Geometry, because CSS decides it and happy-dom sees none of it: a KPI tile has + // no heading to push its actions right, so the head squares them up itself. Two + // competing `margin-left: auto` items would SPLIT the free space and leave this + // action floating mid-head. + await open(page); + await openDashboard(page, 'sales', 'edit'); + + const kpi = page.locator('.dash-gg-tile.is-kpi'); + await kpi.hover(); + const geometry = await kpi.evaluate((tile) => { + const head = tile.querySelector(':scope > .dash-tile-head').getBoundingClientRect(); + const open_ = tile.querySelector('.dash-tile-open').getBoundingClientRect(); + const del = tile.querySelector('.dash-gg-del').getBoundingClientRect(); + return { headLeft: head.left, headRight: head.right, headWidth: head.width, openLeft: open_.left, openRight: open_.right, delLeft: del.left, delRight: del.right }; + }); + // Both actions live in the right-hand quarter of the head, in reading order. + expect(geometry.openLeft).toBeGreaterThan(geometry.headLeft + geometry.headWidth * 0.75); + expect(geometry.openRight).toBeLessThanOrEqual(geometry.delLeft + 1); + expect(geometry.delRight).toBeLessThanOrEqual(geometry.headRight + 1); +}); + +test('the KPI overlay action is reachable by keyboard, which reveals the head', async ({ page }) => { + // The overlay is `opacity: 0` + `pointer-events: none` and is revealed by `:hover` + // and `:focus-within`. A keyboard user never hovers, so focus must be the whole + // path: reveal, then activate. + await open(page); + await openDashboard(page, 'sales'); + + const kpi = page.locator('.dash-gg-tile.is-kpi'); + const action = kpi.locator('.dash-tile-open'); + await action.focus(); + await expect(action).toBeFocused(); + await expect.poll(() => action.evaluate((el) => getComputedStyle(el.parentElement).opacity)).toBe('1'); + await page.keyboard.press('Enter'); + + await expect.poll(() => surface(page)).toBe('query'); + expect((await tabs(page)).at(-1)).toMatchObject({ savedId: 'q-kpi', active: true }); +}); + +test('Saving the opened tab updates the Dashboard copy, not the same-named sibling', async ({ page }) => { + // The acceptance criterion that the identity argument rests on, asserted directly: + // `q-sales` and `q-ops` are both named `Live KPIs`, each owned by a different + // Dashboard. Editing the one this tile opened must write only that copy. + await open(page); + await openDashboard(page, 'sales'); + await tileAction(page, 'Live KPIs').click(); + await expect.poll(() => surface(page)).toBe('query'); + + await page.locator('.cm-content[data-language="sql"]').click(); + await page.keyboard.press('ControlOrMeta+a'); + await page.keyboard.type('SELECT 99 AS v'); + await page.locator('.save-btn').click(); + + await expect.poll(async () => (await page.evaluate(() => window.__queries())) + .find((query) => query.id === 'q-sales').sql).toBe('SELECT 99 AS v'); + const queries = await page.evaluate(() => window.__queries()); + // The same-named copy in the OTHER Dashboard is untouched, and no new query was + // created (a name-keyed write would have hit one of these). + expect(queries.find((query) => query.id === 'q-ops').sql).toBe('SELECT 2 AS v'); + expect(queries.map((query) => query.id)).toEqual(['q-sales', 'q-ops', 'q-kpi', 'q-text']); + // The tab is clean again, and still linked to the Dashboard's copy. + await expect(page.locator('.qtab.active .dirty')).toHaveCount(0); + expect((await tabs(page)).at(-1)).toMatchObject({ savedId: 'q-sales' }); +}); + +test('Back returns to the Dashboard the tile belonged to, at the offset it was left at', async ({ page }) => { + // #471's own acceptance criteria: opening a tile's query must not disturb the + // Dashboard, and ordinary history navigation is the way back — which is exactly + // what removing the global `< Query` button leans on. The URL carries no Dashboard + // id, so before the history snapshot this landed on the collection's FIRST + // Dashboard, at the top of the page. Two Dashboards and a non-zero offset is the + // only shape that can tell the difference. + await open(page, { width: 1280, height: 600 }); + await openDashboard(page, 'ops'); // an entry for a DIFFERENT Dashboard first + await openDashboard(page, 'sales'); // …then the one we actually leave + + const scroller = page.locator('.dash-page'); + await scroller.evaluate((el) => { el.scrollTop = el.scrollHeight; }); + const left = await scroller.evaluate((el) => el.scrollTop); + expect(left, 'the fixture must be scrollable for this to mean anything').toBeGreaterThan(0); + + await tileAction(page, 'Live KPIs').click(); + await expect.poll(() => surface(page)).toBe('query'); + + await page.goBack(); + + await expect.poll(() => surface(page)).toBe('dashboard'); + await expect(page.locator('.dash-page')).toBeVisible(); + // The Dashboard the tile belonged to — `sales`, not `ops` and not the first entry. + expect(await page.evaluate(() => window.__app.mainSurface.dashboardId)).toBe('sales'); + // Its own tiles are what came back (`ops` has a single tile and no KPI). + await expect(page.locator('.dash-tile')).toHaveCount(3); + // …at the offset it was left at, not the top. + await expect.poll(() => page.locator('.dash-page').evaluate((el) => el.scrollTop)) + .toBeGreaterThan(left / 2); +}); + +test('a queryless (Text) tile exposes no action at all', async ({ page }) => { + await open(page); + await openDashboard(page, 'sales'); + + await expect(tileAction(page, 'Runbook')).toHaveCount(0); + // …while its query-backed siblings do have one (the panel tile + the KPI tile). + await expect(page.locator('.dash-tile-open')).toHaveCount(2); +}); + +test('pressing the action never starts a tile drag or reorders the Dashboard', async ({ page }) => { + await open(page); + // Edit mode is where the drag engine is wired at all. + await openDashboard(page, 'sales', 'edit'); + + const action = tileAction(page, 'Live KPIs'); + const card = tileCard(page, 'Live KPIs'); + const before = await card.boundingBox(); + const box = await action.boundingBox(); + + // A modifier-held press-and-drag FROM the action: the identical gesture on the + // tile body would arm a reorder. + await page.keyboard.down('ControlOrMeta'); + await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); + await page.mouse.down(); + await page.mouse.move(box.x + 240, box.y + 120, { steps: 12 }); + await page.mouse.up(); + await page.keyboard.up('ControlOrMeta'); + + await expect(page.locator('.dash-floating')).toHaveCount(0); + await expect(page.locator('.dash-grid.dash-reordering')).toHaveCount(0); + // The tile did not move, and the tile order is unchanged. + expect(await card.boundingBox()).toMatchObject({ x: before.x, y: before.y }); + await expect(tileNames(page)).toHaveText(['Revenue KPI', 'Live KPIs', 'Runbook']); + // Still on the Dashboard — a drag attempt is not an open. + expect(await surface(page)).toBe('dashboard'); +}); + +// A touch device matches neither `:hover` nor `:focus-visible`, so a hover-revealed +// action would be permanently invisible there — and on a phone this action is the +// only way into the query behind a tile. `@media (hover: none)` is what prevents +// that, and it needs real touch emulation to evaluate: a 360px desktop viewport +// still reports `hover: hover`. +test.describe('touch', () => { + test.use({ hasTouch: true, isMobile: true }); + + test('the action is permanently visible where there is no hover and no keyboard', async ({ page, browserName }) => { + test.skip(browserName === 'firefox', 'isMobile emulation is unsupported on Firefox'); + await open(page, { width: 390, height: 844 }); + await openDashboard(page, 'sales'); + + const action = tileAction(page, 'Live KPIs'); + // No hover, no focus — and still visible. + expect(await page.evaluate(() => matchMedia('(hover: none)').matches)).toBe(true); + await expect.poll(() => action.evaluate((el) => getComputedStyle(el).opacity)).toBe('1'); + // Including a KPI tile, whose whole chrome overlay is otherwise hover-revealed — + // and in EDIT mode, where that reveal is additionally `:not(.is-view)`-scoped, so + // a touch device satisfied neither half of it. + for (const mode of ['view', 'edit']) { + await openDashboard(page, 'sales', mode); + const kpiAction = page.locator('.dash-gg-tile.is-kpi .dash-tile-open'); + await expect.poll( + () => kpiAction.evaluate((el) => getComputedStyle(el.parentElement).opacity), + { message: `KPI head hidden on touch in ${mode} mode` }, + ).toBe('1'); + } + await openDashboard(page, 'sales'); + // The tap itself is chromium-only: WebKit's mobile emulation hit-tests a tap to + // the root element, which is a Playwright emulation limit rather than anything + // about this button (its click path is covered on both engines above). + test.skip(browserName !== 'chromium', 'tap emulation is chromium-only here'); + await action.tap(); + await expect.poll(() => surface(page)).toBe('query'); + expect((await tabs(page)).at(-1)).toMatchObject({ savedId: 'q-sales', active: true }); + }); +}); + +test('on a phone the bottom nav is the route off a Dashboard, and offers only Editor', async ({ page }) => { + // #471 removed the toolbar's `< Query` button. A Dashboard with no tiles has no + // per-tile action either, so the mobile rules stop hiding the bottom nav here — + // otherwise a phone would have no way back at all (the sidebar is hidden too). + // + // Opened at desktop width and then narrowed, because the tree that opens a + // Dashboard is itself hidden below the breakpoint — which is exactly the trap this + // test exists for. The mobile rules are CSS-only, so the resize is the whole + // transition. + await open(page); + await openDashboard(page, 'ops'); + await page.setViewportSize({ width: 360, height: 720 }); + + const nav = page.locator('.mobile-nav'); + await expect(nav).toBeVisible(); + await expect(nav.locator('.mobile-nav-btn:visible')).toHaveCount(1); + const editor = nav.locator('.mobile-nav-btn[data-view="editor"]'); + await expect(editor).toBeVisible(); + // Editor is a DESTINATION here, never the panel already on screen, so it must not + // be painted as the active tab. + expect(await editor.evaluate((el) => getComputedStyle(el, '::before').content)).toBe('none'); + + await editor.click(); + await expect.poll(() => surface(page)).toBe('query'); + // And it landed on the editor panel, not merely on the surface. + await expect(page.locator('.main-row')).toHaveAttribute('data-mobile-view', 'editor'); +}); diff --git a/tests/unit/app.test.ts b/tests/unit/app.test.ts index 8266969f..2233eae4 100644 --- a/tests/unit/app.test.ts +++ b/tests/unit/app.test.ts @@ -5055,6 +5055,41 @@ describe('mobile best-effort mode (#126)', () => { expect(mainRow.dataset.mobileView).toBe('results'); }); + // #471 — with the Dashboard toolbar's generic `< Query` button gone, and a + // per-tile Open-in-Workbench action unable to help a Dashboard that has no tiles, + // this bar is a phone's route to the Workbench: the mobile rules stop hiding it on + // the Dashboard surface, and Editor there means "leave the Dashboard" rather than + // "select a panel". Surface first, then the panel — the same order + // `openSavedQuery`/`openVariableTab` use, so the panel the user lands on is the + // one they asked for. + it('on the Dashboard surface, the Editor nav button returns to the Workbench', () => { + const { app } = mobileApp(true); + app.currentWorkspace = { + storageVersion: 5, id: 'w', key: 'workspace', name: 'W', queries: [], dashboards: [], + }; + app.workspaceRouteStatus = 'ready'; + app.mainSurface = { + kind: 'dashboard', dashboardId: 'd', mode: 'view', + currentMember: null, pendingFocus: null, pendingScrollTop: null, + }; + app.state.mobileView.value = 'tables'; + const showQuerySurface = vi.fn(); + app.showQuerySurface = showQuerySurface; + nav(app, 'editor').dispatchEvent(new Event('click', { bubbles: true })); + expect(showQuerySurface).toHaveBeenCalledOnce(); + expect(app.state.mobileView.value).toBe('editor'); + }); + + it('on the Query surface the same button only switches panels, never the surface', () => { + const { app } = mobileApp(true); + const showQuerySurface = vi.fn(); + app.showQuerySurface = showQuerySurface; + app.state.mobileView.value = 'tables'; + nav(app, 'editor').dispatchEvent(new Event('click', { bubbles: true })); + expect(showQuerySurface).not.toHaveBeenCalled(); + expect(app.state.mobileView.value).toBe('editor'); + }); + it('the Schema | Library segmented switches the sidebar pane (data-mobile-tab)', () => { const { app } = mobileApp(true); const sidebar = qs(app.root, '.sidebar'); @@ -5258,7 +5293,7 @@ describe('unified /sql routing', () => { const { app } = readyApp(['first', 'second']); app.openDashboard({ dashboardId: 'second', mode: 'view' }); expect(app.mainSurface).toEqual({ - kind: 'dashboard', dashboardId: 'second', mode: 'view', currentMember: null, pendingFocus: null, + kind: 'dashboard', dashboardId: 'second', mode: 'view', currentMember: null, pendingFocus: null, pendingScrollTop: null, }); expect(app.sqlRoute).toEqual({ surface: 'dashboard', workspaceKey: 'ops', mode: 'view' }); expect(app.renderCurrentSurface).toHaveBeenCalledTimes(1); @@ -5269,19 +5304,27 @@ describe('unified /sql routing', () => { app.openDashboard({ dashboardId: 'a', mode: 'edit', focus: { kind: 'tile', id: 't7' } }); expect(app.mainSurface).toEqual({ kind: 'dashboard', dashboardId: 'a', mode: 'edit', - currentMember: { kind: 'tile', id: 't7' }, pendingFocus: { kind: 'tile', id: 't7' }, + currentMember: { kind: 'tile', id: 't7' }, pendingFocus: { kind: 'tile', id: 't7' }, pendingScrollTop: null, }); }); it('pushes one history entry entering the Dashboard and REPLACES on a mode change', () => { const pushState = vi.spyOn(window.history, 'pushState').mockImplementation(() => {}); const replaceState = vi.spyOn(window.history, 'replaceState').mockImplementation(() => {}); + // #471 added a SECOND reason to call `replaceState`: stamping the Dashboard + // snapshot onto the current entry. The history discipline this test guards is + // about ENTRIES, and a stamp creates none — it is told apart by its STATE + // argument, since every route write passes `null` and only a stamp passes + // `{ dash: … }`. + const urlsReplaced = (): string[] => replaceState.mock.calls + .filter((call) => call[0] === null) + .map((call) => call[2] as string); const { app } = readyApp(['a']); app.openDashboard({ dashboardId: 'a', mode: 'edit' }); expect(pushState).toHaveBeenCalledWith(null, '', '/sql?ws=ops&surface=dashboard'); - expect(replaceState).not.toHaveBeenCalled(); + expect(urlsReplaced()).toEqual([]); app.openDashboard({ dashboardId: 'a', mode: 'view' }); - expect(replaceState).toHaveBeenCalledWith(null, '', '/sql?ws=ops&surface=dashboard&mode=view'); + expect(urlsReplaced()).toEqual(['/sql?ws=ops&surface=dashboard&mode=view']); expect(pushState).toHaveBeenCalledTimes(1); // Leaving for the Query surface is a real navigation again. app.showQuerySurface(); @@ -5334,7 +5377,7 @@ describe('unified /sql routing', () => { app.openDashboard({ dashboardId: 'a', mode: 'edit' }); expect(app.renderCurrentSurface).toHaveBeenCalledTimes(renders); expect(app.mainSurface).toEqual({ - kind: 'dashboard', dashboardId: 'a', mode: 'edit', currentMember: null, pendingFocus: null, + kind: 'dashboard', dashboardId: 'a', mode: 'edit', currentMember: null, pendingFocus: null, pendingScrollTop: null, }); expect(app.state.dashboardTreeRevision.value).toBeGreaterThan(revision); }); @@ -5362,7 +5405,7 @@ describe('unified /sql routing', () => { // the port already delivered it. expect(app.mainSurface).toEqual({ kind: 'dashboard', dashboardId: 'a', mode: 'edit', - currentMember: { kind: 'tile', id: 't7' }, pendingFocus: null, + currentMember: { kind: 'tile', id: 't7' }, pendingFocus: null, pendingScrollTop: null, }); expect(app.state.dashboardTreeRevision.value).toBeGreaterThan(revision); }); @@ -5380,7 +5423,7 @@ describe('unified /sql routing', () => { expect(app.renderCurrentSurface).toHaveBeenCalledTimes(renders + 1); expect(app.mainSurface).toEqual({ kind: 'dashboard', dashboardId: 'a', mode: 'edit', - currentMember: { kind: 'variable', id: 'p' }, pendingFocus: { kind: 'variable', id: 'p' }, + currentMember: { kind: 'variable', id: 'p' }, pendingFocus: { kind: 'variable', id: 'p' }, pendingScrollTop: null, }); }); @@ -5394,7 +5437,7 @@ describe('unified /sql routing', () => { // The Dashboard stays open and unchanged; nothing is marked current. expect(app.renderCurrentSurface).toHaveBeenCalledTimes(renders); expect(app.mainSurface).toEqual({ - kind: 'dashboard', dashboardId: 'a', mode: 'edit', currentMember: null, pendingFocus: null, + kind: 'dashboard', dashboardId: 'a', mode: 'edit', currentMember: null, pendingFocus: null, pendingScrollTop: null, }); expect([...document.querySelectorAll('.share-toast')].at(-1)?.textContent) .toContain('That panel is no longer on this dashboard.'); @@ -5419,7 +5462,7 @@ describe('unified /sql routing', () => { app.showDashboardSurface('view'); expect(app.mainSurface).toEqual({ kind: 'dashboard', dashboardId: 'a', mode: 'view', - currentMember: { kind: 'tile', id: 't7' }, pendingFocus: null, + currentMember: { kind: 'tile', id: 't7' }, pendingFocus: null, pendingScrollTop: null, }); }); @@ -5429,7 +5472,7 @@ describe('unified /sql routing', () => { app.openDashboard({ dashboardId: 'a', mode: 'edit', focus: { kind: 'tile', id: 't7' } }); app.openDashboard({ dashboardId: 'b', mode: 'edit' }); expect(app.mainSurface).toEqual({ - kind: 'dashboard', dashboardId: 'b', mode: 'edit', currentMember: null, pendingFocus: null, + kind: 'dashboard', dashboardId: 'b', mode: 'edit', currentMember: null, pendingFocus: null, pendingScrollTop: null, }); }); @@ -5506,7 +5549,7 @@ describe('unified /sql routing', () => { const { app } = readyApp(['first', 'second']); app.showDashboardSurface('edit'); expect(app.mainSurface).toEqual({ - kind: 'dashboard', dashboardId: 'first', mode: 'edit', currentMember: null, pendingFocus: null, + kind: 'dashboard', dashboardId: 'first', mode: 'edit', currentMember: null, pendingFocus: null, pendingScrollTop: null, }); }); @@ -5523,7 +5566,7 @@ describe('unified /sql routing', () => { app.openDashboard({ dashboardId: 'second', mode: 'edit' }); app.showDashboardSurface('view'); expect(app.mainSurface).toEqual({ - kind: 'dashboard', dashboardId: 'second', mode: 'view', currentMember: null, pendingFocus: null, + kind: 'dashboard', dashboardId: 'second', mode: 'view', currentMember: null, pendingFocus: null, pendingScrollTop: null, }); }); @@ -5577,7 +5620,7 @@ describe('unified /sql routing', () => { dashboards: [dash('shared'), dash('x')], }); expect(kept.mainSurface).toEqual({ - kind: 'dashboard', dashboardId: 'shared', mode: 'view', currentMember: null, pendingFocus: null, + kind: 'dashboard', dashboardId: 'shared', mode: 'view', currentMember: null, pendingFocus: null, pendingScrollTop: null, }); }); @@ -5767,7 +5810,7 @@ describe('unified /sql routing', () => { // after the focus ring has moved on. expect(app.mainSurface).toEqual({ kind: 'dashboard', dashboardId: 'b', mode: 'edit', - currentMember: { kind: 'tile', id: 't1' }, pendingFocus: null, + currentMember: { kind: 'tile', id: 't1' }, pendingFocus: null, pendingScrollTop: null, }); }); @@ -5779,12 +5822,83 @@ describe('unified /sql routing', () => { // The URL carries no Dashboard id, so re-deriving one here would silently // retarget the surface to the collection's first entry. expect(app.mainSurface).toEqual({ - kind: 'dashboard', dashboardId: 'second', mode: 'view', currentMember: null, pendingFocus: null, + kind: 'dashboard', dashboardId: 'second', mode: 'view', currentMember: null, pendingFocus: null, pendingScrollTop: null, }); location.search = '?ws=ops'; await app.handleSqlPopState(); expect(app.mainSurface).toEqual({ kind: 'query' }); }); + + // #471 — the failure this issue's own acceptance criteria made unacceptable. + // Opening a tile's query pushes a Query entry and DISPOSES the Dashboard DOM, so + // Back rebuilds from scratch. The URL carries no Dashboard id, and the session + // now says "query", so the old fallback resolved the collection's FIRST + // Dashboard: the user left `second`, scrolled, and came back to `first` at the + // top. #471 removed the global control that made that round trip avoidable, so + // the history ENTRY has to remember instead. + it('Back out of a tile\'s query returns to the Dashboard the user LEFT', async () => { + const stamps: unknown[] = []; + const replaceState = vi.spyOn(window.history, 'replaceState') + .mockImplementation((state) => { stamps.push(state); }); + const { app, location } = readyApp(['first', 'second'], '?ws=ops&surface=dashboard'); + app.openDashboard({ dashboardId: 'second', mode: 'view' }); + // A tile's Open-in-Workbench: back to the Query surface, pushing an entry. On + // the way out the OUTGOING entry is stamped with what the URL cannot carry. + app.showQuerySurface(); + expect(app.mainSurface).toEqual({ kind: 'query' }); + const leaving = stamps.at(-1); + expect(leaving).toEqual({ + dash: { workspaceKey: 'ops', dashboardId: 'second', currentMember: null, scrollTop: 0 }, + }); + replaceState.mockRestore(); + + // Back. The browser restores the entry AND its state; this fixture never + // navigates, so the restoration is applied by hand — the point under test is + // what `adoptRouteMainSurface` does with it, given a route that says only + // "a Dashboard, in view mode". + window.history.replaceState(leaving, '', '/sql?ws=ops&surface=dashboard&mode=view'); + location.search = '?ws=ops&surface=dashboard&mode=view'; + await app.handleSqlPopState(); + expect(app.mainSurface).toMatchObject({ + kind: 'dashboard', dashboardId: 'second', mode: 'view', + // A restored entry owes no focus DELIVERY: the ring is not re-flashed. (The + // remembered `currentMember` round trip is covered in main-surface.test.ts, + // where a fixture can hold a Dashboard that actually contains the tile.) + pendingFocus: null, + }); + // The offset rides in the same snapshot; this fixture mounts no Dashboard, so + // there is none to record. `dashboard.test.ts` covers capture and restore + // against real DOM, and the e2e covers the whole round trip in a browser. + expect(app.mainSurface).toMatchObject({ pendingScrollTop: 0 }); + }); + + it('ignores a remembered Dashboard from a DIFFERENT workspace', async () => { + // A Dashboard id is unique per workspace, not globally (#457), so a snapshot + // left by another workspace must not resolve even when the id collides. + const { app, location } = readyApp(['first', 'second'], '?ws=ops&surface=dashboard'); + window.history.replaceState( + { dash: { workspaceKey: 'somewhere-else', dashboardId: 'second', currentMember: null, scrollTop: 90 } }, + '', location.search, + ); + app.mainSurface = { kind: 'query' }; + location.search = '?ws=ops&surface=dashboard'; + await app.handleSqlPopState(); + // Falls back to the compatibility entry, exactly as a boot with no snapshot + // does — never to the other workspace's remembered id. + expect(app.mainSurface).toMatchObject({ dashboardId: 'first', pendingScrollTop: null }); + }); + + it('falls back to the compatibility Dashboard when the remembered one is gone', async () => { + const { app, location } = readyApp(['first', 'second'], '?ws=ops&surface=dashboard'); + window.history.replaceState( + { dash: { workspaceKey: 'ops', dashboardId: 'deleted-since', currentMember: null, scrollTop: 90 } }, + '', location.search, + ); + app.mainSurface = { kind: 'query' }; + location.search = '?ws=ops&surface=dashboard'; + await app.handleSqlPopState(); + expect(app.mainSurface).toMatchObject({ dashboardId: 'first' }); + }); }); it('rewrites a newly created workspace key without changing the active surface', () => { @@ -5867,7 +5981,10 @@ describe('unified /sql routing', () => { // select, then re-project — a SAME-workspace projection, which is the only // kind that keeps a selection. app.applyCommittedWorkspace(workspace); - app.mainSurface = { kind: 'dashboard', dashboardId: 'second', mode: 'edit', currentMember: null, pendingFocus: null }; + app.mainSurface = { + kind: 'dashboard', dashboardId: 'second', mode: 'edit', + currentMember: null, pendingFocus: null, pendingScrollTop: null, + }; // Project through the real path: `state.dashboard` is whatever // `applyCommittedWorkspace` put there — the SELECTED document — never a // hand-made one production could not produce. @@ -5921,7 +6038,10 @@ describe('unified /sql routing', () => { // A selection pinned before the entry was deleted elsewhere: the fold must // not guess into another slot. (`state.dashboard` still holds the document // that surface was editing.) - app.mainSurface = { kind: 'dashboard', dashboardId: 'deleted', mode: 'edit', currentMember: null, pendingFocus: null }; + app.mainSurface = { + kind: 'dashboard', dashboardId: 'deleted', mode: 'edit', + currentMember: null, pendingFocus: null, pendingScrollTop: null, + }; app.state.dashboard = { ...only, id: 'deleted', revision: 9 }; app.renderDashboard = vi.fn(); app.reloadDashboardRoute(); diff --git a/tests/unit/dashboard-tree-model.test.ts b/tests/unit/dashboard-tree-model.test.ts index 2d8be010..fa45053d 100644 --- a/tests/unit/dashboard-tree-model.test.ts +++ b/tests/unit/dashboard-tree-model.test.ts @@ -26,7 +26,9 @@ const ws = (over: Partial = {}): TreeWorkspace => ({ const onDashboard = ( dashboardId: string, currentMember: { kind: 'tile' | 'variable'; id: string } | null = null, -): MainSurfaceState => ({ kind: 'dashboard', dashboardId, mode: 'edit', currentMember, pendingFocus: null }); +): MainSurfaceState => ({ + kind: 'dashboard', dashboardId, mode: 'edit', currentMember, pendingFocus: null, pendingScrollTop: null, +}); /** Expand a Dashboard and both its groups — the usual "show me everything" state. */ const allOpen = (dashboardIds: string[]): DashboardTreeUiState => { diff --git a/tests/unit/dashboard-tree.test.ts b/tests/unit/dashboard-tree.test.ts index f3ca8fcb..5eea4260 100644 --- a/tests/unit/dashboard-tree.test.ts +++ b/tests/unit/dashboard-tree.test.ts @@ -247,7 +247,7 @@ describe('renderDashboardTree — structure and ARIA', () => { it('marks the current Dashboard and member distinctly from keyboard focus', () => { const surface: MainSurfaceState = { kind: 'dashboard', dashboardId: 'sales', mode: 'edit', - currentMember: { kind: 'tile', id: 't1' }, pendingFocus: null, + currentMember: { kind: 'tile', id: 't1' }, pendingFocus: null, pendingScrollTop: null, }; const { app, list } = treeApp({ mainSurface: surface }); openAll(app, 'sales'); diff --git a/tests/unit/dashboard.test.ts b/tests/unit/dashboard.test.ts index d39c7fdc..8259e96c 100644 --- a/tests/unit/dashboard.test.ts +++ b/tests/unit/dashboard.test.ts @@ -7,7 +7,7 @@ import { KEYS } from '../../src/state.js'; import { VARIABLE_OPTION_CAP } from '../../src/core/variable-options.js'; import * as storage from '../../src/core/storage.js'; import { CHART_ROW_CAPS } from '../../src/core/chart-data.js'; -import { renderDashboard } from '../../src/ui/dashboard.js'; +import { dashboardScrollTop, disposeDashboardSurface, renderDashboard } from '../../src/ui/dashboard.js'; import type { DashboardRenderTarget } from '../../src/ui/dashboard.js'; import { applyCommand } from '../../src/dashboard/application/dashboard-commands.js'; import { createQueryResolver } from '../../src/dashboard/application/dashboard-query-resolver.js'; @@ -290,6 +290,9 @@ function dashApp(opts: { dashboardId: current?.dashboards[0]?.id ?? null, mode: 'edit', focus: null, + // #471: no owed scroll offset — only a history restoration supplies one, and a + // test that wants it passes `scrollTop` to `render`. + scrollTop: null, setHeader: (header) => { headerSlot.replaceChildren(header); }, }); let surfaceGeneration = 0; @@ -4061,9 +4064,9 @@ describe('renderDashboard — unified live modes (#407)', () => { expect(qs(app.root, '.dash-surface-toolbar')).toBeNull(); expect(qs(app.root, '.dash-surface-title')).toBeNull(); const primary = qs(app.root, '.dash-toolbar-primary'); - // #426: Back to query lives IN this one row now (see the regression test - // below for why it had to come back at all). - expect(qs(primary, '.dash-back-to-query')).not.toBeNull(); + // #471: and no generic Back-to-query control — #426 had put one here; leaving a + // Dashboard is a per-tile act now (see the Open-in-Workbench tests below). + expect(qs(primary, '.dash-back-to-query')).toBeNull(); expect(qsa(app.root, '.dash-toolbar')).toHaveLength(2); // primary + filters // The View/Edit switch reflects the RENDERED mode, and lives in this toolbar. expect(qsa(primary, '.dashboard-mode-switch .editor-mode-btn') @@ -4078,31 +4081,36 @@ describe('renderDashboard — unified live modes (#407)', () => { expect(qs(app.root, '.dash-surface-title')).toBeNull(); expect(qs(app.root, '.dash-surface-toolbar')).toBeNull(); // Acceptance: "The empty-Dashboard state uses the same one-row toolbar - // treatment" — one `.dash-toolbar-primary`, carrying the mode switch. #426: - // and Back to query, so the placeholder is never a dead end either. + // treatment" — one `.dash-toolbar-primary`, carrying the mode switch. expect(qsa(app.root, '.dash-toolbar')).toHaveLength(1); expect(qs(app.root, '.dash-toolbar-primary .dashboard-mode-switch')).not.toBeNull(); - expect(qs(app.root, '.dash-toolbar-primary .dash-back-to-query')).not.toBeNull(); - }); - - // REGRESSION GUARD. #437 removed the Back-to-query control because the header - // still carried `SQL Browser | Dashboard`; #426 removed that pair. Together they - // would have left `g w` and "click a saved query" as the only ways out of a - // Dashboard — and on a phone the mobile rules drop the sidebar and the bottom - // nav, so there would have been NO reachable route back at all. - it('always offers a VISIBLE route back to the Query surface', async () => { + expect(qs(app.root, '.dash-toolbar-primary .dash-back-to-query')).toBeNull(); + }); + + // REGRESSION GUARD, restated by #471. #437 removed the Back-to-query control + // because the header still carried `SQL Browser | Dashboard`; #426 removed that + // pair and put the control back, because on a phone the mobile rules drop the + // sidebar and the bottom nav — so a Dashboard would have had NO reachable route + // back at all. #471 removes the control for good, which is only safe because the + // route moved rather than vanished: per-tile Open-in-Workbench here, plus the + // bottom nav, which no longer hides itself on this surface (asserted against the + // real shell in app.test.ts — a tile-less Dashboard has only that one). + it('offers no generic back control in either mode, in favour of the per-tile route', async () => { for (const mode of ['view', 'edit'] as const) { - const { app } = modeApp({ workspace: wsWith(), mode }); + const ws = wsWith({ queries: [q('q1', 'SELECT 1')], tiles: [{ id: 't1', queryId: 'q1' }] }); + const { app } = modeApp({ workspace: ws, mode }); const showQuerySurface = vi.fn(); + const openSavedQuery = vi.fn(); app.showQuerySurface = showQuerySurface; + app.openSavedQuery = openSavedQuery; await render(app); - const back = qs(app.root, '.dash-toolbar-primary .dash-back-to-query'); - expect(back.getAttribute('aria-label')).toBe('Back to query'); - // Discoverable: the shortcut is named in the tooltip rather than being the - // only way to find the action. - expect(back.getAttribute('title')).toContain('G then W'); - back.click(); - expect(showQuerySurface).toHaveBeenCalledOnce(); + expect(qs(app.root, '.dash-back-to-query')).toBeNull(); + expect(qsa(app.root, '.dash-toolbar-primary .dash-tile-open')).toHaveLength(0); + const open = qs(app.root, '.dash-tile .dash-tile-open'); + open.click(); + // The tile action names a DOCUMENT; it never means "generic back". + expect(openSavedQuery).toHaveBeenCalledExactlyOnceWith('q1'); + expect(showQuerySurface).not.toHaveBeenCalled(); } }); @@ -5234,3 +5242,317 @@ describe('renderDashboard — navigation focus (#425)', () => { }); }); }); + +// ── #471: the per-tile Open-in-Workbench action ───────────────────────────── +// Replaces the Dashboard toolbar's generic `< Query` button. The contract that +// matters is IDENTITY: the action carries the tile's own `queryId` — the +// Dashboard-owned copy #427 gave it — so the tab it opens (and every Save from +// that tab) targets this Dashboard's document, never a same-named Library query. +describe('renderDashboard — per-tile Open in Workbench (#471)', () => { + const openBtns = (app: TestApp): HTMLButtonElement[] => qsa(app.root, '.dash-tile-open'); + const oneTile = (): WsOver => ({ + queries: [q('q1', 'SELECT 1')], + tiles: [{ id: 't1', queryId: 'q1' }], + }); + + it('exposes the action on a query-backed tile in BOTH modes, in BOTH layout engines', async () => { + for (const mode of ['view', 'edit'] as const) { + for (const layout of [ + { type: 'flow', version: 1, preset: 'columns-2', items: {} }, + { type: 'grafana-grid', version: 1, items: { t1: { span: 4 } } }, + ]) { + const { app } = modeApp({ workspace: wsWith({ ...oneTile(), layout }), mode }); + await render(app); + const buttons = openBtns(app); + expect(buttons, `${mode}/${layout.type}`).toHaveLength(1); + // A real