fix(#429): resolve a saved query before navigating, trim blank tile titles (phase 1)

CHANGELOG.md

@@ -109,6 +109,25 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
   no row appears, disappears or moves when the work surface changes.
 
 ### Fixed
+- **Opening a saved query resolves it before it navigates** (#443, #429). Handing
+  `openSavedQuery` an id that names nothing used to switch to the Query surface
+  and push a history entry first, then discover the query was missing — opening
+  no tab, showing no diagnostic, and leaving the user somewhere they never asked
+  to be. It now resolves first: a miss reports *"That query is no longer part of
+  this workspace."* and changes no surface, no route and no tab, while a resolving id
+  behaves exactly as before.
+
+- **A whitespace-only tile title no longer blanks a heading or a screen-reader
+  name** (#476, #429). `tile.title` carries no `minLength`, so a hand-authored or
+  imported `"   "` is a legal document — and being truthy it beat the query-name
+  fallback, leaving the visible `.dash-tile-name` empty and composing accessible
+  names like *"Open, — , in Workbench"*. The Dashboard viewer now trims the
+  authored title before the fallback, so such a title behaves exactly like an
+  absent one and shows the query name instead. A title with surrounding
+  whitespace is kept, trimmed. This changes what existing documents carrying such
+  a title display; no shipped UI writes `tile.title`, so only hand-authored and
+  imported documents are affected.
+
 - **A degraded workspace export no longer loses a Dashboard** (#463). When the
   committed read is unavailable — blocked, over-quota or private-mode IndexedDB —
   the File menu rebuilds the workspace from its live projection. That projection

src/dashboard/application/dashboard-viewer-session.ts

@@ -550,7 +550,16 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa
     const isKpi = type === 'kpi';
     const isText = type === 'text';
     const explicit: Panel | null = isObject(panel) && isObject(panel.cfg) ? (panel as unknown as Panel) : null;
-    const title = (typeof tile.title === 'string' && tile.title) || (query ? queryName(query) : tile.queryId) || tile.id;
+    // #476 — TRIM before the fallback, so a whitespace-only authored title
+    // behaves exactly like an absent one. `dashboardTileV1.title` carries no
+    // `minLength`, so `"   "` is a schema-legal document; left truthy it won the
+    // chain unfiltered and composed blank accessible names ("Open, — , in
+    // Workbench") and a blank `.dash-tile-name` heading. This is the ONE place
+    // the viewer resolves a tile's display title, so trimming here settles it for
+    // every consumer of `state.title` (`tileLabels`, the parameter-analysis
+    // labels, and all of `ui/dashboard.ts`'s composed names alike).
+    const authored = typeof tile.title === 'string' ? tile.title.trim() : '';
+    const title = authored || (query ? queryName(query) : tile.queryId) || tile.id;
     const description = (typeof tile.description === 'string' && tile.description)
       || (typeof query?.spec?.description === 'string' ? query.spec.description : '');
     const state: ViewerTileState = {

src/ui/app.ts

@@ -1895,10 +1895,12 @@ export function createApp(env: CreateAppEnv = {}): App {
     // #426: a deferred single-click was scheduled against the rows of a PROJECTION,
     // and every projection replaces them — not just a workspace switch. Deleting a
     // Dashboard inside the 300ms window would otherwise let the delayed toggle
-    // re-add the id that was just pruned, and a deleted panel's deferred open would
-    // reach `openSavedQuery` with a dead id. Cancelling unconditionally can drop a
+    // re-add the id that was just pruned. Cancelling unconditionally can drop a
     // click when a background commit lands mid-gesture, which is the cheaper error:
-    // the rows that click referred to are gone either way.
+    // the rows that click referred to are gone either way. (#443 removed the other
+    // half of this rationale: a deleted panel's deferred open reaching
+    // `openSavedQuery` with a dead id is now handled at the callee, which reports
+    // and stays put rather than navigating nowhere.)
     cancelDashboardTreeClicks(app);
     invalidateDashboardTree();
     // #425: COMPLETE the fallback, don't just record it. Rewriting the route and
@@ -2443,13 +2445,27 @@ export function createApp(env: CreateAppEnv = {}): App {
 
   // Opening a saved query is a Query-mode act: it returns to the preserved
   // Query surface first, so the tab it opens is the one the user then sees.
+  //
+  // #443 — RESOLVE BEFORE NAVIGATING. Switching first meant an id that resolves
+  // to nothing yanked the user off whatever surface they were on and pushed a
+  // history entry, then opened no tab and said nothing — a dead click that also
+  // lost their place. Report it the way `openDashboard` reports a missing
+  // Dashboard, and leave surface and route exactly as they were. Every current
+  // caller (`dashboard-tree.ts`'s open-query command and its post-assignment
+  // reveal, `dashboard.ts`'s Open in Workbench) addresses a query it just
+  // resolved or just created, so none depended on the unconditional switch.
   app.openSavedQuery = (queryId) => {
     const query = app.state.savedQueries.find((saved) => saved.id === queryId);
+    if (!query) {
+      flashToast('That query is no longer part of this workspace.', { document: doc });
+      return;
+    }
     app.showQuerySurface();
     // Spread, like saved-history.ts's own two call sites: `loadIntoNewTab`
     // accepts the looser `string | Json` shape a `SavedQueryV2` satisfies
     // structurally but not nominally (no index signature).
-    if (query) { loadIntoNewTab(app, { ...query }); toEditorOnMobile(); }
+    loadIntoNewTab(app, { ...query });
+    toEditorOnMobile();
   };
 
   // #457 — opening a variable's option SQL is a Query-mode act for exactly the

src/ui/app.types.ts

@@ -469,7 +469,9 @@ export interface App {
    *  compatibility Dashboard and opens it by id, falling back to the Dashboard
    *  surface's own "Create dashboard" state for an empty collection. */
   showDashboardSurface(mode: DashboardSurfaceMode): void;
-  /** #425 — open a saved query into a tab, switching back to Query mode first. */
+  /** #425 — open a saved query into a tab, switching back to Query mode first.
+   *  #443 — the id is resolved BEFORE anything moves: one that names no saved
+   *  query reports a diagnostic and changes no surface, no route and no tab. */
   openSavedQuery(queryId: string): void;
   /** #457 — open (or re-select) the main-editor tab that edits ONE Dashboard
    *  variable's option SQL, switching back to Query mode first. A variable is

src/ui/dashboard.ts

@@ -2182,8 +2182,9 @@ export async function renderDashboard(
     tileEl.card.classList.toggle('is-kpi', ts.isKpi);
     if (ts.isKpi) {
       tileEl.card.setAttribute('role', 'group');
-      // (`ts.title` is never empty — the session falls back through query
-      // name → queryId → tile id when the tile has no explicit title.)
+      // (`ts.title` is never blank — the session trims the authored title (#476)
+      // and falls back through query name → queryId → tile id when the tile has
+      // no explicit title, or only whitespace for one.)
       tileEl.card.setAttribute('aria-label', ts.title);
     } else {
       tileEl.card.removeAttribute('role');

tests/unit/app.test.ts

@@ -23,6 +23,7 @@ import { savedQuery } from '../helpers/saved-query.js';
 import { fakeIndexedDbFactory } from '../helpers/fake-idb.js';
 import { fakeBroadcastBus } from '../helpers/fake-broadcast.js';
 import { decodeShare } from '../../src/core/share.js';
+import { flashToast } from '../../src/ui/toast.js';
 import type { CreateAppEnv, BroadcastChannelPort } from '../../src/env.types.js';
 import type {
   WorkspaceCommitResult, WorkspaceLoadResult, WorkspaceMarkOpenedResult,
@@ -5660,6 +5661,33 @@ describe('unified /sql routing', () => {
       expect(app.state.tabs.value.some((tab) => tab.savedId === 'owned')).toBe(true);
     });
 
+    // #443: the id used to be handed to `showQuerySurface` before anything knew
+    // whether it resolved, so a dead click threw the user off the surface they
+    // were on, pushed a history entry, opened no tab and said nothing. It stays
+    // put now, and says so once.
+    it('an unresolved saved-query id changes no surface, no route and no tab, and reports once', () => {
+      const { app } = readyApp(['a'], '?ws=ops&surface=dashboard');
+      app.openDashboard({ dashboardId: 'a', mode: 'edit' });
+      const surface = app.mainSurface;
+      const route = app.sqlRoute;
+      const tabs = app.state.tabs.value.length;
+      app.state.savedQueries = [savedQuery({ id: 'q1', name: 'Sales', sql: 'SELECT 1' })];
+      // `flashToast` REUSES one `.share-toast` element per document, so counting
+      // elements can never distinguish one report from two. Materialise the
+      // element, then count how many times a report RAISES it.
+      flashToast('seed', { document });
+      const toastEl = document.querySelector('.share-toast')!;
+      const raised = vi.spyOn(toastEl.classList, 'add');
+
+      app.openSavedQuery('gone');
+
+      expect(app.mainSurface).toEqual(surface);
+      expect(app.sqlRoute).toEqual(route);
+      expect(app.state.tabs.value.length).toBe(tabs);
+      expect(raised).toHaveBeenCalledExactlyOnceWith('show');
+      expect(toastEl.textContent).toBe('That query is no longer part of this workspace.');
+    });
+
     it('clears the surface and invalidates pending Dashboard callbacks on sign-out', () => {
       const { app } = readyApp(['a'], '?ws=ops&surface=dashboard');
       app.openDashboard({ dashboardId: 'a', mode: 'edit' });

tests/unit/dashboard-viewer-session.test.ts

@@ -602,6 +602,48 @@ describe('variables and the affected-panel planner', () => {
     expect(session.state.value.tiles.map((entry) => entry.tileId)).toEqual(['a', 'b', 'c', 'd']);
     session.setTileSearch(''); // identical search is a no-op
   });
+
+  // #476 (via #429 phase 1) — `dashboardTileV1.title` carries no `minLength`, so
+  // a whitespace-only title is a schema-legal document. It used to be truthy and
+  // win the fallback chain unfiltered, leaving `state.title` blank — which is
+  // what `ui/dashboard.ts` composes every accessible name and the visible
+  // heading from. This is a deliberate BEHAVIOUR CHANGE for such documents: they
+  // start showing the query name.
+  it('treats a whitespace-only tile title as absent, and trims a padded one', async () => {
+    const { exec } = makeExec();
+    const document = doc({
+      tiles: [
+        tile('blank', 'qa', { title: '   \t\n ' }),
+        tile('padded', 'qb', { title: '  Revenue  ' }),
+        tile('absent', 'qc'),
+        // No query resolves for this one, so the chain must fall through the
+        // trimmed title PAST the query name to the queryId.
+        tile('orphan', 'qd', { title: ' ' }),
+      ],
+    });
+    const session = createDashboardViewerSession(makeDeps({
+      document, exec,
+      queries: [
+        // The two titled tiles declare ONE variable name at conflicting types, so
+        // the conflict diagnostic below is composed from their resolved labels.
+        query('qa', 'SELECT {p:String} AS n', { name: 'Query A' }),
+        query('qb', 'SELECT {p:Int32} AS n', { name: 'Query B' }),
+        query('qc', 'SELECT 3', { name: 'Query C' }),
+      ],
+    }));
+    await session.start();
+    expect(session.state.value.tiles.map((entry) => entry.title))
+      .toEqual(['Query A', 'Revenue', 'Query C', 'qd']);
+    // The resolved titles are also what labels a variable-conflict diagnostic —
+    // one of the composed names a blank title used to hollow out.
+    const conflicted = session.state.value.variables.find((variable) => variable.name === 'p');
+    expect(conflicted?.diagnostic).toContain('Query A');
+    expect(conflicted?.diagnostic).toContain('Revenue');
+    // …and a blank title is searchable by the query name it now falls back to,
+    // rather than by nothing at all.
+    session.setTileSearch('query a');
+    expect(session.state.value.tiles.map((entry) => entry.tileId)).toEqual(['blank']);
+  });
 });
 
 describe('variable-bar bridge (controls / getVariableField / applyVariable)', () => {

tests/unit/dashboard.test.ts

@@ -5294,6 +5294,38 @@ describe('renderDashboard — per-tile Open in Workbench (#471)', () => {
     expect(button.getAttribute('aria-label')).toBe('Open Revenue by day in Workbench');
   });
 
+  // #476 (via #429 phase 1) — a whitespace-only `tile.title` is schema-legal and
+  // used to win the fallback chain, so the heading rendered blank and every
+  // composed name announced as "Open, — , in Workbench". Asserted at the RENDER
+  // layer, not just at the session that resolves the title, because these three
+  // strings are the user-visible consequence the acceptance criterion names.
+  it('a whitespace-only tile title reaches neither the heading nor any composed name', async () => {
+    const ws = wsWith({
+      queries: [q('q1', 'SELECT 1', { name: 'Revenue by day' })],
+      tiles: [{ id: 't1', queryId: 'q1', title: '   ' }],
+      layout: { type: 'grafana-grid', version: 1, items: { t1: { span: 4 } } },
+    });
+    // Edit mode, so the destructive control's label is in the render too.
+    const { app } = modeApp({ workspace: ws, mode: 'edit' });
+    await render(app);
+    expect(qs(app.root, '.dash-tile-name')?.textContent).toBe('Revenue by day');
+    expect(qs(app.root, '.dash-tile-name')?.getAttribute('title')).toBe('Revenue by day');
+    expect(openBtns(app)[0].getAttribute('aria-label')).toBe('Open Revenue by day in Workbench');
+    expect(qs(app.root, '.dash-gg-del')?.getAttribute('aria-label'))
+      .toBe('Remove Revenue by day from the dashboard');
+  });
+
+  it('keeps an authored title, trimmed of surrounding whitespace', async () => {
+    const ws = wsWith({
+      queries: [q('q1', 'SELECT 1', { name: 'Revenue by day' })],
+      tiles: [{ id: 't1', queryId: 'q1', title: '  Q3 revenue  ' }],
+    });
+    const { app } = modeApp({ workspace: ws, mode: 'view' });
+    await render(app);
+    expect(qs(app.root, '.dash-tile-name')?.textContent).toBe('Q3 revenue');
+    expect(openBtns(app)[0].getAttribute('aria-label')).toBe('Open Q3 revenue in Workbench');
+  });
+
   it('opens the tile\'s own document — same-named copies in different tiles are different ids', async () => {
     // The #464/#471 hazard, at its sharpest: `cloneQueryForDashboardOwner` copies
     // the source NAME verbatim, so two Dashboard copies of one Library query are