fix(insights): stop add-to-dashboard modal from wiping other dashboards - #76418
fix(insights): stop add-to-dashboard modal from wiping other dashboards#76418posthog[bot] wants to merge 2 commits into
Conversation
The add/remove-insight-to-dashboard listeners in `addSavedInsightsModalLogic` built the PATCH payload from `insight.dashboards`, a deprecated field the API stopped serving to session-authenticated requests. That value is always `undefined` in the modal, so every add/remove PATCH sent a single-dashboard array and the backend treated it as the complete desired set, detaching the insight from every other dashboard it was on.
The post-toggle refresh was also unreliable: it minted a fresh `dashboardLogic({id}).mount()` instance, fired `loadDashboard`, then immediately `unmount()`-ed it. Since `loadDashboard`'s loader awaits `breakpoint(200)`, unmounting right after firing could tear the logic down before the load completed, and reusing the scene's already-mounted `dashboardLogic` instance with a narrower props object risked clobbering its existing props.
Fixes:
- Derive the current dashboard membership from `dashboard_tiles` (still served, unlike `dashboards`) instead of the always-undefined deprecated field.
- Reuse the dashboard's own mounted logic via `dashboardLogic.findMounted(...)` instead of mounting/unmounting a transient instance.
- Add a `insight dashboard modal - opened` capture to pair with the existing close event, so open-to-add conversion is measurable.
Added a parameterized test covering both actions that fails against the old code (confirmed locally) and passes with the fix.
Generated-By: PostHog Code
Task-Id: 62e8c002-2bab-433f-baf9-fb0bce6d1691
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🤖 CI report
|
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.25 MiB · 22 files | no change | ███░░░░░░░ 27.7% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.13 MiB · 3,035 files | 🔺 +113 B (+0.0%) | ████████░░ 83.8% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 285.6 KiB | ../node_modules/.pnpm/posthog-js@1.409.5/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 235.5 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 231.5 KiB | ../node_modules/.pnpm/posthog-js@1.409.5/node_modules/posthog-js/dist/module.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.3 KiB | src/lib/api.ts |
| 94.7 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.19 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.19 MiB · 17 files | no change | ████░░░░░░ 38.3% of 5.72 MiB |
| Deferred (lazy) | 2.08 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 718.3 KiB | dist/toolbar/toolbar-app-FT4UJ3IP.css |
| 551.4 KiB | dist/toolbar/chunk-chunk-AGSGCCBT.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-T44C2V5C.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-KBLX73CM.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-FKUW2FOM.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-3JVI3ZTF.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-HJ3ZJMTU.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +2.4 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1381.31 MiB · 🔺 +2.4 KiB (+0.0%)
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
CI's `typescript:check` (tsgo) flagged the mocked `request.json()` result as `DefaultBodyType` (which includes `undefined`), not assignable to the `Record<string, any> | null` captured-payload variable. My local run didn't catch this because `tsgo` wasn't installed until I ran `pnpm install`. Cast the parsed body explicitly. Generated-By: PostHog Code Task-Id: 62e8c002-2bab-433f-baf9-fb0bce6d1691
Problem
The add-insight-to-dashboard modal (
addSavedInsightsModalLogic.ts) has two bugs on a heavily used path:dashboardsarray built frominsight.dashboards. That field is deprecated and the API stopped serving it to session-authenticated callers, so the value is alwaysundefinedin the modal. The PATCH goes out with just the one dashboard ID, and the backend treats that as the complete desired set - silently detaching the insight from every other dashboard it was already on.dashboardLogic({id})instance, firesloadDashboard, then immediately unmounts it.loadDashboard's loader awaitsbreakpoint(200)before doing anything, so the unmount can tear the logic down mid-flight and the dashboard can be left showing stale/empty state even though the add succeeded. Passing a narrower props object also risks clobbering the props already set on the scene's mounteddashboardLogicinstance.closedcapture event with no matchingopenedevent, so this whole path had no conversion signal to catch it.Changes
dashboard_tiles(still served by the API, unlike the deprecateddashboardsfield) instead of the always-undefined field, for both add and remove.dashboardLogicvia.findMounted(...)instead of mounting/unmounting a transient instance.insight dashboard modal - openedcapture alongside the existing close event.How did you test this code?
Added a parameterized Jest test in
addSavedInsightsModalLogic.test.tscovering bothaddInsightToDashboardandremoveInsightFromDashboard, asserting the PATCH payload preserves the insight's other dashboard memberships. Verified it fails against the pre-fix code (loses a dashboard on add) and passes with the fix. Ran the full existing suite for this logic file - all 16 tests pass.Also ran
pnpm --filter=@posthog/frontend typescript:checkandpnpm --filter=@posthog/frontend fix(lint + format) against the changed files, andhogli ci:preflight --fix(clean, no findings).I wasn't able to manually exercise the modal in a running dev environment for this change.
Automatic notifications
Docs update
N/A - no user-facing behavior change beyond the bug fix; no documented workflow affected.
🤖 Agent context
Autonomy: Fully autonomous
dashboardsvsdashboard_tiles(products/product_analytics/backend/api/insight.py) before deciding the fix - confirmeddashboard_tilesis read-only server-side, so the write path still has to go through the deprecateddashboardsfield, just computed from a reliable source./writing-testsbefore adding the regression test.Created with PostHog Desktop from this inbox report.