Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dashboard Usability] Add unsaved filters to app state to url #197313

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,12 @@ import {
VIEW_DASHBOARD_URL,
} from '../dashboard_constants';
import { RedirectToProps } from '../dashboard_container/types';
import { coreServices, dataService, embeddableService } from '../services/kibana_services';
import {
coreServices,
dataService,
embeddableService,
spacesService,
} from '../services/kibana_services';
import { getDashboardCapabilities } from '../utils/get_dashboard_capabilities';
import { dashboardReadonlyBadge, getDashboardPageTitle } from './_dashboard_app_strings';
import { DashboardApp } from './dashboard_app';
Expand Down Expand Up @@ -58,14 +63,14 @@ export async function mountApp({
mountContext,
}: DashboardMountProps) {
let globalEmbedSettings: DashboardEmbedSettings | undefined;
const spaceId = (await spacesService?.getActiveSpace())?.id;

const getUrlStateStorage = (history: RouteComponentProps['history']) =>
createKbnUrlStateStorage({
history,
useHash: coreServices.uiSettings.get('state:storeInSessionStorage'),
...withNotifyOnErrors(coreServices.notifications.toasts),
});

const redirect = (redirectTo: RedirectToProps) => {
let path;
let state;
Expand Down Expand Up @@ -124,6 +129,7 @@ export async function mountApp({
title={title}
kbnUrlStateStorage={getUrlStateStorage(routeProps.history)}
redirectTo={redirect}
spaceId={spaceId}
/>
);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,15 @@ export interface DashboardListingPageProps {
redirectTo: DashboardRedirect;
initialFilter?: string;
title?: string;
spaceId?: string;
}

export const DashboardListingPage = ({
title,
redirectTo,
initialFilter,
kbnUrlStateStorage,
spaceId,
}: DashboardListingPageProps) => {
const [showNoDataPage, setShowNoDataPage] = useState<boolean | undefined>();
useEffect(() => {
Expand Down Expand Up @@ -106,7 +108,7 @@ export const DashboardListingPage = ({
redirectTo({ destination: 'dashboard', id, editMode: viewMode === ViewMode.EDIT });
}}
getDashboardUrl={(id, timeRestore) => {
return getDashboardListItemLink(kbnUrlStateStorage, id, timeRestore);
return getDashboardListItemLink(kbnUrlStateStorage, id, timeRestore, spaceId);
}}
/>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { getDashboardListItemLink } from './get_dashboard_list_item_link';
import { createHashHistory } from 'history';
import { FilterStateStore } from '@kbn/es-query';
import { createKbnUrlStateStorage } from '@kbn/kibana-utils-plugin/public';
import { GLOBAL_STATE_STORAGE_KEY } from '../../dashboard_constants';
import { DASHBOARD_STATE_STORAGE_KEY, GLOBAL_STATE_STORAGE_KEY } from '../../dashboard_constants';

const DASHBOARD_ID = '13823000-99b9-11ea-9eb6-d9e8adceb647';

Expand Down Expand Up @@ -102,3 +102,45 @@ describe('when global filters change', () => {
);
});
});

describe('when the dashboard has unsaved filters', () => {
beforeEach(() => {
kbnUrlStateStorage.set(GLOBAL_STATE_STORAGE_KEY, {
undefined,
});

const filters = [
{
meta: {
alias: null,
disabled: false,
negate: false,
},
query: { query: 'q1' },
$state: {
store: FilterStateStore.APP_STATE,
},
},
{
meta: {
alias: null,
disabled: false,
negate: false,
},
query: { query: 'q1' },
$state: {
store: FilterStateStore.APP_STATE,
},
},
];
kbnUrlStateStorage.set(DASHBOARD_STATE_STORAGE_KEY, {
filters,
});
});
test('propagates the unsaved filters on the query', async () => {
const url = getDashboardListItemLink(kbnUrlStateStorage, DASHBOARD_ID, false, 'default');
expect(url).toMatchInlineSnapshot(
`"http://localhost/#/?_g=()&_a=(filters:!(('$state':(store:appState),meta:(alias:!n,disabled:!f,negate:!f),query:(query:q1)),('$state':(store:appState),meta:(alias:!n,disabled:!f,negate:!f),query:(query:q1))))"`
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,48 @@ import { IKbnUrlStateStorage, setStateToKbnUrl } from '@kbn/kibana-utils-plugin/

import {
DASHBOARD_APP_ID,
DASHBOARD_STATE_STORAGE_KEY,
GLOBAL_STATE_STORAGE_KEY,
createDashboardEditUrl,
} from '../../dashboard_constants';
import { coreServices } from '../../services/kibana_services';
import { DASHBOARD_STATE_SESSION_KEY } from '../../services/dashboard_backup_service';

export const getDashboardListItemLink = (
kbnUrlStateStorage: IKbnUrlStateStorage,
id: string,
timeRestore: boolean
timeRestore: boolean,
spaceId?: string
) => {
const useHash = coreServices.uiSettings.get('state:storeInSessionStorage'); // use hash

let url = coreServices.application.getUrlForApp(DASHBOARD_APP_ID, {
path: `#${createDashboardEditUrl(id)}`,
});

const globalStateInUrl = kbnUrlStateStorage.get<QueryState>(GLOBAL_STATE_STORAGE_KEY) || {};

if (timeRestore) {
delete globalStateInUrl.time;
delete globalStateInUrl.refreshInterval;
}
url = setStateToKbnUrl<QueryState>(GLOBAL_STATE_STORAGE_KEY, globalStateInUrl, { useHash }, url);
// pull the filters off the session storage and put in the url if they exist
const unsavedFiltersToUrl = JSON.parse(
sessionStorage.getItem(DASHBOARD_STATE_SESSION_KEY) ?? '[]'
);

const unsavedFilters = spaceId && unsavedFiltersToUrl ? unsavedFiltersToUrl[spaceId] : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should unsaved queries also be forwarded to be consistent?

Copy link
Contributor

@Heenawter Heenawter Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should be, yes :) I honestly wonder if... all unsaved changes should be forwarded? I can't think of a good argument for why opening a dashboard in a new tab would have different behaviour than the onClick behaviour....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can re-use the code that generates the share link so they are the same

if (unsavedFilters && unsavedFilters[id] && unsavedFilters[id].filters) {
const appStateInUrl = kbnUrlStateStorage.get<QueryState>(DASHBOARD_STATE_STORAGE_KEY) || {};
appStateInUrl.filters = unsavedFilters[id].filters;

url = setStateToKbnUrl<QueryState>(
DASHBOARD_STATE_STORAGE_KEY,
appStateInUrl,
{ useHash },
url
);
}
return url;
};
3 changes: 1 addition & 2 deletions src/plugins/dashboard/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,9 @@ export class DashboardPlugin
({ changes }) => !!(changes.globalFilters || changes.time || changes.refreshInterval)
),
map(async ({ state }) => {
const { isFilterPinned } = await import('@kbn/es-query');
return {
...state,
filters: state.filters?.filter(isFilterPinned),
filters: state.filters,
};
})
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const DASHBOARD_PANELS_SESSION_KEY = 'dashboardPanels';
const DASHBOARD_VIEWMODE_LOCAL_KEY = 'dashboardViewMode';

// this key is named `panels` for BWC reasons, but actually contains the entire dashboard state
const DASHBOARD_STATE_SESSION_KEY = 'dashboardStateManagerPanels';
export const DASHBOARD_STATE_SESSION_KEY = 'dashboardStateManagerPanels';

interface DashboardBackupServiceType {
clearState: (id?: string) => void;
Expand Down