Repository: PostHog/posthog
Pull request: feat(insights): Warn before leaving insight with unsaved changes
## Problem

Closes #7121.
This is a [Quality Launch Week](https://github.com/PostHog/posthog/projects/15) problem.

## Changes

Now before you leave an insight with unsaved changes, you'll be prompted whether you're okay with those being discarded.
If you confirm, the changes will be gone forever, but if you protest, you will be prevented from navigating away.

The browser's native "unsaved changes" prompt is used when navigating away from the page completely (i.e. a full page reload or leaving PostHog):

<img width="292" alt="Screen Shot 2022-03-17 at 15 53 09" src="https://user-images.githubusercontent.com/4550621/158867857-bdb0e84d-2b1d-4150-a2eb-767605c4959a.png">

Because PostHog is a single-page application we can't use this in internal navigation though, as the browser never unloads the page then (which is kind of the point of it). So in this case the classic `confirm()` is used instead:

<img width="468" alt="Screen Shot 2022-03-17 at 15 52 41" src="https://user-images.githubusercontent.com/4550621/158868160-f0f34a49-161a-42a6-9325-54bc459d13d6.png">

## How did you test this code?

~~TBD, probably will add a Cypress test?~~ Added a Cypress test.

Problem statement:
Warn when leaving an insight with unsaved state
## Is your feature request related to a problem?

Currently when you edit an insight, it's possible to leave the page in the middle of editing e.g. an insight name. All changes are thereupon lost.

Given user needs to manually "confirm" every change for titles, this can lead to annoying back-and-forth.

## Describe the solution you'd like

`window.confirm` or equivelent

## Describe alternatives you've considered

Autosaving titles

## Additional context

cc @paolodamico @clarkus 

#### *Thank you* for your feature request – we love each and every one!

Hints from issue discussion:

Commit message: Handle insight ID changes and auto-discard

Patch to review:
diff --git a/frontend/src/lib/components/PropertyGroupFilters/PropertyGroupFilters.tsx b/frontend/src/lib/components/PropertyGroupFilters/PropertyGroupFilters.tsx
index e2635d9dbb474..a8a6e868543a1 100644
--- a/frontend/src/lib/components/PropertyGroupFilters/PropertyGroupFilters.tsx
+++ b/frontend/src/lib/components/PropertyGroupFilters/PropertyGroupFilters.tsx
@@ -45,6 +45,9 @@ export function PropertyGroupFilters({
 
     // Update the logic's internal filters when the props change
     useEffect(() => {
+        // TRICKY: This disrupts `insightLogic`'s `filtersChanged` selector,
+        // as this is a different default value from what was used before logical operators
+        // (back then empty filters were indicated by `[]` instead of `{ type: FilterLogicalOperator.And, values: [] }`)
         setFilters(propertyFilters ?? { type: FilterLogicalOperator.And, values: [] })
     }, [propertyFilters])
 
diff --git a/frontend/src/lib/utils.tsx b/frontend/src/lib/utils.tsx
index 1ac46a9712c9e..bc420669c9768 100644
--- a/frontend/src/lib/utils.tsx
+++ b/frontend/src/lib/utils.tsx
@@ -17,6 +17,7 @@ import {
     PropertyFilter,
     CohortType,
 } from '~/types'
+import equal from 'fast-deep-equal'
 import { tagColors } from 'lib/colors'
 import { WEBHOOK_SERVICES } from 'lib/constants'
 import { KeyMappingInterface } from 'lib/components/PropertyKeyInfo'
@@ -416,7 +417,7 @@ export function formatLabel(label: string, action: ActionFilter): string {
 }
 
 export function objectsEqual(obj1: any, obj2: any): boolean {
-    return JSON.stringify(obj1) === JSON.stringify(obj2)
+    return equal(obj1, obj2)
 }
 
 /** Returns "response" from: obj2 = { ...obj1, ...response }  */
diff --git a/frontend/src/scenes/insights/Insight.tsx b/frontend/src/scenes/insights/Insight.tsx
index b601667316623..86f2bbd532bbb 100644
--- a/frontend/src/scenes/insights/Insight.tsx
+++ b/frontend/src/scenes/insights/Insight.tsx
@@ -1,5 +1,5 @@
 import './Insight.scss'
-import React from 'react'
+import React, { useEffect } from 'react'
 import { useActions, useMountedLogic, useValues, BindLogic } from 'kea'
 import { Row, Col, Button, Popconfirm, Card } from 'antd'
 import { FunnelTab, PathTab, RetentionTab, TrendTab } from './InsightTabs'
@@ -46,11 +46,10 @@ export function Insight({ insightId }: { insightId: InsightShortId }): JSX.Eleme
         activeView,
         insight,
         filtersChanged,
-        savedFilters,
         tagLoading,
     } = useValues(logic)
     useMountedLogic(insightCommandLogic(insightProps))
-    const { setActiveView, saveInsight, setFilters, setInsightMetadata, saveAs } = useActions(logic)
+    const { setActiveView, saveInsight, discardChanges, setInsightMetadata, saveAs } = useActions(logic)
     const { hasAvailableFeature } = useValues(userLogic)
     const { reportHotkeyNavigation } = useActions(eventUsageLogic)
     const { cohortModalVisible } = useValues(personsModalLogic)
@@ -69,6 +68,18 @@ export function Insight({ insightId }: { insightId: InsightShortId }): JSX.Eleme
         reportHotkeyNavigation('insights', hotkey)
     }
 
+    useEffect(() => {
+        const beforeUnloadHandler = (e: BeforeUnloadEvent): void => {
+            // Cancel the event to show unsaved changes dialog
+            e.preventDefault()
+            e.returnValue = ''
+        }
+        if (filtersChanged) {
+            window.addEventListener('beforeunload', beforeUnloadHandler)
+            return () => window.removeEventListener('beforeunload', beforeUnloadHandler)
+        }
+    }, [filtersChanged])
+
     useKeyboardHotkeys({
         t: {
             action: () => handleHotkeyNavigation(InsightType.TRENDS, 't'),
@@ -139,7 +150,7 @@ export function Insight({ insightId }: { insightId: InsightShortId }): JSX.Eleme
                             <Popconfirm
                                 title="Are you sure? This will discard all unsaved changes in this insight."
                                 onConfirm={() => {
-                                    setFilters(savedFilters)
+                                    discardChanges()
                                     reportInsightsTabReset()
                                 }}
                             >
diff --git a/frontend/src/scenes/insights/insightLogic.ts b/frontend/src/scenes/insights/insightLogic.ts
index 8c13b1218f64c..42ae5bb048df1 100644
--- a/frontend/src/scenes/insights/insightLogic.ts
+++ b/frontend/src/scenes/insights/insightLogic.ts
@@ -116,6 +116,7 @@ export const insightLogic = kea<insightLogicType>({
         }),
         saveAs: true,
         saveAsNamingSuccess: (name: string) => ({ name }),
+        discardChanges: true,
         setInsightDescription: (description: string) => ({ description }),
         saveInsight: (options?: Record<string, any>) => ({ setViewMode: options?.setViewMode }),
         setTagLoading: (tagLoading: boolean) => ({ tagLoading }),
@@ -728,6 +729,9 @@ export const insightLogic = kea<insightLogicType>({
                 },
             })
         },
+        discardChanges: () => {
+            actions.setFilters(values.savedFilters)
+        },
     }),
 
     events: ({ actions, cache, props, values }) => ({
diff --git a/frontend/src/scenes/insights/insightSceneLogic.tsx b/frontend/src/scenes/insights/insightSceneLogic.tsx
index 281131df48642..dab6562551901 100644
--- a/frontend/src/scenes/insights/insightSceneLogic.tsx
+++ b/frontend/src/scenes/insights/insightSceneLogic.tsx
@@ -10,6 +10,8 @@ import { cleanFilters } from 'scenes/insights/utils/cleanFilters'
 import { insightLogicType } from 'scenes/insights/insightLogicType'
 import { insightLogic } from 'scenes/insights/insightLogic'
 import { lemonToast } from 'lib/components/lemonToast'
+import { sceneLogic } from 'scenes/sceneLogic'
+import { Scene } from 'scenes/sceneTypes'
 
 export const insightSceneLogic = kea<insightSceneLogicType>({
     path: ['scenes', 'insights', 'insightSceneLogic'],
@@ -130,6 +132,20 @@ export const insightSceneLogic = kea<insightSceneLogicType>({
             const insightId = String(shortId) as InsightShortId
             const oldInsightId = values.insightId
             if (insightId !== oldInsightId || insightMode !== values.insightMode) {
+                // If navigating from an unsaved insight to a different insight within the scene, prompt the user
+                if (
+                    sceneLogic.values.scene === Scene.Insight &&
+                    insightId !== oldInsightId &&
+                    oldInsightId !== 'new' &&
+                    values.insightCache?.logic.values.filtersChanged
+                ) {
+                    if (confirm('Leave insight? Changes you made may not be saved.')) {
+                        values.insightCache.logic.actions.discardChanges()
+                    } else {
+                        history.back()
+                        return
+                    }
+                }
                 actions.setSceneState(insightId, insightMode)
                 if (insightId !== oldInsightId && insightId === 'new') {
                     actions.createNewInsight(filters)
diff --git a/frontend/src/scenes/sceneLogic.ts b/frontend/src/scenes/sceneLogic.ts
index 24d25f07e4729..fd7675aa85c34 100644
--- a/frontend/src/scenes/sceneLogic.ts
+++ b/frontend/src/scenes/sceneLogic.ts
@@ -15,6 +15,7 @@ import { emptySceneParams, preloadedScenes, redirects, routes, sceneConfiguratio
 import { organizationLogic } from './organizationLogic'
 import { FEATURE_FLAGS } from 'lib/constants'
 import { featureFlagLogic } from 'lib/logic/featureFlagLogic'
+import { insightSceneLogic } from './insights/insightSceneLogic'
 
 /** Mapping of some scenes that aren't directly accessible from the sidebar to ones that are - for the sidebar. */
 const sceneNavAlias: Partial<Record<Scene, Scene>> = {
@@ -226,6 +227,20 @@ export const sceneLogic = kea<sceneLogicType>({
             }
         },
         openScene: ({ scene, params, method }) => {
+            // If navigating from an Insight scene to a non-Insight scene and changes are unsaved, prompt the user
+            if (
+                values.scene === Scene.Insight &&
+                scene !== Scene.Insight &&
+                insightSceneLogic.findMounted()?.values.insightCache?.logic.values.filtersChanged
+            ) {
+                if (confirm('Leave insight? Changes you made may not be saved.')) {
+                    insightSceneLogic.findMounted()?.values.insightCache?.logic.actions.discardChanges()
+                } else {
+                    history.back()
+                    return
+                }
+            }
+
             const sceneConfig = sceneConfigurations[scene] || {}
             const { user } = userLogic.values
             const { preflight } = preflightLogic.values


Code review comments:
diff_hunk: @@ -130,6 +132,20 @@ export const insightSceneLogic = kea<insightSceneLogicType>({
             const insightId = String(shortId) as InsightShortId
             const oldInsightId = values.insightId
             if (insightId !== oldInsightId || insightMode !== values.insightMode) {
+                // If navigating from an unsaved insight to a different insight within the scene, prompt the user
+                if (
+                    sceneLogic.values.scene === Scene.Insight &&
+                    insightId !== oldInsightId &&
+                    oldInsightId !== 'new' &&
+                    values.insightCache?.logic.values.filtersChanged
+                ) {
+                    if (confirm('Leave insight? Changes you made may not be saved.')) {
+                        values.insightCache.logic.actions.discardChanges()
+                    } else {
+                        history.back()
+                        return
+                    }
+                }
path: frontend/src/scenes/insights/insightSceneLogic.tsx
text: @user1:
The code here and in `sceneLogic` seems like the most sensible way to do this (lesser of all evils), but could both nearly identical codepaths (here and  `sceneLogic`) be merged somehow? It might even be possible to just run this only in the `openScene` block, as that action should be called as well when the `id` in the url changes.

diff_hunk: @@ -69,6 +68,18 @@ export function Insight({ insightId }: { insightId: InsightShortId }): JSX.Eleme
         reportHotkeyNavigation('insights', hotkey)
     }
 
+    useEffect(() => {
+        const beforeUnloadHandler = (e: BeforeUnloadEvent): void => {
+            // Cancel the event to show unsaved changes dialog
+            e.preventDefault()
+            e.returnValue = ''
+        }
path: frontend/src/scenes/insights/Insight.tsx
text: @user1:
This doesn't seem to prompt for anything? Couldn't get this to do anything when testing either.

@author:
Do you mean you don't get the native browser prompt? 🤔 What browser?