From 77a72e82fc5a45385462be2bf6783ea412aac0f2 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 30 Nov 2022 15:51:10 +0000 Subject: [PATCH 01/16] beginning to cancel dashboard queries. covers hitting refresh while refreshing but not changing filters while refreshing --- .../src/scenes/dashboard/dashboardLogic.tsx | 54 ++++++++++++++++--- 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index 20ec1527e93de..11cf2690e657e 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -1,5 +1,5 @@ import { isBreakpoint, kea } from 'kea' -import api, { getJSONOrThrow } from 'lib/api' +import api, { ApiMethodOptions, getJSONOrThrow } from 'lib/api' import { dashboardsModel } from '~/models/dashboardsModel' import { router } from 'kea-router' import { clearDOMTextSelection, isUserLoggedIn, toParams, uuid } from 'lib/utils' @@ -7,6 +7,7 @@ import { insightsModel } from '~/models/insightsModel' import { AUTO_REFRESH_DASHBOARD_THRESHOLD_HOURS, DashboardPrivilegeLevel, + FEATURE_FLAGS, OrganizationMembershipLevel, } from 'lib/constants' import { DashboardEventSource, eventUsageLogic } from 'lib/utils/eventUsageLogic' @@ -37,6 +38,7 @@ import { Link } from 'lib/components/Link' import { isPathsFilter, isRetentionFilter, isTrendsFilter } from 'scenes/insights/sharedUtils' import { captureTimeToSeeData, TimeToSeeDataPayload } from 'lib/internalMetrics' import { getResponseBytes, sortDates } from '../insights/utils' +import { featureFlagLogic } from 'lib/logic/featureFlagLogic' export const BREAKPOINTS: Record = { sm: 1024, @@ -68,7 +70,7 @@ export type LoadDashboardItemsProps = { refresh?: boolean; action: string } export const dashboardLogic = kea({ path: ['scenes', 'dashboard', 'dashboardLogic'], connect: () => ({ - values: [teamLogic, ['currentTeamId']], + values: [teamLogic, ['currentTeamId'], featureFlagLogic, ['featureFlags']], logic: [dashboardsModel, insightsModel, eventUsageLogic], }), @@ -131,9 +133,15 @@ export const dashboardLogic = kea({ duplicateTile: (tile: DashboardTile) => ({ tile }), loadingDashboardItemsStarted: (action: string, dashboardQueryId: string) => ({ action, dashboardQueryId }), setInitialLoadResponseBytes: (responseBytes: number) => ({ responseBytes }), + abortQuery: (payload: { + queryId: string + // view: InsightType + // scene: Scene | null + exception?: Record + }) => payload, }, - loaders: ({ actions, props, values }) => ({ + loaders: ({ actions, props, values, cache }) => ({ // TODO this is a terrible name... it is "dashboard" but there's a "dashboard" reducer ¯\_(ツ)_/¯ allItems: [ null as DashboardType | null, @@ -147,6 +155,12 @@ export const dashboardLogic = kea({ const dashboardQueryId = uuid() actions.loadingDashboardItemsStarted(action, dashboardQueryId) + // If insight query is in progress, kill that query + if (cache.abortController) { + cache.abortController.abort() + cache.abortController = null + } + try { // :TODO: Send dashboardQueryId forward as well if refreshing const apiUrl = values.apiUrl(refresh) @@ -158,11 +172,11 @@ export const dashboardLogic = kea({ actions.setDates(dashboard.filters.date_from, dashboard.filters.date_to, false) return dashboard - } catch (error: any) { - if (error.status === 404) { + } catch (e: any) { + if (e.status === 404) { throw new Error('Dashboard not found') } - throw error + throw e } }, updateTileColor: async ({ tileId, color }) => { @@ -772,6 +786,7 @@ export const dashboardLogic = kea({ } }, beforeUnmount: () => { + cache.abortController?.abort() if (cache.autoRefreshInterval) { window.clearInterval(cache.autoRefreshInterval) cache.autoRefreshInterval = null @@ -909,6 +924,16 @@ export const dashboardLogic = kea({ true ) + // If a query is in progress, kill that query + // we will use one abort controller for all insight queries for this dashboard + if (cache.abortController) { + cache.abortController.abort() + } + cache.abortController = new AbortController() + const methodOptions: ApiMethodOptions = { + signal: cache.abortController.signal, + } + const refreshStartTime = performance.now() dashboardQueryId = dashboardQueryId ?? uuid() let refreshesFinished = 0 @@ -922,12 +947,13 @@ export const dashboardLogic = kea({ const apiUrl = `api/projects/${values.currentTeamId}/insights/${insight.id}/?${toParams({ refresh: true, from_dashboard: dashboardId, // needed to load insight in correct context + client_query_id: queryId, })}` try { breakpoint() - const refreshedInsightResponse: Response = await api.getResponse(apiUrl) + const refreshedInsightResponse: Response = await api.getResponse(apiUrl, methodOptions) const refreshedInsight: InsightModel = await getJSONOrThrow(refreshedInsightResponse) breakpoint() // reload the cached results inside the insight's logic @@ -968,12 +994,19 @@ export const dashboardLogic = kea({ if (isBreakpoint(e)) { breakpointTriggered = true } else { + if (e.name === 'AbortError' || e.message?.name === 'AbortError') { + actions.abortQuery({ queryId }) + } + actions.setRefreshError(insight.short_id) } } refreshesFinished += 1 if (refreshesFinished === insights.length) { + breakpoint() + cache.abortController = null + const payload: TimeToSeeDataPayload = { type: 'dashboard_load', context: 'dashboard', @@ -1136,6 +1169,13 @@ export const dashboardLogic = kea({ actions.setShouldReportOnAPILoad(true) } }, + abortQuery: ({ queryId }) => { + const { currentTeamId } = values + // TODO this both is and isn't cancelling insights 🤔 + if (values.featureFlags[FEATURE_FLAGS.CANCEL_RUNNING_QUERIES]) { + console.log(api.create(`api/projects/${currentTeamId}/insights/cancel`, { client_query_id: queryId })) + } + }, }), urlToAction: ({ actions }) => ({ From a0fc879572c824d14281c017c722714c23fcdd8f Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 30 Nov 2022 20:18:19 +0000 Subject: [PATCH 02/16] don't handle filter changes yet --- frontend/src/scenes/dashboard/dashboardLogic.tsx | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index 11cf2690e657e..f2af29146c9f1 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -133,15 +133,10 @@ export const dashboardLogic = kea({ duplicateTile: (tile: DashboardTile) => ({ tile }), loadingDashboardItemsStarted: (action: string, dashboardQueryId: string) => ({ action, dashboardQueryId }), setInitialLoadResponseBytes: (responseBytes: number) => ({ responseBytes }), - abortQuery: (payload: { - queryId: string - // view: InsightType - // scene: Scene | null - exception?: Record - }) => payload, + abortQuery: (payload: { queryId: string; exception?: Record }) => payload, }, - loaders: ({ actions, props, values, cache }) => ({ + loaders: ({ actions, props, values }) => ({ // TODO this is a terrible name... it is "dashboard" but there's a "dashboard" reducer ¯\_(ツ)_/¯ allItems: [ null as DashboardType | null, @@ -155,12 +150,6 @@ export const dashboardLogic = kea({ const dashboardQueryId = uuid() actions.loadingDashboardItemsStarted(action, dashboardQueryId) - // If insight query is in progress, kill that query - if (cache.abortController) { - cache.abortController.abort() - cache.abortController = null - } - try { // :TODO: Send dashboardQueryId forward as well if refreshing const apiUrl = values.apiUrl(refresh) From 0b72174c7a15dc41500dfb5f240b8b72547d33f7 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 30 Nov 2022 21:34:07 +0000 Subject: [PATCH 03/16] default dashboard query id to a uuid, and add a prefix --- frontend/src/scenes/dashboard/dashboardLogic.tsx | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index f2af29146c9f1..76fb13473aeda 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -891,7 +891,7 @@ export const dashboardLogic = kea({ actions.resetInterval() actions.refreshAllDashboardItems({ action: 'refresh_manual' }) }, - refreshAllDashboardItems: async ({ tiles, action, initialLoad, dashboardQueryId }, breakpoint) => { + refreshAllDashboardItems: async ({ tiles, action, initialLoad, dashboardQueryId = uuid() }, breakpoint) => { if (!props.id) { // what are we loading the insight card on?! return @@ -924,14 +924,13 @@ export const dashboardLogic = kea({ } const refreshStartTime = performance.now() - dashboardQueryId = dashboardQueryId ?? uuid() + let refreshesFinished = 0 let totalResponseBytes = 0 // array of functions that reload each item const fetchItemFunctions = insights.map((insight) => async () => { - // :TODO: Support query cancellation and use this queryId in the actual query. - const queryId = `${dashboardQueryId}::${uuid()}` + const queryId = `dash-${dashboardQueryId}::${uuid()}` const queryStartTime = performance.now() const apiUrl = `api/projects/${values.currentTeamId}/insights/${insight.id}/?${toParams({ refresh: true, From 2141e1a27c9464f085505fd2dbdb591894a173cf Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 30 Nov 2022 21:36:17 +0000 Subject: [PATCH 04/16] don't change variable in method I'm not changing now --- frontend/src/scenes/dashboard/dashboardLogic.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index 76fb13473aeda..2fd812ffa1fbc 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -161,11 +161,11 @@ export const dashboardLogic = kea({ actions.setDates(dashboard.filters.date_from, dashboard.filters.date_to, false) return dashboard - } catch (e: any) { - if (e.status === 404) { + } catch (error: any) { + if (error.status === 404) { throw new Error('Dashboard not found') } - throw e + throw error } }, updateTileColor: async ({ tileId, color }) => { From 361174277e4b64ce2402d12737fe545c90e20c25 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 30 Nov 2022 22:17:44 +0000 Subject: [PATCH 05/16] cancel when filters change, but the new insights don't load :Sad face: --- .../src/scenes/dashboard/dashboardLogic.tsx | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index 2fd812ffa1fbc..7b18ee1ca1ca9 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -456,6 +456,7 @@ export const dashboardLogic = kea({ [shortId]: { error: true, timer: state[shortId]?.timer || null }, }), refreshAllDashboardItems: () => ({}), + abortQuery: () => ({}), }, ], columns: [ @@ -907,7 +908,7 @@ export const dashboardLogic = kea({ return } - let breakpointTriggered = false + let cancelled = false actions.setRefreshStatuses( insights.map((item) => item.short_id), true @@ -980,12 +981,14 @@ export const dashboardLogic = kea({ } catch (e: any) { console.error(e) if (isBreakpoint(e)) { - breakpointTriggered = true - } else { - if (e.name === 'AbortError' || e.message?.name === 'AbortError') { - actions.abortQuery({ queryId }) + cancelled = true + } else if (e.name === 'AbortError' || e.message?.name === 'AbortError') { + if (!cancelled) { + // cancel all insight requests for this query in one go + actions.abortQuery({ queryId: dashboardQueryId }) } - + cancelled = true + } else { actions.setRefreshError(insight.short_id) } } @@ -1021,7 +1024,7 @@ export const dashboardLogic = kea({ // run 4 item reloaders in parallel function loadNextPromise(): void { - if (!breakpointTriggered && fetchItemFunctions.length > 0) { + if (!cancelled && fetchItemFunctions.length > 0) { fetchItemFunctions.shift()?.().then(loadNextPromise) } } @@ -1033,6 +1036,11 @@ export const dashboardLogic = kea({ eventUsageLogic.actions.reportDashboardRefreshed(dashboardId, values.lastRefreshed) }, updateAndRefreshDashboard: async (_, breakpoint) => { + if (cache.abortController) { + cache.abortController.abort() + } + cache.abortController = null + await breakpoint(200) await api.update(`api/projects/${values.currentTeamId}/dashboards/${props.id}`, { filters: values.filters, From d2886e3f31f4ab237f6ce2a1427dfd40366c8690 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 30 Nov 2022 22:53:16 +0000 Subject: [PATCH 06/16] no need to manually set dates --- frontend/src/scenes/dashboard/dashboardLogic.tsx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index 7b18ee1ca1ca9..e78bbf6a630f5 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -158,8 +158,6 @@ export const dashboardLogic = kea({ actions.setInitialLoadResponseBytes(getResponseBytes(dashboardResponse)) - actions.setDates(dashboard.filters.date_from, dashboard.filters.date_to, false) - return dashboard } catch (error: any) { if (error.status === 404) { @@ -261,6 +259,11 @@ export const dashboardLogic = kea({ ...state, properties: properties || null, }), + loadDashboardItemsSuccess: (state, { allItems }) => ({ + ...state, + date_from: allItems?.filters.date_from || null, + date_to: allItems?.filters.date_to || null, + }), }, ], allItems: [ @@ -1167,9 +1170,8 @@ export const dashboardLogic = kea({ }, abortQuery: ({ queryId }) => { const { currentTeamId } = values - // TODO this both is and isn't cancelling insights 🤔 if (values.featureFlags[FEATURE_FLAGS.CANCEL_RUNNING_QUERIES]) { - console.log(api.create(`api/projects/${currentTeamId}/insights/cancel`, { client_query_id: queryId })) + api.create(`api/projects/${currentTeamId}/insights/cancel`, { client_query_id: queryId }) } }, }), From 3517e6cbca8ab283d6ed4449947cc546dc98874b Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Thu, 1 Dec 2022 10:25:48 +0000 Subject: [PATCH 07/16] use the unused interface --- frontend/src/scenes/dashboard/dashboardLogic.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index e78bbf6a630f5..ccdbefb2c3fd0 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -85,7 +85,7 @@ export const dashboardLogic = kea({ actions: { loadExportedDashboard: (dashboard: DashboardType | null) => ({ dashboard }), - loadDashboardItems: (payload: { refresh?: boolean; action: string }) => payload, + loadDashboardItems: (payload: LoadDashboardItemsProps) => payload, triggerDashboardUpdate: (payload) => ({ payload }), /** The current state in which the dashboard is being viewed, see DashboardMode. */ setDashboardMode: (mode: DashboardMode | null, source: DashboardEventSource | null) => ({ mode, source }), From 69d655f8d564633db762a736cf2ec4618ae67b6e Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Thu, 1 Dec 2022 13:01:19 +0000 Subject: [PATCH 08/16] cancel queries when navigating away --- frontend/src/models/insightsModel.tsx | 1 + .../src/scenes/dashboard/dashboardLogic.tsx | 3 +++ frontend/src/scenes/insights/insightLogic.ts | 5 ++++- frontend/src/scenes/sceneLogic.test.tsx | 20 +++++++++++++++++++ frontend/src/scenes/sceneLogic.ts | 12 +++++++++++ 5 files changed, 40 insertions(+), 1 deletion(-) diff --git a/frontend/src/models/insightsModel.tsx b/frontend/src/models/insightsModel.tsx index 22f104bda6717..da26cad0580df 100644 --- a/frontend/src/models/insightsModel.tsx +++ b/frontend/src/models/insightsModel.tsx @@ -22,6 +22,7 @@ export const insightsModel = kea({ dashboardId, insightIds, }), + abortRunningQueries: true, }), listeners: ({ actions }) => ({ renameInsight: async ({ item }) => { diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index c7f8fb30dfdae..f9d2bd22a8208 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -830,6 +830,9 @@ export const dashboardLogic = kea({ }, }), listeners: ({ actions, values, cache, props, sharedListeners }) => ({ + [insightsModel.actionTypes.abortRunningQueries]: () => { + cache.abortController?.abort() + }, setRefreshError: sharedListeners.reportRefreshTiming, setRefreshStatuses: sharedListeners.reportRefreshTiming, setRefreshStatus: sharedListeners.reportRefreshTiming, diff --git a/frontend/src/scenes/insights/insightLogic.ts b/frontend/src/scenes/insights/insightLogic.ts index 3490a472284c5..b9939170e1f1f 100644 --- a/frontend/src/scenes/insights/insightLogic.ts +++ b/frontend/src/scenes/insights/insightLogic.ts @@ -749,7 +749,10 @@ export const insightLogic = kea([ }, ], }), - listeners(({ actions, selectors, values }) => ({ + listeners(({ actions, selectors, values, cache }) => ({ + [insightsModel.actionTypes.abortRunningQueries]: () => { + cache.abortController?.abort() + }, setFiltersMerge: ({ filters }) => { actions.setFilters({ ...values.filters, ...filters }) }, diff --git a/frontend/src/scenes/sceneLogic.test.tsx b/frontend/src/scenes/sceneLogic.test.tsx index edf4c4b4d6f3b..83529a1cfbe90 100644 --- a/frontend/src/scenes/sceneLogic.test.tsx +++ b/frontend/src/scenes/sceneLogic.test.tsx @@ -9,6 +9,7 @@ import { urls } from 'scenes/urls' import { kea, path } from 'kea' import type { logicType } from './sceneLogic.testType' +import { insightsModel } from '~/models/insightsModel' export const Component = (): JSX.Element =>
export const logic = kea([path(['scenes', 'sceneLogic', 'test'])]) @@ -17,6 +18,7 @@ const sceneImport = (): any => ({ scene: { component: Component, logic: logic } const testScenes: Record any> = { [Scene.Annotations]: sceneImport, [Scene.MySettings]: sceneImport, + [Scene.Dashboard]: sceneImport, } describe('sceneLogic', () => { @@ -26,6 +28,7 @@ describe('sceneLogic', () => { initKeaTests() await expectLogic(teamLogic).toDispatchActions(['loadCurrentTeamSuccess']) featureFlagLogic.mount() + insightsModel.mount() router.actions.push(urls.annotations()) logic = sceneLogic({ scenes: testScenes }) logic.mount() @@ -51,6 +54,23 @@ describe('sceneLogic', () => { }) }) + it('reacts to navigating away from scenes that might have cancellable queries', async () => { + await expectLogic(logic, () => { + logic.actions.setScene(Scene.Dashboard, { params: { dashboardId: 1 }, searchParams: {}, hashParams: {} }) + router.actions.push(urls.mySettings()) + }).toDispatchActions([insightsModel.actionTypes.abortRunningQueries]) + + await expectLogic(logic, () => { + logic.actions.setScene(Scene.Insight, { params: { insightId: 1 }, searchParams: {}, hashParams: {} }) + router.actions.push(urls.mySettings()) + }).toDispatchActions([insightsModel.actionTypes.abortRunningQueries]) + + await expectLogic(logic, () => { + logic.actions.setScene(Scene.Dashboards, { params: {}, searchParams: {}, hashParams: {} }) + router.actions.push(urls.mySettings()) + }).toNotHaveDispatchedActions([insightsModel.actionTypes.abortRunningQueries]) + }) + it('persists the loaded scenes', async () => { const expectedAnnotation = partial({ name: Scene.Annotations, diff --git a/frontend/src/scenes/sceneLogic.ts b/frontend/src/scenes/sceneLogic.ts index b97f62b9a56ec..474dc699b3a5a 100644 --- a/frontend/src/scenes/sceneLogic.ts +++ b/frontend/src/scenes/sceneLogic.ts @@ -15,6 +15,7 @@ import { organizationLogic } from './organizationLogic' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' import { UPGRADE_LINK } from 'lib/constants' import { appContextLogic } from './appContextLogic' +import { insightsModel } from '~/models/insightsModel' /** Mapping of some scenes that aren't directly accessible from the sidebar to ones that are - for the sidebar. */ const sceneNavAlias: Partial> = { @@ -288,9 +289,20 @@ export const sceneLogic = kea({ }, loadScene: async ({ scene, params, method }, breakpoint) => { const clickedLink = method === 'PUSH' + if (values.scene === scene) { actions.setScene(scene, params, clickedLink) return + } else { + const scenesThatMightAbortClickHouseQueries = [Scene.Insight, Scene.Dashboard] + if ( + values.scene && + scenesThatMightAbortClickHouseQueries.includes(values.scene) && + !scenesThatMightAbortClickHouseQueries.includes(scene) + ) { + // we're navigating away from a scene that might be running long ClickHouse queries + insightsModel.actions.abortRunningQueries() + } } if (!props.scenes?.[scene]) { From 1f49284ad98561b08bf1c70acbe8803a8258be14 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Thu, 1 Dec 2022 13:25:19 +0000 Subject: [PATCH 09/16] wip cancelling insights on navigation --- frontend/src/scenes/insights/insightLogic.ts | 7 ++++++- frontend/src/scenes/sceneLogic.ts | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/frontend/src/scenes/insights/insightLogic.ts b/frontend/src/scenes/insights/insightLogic.ts index b9939170e1f1f..6cc5b5a022c00 100644 --- a/frontend/src/scenes/insights/insightLogic.ts +++ b/frontend/src/scenes/insights/insightLogic.ts @@ -346,6 +346,7 @@ export const insightLogic = kea([ } response = await getJSONOrThrow(fetchResponse) } catch (e: any) { + let cancelled = false if (e.name === 'AbortError' || e.message?.name === 'AbortError') { actions.abortQuery({ queryId, @@ -353,6 +354,7 @@ export const insightLogic = kea([ scene: scene, exception: e, }) + cancelled = true } breakpoint() cache.abortController = null @@ -376,7 +378,10 @@ export const insightLogic = kea([ e.message ) } - throw e + + if (!cancelled) { + throw e + } } breakpoint() diff --git a/frontend/src/scenes/sceneLogic.ts b/frontend/src/scenes/sceneLogic.ts index 474dc699b3a5a..f2924456542c9 100644 --- a/frontend/src/scenes/sceneLogic.ts +++ b/frontend/src/scenes/sceneLogic.ts @@ -295,6 +295,7 @@ export const sceneLogic = kea({ return } else { const scenesThatMightAbortClickHouseQueries = [Scene.Insight, Scene.Dashboard] + console.log('Loading scene', values.scene, scene, params) if ( values.scene && scenesThatMightAbortClickHouseQueries.includes(values.scene) && From bc4b16c195582d9217d0fde8f5b8a86b4b544d08 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 6 Dec 2022 13:23:16 +0000 Subject: [PATCH 10/16] prefix doesn't add enough to keep it --- frontend/src/scenes/dashboard/dashboardLogic.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index f3c6be0c8c4d0..3eaa5dbc15c50 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -987,7 +987,7 @@ export const dashboardLogic = kea({ // array of functions that reload each item const fetchItemFunctions = insights.map((insight) => async () => { - const queryId = `dash-${dashboardQueryId}::${uuid()}` + const queryId = `${dashboardQueryId}::${uuid()}` const queryStartTime = performance.now() const apiUrl = `api/projects/${values.currentTeamId}/insights/${insight.id}/?${toParams({ refresh: true, From 5b3d85f0d00c37f5cd27bda96fe45f6427a52bbb Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 6 Dec 2022 13:30:14 +0000 Subject: [PATCH 11/16] remove console.log --- frontend/src/scenes/sceneLogic.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/scenes/sceneLogic.ts b/frontend/src/scenes/sceneLogic.ts index f2924456542c9..c38e60d9a165d 100644 --- a/frontend/src/scenes/sceneLogic.ts +++ b/frontend/src/scenes/sceneLogic.ts @@ -295,7 +295,7 @@ export const sceneLogic = kea({ return } else { const scenesThatMightAbortClickHouseQueries = [Scene.Insight, Scene.Dashboard] - console.log('Loading scene', values.scene, scene, params) + if ( values.scene && scenesThatMightAbortClickHouseQueries.includes(values.scene) && From ab62335e044305bb403136a67e304ff929d23c45 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 6 Dec 2022 13:53:43 +0000 Subject: [PATCH 12/16] navigating away from homepage is cancellable too --- frontend/src/scenes/sceneLogic.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/scenes/sceneLogic.ts b/frontend/src/scenes/sceneLogic.ts index c38e60d9a165d..7631981c0f07d 100644 --- a/frontend/src/scenes/sceneLogic.ts +++ b/frontend/src/scenes/sceneLogic.ts @@ -294,7 +294,7 @@ export const sceneLogic = kea({ actions.setScene(scene, params, clickedLink) return } else { - const scenesThatMightAbortClickHouseQueries = [Scene.Insight, Scene.Dashboard] + const scenesThatMightAbortClickHouseQueries = [Scene.Insight, Scene.Dashboard, Scene.ProjectHomepage] if ( values.scene && From be4dd0e0117a8053ae89b1921fe1ca60f511f255 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 6 Dec 2022 13:55:08 +0000 Subject: [PATCH 13/16] metric on number of requested cancellations --- posthog/api/insight.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/posthog/api/insight.py b/posthog/api/insight.py index 2c54ed2756e81..704373f5dd23e 100644 --- a/posthog/api/insight.py +++ b/posthog/api/insight.py @@ -787,10 +787,12 @@ def activity(self, request: request.Request, **kwargs): def cancel(self, request: request.Request, **kwargs): if "client_query_id" not in request.data: raise serializers.ValidationError({"client_query_id": "Field is required."}) + sync_execute( f"KILL QUERY ON CLUSTER '{CLICKHOUSE_CLUSTER}' WHERE query_id LIKE %(client_query_id)s", {"client_query_id": f"{self.team.pk}_{request.data['client_query_id']}%"}, ) + statsd.incr("clickhouse.insight.query.cancellation_requested", tags={"team_id": self.team.pk}) return Response(status=status.HTTP_201_CREATED) @action(methods=["POST"], detail=False) From b41f41f7378935de6951a260175f04dff9fb427f Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 6 Dec 2022 15:47:43 +0000 Subject: [PATCH 14/16] frustration and prodding --- frontend/src/models/insightsModel.tsx | 1 - .../scenes/dashboard/dashboardLogic.test.ts | 73 +--------- .../src/scenes/dashboard/dashboardLogic.tsx | 131 ++++++++++-------- .../src/scenes/insights/insightLogic.test.ts | 2 + frontend/src/scenes/insights/insightLogic.ts | 21 ++- frontend/src/scenes/sceneLogic.test.tsx | 18 ++- frontend/src/scenes/sceneLogic.ts | 23 ++- 7 files changed, 114 insertions(+), 155 deletions(-) diff --git a/frontend/src/models/insightsModel.tsx b/frontend/src/models/insightsModel.tsx index da26cad0580df..22f104bda6717 100644 --- a/frontend/src/models/insightsModel.tsx +++ b/frontend/src/models/insightsModel.tsx @@ -22,7 +22,6 @@ export const insightsModel = kea({ dashboardId, insightIds, }), - abortRunningQueries: true, }), listeners: ({ actions }) => ({ renameInsight: async ({ item }) => { diff --git a/frontend/src/scenes/dashboard/dashboardLogic.test.ts b/frontend/src/scenes/dashboard/dashboardLogic.test.ts index ce18870f1bc7c..e6e1358f40fe4 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.test.ts +++ b/frontend/src/scenes/dashboard/dashboardLogic.test.ts @@ -16,7 +16,6 @@ import { InsightShortId, InsightType, TextModel, - TileLayout, } from '~/types' import { resumeKeaLoadersErrors, silenceKeaLoadersErrors } from '~/initKea' import { useMocks } from '~/mocks/jest' @@ -299,77 +298,6 @@ describe('dashboardLogic', () => { ], }, }) - .toDispatchActions([ - // frustratingly the same properties are sent in a different order - // and the matcher cares about the order - logic.actionCreators.saveLayouts([ - { - id: 0, - layouts: { - sm: { i: '0', x: 0, y: 0, w: 8, h: 8, minW: 3, minH: 5 }, - xs: { i: '0', x: 0, y: 0, w: 1, h: 8, minW: 1, minH: 5 }, - }, - }, - { - id: 1, - layouts: { - sm: { i: '1', x: 0, y: 8, w: 6, h: 5, minW: 3, minH: 5 }, - xs: { i: '1', x: 0, y: 8, w: 1, h: 5, minW: 1, minH: 5 }, - }, - }, - { - id: 4, - layouts: { - sm: { i: '4', x: 6, y: 8, w: 6, h: 6, minW: 3, minH: 5 }, - xs: { i: '4', x: 0, y: 13, w: 1, h: 6, minW: 1, minH: 5 }, - }, - }, - ]), - ]) - }) - - it('saving layouts with no provided tiles updates all tiles', async () => { - jest.spyOn(api, 'update') - - await expectLogic(logic, () => { - logic.actions.saveLayouts() - }).toFinishAllListeners() - - expect(api.update).toHaveBeenCalledWith(`api/projects/${MOCK_TEAM_ID}/dashboards/5`, { - no_items_field: true, - tiles: [ - { - id: 9, - layouts: {}, - }, - { - id: 10, - layouts: {}, - }, - { - id: 4, - layouts: {}, - }, - ], - }) - }) - - it('saving layouts with provided tiles updates only those tiles', async () => { - jest.spyOn(api, 'update') - - await expectLogic(logic, () => { - logic.actions.saveLayouts([{ id: 1, layouts: { sm: {} as TileLayout, xs: {} as TileLayout } }]) - }).toFinishAllListeners() - - expect(api.update).toHaveBeenCalledWith(`api/projects/${MOCK_TEAM_ID}/dashboards/5`, { - no_items_field: true, - tiles: [ - { - id: 1, - layouts: { sm: {} as TileLayout, xs: {} as TileLayout }, - }, - ], - }) }) }) @@ -804,6 +732,7 @@ describe('dashboardLogic', () => { }) }) }) + describe('lastRefreshed', () => { it('should be the earliest refreshed dashboard', async () => { logic = dashboardLogic({ id: 5 }) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index 3eaa5dbc15c50..2b3599d55bb4d 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -1,8 +1,21 @@ -import { isBreakpoint, kea } from 'kea' +import { + actions, + connect, + events, + isBreakpoint, + kea, + key, + listeners, + path, + props, + reducers, + selectors, + sharedListeners, +} from 'kea' import api, { ApiMethodOptions, getJSONOrThrow } from 'lib/api' import { dashboardsModel } from '~/models/dashboardsModel' -import { router } from 'kea-router' -import { areObjectValuesEmpty, clearDOMTextSelection, isUserLoggedIn, toParams, uuid } from 'lib/utils' +import { router, urlToAction } from 'kea-router' +import { clearDOMTextSelection, isUserLoggedIn, toParams, uuid } from 'lib/utils' import { insightsModel } from '~/models/insightsModel' import { AUTO_REFRESH_DASHBOARD_THRESHOLD_HOURS, @@ -40,6 +53,9 @@ import { isPathsFilter, isRetentionFilter, isTrendsFilter } from 'scenes/insight import { captureTimeToSeeData, TimeToSeeDataPayload } from 'lib/internalMetrics' import { getResponseBytes, sortDates } from '../insights/utils' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' +import { Scene } from 'scenes/sceneTypes' +import { subscriptions } from 'kea-subscriptions' +import { loaders } from 'kea-loaders' export const BREAKPOINTS: Record = { sm: 1024, @@ -108,29 +124,26 @@ function updateExistingInsightState({ cachedInsight, dashboardId, refreshedInsig } } -export const dashboardLogic = kea({ - path: ['scenes', 'dashboard', 'dashboardLogic'], - connect: () => ({ +export const dashboardLogic = kea([ + path(['scenes', 'dashboard', 'dashboardLogic']), + connect(() => ({ values: [teamLogic, ['currentTeamId'], featureFlagLogic, ['featureFlags']], logic: [dashboardsModel, insightsModel, eventUsageLogic], - }), - - props: {} as DashboardLogicProps, - - key: (props) => { + })), + props({} as DashboardLogicProps), + key((props) => { if (typeof props.id === 'string') { throw Error('Must init dashboardLogic with a numeric key') } return props.id ?? 'new' - }, - - actions: { + }), + actions({ loadExportedDashboard: (dashboard: DashboardType | null) => ({ dashboard }), loadDashboardItems: (payload: LoadDashboardItemsProps) => payload, triggerDashboardUpdate: (payload) => ({ payload }), /** The current state in which the dashboard is being viewed, see DashboardMode. */ setDashboardMode: (mode: DashboardMode | null, source: DashboardEventSource | null) => ({ mode, source }), - saveLayouts: (tilesToSave: DashboardTileLayoutUpdatePayload[] = []) => ({ tilesToSave }), + saveLayouts: true, updateLayouts: (layouts: Layouts) => ({ layouts }), updateContainerWidth: (containerWidth: number, columns: number) => ({ containerWidth, columns }), updateTileColor: (tileId: number, color: string | null) => ({ tileId, color }), @@ -175,9 +188,8 @@ export const dashboardLogic = kea({ loadingDashboardItemsStarted: (action: string, dashboardQueryId: string) => ({ action, dashboardQueryId }), setInitialLoadResponseBytes: (responseBytes: number) => ({ responseBytes }), abortQuery: (payload: { queryId: string; exception?: Record }) => payload, - }, - - loaders: ({ actions, props, values }) => ({ + }), + loaders(({ actions, props, values }) => ({ // TODO this is a terrible name... it is "dashboard" but there's a "dashboard" reducer ¯\_(ツ)_/¯ allItems: [ null as DashboardType | null, @@ -279,8 +291,8 @@ export const dashboardLogic = kea({ }, }, ], - }), - reducers: ({ props }) => ({ + })), + reducers(({ props }) => ({ receivedErrorsFromAPI: [ false, { @@ -567,8 +579,8 @@ export const dashboardLogic = kea({ setTextTileId: (_, { textTileId }) => textTileId, }, ], - }), - selectors: ({ actions }) => ({ + })), + selectors(() => ({ asDashboardTemplate: [ (s) => [s.allItems], (dashboard: DashboardType): string => { @@ -678,8 +690,6 @@ export const dashboardLogic = kea({ layouts: [ (s) => [s.tiles], (tiles) => { - const tilesWithNoLayout = tiles.filter((t) => !t.layouts || areObjectValuesEmpty(t.layouts)) - const allLayouts: Partial> = {} for (const col of Object.keys(BREAKPOINT_COLUMN_COUNTS) as (keyof typeof BREAKPOINT_COLUMN_COUNTS)[]) { @@ -761,15 +771,6 @@ export const dashboardLogic = kea({ allLayouts[col] = cleanLayouts } - if (tilesWithNoLayout.length > 0) { - const layoutsByTileId = layoutsByTile(allLayouts) - actions.saveLayouts( - tilesWithNoLayout.map((t) => ({ - id: t.id, - layouts: layoutsByTileId[t.id], - })) - ) - } return allLayouts }, ], @@ -808,8 +809,8 @@ export const dashboardLogic = kea({ }, ], ], - }), - events: ({ actions, cache, props }) => ({ + })), + events(({ actions, cache, props }) => ({ afterMount: () => { if (props.id) { if (props.dashboard) { @@ -829,8 +830,8 @@ export const dashboardLogic = kea({ cache.autoRefreshInterval = null } }, - }), - sharedListeners: ({ values, props }) => ({ + })), + sharedListeners(({ values, props }) => ({ reportRefreshTiming: ({ shortId }) => { const refreshStatus = values.refreshStatus[shortId] @@ -849,11 +850,8 @@ export const dashboardLogic = kea({ eventUsageLogic.actions.reportDashboardLoadingTime(loadingMilliseconds, props.id) } }, - }), - listeners: ({ actions, values, cache, props, sharedListeners }) => ({ - [insightsModel.actionTypes.abortRunningQueries]: () => { - cache.abortController?.abort() - }, + })), + listeners(({ actions, values, cache, props, sharedListeners }) => ({ setRefreshError: sharedListeners.reportRefreshTiming, setRefreshStatuses: sharedListeners.reportRefreshTiming, setRefreshStatus: sharedListeners.reportRefreshTiming, @@ -891,7 +889,7 @@ export const dashboardLogic = kea({ updateLayouts: () => { actions.saveLayouts() }, - saveLayouts: async ({ tilesToSave }, breakpoint) => { + saveLayouts: async (_, breakpoint) => { await breakpoint(300) if (!isUserLoggedIn()) { // If user is anonymous (i.e. viewing a shared dashboard logged out), we don't save any layout changes. @@ -902,16 +900,25 @@ export const dashboardLogic = kea({ return } - const layoutsToUpdate = tilesToSave.length - ? tilesToSave - : (values.allItems?.tiles || []).map((tile) => ({ id: tile.id, layouts: tile.layouts })) + try { + const layoutsToUpdate = (values.allItems?.tiles || []).map((tile) => ({ + id: tile.id, + layouts: tile.layouts, + })) - breakpoint() + breakpoint() - return await api.update(`api/projects/${values.currentTeamId}/dashboards/${props.id}`, { - tiles: layoutsToUpdate, - no_items_field: true, - }) + await api.update(`api/projects/${values.currentTeamId}/dashboards/${props.id}`, { + tiles: layoutsToUpdate, + no_items_field: true, + }) + } catch (e: any) { + if (isBreakpoint(e)) { + console.log('out of order layout save was ignored') + } else { + throw e + } + } }, moveToDashboardSuccess: ({ payload }) => { if (payload?.toDashboard === undefined || payload?.tile === undefined) { @@ -1226,9 +1233,8 @@ export const dashboardLogic = kea({ api.create(`api/projects/${currentTeamId}/insights/cancel`, { client_query_id: queryId }) } }, - }), - - urlToAction: ({ actions }) => ({ + })), + urlToAction(({ actions }) => ({ '/dashboard/:id/subscriptions(/:subscriptionId)': ({ subscriptionId }) => { const id = subscriptionId ? subscriptionId == 'new' @@ -1255,5 +1261,18 @@ export const dashboardLogic = kea({ actions.setDashboardMode(null, null) actions.setTextTileId(textTileId === undefined ? 'new' : textTileId !== 'new' ? Number(textTileId) : 'new') }, - }), -}) + })), + subscriptions(({ cache }) => ({ + scene: (value: Scene | null | undefined, lastValue: Scene | null | undefined) => { + const scenesThatMightAbortClickHouseQueries = [Scene.Insight, Scene.Dashboard, Scene.ProjectHomepage] + if ( + value && + lastValue && + scenesThatMightAbortClickHouseQueries.includes(lastValue) && + !scenesThatMightAbortClickHouseQueries.includes(value) + ) { + cache.abortController?.abort() + } + }, + })), +]) diff --git a/frontend/src/scenes/insights/insightLogic.test.ts b/frontend/src/scenes/insights/insightLogic.test.ts index 5bd6cba7a4b62..e03d228169b85 100644 --- a/frontend/src/scenes/insights/insightLogic.test.ts +++ b/frontend/src/scenes/insights/insightLogic.test.ts @@ -32,6 +32,7 @@ import { MOCK_DEFAULT_TEAM } from 'lib/api.mock' import { dashboardsModel } from '~/models/dashboardsModel' import { insightsModel } from '~/models/insightsModel' import { DashboardPrivilegeLevel, DashboardRestrictionLevel } from 'lib/constants' +import { sceneLogic } from 'scenes/sceneLogic' const API_FILTERS: Partial = { insight: InsightType.TRENDS as InsightType, @@ -210,6 +211,7 @@ describe('insightLogic', () => { .toFinishAllListeners() .toMatchValues({ currentTeam: partial({ test_account_filters_default_checked: true }) }) insightsModel.mount() + sceneLogic.mount() }) it('requires props', () => { diff --git a/frontend/src/scenes/insights/insightLogic.ts b/frontend/src/scenes/insights/insightLogic.ts index 6cc5b5a022c00..9be0d753f66b0 100644 --- a/frontend/src/scenes/insights/insightLogic.ts +++ b/frontend/src/scenes/insights/insightLogic.ts @@ -59,6 +59,7 @@ import { toLocalFilters } from './filters/ActionFilter/entityFilterLogic' import { loaders } from 'kea-loaders' import { legacyInsightQuery } from '~/queries/query' import { tagsModel } from '~/models/tagsModel' +import { subscriptions } from 'kea-subscriptions' const IS_TEST_MODE = process.env.NODE_ENV === 'test' const SHOW_TIMEOUT_MESSAGE_AFTER = 15000 @@ -104,6 +105,8 @@ export const insightLogic = kea([ ['cohortsById'], mathsLogic, ['mathDefinitions'], + sceneLogic, + ['scene'], ], actions: [tagsModel, ['loadTags']], logic: [eventUsageLogic, dashboardsModel, prompt({ key: `save-as-insight` })], @@ -754,10 +757,7 @@ export const insightLogic = kea([ }, ], }), - listeners(({ actions, selectors, values, cache }) => ({ - [insightsModel.actionTypes.abortRunningQueries]: () => { - cache.abortController?.abort() - }, + listeners(({ actions, selectors, values }) => ({ setFiltersMerge: ({ filters }) => { actions.setFilters({ ...values.filters, ...filters }) }, @@ -1050,6 +1050,19 @@ export const insightLogic = kea([ } }, })), + subscriptions(({ cache }) => ({ + scene: (value: Scene | null | undefined, lastValue: Scene | null | undefined) => { + const scenesThatMightAbortClickHouseQueries = [Scene.Insight, Scene.Dashboard, Scene.ProjectHomepage] + if ( + value && + lastValue && + scenesThatMightAbortClickHouseQueries.includes(lastValue) && + !scenesThatMightAbortClickHouseQueries.includes(value) + ) { + cache.abortController?.abort() + } + }, + })), events(({ props, cache, values, actions }) => ({ afterMount: () => { if (!props.cachedInsight || !props.cachedInsight?.result || !!props.cachedInsight?.filters) { diff --git a/frontend/src/scenes/sceneLogic.test.tsx b/frontend/src/scenes/sceneLogic.test.tsx index 83529a1cfbe90..2486dfd68c659 100644 --- a/frontend/src/scenes/sceneLogic.test.tsx +++ b/frontend/src/scenes/sceneLogic.test.tsx @@ -54,21 +54,19 @@ describe('sceneLogic', () => { }) }) - it('reacts to navigating away from scenes that might have cancellable queries', async () => { - await expectLogic(logic, () => { - logic.actions.setScene(Scene.Dashboard, { params: { dashboardId: 1 }, searchParams: {}, hashParams: {} }) - router.actions.push(urls.mySettings()) - }).toDispatchActions([insightsModel.actionTypes.abortRunningQueries]) + it('exposes the last transition', async () => { + await expectLogic(logic).toMatchValues({ lastTransition: { from: null, to: null } }) await expectLogic(logic, () => { - logic.actions.setScene(Scene.Insight, { params: { insightId: 1 }, searchParams: {}, hashParams: {} }) - router.actions.push(urls.mySettings()) - }).toDispatchActions([insightsModel.actionTypes.abortRunningQueries]) + logic.actions.setScene(Scene.Dashboard, { params: { dashboardId: 1 }, searchParams: {}, hashParams: {} }) + }).toMatchValues({ lastTransition: { from: null, to: Scene.Dashboard } }) await expectLogic(logic, () => { - logic.actions.setScene(Scene.Dashboards, { params: {}, searchParams: {}, hashParams: {} }) + logic.actions.setScene(Scene.Dashboard, { params: { dashboardId: 1 }, searchParams: {}, hashParams: {} }) router.actions.push(urls.mySettings()) - }).toNotHaveDispatchedActions([insightsModel.actionTypes.abortRunningQueries]) + }) + .toFinishAllListeners() + .toMatchValues({ lastTransition: { from: Scene.Dashboard, to: Scene.MySettings } }) }) it('persists the loaded scenes', async () => { diff --git a/frontend/src/scenes/sceneLogic.ts b/frontend/src/scenes/sceneLogic.ts index 7631981c0f07d..d175cf8dc13de 100644 --- a/frontend/src/scenes/sceneLogic.ts +++ b/frontend/src/scenes/sceneLogic.ts @@ -15,7 +15,11 @@ import { organizationLogic } from './organizationLogic' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' import { UPGRADE_LINK } from 'lib/constants' import { appContextLogic } from './appContextLogic' -import { insightsModel } from '~/models/insightsModel' + +export interface SceneTransition { + from: Scene | null + to: Scene | null +} /** Mapping of some scenes that aren't directly accessible from the sidebar to ones that are - for the sidebar. */ const sceneNavAlias: Partial> = { @@ -84,6 +88,12 @@ export const sceneLogic = kea({ setScene: (_, payload) => payload.scene, }, ], + lastTransition: [ + { from: null, to: null } as SceneTransition, + { + setScene: (state, payload) => ({ from: state.to, to: payload.scene }), + }, + ], loadedScenes: [ preloadedScenes, { @@ -293,17 +303,6 @@ export const sceneLogic = kea({ if (values.scene === scene) { actions.setScene(scene, params, clickedLink) return - } else { - const scenesThatMightAbortClickHouseQueries = [Scene.Insight, Scene.Dashboard, Scene.ProjectHomepage] - - if ( - values.scene && - scenesThatMightAbortClickHouseQueries.includes(values.scene) && - !scenesThatMightAbortClickHouseQueries.includes(scene) - ) { - // we're navigating away from a scene that might be running long ClickHouse queries - insightsModel.actions.abortRunningQueries() - } } if (!props.scenes?.[scene]) { From f1079bc2a6c12afbf75a2ec5ce66925cc6339d84 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 6 Dec 2022 18:26:57 +0000 Subject: [PATCH 15/16] test fangling --- .../scenes/dashboard/dashboardLogic.test.ts | 13 ++++---- .../src/scenes/dashboard/dashboardLogic.tsx | 30 +++++++++---------- .../src/scenes/insights/insightLogic.test.ts | 18 ++--------- 3 files changed, 22 insertions(+), 39 deletions(-) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.test.ts b/frontend/src/scenes/dashboard/dashboardLogic.test.ts index e6e1358f40fe4..f4650e1473804 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.test.ts +++ b/frontend/src/scenes/dashboard/dashboardLogic.test.ts @@ -23,6 +23,7 @@ import { dayjs, now } from 'lib/dayjs' import { teamLogic } from 'scenes/teamLogic' import { MOCK_TEAM_ID } from 'lib/api.mock' import api from 'lib/api' +import { sceneLogic } from 'scenes/sceneLogic' const dashboardJson = _dashboardJson as any as DashboardType @@ -273,6 +274,7 @@ describe('dashboardLogic', () => { initKeaTests() dashboardsModel.mount() insightsModel.mount() + sceneLogic.mount() }) describe('tile layouts', () => { @@ -820,18 +822,15 @@ describe('dashboardLogic', () => { beforeEach(async () => { logic = dashboardLogic({ id: 5 }) logic.mount() - await expectLogic(logic).toFinishAllListeners() }) it('can remove text tiles', async () => { await expectLogic(logic, () => { logic.actions.removeTile(TEXT_TILE) - }) - .toFinishAllListeners() - .toDispatchActions([ - dashboardsModel.actionTypes.tileRemovedFromDashboard, - logic.actionTypes.removeTileSuccess, - ]) + }).toDispatchActions([ + dashboardsModel.actionTypes.tileRemovedFromDashboard, + logic.actionTypes.removeTileSuccess, + ]) expect(logic.values.textTiles).toEqual([]) }) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index 2b3599d55bb4d..c557a79a5d93f 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -15,7 +15,7 @@ import { import api, { ApiMethodOptions, getJSONOrThrow } from 'lib/api' import { dashboardsModel } from '~/models/dashboardsModel' import { router, urlToAction } from 'kea-router' -import { clearDOMTextSelection, isUserLoggedIn, toParams, uuid } from 'lib/utils' +import { clearDOMTextSelection, toParams, uuid } from 'lib/utils' import { insightsModel } from '~/models/insightsModel' import { AUTO_REFRESH_DASHBOARD_THRESHOLD_HOURS, @@ -56,6 +56,7 @@ import { featureFlagLogic } from 'lib/logic/featureFlagLogic' import { Scene } from 'scenes/sceneTypes' import { subscriptions } from 'kea-subscriptions' import { loaders } from 'kea-loaders' +import { sceneLogic } from 'scenes/sceneLogic' export const BREAKPOINTS: Record = { sm: 1024, @@ -127,15 +128,15 @@ function updateExistingInsightState({ cachedInsight, dashboardId, refreshedInsig export const dashboardLogic = kea([ path(['scenes', 'dashboard', 'dashboardLogic']), connect(() => ({ - values: [teamLogic, ['currentTeamId'], featureFlagLogic, ['featureFlags']], + values: [teamLogic, ['currentTeamId'], featureFlagLogic, ['featureFlags'], sceneLogic, ['scene']], logic: [dashboardsModel, insightsModel, eventUsageLogic], })), props({} as DashboardLogicProps), - key((props) => { + key((props: DashboardLogicProps) => { if (typeof props.id === 'string') { throw Error('Must init dashboardLogic with a numeric key') } - return props.id ?? 'new' + return props.id?.toString() || 'new' }), actions({ loadExportedDashboard: (dashboard: DashboardType | null) => ({ dashboard }), @@ -890,24 +891,21 @@ export const dashboardLogic = kea([ actions.saveLayouts() }, saveLayouts: async (_, breakpoint) => { - await breakpoint(300) - if (!isUserLoggedIn()) { - // If user is anonymous (i.e. viewing a shared dashboard logged out), we don't save any layout changes. - return - } - if (!props.id) { - // what are we saving layouts against?! - return - } - try { + await breakpoint(300) + + if (!props.id) { + // what are we saving layouts against?! + return + } + + breakpoint() + const layoutsToUpdate = (values.allItems?.tiles || []).map((tile) => ({ id: tile.id, layouts: tile.layouts, })) - breakpoint() - await api.update(`api/projects/${values.currentTeamId}/dashboards/${props.id}`, { tiles: layoutsToUpdate, no_items_field: true, diff --git a/frontend/src/scenes/insights/insightLogic.test.ts b/frontend/src/scenes/insights/insightLogic.test.ts index e03d228169b85..d89d8a1b25aca 100644 --- a/frontend/src/scenes/insights/insightLogic.test.ts +++ b/frontend/src/scenes/insights/insightLogic.test.ts @@ -13,12 +13,10 @@ import { InsightModel, InsightShortId, InsightType, - ItemMode, PropertyFilterType, PropertyGroupFilter, PropertyOperator, } from '~/types' -import { eventUsageLogic } from 'lib/utils/eventUsageLogic' import { combineUrl, router } from 'kea-router' import { dashboardLogic } from 'scenes/dashboard/dashboardLogic' import { savedInsightsLogic } from 'scenes/saved-insights/savedInsightsLogic' @@ -111,6 +109,7 @@ describe('insightLogic', () => { useAvailableFeatures([AvailableFeature.DASHBOARD_COLLABORATION]) useMocks({ get: { + '/api/projects/:team/tags/': { results: ['good', 'bad'] }, '/api/projects/:team/insights/trend/': (req) => { if (JSON.parse(req.url.searchParams.get('events') || '[]')?.[0]?.throw) { return [500, { status: 0, detail: 'error from the API' }] @@ -297,20 +296,7 @@ describe('insightLogic', () => { await expectLogic(logic, () => { logic.actions.setFilters({ insight: InsightType.FUNNELS }) - }).toDispatchActions([ - eventUsageLogic.actionCreators.reportInsightViewed( - insight, - { insight: InsightType.FUNNELS }, - ItemMode.View, - true, - false, - 0, - { - changed_insight: InsightType.TRENDS, - }, - false - ), - ]) + }).toDispatchActions(['reportInsightViewed']) }) }) From 2161dfa53cb6413a91273fa461fc57e4d06edd41 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 6 Dec 2022 20:05:45 +0000 Subject: [PATCH 16/16] no need to change scenelogic --- frontend/src/scenes/sceneLogic.test.tsx | 18 ------------------ frontend/src/scenes/sceneLogic.ts | 12 ------------ 2 files changed, 30 deletions(-) diff --git a/frontend/src/scenes/sceneLogic.test.tsx b/frontend/src/scenes/sceneLogic.test.tsx index 2486dfd68c659..edf4c4b4d6f3b 100644 --- a/frontend/src/scenes/sceneLogic.test.tsx +++ b/frontend/src/scenes/sceneLogic.test.tsx @@ -9,7 +9,6 @@ import { urls } from 'scenes/urls' import { kea, path } from 'kea' import type { logicType } from './sceneLogic.testType' -import { insightsModel } from '~/models/insightsModel' export const Component = (): JSX.Element =>
export const logic = kea([path(['scenes', 'sceneLogic', 'test'])]) @@ -18,7 +17,6 @@ const sceneImport = (): any => ({ scene: { component: Component, logic: logic } const testScenes: Record any> = { [Scene.Annotations]: sceneImport, [Scene.MySettings]: sceneImport, - [Scene.Dashboard]: sceneImport, } describe('sceneLogic', () => { @@ -28,7 +26,6 @@ describe('sceneLogic', () => { initKeaTests() await expectLogic(teamLogic).toDispatchActions(['loadCurrentTeamSuccess']) featureFlagLogic.mount() - insightsModel.mount() router.actions.push(urls.annotations()) logic = sceneLogic({ scenes: testScenes }) logic.mount() @@ -54,21 +51,6 @@ describe('sceneLogic', () => { }) }) - it('exposes the last transition', async () => { - await expectLogic(logic).toMatchValues({ lastTransition: { from: null, to: null } }) - - await expectLogic(logic, () => { - logic.actions.setScene(Scene.Dashboard, { params: { dashboardId: 1 }, searchParams: {}, hashParams: {} }) - }).toMatchValues({ lastTransition: { from: null, to: Scene.Dashboard } }) - - await expectLogic(logic, () => { - logic.actions.setScene(Scene.Dashboard, { params: { dashboardId: 1 }, searchParams: {}, hashParams: {} }) - router.actions.push(urls.mySettings()) - }) - .toFinishAllListeners() - .toMatchValues({ lastTransition: { from: Scene.Dashboard, to: Scene.MySettings } }) - }) - it('persists the loaded scenes', async () => { const expectedAnnotation = partial({ name: Scene.Annotations, diff --git a/frontend/src/scenes/sceneLogic.ts b/frontend/src/scenes/sceneLogic.ts index d175cf8dc13de..b97f62b9a56ec 100644 --- a/frontend/src/scenes/sceneLogic.ts +++ b/frontend/src/scenes/sceneLogic.ts @@ -16,11 +16,6 @@ import { featureFlagLogic } from 'lib/logic/featureFlagLogic' import { UPGRADE_LINK } from 'lib/constants' import { appContextLogic } from './appContextLogic' -export interface SceneTransition { - from: Scene | null - to: Scene | null -} - /** Mapping of some scenes that aren't directly accessible from the sidebar to ones that are - for the sidebar. */ const sceneNavAlias: Partial> = { [Scene.Action]: Scene.DataManagement, @@ -88,12 +83,6 @@ export const sceneLogic = kea({ setScene: (_, payload) => payload.scene, }, ], - lastTransition: [ - { from: null, to: null } as SceneTransition, - { - setScene: (state, payload) => ({ from: state.to, to: payload.scene }), - }, - ], loadedScenes: [ preloadedScenes, { @@ -299,7 +288,6 @@ export const sceneLogic = kea({ }, loadScene: async ({ scene, params, method }, breakpoint) => { const clickedLink = method === 'PUSH' - if (values.scene === scene) { actions.setScene(scene, params, clickedLink) return