diff --git a/CHANGELOG.md b/CHANGELOG.md index 484746ca..b1509777 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,22 @@ auto-generated per-PR notes; this file is the curated, human-readable history. a Logs panel, and a Text panel explaining the demo. ### Fixed +- **Opening a Filter-role saved or shared query now always starts in the + Filter preview** (#244, folds in #249). Library-row activation previously + restored whichever result view (Table/JSON/Panel) was already open, + independent of the query's Dashboard role; a Filter-role query with SQL + that can't auto-run (empty/DDL, reachable via an import or legacy + localStorage entry that bypassed SQL-shape validation) fell through to no + view change at all. Separately, the share-link/OAuth-handoff bootstrap path + only ever restored `resultView` for a *queryless* Panel link — any + SQL-bearing shared Filter query, or a SQL-bearing shared Panel query with a + persisted `spec.view`, always landed on the default Table view. Both call + sites now resolve the same `rolePreviewView(spec) || queryView(query)` + precedence (`src/core/result-choice.js`): a role-owned transient preview + wins over a persisted view, which wins over the queryless-Panel/default + fallback. The preview stays transient — no `spec.view: "filter"` is ever + persisted — and ordinary reruns after the query is open continue to + preserve the user's current Table/JSON/Filter selection. - Review follow-ups on the Dashboard Filter sources work above, found in a UI/UX pass on #232 before merge: a curated field's clear (×) button now reports the cleared value (not the stale prior selection) to diff --git a/src/core/result-choice.js b/src/core/result-choice.js index 56a23c10..ce08f030 100644 --- a/src/core/result-choice.js +++ b/src/core/result-choice.js @@ -27,6 +27,18 @@ export function effectiveDashboardRole(spec) { return typeof role === 'string' && role ? role : 'panel'; } +// The transient preview a role owns on initial Library launch — never a +// persisted `spec.view` (Filter has none to persist; #244). `null` defers to +// the query's persisted view / the caller's other fallbacks. +export function rolePreviewView(spec) { + switch (effectiveDashboardRole(spec)) { + case 'filter': + return 'filter'; + default: + return null; + } +} + export function resultChoiceForSpec(spec) { if (effectiveDashboardRole(spec) === 'filter') return 'role:filter'; const type = spec?.panel?.cfg?.type; diff --git a/src/main.js b/src/main.js index 37ea37fe..736dcd77 100644 --- a/src/main.js +++ b/src/main.js @@ -14,6 +14,8 @@ import { exchangeCodeForTokens, bearerFromTokens } from './net/oauth.js'; import { decodeShare } from './core/share.js'; import { cloneJson, queryName, queryPanel, queryView, upgradeSavedQuery } from './core/saved-query.js'; import { isDashboardRoute } from './core/dashboard.js'; +import { rolePreviewView } from './core/result-choice.js'; +import { isQuerylessPanel } from './core/panel-cfg.js'; import { setTabSpecDraft } from './state.js'; export async function bootstrap(app, env) { @@ -86,11 +88,17 @@ export async function bootstrap(app, env) { t0.name = queryName(shared); t0.specVersion = shared.specVersion; setTabSpecDraft(t0, cloneJson(shared.spec)); - if (panel && panel.cfg) { - // A panel-only link (no SQL to run) must open the Panel drawer, or - // the recipient lands on an empty Table view and sees nothing. - if (!shared.sql) app.state.resultView.value = queryView(shared) || 'panel'; - } + // Restore the initial result view with the same role-aware precedence as + // Library activation (#244): a role-owned transient preview (Filter) + // wins over the persisted view, regardless of whether the share carries + // SQL to run — a SQL-bearing share never auto-runs here, so this only + // pre-selects the drawer the recipient lands on before they click Run. + const launchView = rolePreviewView(shared.spec) || queryView(shared); + if (launchView) app.state.resultView.value = launchView === 'chart' ? 'panel' : launchView; + // A queryless panel with no role/persisted view (no SQL to run) still + // needs the Panel drawer open, or the recipient lands on an empty Table + // view and sees nothing. + else if (!shared.sql && isQuerylessPanel(panel)) app.state.resultView.value = 'panel'; hist.replaceState(null, '', loc.pathname + loc.search); } } diff --git a/src/ui/saved-history.js b/src/ui/saved-history.js index bcf52041..f8366b9c 100644 --- a/src/ui/saved-history.js +++ b/src/ui/saved-history.js @@ -14,7 +14,7 @@ import { import { isAutoRunnable } from '../core/sql-split.js'; import { isQuerylessPanel } from '../core/panel-cfg.js'; import { queryDescription, queryFavorite, queryName, queryPanel, queryView } from '../core/saved-query.js'; -import { effectiveDashboardRole } from '../core/result-choice.js'; +import { effectiveDashboardRole, rolePreviewView } from '../core/result-choice.js'; import { filterRoleBadge } from './tabs.js'; // Make a Library/History row draggable; dropping it on the editor inserts the @@ -114,7 +114,12 @@ function renderSaved(app, list) { const name = queryName(q); const description = queryDescription(q); const panel = queryPanel(q); - const view = queryView(q); + // Library launch precedence (#244): a role-owned transient preview (Filter) + // wins over the persisted view, even when dormant Panel state persists a + // `spec.view` of its own — the role reflects the query's *current* + // intended representation, the dormant view is just preserved for later. + const rolePreview = rolePreviewView(q.spec); + const launchView = rolePreview || queryView(q); const star = h('button', { class: 'sv-star' + (favorite ? ' on' : ''), title: favorite ? 'Unfavorite' : 'Favorite', onclick: (e) => { @@ -135,8 +140,13 @@ function renderSaved(app, list) { // nothing. `run({view})` handles the auto-runnable path as before. const open = () => { app.actions.loadIntoNewTab(q); - if (isAutoRunnable(q.sql)) app.actions.run({ view }); - else if (SAVED_VIEWS.has(view)) app.state.resultView.value = view; + if (isAutoRunnable(q.sql)) app.actions.run({ view: launchView }); + // A role-owned preview isn't in SAVED_VIEWS (it's transient, never + // persisted — #244) but still wins here: a Filter-role entry that can't + // auto-run (e.g. empty/DDL SQL from an import that skipped validation) + // still opens the Filter drawer, which renders its own empty state, + // rather than falling through to a dormant Table/JSON/Panel view. + else if (rolePreview || SAVED_VIEWS.has(launchView)) app.state.resultView.value = launchView; // A queryless panel without a remembered view (hand-authored/imported // file) still needs the Panel drawer open, or clicking it shows nothing. else if (isQuerylessPanel(panel)) app.state.resultView.value = 'panel'; diff --git a/tests/unit/main.test.js b/tests/unit/main.test.js index 202c326d..c0979ba7 100644 --- a/tests/unit/main.test.js +++ b/tests/unit/main.test.js @@ -181,6 +181,84 @@ describe('bootstrap', () => { }); }); + // v2 share hash: { __asb: 2, query: { sql, specVersion, spec } } (src/core/share.js). + const v2Hash = (query) => '#' + btoa(unescape(encodeURIComponent(JSON.stringify({ + __asb: 2, query: { specVersion: 1, spec: { name: 'Shared query', favorite: false }, ...query }, + })))); + const v2Env = (query) => { + const hash = v2Hash(query); + return fakeEnv({ location: { href: 'https://ch/sql' + hash, origin: 'https://ch', pathname: '/sql', search: '', hash } }); + }; + + it('opens Filter for a v2 share carrying Filter-role SQL, before any run is possible (#244)', async () => { + const app = fakeApp(); + const env = v2Env({ sql: 'SELECT 1', spec: { name: 'Shared query', favorite: false, dashboard: { role: 'filter' } } }); + await bootstrap(app, env); + expect(app.state.tabs.value[0].sqlDraft).toBe('SELECT 1'); + expect(app.state.resultView.value).toBe('filter'); + }); + + it('Filter role wins over a dormant persisted view:"panel" carried in a share (#244)', async () => { + const app = fakeApp(); + const panelCfg = { cfg: { type: 'kpi' } }; + const env = v2Env({ + sql: 'SELECT 1', + spec: { name: 'Shared query', favorite: false, view: 'panel', dashboard: { role: 'filter' }, panel: panelCfg }, + }); + await bootstrap(app, env); + expect(app.state.resultView.value).toBe('filter'); + // dormant Panel state and the persisted view survive untouched in the tab's Spec. + expect(app.state.tabs.value[0].specParsed.view).toBe('panel'); + expect(app.state.tabs.value[0].specParsed.panel).toEqual(panelCfg); + }); + + it('restores a SQL-bearing shared Panel query\'s persisted view:"panel" (no role)', async () => { + const app = fakeApp(); + const panelCfg = { cfg: { type: 'kpi' } }; + const env = v2Env({ sql: 'SELECT 1', spec: { name: 'Shared query', favorite: false, view: 'panel', panel: panelCfg } }); + await bootstrap(app, env); + expect(app.state.resultView.value).toBe('panel'); + }); + + it.each(['table', 'json'])('restores a SQL-bearing shared query\'s persisted %s preference', async (view) => { + const app = fakeApp(); + const env = v2Env({ sql: 'SELECT 1', spec: { name: 'Shared query', favorite: false, view } }); + await bootstrap(app, env); + expect(app.state.resultView.value).toBe(view); + }); + + it('leaves the default result view alone for a share with no role and no persisted view', async () => { + const app = fakeApp(); + const env = v2Env({ sql: 'SELECT 1' }); + await bootstrap(app, env); + expect(app.state.resultView.value).toBe('table'); // fakeApp()'s untouched default + }); + + it('restores Filter for a Filter-role share stashed through the OAuth round-trip (#244)', async () => { + const app = fakeApp({ token: valid, isSignedIn: () => true }); + const env = fakeEnv({ location: { href: 'https://ch/sql', origin: 'https://ch', pathname: '/sql', search: '', hash: '' } }); + env.sessionStorage.setItem('oauth_shared', JSON.stringify({ + sql: 'SELECT 1', specVersion: 1, spec: { name: 'Shared query', favorite: false, dashboard: { role: 'filter' } }, + })); + await bootstrap(app, env); + expect(app.state.resultView.value).toBe('filter'); + }); + + it('maps a legacy persisted view:"chart" through the Panel compatibility path in a share', async () => { + const app = fakeApp(); + const panelCfg = { cfg: { type: 'pie', x: 0, y: [1], series: null } }; + const env = v2Env({ sql: 'SELECT 1', spec: { name: 'Shared query', favorite: false, view: 'chart', panel: panelCfg } }); + await bootstrap(app, env); + expect(app.state.resultView.value).toBe('panel'); + }); + + it('never persists a transient view:"filter" into the restored tab Spec (#244)', async () => { + const app = fakeApp(); + const env = v2Env({ sql: 'SELECT 1', spec: { name: 'Shared query', favorite: false, dashboard: { role: 'filter' } } }); + await bootstrap(app, env); + expect(app.state.tabs.value[0].specParsed.view).toBeUndefined(); + }); + it('restores a shared query (SQL + chart) from sessionStorage after the OAuth round-trip', async () => { // The hash is gone after the IdP redirect; the stash carries it through. const app = fakeApp({ token: valid, isSignedIn: () => true }); diff --git a/tests/unit/result-choice.test.js b/tests/unit/result-choice.test.js index 11ae7e8d..3359fd25 100644 --- a/tests/unit/result-choice.test.js +++ b/tests/unit/result-choice.test.js @@ -1,7 +1,7 @@ import { describe, expect, it } from 'vitest'; import { applyResultChoice, DASHBOARD_ROLE_RESULT_CHOICES, effectiveDashboardRole, - PANEL_RESULT_CHOICES, resultChoiceForSpec, + PANEL_RESULT_CHOICES, resultChoiceForSpec, rolePreviewView, } from '../../src/core/result-choice.js'; const query = (spec) => ({ id: 'q', sql: 'SELECT 1', specVersion: 1, spec }); @@ -43,4 +43,13 @@ describe('result choices', () => { expect(applyResultChoice(source, choice).spec.dashboard).toBeUndefined(); expect(applyResultChoice(source, null)).toBe(source); }); + it('rolePreviewView: Filter owns the transient launch preview; every other role defers (#244)', () => { + expect(rolePreviewView({ dashboard: { role: 'filter' } })).toBe('filter'); + expect(rolePreviewView({})).toBeNull(); + expect(rolePreviewView({ dashboard: { role: 'panel' } })).toBeNull(); + expect(rolePreviewView(undefined)).toBeNull(); + // dormant Panel state alongside the role has no bearing on the pure helper — + // precedence over it is the caller's job (saved-history.js). + expect(rolePreviewView({ dashboard: { role: 'filter' }, view: 'panel', panel: { cfg: { type: 'kpi' } } })).toBe('filter'); + }); }); diff --git a/tests/unit/saved-history.test.js b/tests/unit/saved-history.test.js index c47db1fb..a159352c 100644 --- a/tests/unit/saved-history.test.js +++ b/tests/unit/saved-history.test.js @@ -78,6 +78,71 @@ describe('renderSavedHistory', () => { expect(app.actions.run).not.toHaveBeenCalled(); }); + it.each(['table', 'json', 'panel'])( + 'saved: a Filter-role query always launches into the Filter preview, independent of the current result view (was %s) (#244)', + (previousView) => { + const app = makeApp(); + app.state.sidePanel.value = 'saved'; + app.state.resultView.value = previousView; + setSaved(app, [{ id: 'f', name: 'Options', sql: 'SELECT 1', dashboard: { role: 'filter' } }]); + renderSavedHistory(app); + click(app.dom.savedList.querySelector('.saved-row')); + expect(app.actions.run).toHaveBeenCalledWith({ view: 'filter' }); + }, + ); + + it('saved: Filter role takes precedence over a dormant persisted spec.view and Panel config, without touching either (#244)', () => { + const app = makeApp(); + app.state.sidePanel.value = 'saved'; + setSaved(app, [ + { id: 'f1', name: 'No persisted view', sql: 'SELECT 1', dashboard: { role: 'filter' } }, + { + id: 'f2', name: 'Dormant Panel view', sql: 'SELECT 1', + dashboard: { role: 'filter' }, view: 'panel', panel: { cfg: { type: 'kpi' } }, + }, + ]); + renderSavedHistory(app); + const rows = app.dom.savedList.querySelectorAll('.saved-row'); + click(rows[0]); + expect(app.actions.run).toHaveBeenLastCalledWith({ view: 'filter' }); + click(rows[1]); + expect(app.actions.run).toHaveBeenLastCalledWith({ view: 'filter' }); // role wins, not 'panel' + // launch never mutates the saved entry — dormant view/panel survive untouched + const dormant = app.state.savedQueries.find((q) => q.id === 'f2'); + expect(dormant.spec.view).toBe('panel'); + expect(dormant.spec.panel).toEqual({ cfg: { type: 'kpi' } }); + expect(dormant.spec.dashboard).toEqual({ role: 'filter' }); // no spec.view:'filter' persisted + expect(app.saveJSON).not.toHaveBeenCalled(); + }); + + it('saved: a Filter-role query that cannot auto-run still opens the Filter drawer instead of a dormant Panel view or nothing (#244)', () => { + // A Filter-role entry with empty/DDL/multi-statement SQL can't auto-run + // (isAutoRunnable is false) — e.g. one hand-authored, imported, or loaded + // from localStorage without the SQL-shape validation the Spec editor + // enforces. The role must still win the launch view: `SAVED_VIEWS` + // deliberately excludes 'filter' (it's never persisted), so this only + // opens correctly if the role bypasses that persisted-view check. + const app = makeApp(); + app.state.sidePanel.value = 'saved'; + app.state.resultView.value = 'table'; + setSaved(app, [ + { id: 'f1', name: 'Empty Filter', sql: '', dashboard: { role: 'filter' } }, + { + id: 'f2', name: 'DDL Filter with dormant Panel', sql: 'CREATE TABLE t (a Int8)', + dashboard: { role: 'filter' }, view: 'panel', panel: { cfg: { type: 'kpi' } }, + }, + ]); + renderSavedHistory(app); + const rows = app.dom.savedList.querySelectorAll('.saved-row'); + click(rows[0]); + expect(app.actions.run).not.toHaveBeenCalled(); // not auto-runnable + expect(app.state.resultView.value).toBe('filter'); + app.state.resultView.value = 'table'; // reset before the second row + click(rows[1]); + expect(app.actions.run).not.toHaveBeenCalled(); + expect(app.state.resultView.value).toBe('filter'); // role wins, not the dormant 'panel' + }); + it('saved: live count + star toggles favorite and re-sorts favorites first', () => { const app = makeApp(); app.state.sidePanel.value = 'saved';