Repository navigation
feat(today): add to home from insight menus #131365
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import { useActions, useValues } from 'kea' | ||
|
|
||
| import { LemonButton } from 'lib/lemon-ui/LemonButton' | ||
|
|
||
| import { HomePinInsight, homePinsLogic } from './homePinsLogic' | ||
|
|
||
| export function AddToHomeButton({ insight }: { insight: HomePinInsight }): JSX.Element { | ||
| const { homePinState } = useValues(homePinsLogic) | ||
| const { toggleHomePin } = useActions(homePinsLogic) | ||
| const { pinned, disabledReason } = homePinState(insight) | ||
|
|
||
| return ( | ||
| <LemonButton | ||
| onClick={() => toggleHomePin(insight, 'dashboard_card')} | ||
| disabledReason={disabledReason} | ||
| fullWidth | ||
| data-attr="insight-card-add-to-home" | ||
| > | ||
| {pinned ? 'Remove from home' : 'Add to home'} | ||
| </LemonButton> | ||
| ) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import { expectLogic } from 'kea-test-utils' | ||
| import posthog from 'posthog-js' | ||
|
|
||
| import { useMocks } from '~/mocks/jest' | ||
| import { initKeaTests } from '~/test/init' | ||
| import { InsightShortId } from '~/types' | ||
|
|
||
| import type { PulsePinApi } from 'products/today/frontend/generated/api.schemas' | ||
|
|
||
| import { homePinsLogic } from './homePinsLogic' | ||
|
|
||
| const pin = (shortId: string): PulsePinApi => ({ key: `insight:${shortId}`, name: shortId, dashboard_id: null }) | ||
| const insight = { short_id: 'new' as InsightShortId, name: 'Signups' } | ||
|
|
||
| describe('homePinsLogic', () => { | ||
| let storedPins: PulsePinApi[] | ||
| let savedPins: PulsePinApi[][] | ||
|
|
||
| beforeEach(() => { | ||
| savedPins = [] | ||
| useMocks({ | ||
| get: { '/api/projects/:team_id/today/pulse_pins/': () => [200, { pins: storedPins }] }, | ||
| put: { | ||
| '/api/projects/:team_id/today/pulse_pins/': async ({ request }) => { | ||
| const { pins } = (await request.json()) as { pins: PulsePinApi[] } | ||
| savedPins.push(pins) | ||
| return [200, { pins }] | ||
| }, | ||
| }, | ||
| }) | ||
| initKeaTests() | ||
| jest.spyOn(posthog, 'capture').mockImplementation(() => undefined) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| jest.restoreAllMocks() | ||
| }) | ||
|
|
||
| it.each([ | ||
| { | ||
| name: 'adds the insight after the existing pins', | ||
| stored: [pin('a')], | ||
| label: { pinned: false, disabledReason: undefined }, | ||
| saved: [[pin('a'), { key: 'insight:new', name: 'Signups', dashboard_id: null }]], | ||
| event: 'today pulse number pinned', | ||
| }, | ||
| { | ||
| name: 'removes a pinned insight even when the home is full', | ||
| stored: [pin('a'), pin('new'), pin('c'), pin('d')], | ||
| label: { pinned: true, disabledReason: undefined }, | ||
| saved: [[pin('a'), pin('c'), pin('d')]], | ||
| event: 'today pulse number unpinned', | ||
| }, | ||
| { | ||
| name: 'does not add a fifth number', | ||
| stored: [pin('a'), pin('b'), pin('c'), pin('d')], | ||
| label: { pinned: false, disabledReason: 'Your home already shows 4 numbers. Remove one first.' }, | ||
| saved: [], | ||
| event: null, | ||
| }, | ||
| ])('$name', async ({ stored, label, saved, event }) => { | ||
| storedPins = stored | ||
| const logic = homePinsLogic() | ||
| logic.mount() | ||
| await expectLogic(logic).toFinishAllListeners() | ||
|
|
||
| expect(logic.values.homePinState(insight)).toEqual(label) | ||
|
|
||
| logic.actions.toggleHomePin(insight, 'dashboard_card') | ||
| await expectLogic(logic).toFinishAllListeners() | ||
|
|
||
| expect(savedPins).toEqual(saved) | ||
| if (event) { | ||
| expect(posthog.capture).toHaveBeenCalledWith(event, { | ||
| kind: 'insight', | ||
| source: 'dashboard_card', | ||
| pinned_count_before: stored.length, | ||
| }) | ||
| } else { | ||
| expect(posthog.capture).not.toHaveBeenCalled() | ||
| } | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,178 @@ | ||
| import { MakeLogicType, actions, afterMount, connect, kea, listeners, path, reducers, selectors } from 'kea' | ||
| import { loaders } from 'kea-loaders' | ||
| import posthog from 'posthog-js' | ||
|
|
||
| import { toast } from '@posthog/quill' | ||
|
|
||
| import { ApiError } from 'lib/api' | ||
| import { teamLogic } from 'scenes/teamLogic' | ||
|
|
||
| import { isInsightVizNode, isTrendsQuery } from '~/queries/schema/node-guards' | ||
| import { ChartDisplayType, type InsightModel } from '~/types' | ||
|
|
||
| import { todayPulsePinsRetrieve, todayPulsePinsUpdate } from 'products/today/frontend/generated/api' | ||
| import type { PulsePinApi } from 'products/today/frontend/generated/api.schemas' | ||
|
|
||
| export const HOME_PIN_LIMIT = 4 | ||
|
|
||
| const HOME_PIN_DISPLAYS: ChartDisplayType[] = [ | ||
| ChartDisplayType.ActionsLineGraph, | ||
| ChartDisplayType.ActionsBar, | ||
| ChartDisplayType.ActionsStackedBar, | ||
| ChartDisplayType.ActionsAreaGraph, | ||
| ] | ||
|
|
||
| export type HomePinSource = 'dashboard_card' | 'insight_menu' | ||
|
|
||
| export type HomePinInsight = Pick<InsightModel, 'short_id'> & Partial<Pick<InsightModel, 'name' | 'derived_name'>> | ||
|
|
||
| export interface HomePinState { | ||
| pinned: boolean | ||
| disabledReason: string | undefined | ||
| } | ||
|
|
||
| export function isHomePinnable(insight: Partial<InsightModel>): insight is Partial<InsightModel> & HomePinInsight { | ||
| const query = insight.query | ||
| return ( | ||
| !!insight.short_id && | ||
| isInsightVizNode(query) && | ||
| isTrendsQuery(query.source) && | ||
| HOME_PIN_DISPLAYS.includes(query.source.trendsFilter?.display ?? ChartDisplayType.ActionsLineGraph) | ||
| ) | ||
| } | ||
|
|
||
| function homePinKey(insight: HomePinInsight): string { | ||
| return `insight:${insight.short_id}` | ||
| } | ||
|
|
||
| // Generated by kea-typegen. Update if you're an agent, ignore if you're human. | ||
| export interface homePinsLogicValues { | ||
| currentProjectId: number | null | ||
| homePinState: (insight: HomePinInsight) => HomePinState | ||
| homePins: PulsePinApi[] | null | ||
| homePinsLoading: boolean | ||
| homePinsSaving: boolean | ||
| } | ||
|
|
||
| // Generated by kea-typegen. Update if you're an agent, ignore if you're human. | ||
| export interface homePinsLogicActions { | ||
| loadHomePins: () => any | ||
| loadHomePinsFailure: ( | ||
| error: string, | ||
| errorObject?: any | ||
| ) => { | ||
| error: string | ||
| errorObject?: any | ||
| } | ||
| loadHomePinsSuccess: ( | ||
| homePins: PulsePinApi[] | null, | ||
| payload?: any | ||
| ) => { | ||
| homePins: PulsePinApi[] | null | ||
| payload?: any | ||
| } | ||
| setHomePinsSaving: (saving: boolean) => { | ||
| saving: boolean | ||
| } | ||
| toggleHomePin: ( | ||
| insight: HomePinInsight, | ||
| source: HomePinSource | ||
| ) => { | ||
| insight: HomePinInsight | ||
| source: HomePinSource | ||
| } | ||
| } | ||
|
|
||
| export type homePinsLogicType = MakeLogicType<homePinsLogicValues, homePinsLogicActions> | ||
|
|
||
| function nextHomePins(pins: PulsePinApi[], insight: HomePinInsight, pinned: boolean): PulsePinApi[] { | ||
| const key = homePinKey(insight) | ||
| if (pinned) { | ||
| return pins.filter((pin) => pin.key !== key) | ||
| } | ||
| return [...pins, { key, name: insight.name || insight.derived_name || 'Untitled insight', dashboard_id: null }] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 · Preserve dashboard context when creating a pin Pins from dashboard cards always save dashboard_id as null. The pin API uses this field to preserve dashboard context. In products/today/backend/logic/pinned.py, _insight selects the standalone query when this field is null. The home metric therefore drops dashboard property filters, tile overrides, and variables. For example, a dashboard filtered to one region produces an unfiltered home metric. Pass dashboardId from InsightMeta through AddToHomeButton and toggleHomePin into the saved pin. Keep null for standalone insights. Follow the existing TodayPulse pin action, which preserves metric.dashboard_id. |
||
| } | ||
|
|
||
| export const homePinsLogic = kea<homePinsLogicType>([ | ||
| path(['lib', 'components', 'AddToHome', 'homePinsLogic']), | ||
| connect(() => ({ | ||
| values: [teamLogic, ['currentProjectId']], | ||
| })), | ||
| actions({ | ||
| toggleHomePin: (insight: HomePinInsight, source: HomePinSource) => ({ insight, source }), | ||
| setHomePinsSaving: (saving: boolean) => ({ saving }), | ||
| }), | ||
| loaders(({ values }) => ({ | ||
| homePins: [ | ||
| null as PulsePinApi[] | null, | ||
| { | ||
| loadHomePins: async (): Promise<PulsePinApi[] | null> => { | ||
| try { | ||
| const { pins } = await todayPulsePinsRetrieve(String(values.currentProjectId)) | ||
| return pins | ||
| } catch (error) { | ||
| if (error instanceof ApiError && error.status === 404) { | ||
| return [] | ||
|
Comment on lines
+109
to
+115
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 · Discard load results after the menu unmounts This loader returns results without a breakpoint check. If the user closes and reopens the menu while its GET is pending, the old request can finish after the new request or a successful save. kea-loaders dispatches the old result to the same action path, which overwrites the new mount's homePins. I reproduced a successful pin disappearing from local state when the previous menu's GET finished. A later save can also remove that pin from storage. Accept the loader's breakpoint argument and call breakpoint() after the request completes, before returning pins. Check cancellation before returning the 404 fallback too. Follow the existing loadPulseRecentInsights pattern in todayHighlightsLogic.ts. |
||
| } | ||
| throw error | ||
| } | ||
| }, | ||
| }, | ||
| ], | ||
| })), | ||
| reducers({ | ||
| homePinsSaving: [false, { setHomePinsSaving: (_, { saving }) => saving }], | ||
| }), | ||
| selectors({ | ||
| homePinState: [ | ||
| (s) => [s.homePins, s.homePinsLoading, s.homePinsSaving], | ||
| (homePins: PulsePinApi[] | null, homePinsLoading: boolean, homePinsSaving: boolean) => | ||
| (insight: HomePinInsight): HomePinState => { | ||
| const pinned = !!homePins?.some((pin) => pin.key === homePinKey(insight)) | ||
| const disabledReason = | ||
| homePinsLoading || homePinsSaving | ||
| ? 'Loading your home numbers' | ||
| : homePins === null | ||
| ? "Couldn't load your home numbers. Open this menu again to retry." | ||
| : !pinned && homePins.length >= HOME_PIN_LIMIT | ||
| ? `Your home already shows ${HOME_PIN_LIMIT} numbers. Remove one first.` | ||
| : undefined | ||
| return { pinned, disabledReason } | ||
| }, | ||
| ], | ||
| }), | ||
| listeners(({ actions, values, cache }) => ({ | ||
| toggleHomePin: async ({ insight, source }) => { | ||
| const pins = values.homePins | ||
| const { pinned, disabledReason } = values.homePinState(insight) | ||
| if (!pins || disabledReason) { | ||
| return | ||
| } | ||
| const next = nextHomePins(pins, insight, pinned) | ||
| actions.setHomePinsSaving(true) | ||
| let saved: PulsePinApi[] | ||
| try { | ||
| saved = (await todayPulsePinsUpdate(String(values.currentProjectId), { pins: next })).pins | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 · Keep pin requests within the insight's source project The new card action also appears on CrossProjectTile cards, which pass projectId for an insight from another project. This request always targets currentProjectId. Pin keys contain only the insight's short_id, and get_insight_series resolves that identifier within the request's project. The save succeeds but creates an unavailable pin in the active project's home. A matching identifier can resolve to an unrelated insight instead. Hide or disable the action when the card's projectId differs from currentProjectId. Alternatively, pass the source project into the logic and scope both pin loading and saving to that project.
Comment on lines
+151
to
+155
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 · Keep the save guard active after the menu closes The menu closes after a click and unmounts homePinsLogic while the PUT can still be pending. Opening another menu creates fresh state with homePinsSaving=false and loads the previous pin list. A second pin then sends another replacement list that excludes the first pin. I reproduced this with controlled API promises: pinning A and then B stores only B, although both actions report success. Keep the shared logic mounted until each save completes, or move mutation state outside the popup lifecycle. Block subsequent mutations until the saved list is reconciled. |
||
| } catch { | ||
| toast.error({ title: "Couldn't update your home. Try again." }) | ||
| if (!cache.disposables.isDisposed) { | ||
| actions.setHomePinsSaving(false) | ||
| } | ||
| return | ||
| } | ||
| posthog.capture(pinned ? 'today pulse number unpinned' : 'today pulse number pinned', { | ||
| kind: 'insight', | ||
| source, | ||
| pinned_count_before: pins.length, | ||
| }) | ||
| toast.success({ title: pinned ? 'Removed from home' : 'Added to home' }) | ||
| if (!cache.disposables.isDisposed) { | ||
| actions.loadHomePinsSuccess(saved) | ||
| actions.setHomePinsSaving(false) | ||
| } | ||
| }, | ||
| })), | ||
| afterMount(({ actions }) => { | ||
| actions.loadHomePins() | ||
| }), | ||
| ]) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import { useActions, useValues } from 'kea' | ||
|
|
||
| import { IconHome } from '@posthog/icons' | ||
| import { DropdownMenuItem } from '@posthog/quill' | ||
|
|
||
| import { HomePinInsight, homePinsLogic } from 'lib/components/AddToHome/homePinsLogic' | ||
|
|
||
| export function AddToHomeMenuItem({ insight }: { insight: HomePinInsight }): JSX.Element { | ||
| const { homePinState } = useValues(homePinsLogic) | ||
| const { toggleHomePin } = useActions(homePinsLogic) | ||
| const { pinned, disabledReason } = homePinState(insight) | ||
|
|
||
| return ( | ||
| <DropdownMenuItem | ||
| data-attr="insight-more-menu-add-to-home" | ||
| onClick={() => toggleHomePin(insight, 'insight_menu')} | ||
| disabledReason={disabledReason} | ||
| > | ||
| <IconHome /> | ||
| {pinned ? 'Remove from home' : 'Add to home'} | ||
| </DropdownMenuItem> | ||
| ) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| } from '@posthog/quill' | ||
|
|
||
| import { AccessControlAction } from 'lib/components/AccessControlAction' | ||
| import { isHomePinnable } from 'lib/components/AddToHome/homePinsLogic' | ||
| import { InsightSubscribeProminentButton } from 'lib/components/Scenes/InsightSubscribeProminentButton' | ||
| import { useFeatureFlag } from 'lib/hooks/useFeatureFlag' | ||
| import { insightDataLogic } from 'scenes/insights/insightDataLogic' | ||
|
|
@@ -26,11 +27,12 @@ | |
| import { insightModalsLogic } from 'products/product_analytics/frontend/insights/insightScene/insightModalsLogic' | ||
| import { urlForSubscriptions } from 'products/subscriptions/frontend/components/Subscriptions/utils' | ||
|
|
||
| import { AddToHomeMenuItem } from './AddToHomeMenuItem' | ||
| import { InsightShareMenu, useInsightShareItems } from './InsightShareMenu' | ||
| import { openInsightEditor } from './openInsightEditor' | ||
| import { QuillInsightSaveButton } from './QuillInsightSaveButton' | ||
|
|
||
| export function QuillInsightPageHeaderActions({ | ||
| insightLogicProps, | ||
| isPersistedInsight, | ||
| canCreateAlertForInsight, | ||
|
|
@@ -112,6 +114,7 @@ | |
| <IconPlus /> | ||
| Add to dashboard | ||
| </DropdownMenuItem> | ||
| {isHomePinnable(insight) && <AddToHomeMenuItem insight={insight} />} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 · Make the insight action reachable on wide layouts This action exists only in SceneTitleSecondaryActions.menu. That component hides its menu trigger when the content container exceeds 5xl. Neither its wide-layout children nor InsightSceneMenuBar provides the new action. Users on wide insight pages cannot add or remove a home number. Add the action to InsightSceneMenuBar under TODAY_RAIL_NAV, or provide an overflow menu that remains available on wide layouts. Retain the existing eligibility and disabled-state checks. |
||
| {canCreateAlertForInsight && ( | ||
| <DropdownMenuItem | ||
| data-attr="insight-more-menu-alerts" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2 · Reject daily trends with weekday filters before pinning
This predicate accepts daily trends with nonempty query.source.dateRange.daysOfWeek. However, products/product_analytics/backend/insight_series.py::_supports_series rejects these queries. The pin request succeeds and shows 'Added to home', but the home resolver returns no metric. Home then shows 'Could not load' and reserves one of the four slots for a number that Retry cannot load. Match the backend eligibility rule before offering Add to home. Reject nonempty dateRange.daysOfWeek when the normalized interval is day. Preserve support for weekly and monthly trends.