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

feat(product analytics): Remove insight exit pop up #26878

Merged
merged 28 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
7dd2412
fix: typeings
k11kirky Dec 12, 2024
724cc7e
fix: limit to scene
k11kirky Dec 12, 2024
ecc7186
feat: update tests and resolve pr comments
k11kirky Dec 16, 2024
8e54242
fix: reload cyprus and tsc issue
k11kirky Dec 16, 2024
cd3a118
feat: scope to currentTeam
k11kirky Dec 17, 2024
b9b64bc
Merge branch 'master' into feat/remove-confirm-leave-alert
k11kirky Dec 17, 2024
26c9b84
feat: scope this to only new insights and use JSONCrush for urls and …
k11kirky Dec 17, 2024
7a28c2c
Merge branch 'feat/remove-confirm-leave-alert' of github.com:PostHog/…
k11kirky Dec 17, 2024
a87bfe2
fix: jest config
k11kirky Dec 17, 2024
04f49c4
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 17, 2024
f89b52b
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 17, 2024
25c3b50
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 17, 2024
6663b0c
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 18, 2024
bfb3605
Merge branch 'master' into feat/remove-confirm-leave-alert
k11kirky Dec 18, 2024
7fe7117
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 18, 2024
4b89db2
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 18, 2024
3b85bfd
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 18, 2024
94652dc
Merge branch 'master' into feat/remove-confirm-leave-alert
k11kirky Dec 18, 2024
e319740
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 18, 2024
a788622
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 18, 2024
af4f7cb
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 18, 2024
33fe8dc
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 18, 2024
ad60076
fix: e2e test update
k11kirky Dec 18, 2024
0c3384a
Merge branch 'feat/remove-confirm-leave-alert' of github.com:PostHog/…
k11kirky Dec 18, 2024
0575ed7
Merge branch 'master' into feat/remove-confirm-leave-alert
k11kirky Dec 18, 2024
81d35d7
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 18, 2024
2c7dbac
Update UI snapshots for `chromium` (1)
github-actions[bot] Dec 18, 2024
0cf6902
Update UI snapshots for `chromium` (2)
github-actions[bot] Dec 18, 2024
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
46 changes: 46 additions & 0 deletions cypress/e2e/insights-reload-query.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
describe('ReloadInsight component', () => {
beforeEach(() => {
// Clear local storage before each test to ensure a clean state
cy.clearLocalStorage()
})

it('saves the query to the URL and localStorage, and reloads it when prompted', () => {
// Visit the new insight creation page
cy.visit('/insights/new')

cy.wait(2000)

cy.get('[data-attr="math-selector-0"]').click({ force: true })
cy.get('[data-attr="math-dau-0"]').click({ force: true })

// Check that the 'draft-query' item is stored in localStorage
cy.window().then((window) => {
const currentTeamId = window.POSTHOG_APP_CONTEXT.current_team.id
const draftQuery = window.localStorage.getItem(`draft-query-${currentTeamId}`)
expect(draftQuery).to.not.be.null

const draftQueryObj = JSON.parse(draftQuery)
expect(draftQueryObj).to.have.property('query')

expect(draftQueryObj).to.have.property('query')

const firstSeries = draftQueryObj.query.source.series[0]

expect(firstSeries).to.include({
event: '$pageview',
math: 'dau',
})
})

// Navigate away to the "Saved Insights" page
cy.visit('/saved_insights')

// Verify that the ReloadInsight component displays a message about the unsaved insight
cy.contains('You have an unsaved insight from').should('exist')

// Click the link to reload the unsaved insight
cy.contains('Click here').click()

cy.get('[data-attr="math-selector-0"]').should('contain', 'Unique users')
})
})
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
5 changes: 4 additions & 1 deletion frontend/src/scenes/insights/Insight.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { DebugCHQueries } from 'lib/components/CommandPalette/DebugCHQueries'
import { isObject } from 'lib/utils'
import { InsightPageHeader } from 'scenes/insights/InsightPageHeader'
import { insightSceneLogic } from 'scenes/insights/insightSceneLogic'
import { ReloadInsight } from 'scenes/saved-insights/ReloadInsight'
import { urls } from 'scenes/urls'

import { Query } from '~/queries/Query/Query'
Expand All @@ -21,7 +22,7 @@ export interface InsightSceneProps {

export function Insight({ insightId }: InsightSceneProps): JSX.Element {
// insightSceneLogic
const { insightMode, insight, filtersOverride, variablesOverride } = useValues(insightSceneLogic)
const { insightMode, insight, filtersOverride, variablesOverride, freshQuery } = useValues(insightSceneLogic)

// insightLogic
const logic = insightLogic({
Expand Down Expand Up @@ -79,6 +80,8 @@ export function Insight({ insightId }: InsightSceneProps): JSX.Element {
</div>
)}

{freshQuery ? <ReloadInsight /> : null}

<Query
query={isInsightVizNode(query) ? { ...query, full: true } : query}
setQuery={setQuery}
Expand Down
59 changes: 53 additions & 6 deletions frontend/src/scenes/insights/insightDataLogic.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { actions, connect, kea, key, listeners, path, props, propsChanged, reducers, selectors } from 'kea'
import { actionToUrl, router } from 'kea-router'
import { objectsEqual } from 'lib/utils'
import { DATAWAREHOUSE_EDITOR_ITEM_ID } from 'scenes/data-warehouse/external/dataWarehouseExternalSceneLogic'
import { keyForInsightLogicProps } from 'scenes/insights/sharedUtils'
import { Scene } from 'scenes/sceneTypes'
import { filterTestAccountsDefaultsLogic } from 'scenes/settings/environment/filterTestAccountDefaultsLogic'

import { examples } from '~/queries/examples'
Expand All @@ -14,10 +16,13 @@ import { DataVisualizationNode, InsightVizNode, Node, NodeKind } from '~/queries
import { isDataTableNode, isDataVisualizationNode, isHogQuery, isInsightVizNode } from '~/queries/utils'
import { ExportContext, InsightLogicProps, InsightType } from '~/types'

import { teamLogic } from '../teamLogic'
import type { insightDataLogicType } from './insightDataLogicType'
import { insightDataTimingLogic } from './insightDataTimingLogic'
import { insightLogic } from './insightLogic'
import { insightSceneLogic } from './insightSceneLogic'
import { insightUsageLogic } from './insightUsageLogic'
import { crushDraftQueryForLocalStorage, crushDraftQueryForURL, isQueryTooLarge } from './utils'
import { compareQuery } from './utils/queryUtils'

export const insightDataLogic = kea<insightDataLogicType>([
Expand All @@ -29,6 +34,10 @@ export const insightDataLogic = kea<insightDataLogicType>([
values: [
insightLogic,
['insight', 'savedInsight'],
insightSceneLogic,
['insightId', 'insightMode', 'activeScene'],
teamLogic,
['currentTeamId'],
dataNodeLogic({
key: insightVizDataNodeKey(props),
loadPriority: props.loadPriority,
Expand All @@ -49,7 +58,7 @@ export const insightDataLogic = kea<insightDataLogicType>([
],
actions: [
insightLogic,
['setInsight', 'loadInsightSuccess'],
['setInsight'],
dataNodeLogic({ key: insightVizDataNodeKey(props) } as DataNodeLogicProps),
['loadData', 'loadDataSuccess', 'loadDataFailure', 'setResponse as setInsightData'],
],
Expand Down Expand Up @@ -187,21 +196,59 @@ export const insightDataLogic = kea<insightDataLogicType>([
actions.setInsightData({ ...values.insightData, result })
}
},
loadInsightSuccess: ({ insight }) => {
k11kirky marked this conversation as resolved.
Show resolved Hide resolved
if (insight.query) {
actions.setQuery(insight.query)
}
},
cancelChanges: () => {
const savedQuery = values.savedInsight.query
const savedResult = values.savedInsight.result
actions.setQuery(savedQuery || null)
actions.setInsightData({ ...values.insightData, result: savedResult ? savedResult : null })
},
setQuery: ({ query }) => {
// if the query is not changed, don't save it
if (!query || !values.queryChanged) {
return
}
// only run on insight scene
if (insightSceneLogic.values.activeScene !== Scene.Insight) {
return
}
// don't save for saved insights
if (insightSceneLogic.values.insightId !== 'new') {
return
}

if (isQueryTooLarge(query)) {
localStorage.removeItem(`draft-query-${values.currentTeamId}`)
}
localStorage.setItem(
`draft-query-${values.currentTeamId}`,
crushDraftQueryForLocalStorage(query, Date.now())
)
},
})),
propsChanged(({ actions, props, values }) => {
if (props.cachedInsight?.query && !objectsEqual(props.cachedInsight.query, values.query)) {
actions.setQuery(props.cachedInsight.query)
}
}),
actionToUrl(({ values }) => ({
setQuery: ({ query }) => {
if (
values.queryChanged &&
insightSceneLogic.values.activeScene === Scene.Insight &&
insightSceneLogic.values.insightId === 'new'
) {
// query is changed and we are in edit mode
return [
router.values.currentLocation.pathname,
{
...router.values.currentLocation.searchParams,
},
{
...router.values.currentLocation.hashParams,
q: crushDraftQueryForURL(query),
},
]
}
},
})),
])
2 changes: 2 additions & 0 deletions frontend/src/scenes/insights/insightLogic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,8 @@ export const insightLogic: LogicWrapper<insightLogicType> = kea<insightLogicType
? await insightsApi.update(insightNumericId, insightRequest)
: await insightsApi.create(insightRequest)
savedInsightsLogic.findMounted()?.actions.loadInsights() // Load insights afresh
// remove draft query from local storage
localStorage.removeItem('draft-query')
actions.saveInsightSuccess()
} catch (e) {
actions.saveInsightFailure()
Expand Down
22 changes: 2 additions & 20 deletions frontend/src/scenes/insights/insightSceneLogic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,41 +44,23 @@ describe('insightSceneLogic', () => {
})
})

it('redirects when opening /insight/new with insight type in theurl', async () => {
it('redirects maintaining url params when opening /insight/new with insight type in theurl', async () => {
router.actions.push(urls.insightNew(InsightType.FUNNELS))
await expectLogic(logic).toFinishAllListeners()
await expectLogic(router)
.delay(1)
.toMatchValues({
location: partial({
pathname: addProjectIdIfMissing(urls.insightNew(), MOCK_TEAM_ID),
search: '',
hash: '',
}),
})

expect((logic.values.insightLogicRef?.logic.values.insight.query as InsightVizNode).source?.kind).toEqual(
'FunnelsQuery'
)
})

it('redirects when opening /insight/new with query in the url', async () => {
it('redirects maintaining url params when opening /insight/new with query in the url', async () => {
router.actions.push(
urls.insightNew(undefined, undefined, {
kind: NodeKind.InsightVizNode,
source: examples.InsightPathsQuery,
} as InsightVizNode)
)
await expectLogic(logic).toFinishAllListeners()
await expectLogic(router)
.delay(1)
.toMatchValues({
location: partial({
pathname: addProjectIdIfMissing(urls.insightNew(), MOCK_TEAM_ID),
search: '',
hash: '',
}),
})

expect((logic.values.insightLogicRef?.logic.values.insight.query as InsightVizNode).source?.kind).toEqual(
'PathsQuery'
Expand Down
50 changes: 36 additions & 14 deletions frontend/src/scenes/insights/insightSceneLogic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,18 @@ import { insightDataLogic } from './insightDataLogic'
import { insightDataLogicType } from './insightDataLogicType'
import type { insightSceneLogicType } from './insightSceneLogicType'
import { summarizeInsight } from './summarizeInsight'
import { parseDraftQueryFromLocalStorage, parseDraftQueryFromURL } from './utils'

const NEW_INSIGHT = 'new' as const
export type InsightId = InsightShortId | typeof NEW_INSIGHT | null

export const insightSceneLogic = kea<insightSceneLogicType>([
path(['scenes', 'insights', 'insightSceneLogic']),
connect(() => ({
logic: [eventUsageLogic],
values: [
teamLogic,
['currentTeam'],
['currentTeam', 'currentTeamId'],
sceneLogic,
['activeScene'],
preflightLogic,
Expand Down Expand Up @@ -73,10 +77,11 @@ export const insightSceneLogic = kea<insightSceneLogicType>([
unmount,
}),
setOpenedWithQuery: (query: Node | null) => ({ query }),
setFreshQuery: (freshQuery: boolean) => ({ freshQuery }),
}),
reducers({
insightId: [
null as null | 'new' | InsightShortId,
null as null | InsightId,
{
setSceneState: (_, { insightId }) => insightId,
},
Expand Down Expand Up @@ -150,6 +155,7 @@ export const insightSceneLogic = kea<insightSceneLogicType>([
},
],
openedWithQuery: [null as Node | null, { setOpenedWithQuery: (_, { query }) => query }],
freshQuery: [false, { setFreshQuery: (_, { freshQuery }) => freshQuery }],
}),
selectors(() => ({
insightSelector: [(s) => [s.insightLogicRef], (insightLogicRef) => insightLogicRef?.logic.selectors.insight],
Expand Down Expand Up @@ -332,24 +338,20 @@ export const insightSceneLogic = kea<insightSceneLogicType>([

let queryFromUrl: Node | null = null
if (q) {
queryFromUrl = JSON.parse(q)
const validQuery = parseDraftQueryFromURL(q)
if (validQuery) {
queryFromUrl = validQuery
} else {
console.error('Invalid query', q)
}
} else if (insightType && Object.values(InsightType).includes(insightType)) {
queryFromUrl = getDefaultQuery(insightType, values.filterTestAccountsDefault)
}

// Redirect to a simple URL if we had a query in the URL
if (q || insightType) {
router.actions.replace(
insightId === 'new'
? urls.insightNew(undefined, dashboard)
: insightMode === ItemMode.Edit
? urls.insightEdit(insightId)
: urls.insightView(insightId)
)
}
actions.setFreshQuery(false)

// reset the insight's state if we have to
if (initial || method === 'PUSH' || queryFromUrl) {
if (initial || queryFromUrl || method === 'PUSH') {
if (insightId === 'new') {
const query = queryFromUrl || getDefaultQuery(InsightType.TRENDS, values.filterTestAccountsDefault)
values.insightLogicRef?.logic.actions.setInsight(
Expand All @@ -364,6 +366,10 @@ export const insightSceneLogic = kea<insightSceneLogicType>([
}
)

if (!queryFromUrl) {
actions.setFreshQuery(true)
}

actions.setOpenedWithQuery(query)

eventUsageLogic.actions.reportInsightCreated(query)
Expand Down Expand Up @@ -416,6 +422,22 @@ export const insightSceneLogic = kea<insightSceneLogicType>([

const metadataChanged = !!values.insightLogicRef?.logic.values.insightChanged
const queryChanged = !!values.insightDataLogicRef?.logic.values.queryChanged
const draftQueryFromLocalStorage = localStorage.getItem(`draft-query-${values.currentTeamId}`)
let draftQuery: { query: Node; timestamp: number } | null = null
if (draftQueryFromLocalStorage) {
const parsedQuery = parseDraftQueryFromLocalStorage(draftQueryFromLocalStorage)
if (parsedQuery) {
draftQuery = parsedQuery
} else {
// If the draft query is invalid, remove it
localStorage.removeItem(`draft-query-${values.currentTeamId}`)
}
}
const query = values.insightDataLogicRef?.logic.values.query

if (draftQuery && query && objectsEqual(draftQuery.query, query)) {
return false
}

return metadataChanged || queryChanged
},
Expand Down
Loading
Loading