fix(insights): Remove insight reuse on dashboard and insights (#21471)

* Remove find insight from mounted logic

We rather load fresh to be save

* Do not set insights anymore

* Adjust dataNodeLogic

* Make insightLogic not load things on mount when cached

* Make sure refresh happens on filter change

* Make dataNodeLogic never load for dashboard with cached results

* Fix usage of `updateTileOnDashboards`

Fixes remaining unwanted propagation of dashboard filter changes to other dashboards.

* Fix "Remove from dashboard" buttons being out of date

Co-authored-by: Michael Matloka <[email protected]>

cypress/e2e/dashboard.cy.ts

@@ -1,5 +1,6 @@
 import { randomString } from '../support/random'
 import { insight, dashboards, dashboard } from '../productAnalytics'
+import { urls } from 'scenes/urls'
 
 describe('Dashboard', () => {
     beforeEach(() => {
@@ -20,25 +21,93 @@ describe('Dashboard', () => {
     })
 
     it('Adding new insight to dashboard works', () => {
-        const dashboardName = randomString('Dashboard with insight A')
+        const dashboardName = randomString('Dashboard with matching filter')
         const insightName = randomString('insight to add to dashboard')
 
-        // create and visit a dashboard to get it into turbomode cache
+        // Create and visit a dashboard to get it into turbo mode cache
         dashboards.createAndGoToEmptyDashboard(dashboardName)
 
         insight.create(insightName)
 
         insight.addInsightToDashboard(dashboardName, { visitAfterAdding: true })
 
         cy.get('.CardMeta h4').should('have.text', insightName)
+
+        dashboard.addPropertyFilter()
+        cy.get('main').contains('There are no matching events for this query').should('not.exist')
+
+        cy.clickNavMenu('dashboards')
+        const dashboardNonMatching = randomString('Dashboard with non-matching filter')
+        dashboards.createAndGoToEmptyDashboard(dashboardNonMatching)
+
+        insight.visitInsight(insightName)
+        insight.addInsightToDashboard(dashboardNonMatching, { visitAfterAdding: true })
+
+        dashboard.addPropertyFilter('Browser', 'Hogbrowser')
+        cy.get('main').contains('There are no matching events for this query').should('exist')
+
+        // Go back and forth to make sure the filters are correctly applied
+        for (let i = 0; i < 3; i++) {
+            cy.clickNavMenu('dashboards')
+            dashboards.visitDashboard(dashboardName)
+            cy.get('.CardMeta h4').should('have.text', insightName)
+            cy.get('h4').contains('Refreshing').should('not.exist')
+            cy.get('main').contains('There are no matching events for this query').should('not.exist')
+
+            cy.clickNavMenu('dashboards')
+            dashboards.visitDashboard(dashboardNonMatching)
+            cy.get('.CardMeta h4').should('have.text', insightName)
+            cy.get('h4').contains('Refreshing').should('not.exist')
+            cy.get('main').contains('There are no matching events for this query').should('exist')
+        }
+    })
+
+    it('Dashboard filter updates are correctly isolated for one insight on multiple dashboards', () => {
+        const dashboardAName = randomString('Dashboard with insight A')
+        const dashboardBName = randomString('Dashboard with insight B')
+        const insightName = randomString('insight to add to dashboard')
+
+        // Create and visit two dashboards to get them into turbo mode cache
+        dashboards.createAndGoToEmptyDashboard(dashboardAName)
+        cy.clickNavMenu('dashboards')
+        dashboards.createAndGoToEmptyDashboard(dashboardBName)
+
+        insight.create(insightName)
+
+        // Add that one insight to both dashboards
+        insight.addInsightToDashboard(dashboardAName, { visitAfterAdding: false })
+        cy.get('[aria-label="close"]').click()
+        insight.addInsightToDashboard(dashboardBName, { visitAfterAdding: false })
+        cy.get('[aria-label="close"]').click()
+
+        // Let's get dashboard A mounted
+        cy.clickNavMenu('dashboards')
+        dashboards.visitDashboard(dashboardAName)
+        cy.get('[data-attr=date-filter]').contains('No date range override')
+        cy.get('.InsightCard h5').should('have.length', 1).contains('Last 7 days')
+        // Now let's see dashboard B
+        cy.clickNavMenu('dashboards')
+        dashboards.visitDashboard(dashboardBName)
+        cy.get('[data-attr=date-filter]').contains('No date range override')
+        cy.get('.InsightCard h5').should('have.length', 1).contains('Last 7 days')
+        // Override the time range on dashboard B
+        cy.get('[data-attr=date-filter]').contains('No date range override').click()
+        cy.get('div').contains('Yesterday').should('exist').click()
+        cy.get('[data-attr=date-filter]').contains('Yesterday')
+        cy.get('.InsightCard h5').should('have.length', 1).contains('Yesterday')
+        // Cool, now back to A and make sure the insight is still using the original range there, not the one from B
+        cy.clickNavMenu('dashboards')
+        dashboards.visitDashboard(dashboardAName)
+        cy.get('[data-attr=date-filter]').contains('No date range override')
+        cy.get('.InsightCard h5').should('have.length', 1).contains('Last 7 days') // This must not be "Yesterday"!
     })
 
     it('Adding new insight to dashboard does not clear filters', () => {
         const dashboardName = randomString('to add an insight to')
         const firstInsight = randomString('insight to add to dashboard')
         const secondInsight = randomString('another insight to add to dashboard')
 
-        // create and visit a dashboard to get it into turbomode cache
+        // Create and visit a dashboard to get it into turbo mode cache
         dashboards.createAndGoToEmptyDashboard(dashboardName)
         dashboard.addInsightToEmptyDashboard(firstInsight)
 

cypress/productAnalytics/index.ts

@@ -67,7 +67,7 @@ export const insight = {
     },
     visitInsight: (insightName: string): void => {
         cy.clickNavMenu('savedinsights')
-        cy.contains('.row-name > .Link', insightName).click()
+        cy.contains('.Link', insightName).click()
     },
     create: (insightName: string, insightType: string = 'TRENDS'): void => {
         cy.clickNavMenu('savedinsights')
@@ -169,6 +169,14 @@ export const dashboard = {
         cy.get('[data-attr=insight-save-button]').contains('Save & add to dashboard').click()
         cy.wait('@postInsight')
     },
+    addPropertyFilter(type: string = 'Browser', value: string = 'Chrome'): void {
+        cy.get('.PropertyFilterButton').should('have.length', 0)
+        cy.get('[data-attr="property-filter-0"]').click()
+        cy.get('[data-attr="taxonomic-filter-searchfield"]').click().type('Browser').wait(1000)
+        cy.get('[data-attr="prop-filter-event_properties-0"]').click({ force: true })
+        cy.get('.LemonInput').type(value)
+        cy.contains('.LemonButton__content', value).click({ force: true })
+    },
     addAnyFilter(): void {
         cy.get('.PropertyFilterButton').should('have.length', 0)
         cy.get('[data-attr="property-filter-0"]').click()

frontend/src/lib/components/AddToDashboard/AddToDashboardModal.tsx

@@ -42,7 +42,6 @@ const DashboardRelationRow = ({
 }: DashboardRelationRowProps): JSX.Element => {
     const logic = addToDashboardModalLogic({
         insight: insight,
-        fromDashboard: insight.dashboards?.[0] || undefined,
     })
     const { addToDashboard, removeFromDashboard } = useActions(logic)
     const { dashboardWithActiveAPICall } = useValues(logic)
@@ -104,7 +103,6 @@ export function AddToDashboardModal({
 }: SaveToDashboardModalProps): JSX.Element {
     const logic = addToDashboardModalLogic({
         insight: insight,
-        fromDashboard: insight.dashboards?.[0] || undefined,
     })
 
     const { searchQuery, currentDashboards, orderedDashboards, scrollIndex } = useValues(logic)
@@ -128,7 +126,10 @@ export function AddToDashboardModal({
 
     return (
         <LemonModal
-            onClose={closeModal}
+            onClose={() => {
+                closeModal()
+                setSearchQuery('')
+            }}
             isOpen={isOpen}
             title="Add to dashboard"
             footer={
@@ -139,7 +140,7 @@ export function AddToDashboardModal({
                             onClick={addNewDashboard}
                             disabledReason={
                                 !canEditInsight
-                                    ? 'You do not have permission to add this Insight to dashboards'
+                                    ? 'You do not have permission to add this insight to dashboards'
                                     : undefined
                             }
                         >
@@ -162,9 +163,8 @@ export function AddToDashboardModal({
                     onChange={(newValue) => setSearchQuery(newValue)}
                 />
                 <div className="text-muted-alt">
-                    This insight is referenced on{' '}
-                    <strong className="text-default">{insight.dashboard_tiles?.length}</strong>{' '}
-                    {pluralize(insight.dashboard_tiles?.length || 0, 'dashboard', 'dashboards', false)}
+                    This insight is referenced on <strong className="text-default">{currentDashboards.length}</strong>{' '}
+                    {pluralize(currentDashboards.length, 'dashboard', 'dashboards', false)}
                 </div>
                 {/* eslint-disable-next-line react/forbid-dom-props */}
                 <div style={{ minHeight: 420 }}>

frontend/src/lib/components/AddToDashboard/addToDashboardModalLogic.ts

@@ -14,7 +14,6 @@ import type { addToDashboardModalLogicType } from './addToDashboardModalLogicTyp
 
 export interface AddToDashboardModalLogicProps {
     insight: Partial<InsightModel>
-    fromDashboard?: number
 }
 
 // Helping kea-typegen navigate the exported default class for Fuse
@@ -30,16 +29,27 @@ export const addToDashboardModalLogic = kea<addToDashboardModalLogicType>([
         return insight.short_id
     }),
     path(['lib', 'components', 'AddToDashboard', 'saveToDashboardModalLogic']),
-    connect((props: AddToDashboardModalLogicProps) => ({
-        actions: [
-            insightLogic({ dashboardItemId: props.insight.short_id, cachedInsight: props.insight }),
-            ['updateInsight', 'updateInsightSuccess', 'updateInsightFailure'],
-            eventUsageLogic,
-            ['reportSavedInsightToDashboard', 'reportRemovedInsightFromDashboard', 'reportCreatedDashboardFromModal'],
-            newDashboardLogic,
-            ['showNewDashboardModal'],
-        ],
-    })),
+    connect((props: AddToDashboardModalLogicProps) => {
+        const builtInsightLogic = insightLogic({
+            dashboardItemId: props.insight.short_id,
+            cachedInsight: props.insight,
+        })
+        return {
+            values: [builtInsightLogic, ['insight']],
+            actions: [
+                builtInsightLogic,
+                ['updateInsight', 'updateInsightSuccess', 'updateInsightFailure'],
+                eventUsageLogic,
+                [
+                    'reportSavedInsightToDashboard',
+                    'reportRemovedInsightFromDashboard',
+                    'reportCreatedDashboardFromModal',
+                ],
+                newDashboardLogic,
+                ['showNewDashboardModal'],
+            ],
+        }
+    }),
     actions({
         addNewDashboard: true,
         setSearchQuery: (query: string) => ({ query }),
@@ -79,8 +89,8 @@ export const addToDashboardModalLogic = kea<addToDashboardModalLogicType>([
                     : nameSortedDashboards,
         ],
         currentDashboards: [
-            (s, p) => [s.filteredDashboards, p.insight],
-            (filteredDashboards, insight: InsightModel): DashboardBasicType[] =>
+            (s) => [s.filteredDashboards, s.insight],
+            (filteredDashboards, insight): DashboardBasicType[] =>
                 filteredDashboards.filter((d) => insight.dashboard_tiles?.map((dt) => dt.dashboard_id)?.includes(d.id)),
         ],
         availableDashboards: [

frontend/src/lib/components/Cards/TextCard/TextCard.tsx

@@ -133,7 +133,7 @@ export function TextCardInternal(
                                         fullWidth
                                         data-attr="remove-text-tile-from-dashboard"
                                     >
-                                        Remove from dashboard
+                                        Delete
                                     </LemonButton>
                                 )}
                             </>

frontend/src/mocks/fixtures/api/projects/team_id/insights/stickiness.json

@@ -1,6 +1,6 @@
 {
     "id": 51,
-    "short_id": "jEAIVJnI",
+    "short_id": "xEAIVJnI",
     "name": "",
     "derived_name": "User stickiness based on Pageview",
     "filters": {

frontend/src/mocks/fixtures/api/projects/team_id/insights/trendsBarBreakdown.json

@@ -1,6 +1,6 @@
 {
     "id": 51,
-    "short_id": "jEAIVJnK",
+    "short_id": "xEAIVJnK",
     "name": "",
     "derived_name": "Pageview count by event's Library Version",
     "filters": {

frontend/src/mocks/fixtures/api/projects/team_id/insights/trendsCumulativeBreakdown.json

@@ -1,6 +1,6 @@
 {
     "id": 51,
-    "short_id": "jEAIVJnM",
+    "short_id": "xEAIVJnM",
     "name": "",
     "derived_name": "Pageview count by event's Browser Version",
     "filters": {

frontend/src/queries/nodes/DataNode/dataNodeLogic.ts

@@ -113,15 +113,16 @@ export const dataNodeLogic = kea<dataNodeLogicType>([
         if (oldProps.query && props.query.kind !== oldProps.query.kind) {
             actions.clearResponse()
         }
-        if (!queryEqual(props.query, oldProps.query)) {
-            if (
-                !props.cachedResults ||
-                (isInsightQueryNode(props.query) && !props.cachedResults['result'] && !props.cachedResults['results'])
-            ) {
-                actions.loadData()
-            } else {
-                actions.setResponse(props.cachedResults)
-            }
+        if (
+            !(props.cachedResults && props.key.includes('dashboard')) && // Don't load data on dashboard if cached results are available
+            !queryEqual(props.query, oldProps.query) &&
+            (!props.cachedResults ||
+                (isInsightQueryNode(props.query) && !props.cachedResults['result'] && !props.cachedResults['results']))
+        ) {
+            actions.loadData()
+        } else if (props.cachedResults) {
+            // Use cached results if available, otherwise this logic will load the data again
+            actions.setResponse(props.cachedResults)
         }
     }),
     actions({
@@ -624,7 +625,9 @@ export const dataNodeLogic = kea<dataNodeLogicType>([
         },
     })),
     afterMount(({ actions, props }) => {
-        if (Object.keys(props.query || {}).length > 0) {
+        if (Object.keys(props.query || {}).length > 0 && !props.key.includes('dashboard')) {
+            // Attention: When on dashboard we don't want to load data on mount
+            // as it will have be loaded by some other logic
             actions.loadData()
         }