Repository navigation
feat(replay-vision): add internal notes with @-mentions to observations - #122359
Conversation
|
😎 Merged successfully - details. |
There was a problem hiding this comment.
PostHog Review · Standard · reviewed affb636: 3 Must fix, 2 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
| const commentsLogicProps: CommentsLogicProps = { | ||
| scope: ActivityScope.REPLAY_OBSERVATION, | ||
| item_id: observation.id, | ||
| item_context: { | ||
| session_id: observation.session_id, | ||
| scanner_id: observation.scanner_id, |
There was a problem hiding this comment.
P1 · Enforce observation access checks on the new comment scope
A project member denied access to a scanner can still list its observation notes through /comments/?scope=ReplayObservation. The generic comments API checks neither scanner RBAC nor recording access, unlike ReplayObservationViewSet. Reuse those checks for comment reads and writes, following the security rule.
There was a problem hiding this comment.
Fixed in aa97254. The comments API now checks access to the observation for this scope: the same gates as the observations API (scanner RBAC, session recording viewer, and experiment access). This covers the scoped list, detail actions and writes. An unscoped list leaves these notes out. The API also needs the replay_scanner:read and session_recording:read scopes.
| "EarlyAccessFeature", | ||
| "SessionRecordingPlaylist", | ||
| "ReplayScanner", | ||
| "ReplayObservation", |
There was a problem hiding this comment.
P1 · Mask observation note content in activity logs
When someone posts an observation note, log_comment_activity copies its text into detail.changes[].after. The new scope has no entry in field_with_masked_contents or activity visibility restrictions, so callers with activity_log:read can retrieve notes from observations they cannot access. Mask note content, including replies, before logging.
There was a problem hiding this comment.
Fixed in aa97254. Added ReplayObservation to field_with_masked_contents, so root notes and replies are logged as "masked".
| "Survey": "survey", | ||
| "Experiment": "experiment", | ||
| "ErrorTracking": "error_tracking", | ||
| "ReplayObservation": "replay_observation", |
There was a problem hiding this comment.
P1 · Authorize mention recipients against the observation
Mentioning a project member who lacks access to the observation sends them note text through email and in-app notifications. _mentions_allowed_for_comment_target passes this scope through unchanged, and notification authorization checks the comment resource rather than its observation source. Filter recipients by observation access before fan-out.
There was a problem hiding this comment.
Fixed in aa97254. _mentions_allowed_for_comment_target now keeps only the mentioned users who can view the observation, before the email, the in-app notification and the event go out.
| <CommentsList {...commentsLogicProps} noun="observation" /> | ||
| <CommentComposer {...commentsLogicProps} /> |
There was a problem hiding this comment.
P2 · Prevent simultaneous composers from sharing one editor reference
Opening the discussion panel on a succeeded observation mounts another CommentComposer with the same commentsLogic key. Both editors overwrite its single richContentEditor reference, so submitting inline notes reads the other editor's content and can silently send nothing. Closing the panel also clears that reference; use one composer or isolate editor state per surface.
There was a problem hiding this comment.
Fixed in aa97254. While the discussion panel is open, the inline section shows a short message instead of its list and composer. Only one surface edits the thread at a time.
| [SIDE_PANEL_CONTEXT_KEY]: [ | ||
| (s) => [s.observationId], | ||
| (observationId: string): SidePanelSceneContext => ({ | ||
| activity_scope: ActivityScope.REPLAY_OBSERVATION, | ||
| activity_item_id: observationId, | ||
| }), |
There was a problem hiding this comment.
P2 · Disable discussion composition for unfinished observations
With today-rail-nav off, opening Discussion on a failed or ineligible observation exposes a working composer because this context always enables discussions. This bypasses the inline success guard, and retrying replaces the observation ID, stranding those notes. Set discussions_disabled until the observation has succeeded.
There was a problem hiding this comment.
Fixed in aa97254. The scene logic now gets the observation status and sets discussions_disabled until the scan succeeded.
🤖 CI report
|
| Function | Location | Complexity | Limit |
|---|---|---|---|
ReplayObservationSceneComponent |
products/replay_vision/frontend/observations/ReplayObservation.tsx:59 |
33 | 10 |
CommentComposer |
frontend/src/lib/components/Comments/CommentComposer.tsx:22 |
21 | 10 |
Comment |
frontend/src/lib/components/Comments/Comment.tsx:287 |
15 | 10 |
CommentWithReplies |
frontend/src/lib/components/Comments/Comment.tsx:382 |
14 | 10 |
CommentTopRow |
frontend/src/lib/components/Comments/Comment.tsx:189 |
13 | 10 |
QuillReplayObservationActions |
products/replay_vision/frontend/observations/QuillReplayObservationActions.tsx:11 |
11 | 10 |
✅ Duplication (Python) — clean
New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
✅ Duplication (TypeScript) — clean
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
⚠️ Comment density — 4% of added code lines are comments (28 of 704)
This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.
Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.
Files with the most added comment lines:
| File | Comment lines | Added lines |
|---|---|---|
products/replay_vision/backend/tests/test_observation_notes.py |
7 | 282 |
products/replay_vision/frontend/observations/replayObservationSceneLogic.ts |
5 | 94 |
posthog/api/comments.py |
4 | 83 |
posthog/models/comment/comment.py |
2 | 3 |
posthog/models/comment/utils.py |
2 | 13 |
frontend/src/layout/navigation-3000/sidepanel/types.ts |
1 | 2 |
frontend/src/lib/components/Comments/commentUtils.ts |
1 | 5 |
posthog/models/activity_logging/activity_log.py |
1 | 3 |
This check does not block merging. It updates on every push and clears when the share drops.
⚠️ Bundle size — 🔺 +80.4 KiB (+0.1%)
Uncompressed size of every built .js bundle, compared against the base branch.
Total: 77.03 MiB · 🔺 +80.4 KiB (+0.1%)
| File | Size | Δ vs base |
|---|---|---|
posthog-app/src/scenes/authentication/auth/Auth.js |
39.2 KiB | 🔺 +39.2 KiB (new) |
posthog-app/_parent/products/workflows/frontend/Transactional/TransactionalScene.js |
32.5 KiB | 🔺 +32.5 KiB (new) |
posthog-app/src/scenes/authentication/signup/Signup.js |
removed | 🟢 -20.9 KiB (-100.0%) |
posthog-app/src/scenes/authentication/login/Login.js |
removed | 🟢 -12.4 KiB (-100.0%) |
exporter/src/exporter/exporter.js |
32.1 KiB | 🔺 +11.2 KiB (+53.6%) |
posthog-app/src/scenes/web-analytics/WebAnalyticsScene.js |
247.8 KiB | 🔺 +10.3 KiB (+4.3%) |
posthog-app/src/scenes/experiments/Experiment.js |
418.0 KiB | 🔺 +6.5 KiB (+1.6%) |
render-query/src/render-query/render-query.js |
24.92 MiB | 🔺 +5.5 KiB (+0.0%) |
exporter/src/queries/schema.js |
1.27 MiB | 🟢 -2.1 KiB (-0.2%) |
posthog-app/src/queries/schema.js |
1.27 MiB | 🟢 -2.1 KiB (-0.2%) |
posthog-app/src/scenes/analytics/AnalyticsList.js |
29.9 KiB | 🔺 +1.1 KiB (+3.7%) |
posthog-app/src/scenes/authentication/auth/loginLogic.js |
1.1 KiB | 🔺 +1.1 KiB (new) |
posthog-app/src/scenes/authentication/login/loginLogic.js |
removed | 🟢 -1.1 KiB (-100.0%) |
exporter/src/exporter/scenes/ExporterNotebookScene.js |
3.90 MiB | 🔺 +1023 B (+0.0%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
⚠️ Eager graph — 3 over budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.01 MiB · 24 files | 🔺 +2.4 KiB (+0.2%) | █████████░ 88.0% of 1.14 MiB |
logged-out boot: index + App + bootApp (preloaded by every page, including /login)src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts |
2.97 MiB · 620 files | 🔺 +4.6 KiB (+0.2%) | ██████████ 98.0% of 3.03 MiB |
🟡 authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
7.58 MiB · 2,566 files | 🔺 +93.1 KiB (+1.2%) | ██████████ 101.2% of 7.50 MiB |
dashboard scenesrc/scenes/dashboard/Dashboard.tsx |
9.95 MiB · 3,731 files | 🔺 +9.4 KiB (+0.1%) | ██████████ 99.4% of 10.01 MiB |
🟡 today home pathsrc/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx |
7.77 MiB · 2,659 files | 🔺 +95.6 KiB (+1.2%) | ██████████ 100.3% of 7.74 MiB |
🟡 AI-first home pathsrc/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/ai-first/AiFirstHomepage.tsx |
8.01 MiB · 2,652 files | 🔺 +95.3 KiB (+1.2%) | ██████████ 101.3% of 7.91 MiB |
events scenesrc/scenes/activity/explore/EventsScene.tsx |
9.39 MiB · 3,512 files | 🔺 +7.6 KiB (+0.1%) | ██████████ 97.9% of 9.59 MiB |
web analytics scenesrc/scenes/web-analytics/WebAnalyticsScene.tsx |
10.33 MiB · 3,763 files | 🔺 +18.0 KiB (+0.2%) | ██████████ 95.4% of 10.83 MiB |
replay detail scenesrc/scenes/session-recordings/detail/SessionRecordingDetail.tsx |
8.42 MiB · 3,088 files | 🔺 +5.4 KiB (+0.1%) | █████████░ 88.9% of 9.47 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 node_modules/@posthog/brand/dist/generated/hoggies/svg/ stays out of src/index.tsx
🟢 node_modules/@posthog/brand/dist/generated/hoggies/components/ stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/layout/navigation-3000/navigationLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/dashboard/dashboardLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/lemon-ui/LemonMarkdown/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/RichContentEditor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/CodeSnippet/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/taxonomy/core-filter-definitions-by-group.json stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/zod/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/billing/billingLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/ProductEmptyState/productSetupPreloadLogic.ts stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 products/dashboards/frontend/widgets/previews/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts stays out of src/scenes/AuthenticatedShell.tsx
🟢 products/wizard/frontend/runs/WizardRunDetailsDrawer.tsx stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/layout/today/TodayShell.tsx stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/layout/today/TodayContextPanelFloating.tsx stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/layout/today/TodaySidePanel.tsx stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/layout/today/TodayTopBar.tsx stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/layout/today/todayRailItems.tsx stays out of src/scenes/AuthenticatedShell.tsx
🟢 zod/v4/locales/de.js stays out of src/scenes/AuthenticatedShell.tsx
🟢 node_modules/@posthog/brand/dist/generated/hoggies/svg/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 node_modules/@posthog/brand/dist/generated/hoggies/components/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/playlist/SessionRecordingsPlaylist.tsx stays out of src/scenes/dashboard/Dashboard.tsx
🟢 src/scenes/web-analytics/tiles/WebAnalyticsTile.tsx stays out of src/scenes/dashboard/Dashboard.tsx
🟢 src/queries/validators.js stays out of src/scenes/dashboard/Dashboard.tsx
🟢 src/scenes/project-homepage/ai-first/AiFirstHomepage.tsx stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx
🟢 src/scenes/project-homepage/today/TodayReportPage.tsx stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx
🟢 src/queries/Query/Query.tsx stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx
🟢 packages/quill/packages/charts/ stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx
🟢 products/posthog_ai/frontend/components/tool/widgets/VisualizationWidget.tsx stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/ai-first/AiFirstHomepage.tsx
🟢 products/posthog_ai/frontend/components/tool/widgets/RecordingsWidget.tsx stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/ai-first/AiFirstHomepage.tsx
🟢 src/queries/Query/Query.tsx stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/ai-first/AiFirstHomepage.tsx
🟢 products/posthog_ai/frontend/api/primitives.ts stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/ai-first/AiFirstHomepage.tsx
🟢 src/scenes/notebooks/Notebook/notebookLogic.ts stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/ai-first/AiFirstHomepage.tsx
🟢 src/scenes/session-recordings/playlist/SessionRecordingsPlaylist.tsx stays out of src/scenes/activity/explore/EventsScene.tsx
🟢 src/scenes/web-analytics/tiles/WebAnalyticsTile.tsx stays out of src/scenes/activity/explore/EventsScene.tsx
🟢 src/queries/validators.js stays out of src/scenes/activity/explore/EventsScene.tsx
🟢 src/scenes/web-analytics/LiveMetricsDashboard/LiveWebAnalyticsMetrics.tsx stays out of src/scenes/web-analytics/WebAnalyticsScene.tsx
🟢 products/product_analytics/frontend/insights/trends/WorldMap/countryVectors.tsx stays out of src/scenes/web-analytics/WebAnalyticsScene.tsx
🟢 src/scenes/settings/SettingsMap.tsx stays out of src/scenes/session-recordings/detail/SessionRecordingDetail.tsx
🟢 @jspm/core/nodelibs/browser/fs stays out of src/scenes/session-recordings/detail/SessionRecordingDetail.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.7 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 4.0 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.7 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.6 KiB | src/lib/utils/bootTiming.ts |
| 1.6 KiB | src/index.tsx |
| 1.2 KiB | src/RootErrorBoundary.tsx |
| 949 B | src/scenes/ChunkLoadErrorBoundary.tsx |
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
| Size | File |
|---|---|
| 322.9 KiB | ../node_modules/.pnpm/posthog-js@1.438.23_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 217.5 KiB | ../node_modules/.pnpm/@posthog+icons@0.39.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.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 |
| 102.1 KiB | src/products.tsx |
| 86.1 KiB | src/lib/api.ts |
| 68.3 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 41.9 KiB | ../node_modules/.pnpm/@dnd-kit+core@6.0.8_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@dnd-kit/core/dist/core.esm.js |
| 36.4 KiB | ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js |
| 27.0 KiB | ../node_modules/.pnpm/tailwind-merge@2.2.2/node_modules/tailwind-merge/dist/bundle-mjs.mjs |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 322.9 KiB | ../node_modules/.pnpm/posthog-js@1.438.23_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 286.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 239.9 KiB | ../packages/quill/packages/quill/dist/index.js |
| 217.5 KiB | ../node_modules/.pnpm/@posthog+icons@0.39.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.8 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 |
| 102.1 KiB | src/products.tsx |
| 94.5 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 92.5 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 86.1 KiB | src/lib/api.ts |
Largest files eagerly shipped from src/scenes/dashboard/Dashboard.tsx
| Size | File |
|---|---|
| 322.9 KiB | ../node_modules/.pnpm/posthog-js@1.438.23_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 286.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 239.9 KiB | ../packages/quill/packages/quill/dist/index.js |
| 217.5 KiB | ../node_modules/.pnpm/@posthog+icons@0.39.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.8 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 144.1 KiB | ../packages/quill/packages/primitives/dist/index.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 |
| 102.1 KiB | src/products.tsx |
| 94.5 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 92.5 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx
| Size | File |
|---|---|
| 322.9 KiB | ../node_modules/.pnpm/posthog-js@1.438.23_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 286.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 239.9 KiB | ../packages/quill/packages/quill/dist/index.js |
| 217.5 KiB | ../node_modules/.pnpm/@posthog+icons@0.39.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.8 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 |
| 102.1 KiB | src/products.tsx |
| 94.5 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 92.5 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 86.1 KiB | src/lib/api.ts |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/ai-first/AiFirstHomepage.tsx
| Size | File |
|---|---|
| 322.9 KiB | ../node_modules/.pnpm/posthog-js@1.438.23_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 286.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 239.9 KiB | ../packages/quill/packages/quill/dist/index.js |
| 217.5 KiB | ../node_modules/.pnpm/@posthog+icons@0.39.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.8 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 144.1 KiB | ../packages/quill/packages/primitives/dist/index.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 |
| 102.1 KiB | src/products.tsx |
| 94.5 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 92.5 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
Largest files eagerly shipped from src/scenes/activity/explore/EventsScene.tsx
| Size | File |
|---|---|
| 322.9 KiB | ../node_modules/.pnpm/posthog-js@1.438.23_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 286.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 239.9 KiB | ../packages/quill/packages/quill/dist/index.js |
| 217.5 KiB | ../node_modules/.pnpm/@posthog+icons@0.39.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.8 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 144.1 KiB | ../packages/quill/packages/primitives/dist/index.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 |
| 102.1 KiB | src/products.tsx |
| 94.5 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 92.5 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
Largest files eagerly shipped from src/scenes/web-analytics/WebAnalyticsScene.tsx
| Size | File |
|---|---|
| 322.9 KiB | ../node_modules/.pnpm/posthog-js@1.438.23_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 286.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 239.9 KiB | ../packages/quill/packages/quill/dist/index.js |
| 217.5 KiB | ../node_modules/.pnpm/@posthog+icons@0.39.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 182.9 KiB | src/queries/validators.js |
| 153.8 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 144.1 KiB | ../packages/quill/packages/primitives/dist/index.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 |
| 102.1 KiB | src/products.tsx |
| 94.5 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Largest files eagerly shipped from src/scenes/session-recordings/detail/SessionRecordingDetail.tsx
| Size | File |
|---|---|
| 322.9 KiB | ../node_modules/.pnpm/posthog-js@1.438.23_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 316.1 KiB | ../node_modules/.pnpm/posthog-js@1.438.23_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 286.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 239.9 KiB | ../packages/quill/packages/quill/dist/index.js |
| 217.5 KiB | ../node_modules/.pnpm/@posthog+icons@0.39.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.8 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 |
| 102.1 KiB | src/products.tsx |
| 94.5 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 92.5 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/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.30 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.30 MiB · 18 files | 🔺 +2.6 KiB (+0.1%) | ████░░░░░░ 40.2% of 5.72 MiB |
| Deferred (lazy) | 2.11 MiB · 40 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.2 KiB | no change | █░░░░░░░░░ 6.0% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 914.0 KiB | dist/toolbar/toolbar-app-IGYXPTNX.css |
| 694.7 KiB | dist/toolbar/chunk-chunk-7UPQYOWP.js |
| 259.9 KiB | dist/toolbar/chunk-chunk-KENEMLMX.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 130.4 KiB | dist/toolbar/chunk-chunk-Z5Y2DR5C.js |
| 74.8 KiB | dist/toolbar/toolbar-app-FATZAGBL.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-KPWX2FCN.js |
| 21.7 KiB | dist/toolbar/chunk-chunk-CBVK2IL4.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +1010.1 KiB (+0.1%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 971.23 MiB · 🔺 +1010.1 KiB (+0.1%)
ℹ️ MCP UI apps size — 35 app(s), 28999.8 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 844.3 KB | 211.1 KB |
| explain-chart | 1004.9 KB | 211.1 KB |
| action | 767.7 KB | 211.1 KB |
| action-list | 835.3 KB | 211.1 KB |
| cohort | 766.7 KB | 211.1 KB |
| cohort-list | 834.3 KB | 211.1 KB |
| email-template | 766.6 KB | 211.1 KB |
| error-details | 771.6 KB | 211.1 KB |
| error-issue | 768.9 KB | 211.1 KB |
| error-issue-list | 836.8 KB | 211.1 KB |
| experiment | 832.4 KB | 211.1 KB |
| experiment-list | 836.1 KB | 211.1 KB |
| experiment-results | 835.0 KB | 211.1 KB |
| feature-flag | 834.7 KB | 211.1 KB |
| feature-flag-list | 838.4 KB | 211.1 KB |
| feature-flag-testing | 770.0 KB | 211.1 KB |
| inline-scan | 767.2 KB | 211.1 KB |
| insight-actors | 833.8 KB | 211.1 KB |
| llm-costs | 831.1 KB | 211.1 KB |
| session-recording | 769.0 KB | 211.1 KB |
| survey | 768.3 KB | 211.1 KB |
| survey-global-stats | 831.1 KB | 211.1 KB |
| survey-list | 836.0 KB | 211.1 KB |
| survey-stats | 831.1 KB | 211.1 KB |
| trace-span | 767.1 KB | 211.1 KB |
| trace-span-list | 835.2 KB | 211.1 KB |
| vision-observation-list | 834.4 KB | 211.1 KB |
| workflow | 767.1 KB | 211.1 KB |
| workflow-list | 834.7 KB | 211.1 KB |
| workflow-setup-status | 766.7 KB | 211.1 KB |
| query-results | 1087.6 KB | 211.1 KB |
| render-ui | 1186.5 KB | 211.1 KB |
| visual-review-snapshots | 771.6 KB | 211.1 KB |
| workflow-data-suggestions | 769.4 KB | 211.1 KB |
| workflow-sending-rules | 768.1 KB | 211.1 KB |
⚠️ Backend coverage — 87.0% of changed backend lines covered — 30 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): █████████████████░░░ 87.0% (216 / 246)
| File | Patch | Uncovered changed lines |
|---|---|---|
posthog/api/comments.py |
52.6% | 101, 106–109, 271–274, 469, 474–477, 494, 501–504, 796, 962–963, 972, 975, 1065, 1068, 1243 |
posthog/tasks/comment_slack_sync.py |
66.7% | 211 |
products/replay_vision/backend/facade/api.py |
93.9% | 341, 356 |
🤖 Agents: add a test only if an uncovered line exposes a realistic regression that existing tests miss. Otherwise explain why no new test is needed under "How did you test this code?". Gap list: the patch-coverage artifact on this run (gh run download 216697383100787 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
demo |
███████████░░░░░░░░░ 56.4% |
1,542 / 2,736 |
data_tools |
████████████░░░░░░░░ 60.9% |
95 / 156 |
warehouse_sources_queue |
█████████████░░░░░░░ 65.3% |
2,163 / 3,314 |
authorization |
████████████████░░░░ 79.7% |
181 / 227 |
batch_exports |
████████████████░░░░ 81.7% |
22,938 / 28,087 |
engineering_analytics |
█████████████████░░░ 85.0% |
11,468 / 13,498 |
commands |
█████████████████░░░ 85.5% |
1,618 / 1,893 |
mcp_analytics |
█████████████████░░░ 86.2% |
5,720 / 6,638 |
ml_inference |
█████████████████░░░ 86.3% |
524 / 607 |
apm |
█████████████████░░░ 86.6% |
1,616 / 1,866 |
posthog_ai |
██████████████████░░ 87.9% |
4,296 / 4,886 |
notebooks |
██████████████████░░ 88.5% |
15,330 / 17,321 |
mcp_registry |
██████████████████░░ 89.0% |
1,596 / 1,794 |
webmcp |
██████████████████░░ 89.3% |
292 / 327 |
dashboards |
██████████████████░░ 89.4% |
8,034 / 8,986 |
warehouse_suggestions |
██████████████████░░ 89.5% |
3,558 / 3,977 |
cohorts |
██████████████████░░ 89.6% |
8,571 / 9,562 |
product_tours |
██████████████████░░ 89.7% |
1,355 / 1,510 |
signals |
██████████████████░░ 90.0% |
89,488 / 99,379 |
today |
██████████████████░░ 90.6% |
3,496 / 3,859 |
visual_review |
██████████████████░░ 90.6% |
9,858 / 10,875 |
tasks |
██████████████████░░ 90.7% |
80,298 / 88,519 |
managed_warehouse |
██████████████████░░ 91.0% |
12,253 / 13,462 |
data_warehouse |
██████████████████░░ 91.1% |
15,337 / 16,833 |
canvas |
██████████████████░░ 91.2% |
8,189 / 8,980 |
autoresearch |
██████████████████░░ 91.4% |
11,618 / 12,705 |
data_modeling |
██████████████████░░ 91.5% |
10,755 / 11,758 |
cdp |
██████████████████░░ 91.5% |
10,293 / 11,249 |
business_knowledge |
██████████████████░░ 91.7% |
10,276 / 11,211 |
metrics |
██████████████████░░ 91.7% |
10,627 / 11,590 |
wizard |
██████████████████░░ 91.7% |
6,870 / 7,490 |
slack_app |
██████████████████░░ 91.7% |
21,671 / 23,624 |
streamlit_apps |
██████████████████░░ 91.9% |
3,139 / 3,415 |
error_tracking |
██████████████████░░ 92.1% |
19,065 / 20,707 |
conversations |
██████████████████░░ 92.3% |
30,713 / 33,288 |
review_hog |
██████████████████░░ 92.3% |
18,059 / 19,562 |
early_access_features |
███████████████████░ 92.6% |
1,339 / 1,446 |
managed_migrations |
███████████████████░ 92.6% |
1,630 / 1,760 |
stamphog |
███████████████████░ 92.7% |
8,876 / 9,579 |
alerts_platform |
███████████████████░ 92.8% |
8,193 / 8,830 |
notifications |
███████████████████░ 92.8% |
1,218 / 1,312 |
alerts |
███████████████████░ 92.9% |
10,387 / 11,182 |
aeo |
███████████████████░ 93.0% |
2,152 / 2,313 |
ai_training |
███████████████████░ 93.3% |
598 / 641 |
web_analytics |
███████████████████░ 93.3% |
26,136 / 28,012 |
cross_project_dashboards |
███████████████████░ 93.4% |
880 / 942 |
surveys |
███████████████████░ 93.4% |
6,941 / 7,429 |
exports |
███████████████████░ 93.6% |
4,191 / 4,479 |
ai_gateway |
███████████████████░ 93.6% |
787 / 841 |
approvals |
███████████████████░ 93.7% |
4,994 / 5,331 |
context_layer |
███████████████████░ 93.8% |
3,981 / 4,244 |
mcp_store |
███████████████████░ 93.9% |
9,188 / 9,784 |
marketing_analytics |
███████████████████░ 93.9% |
21,234 / 22,605 |
billing_alerts |
███████████████████░ 94.0% |
2,322 / 2,470 |
messaging |
███████████████████░ 94.0% |
5,039 / 5,358 |
ai_observability |
███████████████████░ 94.0% |
29,508 / 31,375 |
customer_analytics |
███████████████████░ 94.1% |
28,186 / 29,950 |
workflows |
███████████████████░ 94.3% |
25,284 / 26,814 |
replay_vision |
███████████████████░ 94.3% |
33,499 / 35,511 |
legal_documents |
███████████████████░ 94.4% |
2,313 / 2,451 |
logs |
███████████████████░ 94.4% |
19,309 / 20,457 |
actions |
███████████████████░ 94.7% |
1,004 / 1,060 |
reminders |
███████████████████░ 94.8% |
760 / 802 |
experiments |
███████████████████░ 94.8% |
40,475 / 42,708 |
endpoints |
███████████████████░ 94.9% |
9,369 / 9,875 |
growth |
███████████████████░ 94.9% |
11,348 / 11,960 |
annotations |
███████████████████░ 95.1% |
1,095 / 1,151 |
skills |
███████████████████░ 95.2% |
7,630 / 8,015 |
uptime_monitoring |
███████████████████░ 95.3% |
306 / 321 |
access_control |
███████████████████░ 95.5% |
8,281 / 8,671 |
subscriptions |
███████████████████░ 95.6% |
13,557 / 14,182 |
data_catalog |
███████████████████░ 95.7% |
4,776 / 4,990 |
product_analytics |
███████████████████░ 95.8% |
29,894 / 31,214 |
feature_flags |
███████████████████░ 95.8% |
28,839 / 30,092 |
revenue_analytics |
███████████████████░ 95.9% |
1,877 / 1,958 |
data_quality |
███████████████████░ 96.0% |
8,561 / 8,915 |
tracing |
███████████████████░ 96.3% |
4,404 / 4,572 |
warehouse_sources |
███████████████████░ 96.5% |
481,961 / 499,393 |
security |
███████████████████░ 96.7% |
1,549 / 1,602 |
account_audits |
███████████████████░ 96.7% |
770 / 796 |
pulse |
███████████████████░ 97.1% |
2,023 / 2,083 |
analytics_platform |
████████████████████ 98.0% |
2,828 / 2,885 |
groups |
████████████████████ 98.2% |
1,838 / 1,871 |
field_notes |
████████████████████ 99.4% |
172 / 173 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
|
|
Risk: No findings The only change since the last review is regenerated Playwright snapshot hashes in frontend/snapshots.yml for replay-vision observation-detail stories — test baselines only, with no production-code or security-relevant changes. Sentinel reviewed |
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from each directory's |
| viewer_ids = observation_viewer_user_ids(team_id=team_id, observation_id=item_id, user_ids=mentioned_user_ids) | ||
| return [user_id for user_id in mentioned_user_ids if user_id in viewer_ids] |
There was a problem hiding this comment.
P1 · Recheck observation access when reading mention notifications
After a mentioned member loses scanner, recording, or experiment access, the notifications API still returns the stored note body. This filter checks access only before fan-out; send_mention_notifications stores resource_type=comment, which NotificationsViewSet._filter_by_access_control skips. Gate notification reads on current observation access.
There was a problem hiding this comment.
Fixed in 18486b0. A mention notification for an observation note no longer stores the note text. Its body is a fixed placeholder that tells the reader to open the observation, and the comments API checks observation access each time the note is read. So a member who loses access later sees no note text in their notifications.
| return { | ||
| user.id | ||
| for user in User.objects.filter(id__in=list(user_ids)) | ||
| if (access := recording_viewer_access(user, team)) is not None | ||
| and can_view_observation(access, team_id, observation_id) |
There was a problem hiding this comment.
P2 · Batch observation mention access checks
When a note mentions many teammates, observation_viewer_user_ids runs membership/RBAC queries and reloads the identical observation and scanner for every recipient. The unbounded mentions list turns one synchronous comment write into a linear chain of database round trips before saving the note. Load the target once and batch recipient access data.
There was a problem hiding this comment.
Fixed in 18486b0. observation_viewer_user_ids now loads the observation and scanner once, and reads the active org memberships of all mentioned users in one query. Only the per-user access resolution stays per user, because access control is per member.
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 18486b0: 1 Should fix.
This is review round 3 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
| def _mention_fanout_content(comment: "Comment") -> str: | ||
| if comment.scope in OBSERVATION_COMMENT_SCOPES: | ||
| return OBSERVATION_NOTE_PLACEHOLDER | ||
| return extract_plain_text_from_rich_content(comment.rich_content) or comment.content |
There was a problem hiding this comment.
P2 · Protect observation notes in queued mention emails
When a recipient loses observation access before a queued mention email runs, send_discussions_mentioned still sends comment.content without rechecking access. _mention_fanout_content protects only notifications and CDP events, so the email discloses the restricted note. Revalidate recipients in the email task or use the same placeholder there.
There was a problem hiding this comment.
Fixed in f061a1a. send_discussions_mentioned now uses the same placeholder for observation notes as the in-app notification and the CDP event, so a queued email never carries the note text. test_mention_email_sent_after_access_is_revoked_carries_no_note_text removes the recipient's scanner access after the mention and before the email task runs, and checks that the email has only the placeholder. The test fails without the fix.
There was a problem hiding this comment.
PostHog Review · Standard · reviewed b933a8e: 1 Must fix.
This is review round 4 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
| if not _can_view_observation( | ||
| team_id=self.team_id, item_id=target[1], user_access_control=self.user_access_control | ||
| ): |
There was a problem hiding this comment.
P1 · Keep observation threads within one authorization boundary
Changing a thread root's item_id to another observation leaves its replies on the original observation. /thread checks only the root here and returns replies by source_comment_id, so members allowed on the new observation can read other users' notes from a denied scanner. Reject target changes on populated threads or authorize each returned reply.
There was a problem hiding this comment.
Fixed in 0d2bd31. Instead of authorizing each reply in /thread, an observation note now stays on the observation it was written on. An update that changes the scope or item_id of an observation note, or that moves another comment into the observation scope, returns a 400. Replies already take the root's scope and item_id on create, so a whole thread is always on one observation and under one access check. Covered by test_comment_cannot_move_across_an_observation_boundary.
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
Independent assurance covers the risky changes, but @PostHog's queued-email disclosure concern remains present in the current code. Fix that email path and request another review from Hayne Ryu or Tue Haulund before re-requesting approval.
- Author wrote 0% of the modified lines and has 88 merged PRs in these paths (familiarity MODERATE).
- PostHog Review (Standard) reviewed the current head: no findings.
- @PostHog's outdated comment on posthog/models/comment/utils.py about queued mention emails remains unaddressed: posthog/api/comments.py:575-577 queues observation mentions, while posthog/tasks/email.py:2097-2152 sends the stored comment body without rechecking observation access or substituting the placeholder. A recipient whose access is revoked before delivery can still receive restricted note content. Apply the placeholder to email content or reauthorize recipients in the email task, add a revocation-before-delivery regression test, and request review again.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 290L, 16F substantive, 531L/22F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (531L, 22F, cross-cutting, feat) |
| stamphog 2.6.1 | .stamphog/policy.yml @ 0d2bd31 · reviewed head 0d2bd31 |
| and (scope, item_id) != (instance.scope, instance.item_id) | ||
| and (instance.scope in OBSERVATION_COMMENT_SCOPES or scope in OBSERVATION_COMMENT_SCOPES) | ||
| ): | ||
| raise exceptions.ValidationError({"item_id": "An observation note cannot move to another item."}) |
There was a problem hiding this comment.
🤖 Agent-drafted, reviewed by Tue.
A note can still move threads by PATCHing source_comment to another comment's root. Its scope doesn't change, so this check passes. The thread endpoint only checks the root, so the note shows up for anyone who can read that discussion. Could we reject source_comment changes for observation notes, and add a test for it?
There was a problem hiding this comment.
Good catch, fixed in 1be0808. An observation note's thread is now fixed after creation: an update that changes its source_comment returns a 400, as a change to its scope or item_id already did. The rule also applies when the new parent is an observation note, so other comments cannot join an observation thread. test_comment_cannot_move_across_an_observation_boundary has a new reply_to_other_thread case, and it fails without the fix.
|
|
||
| # Mention notifications and events outlive the access check made when they are sent, and the notifications | ||
| # API and CDP destinations do not recheck the observation. So they carry this text instead of the note. | ||
| OBSERVATION_NOTE_PLACEHOLDER = "A note on a Replay Vision observation. Open it in PostHog to read it." |
There was a problem hiding this comment.
🤖 Agent-drafted, reviewed by Tue.
send_to_slack (L1225) doesn't exclude observation notes, so their full text can go to a Slack channel. Replies from Slack also come back as notes from people who haven't passed the access check. Could we block observation scopes there and in the Slack reply sync?
There was a problem hiding this comment.
Fixed in 1be0808. send_to_slack returns a 400 for observation notes. Both sync directions skip them too: the outbound mirror receiver and _reply_skip_reason, and ingest_slack_discussion_reply for a mirror with this scope. A mirror can only start from send_to_slack, so those two checks are a second guard. The frontend also hides "Send to Slack" and the composer's Slack toggle for this scope. test_note_cannot_be_sent_to_slack checks the error message, so it fails without the fix.
| } | ||
|
|
||
| /** Why notes are off for an observation, or null when they are on. */ | ||
| export function observationNotesDisabledReason(status: ObservationStatusEnumApi | null): string | null { |
There was a problem hiding this comment.
🤖 Agent-drafted, reviewed by Tue.
The succeeded-only rule is frontend-only. The API and MCP still accept notes on failed or pending observations. Should the backend enforce it too?
There was a problem hiding this comment.
Yes, fixed in 1be0808. The comments API now accepts a note write only when the observation's scan succeeded (observation_accepts_notes in the Replay Vision facade). Otherwise it returns a 400 on item_id. test_notes_need_a_finished_scan covers failed and running scans.
| ) | ||
|
|
||
|
|
||
| def can_view_observation(access: UserAccessControl, team_id: int, observation_id: str) -> bool: |
There was a problem hiding this comment.
🤖 Agent-drafted, reviewed by Tue.
When an observation is deleted (retry, scanner delete, recording deletion), its notes stay in the database. Nobody can read them, but the text remains. Should we delete them in those paths?
There was a problem hiding this comment.
Fixed in 1be0808 for scanner deletes, which is where observations with notes go away. A pre_delete receiver on ReplayScanner reads the team's notes (few) and deletes the ones on that scanner's observations, in the same transaction. That covers the API and the Max tool. I put it on the scanner, not the observation, because a per-observation receiver adds one query for each observation in a large scanner delete. The other paths: a retry deletes only failed or ineligible rows, which can no longer have notes. A team delete cascades comments through the team FK. I found no code that deletes observations when a recording is deleted: the observation stays and shows "This recording is no longer available". Let me know if I missed a path. test_scanner_delete_removes_the_notes_on_its_observations covers it, including a note on another scanner that stays.
| @@ -0,0 +1,63 @@ | |||
| import { useActions, useValues } from 'kea' | |||
There was a problem hiding this comment.
🤖 Agent-drafted, reviewed by Tue.
nit: could the Quill button go in its own file, with the hook logic moved into the scene logic?
There was a problem hiding this comment.
Done in 1be0808. The open state, the disabled reason and the toggle are now in replayObservationSceneLogic, and QuillObservationNotesButton.tsx is its own file. One exception: the note count. It comes from sidePanelDiscussionLogic, and that logic reads the global sceneLogic. When I connected it from the scene logic, the scene logic started to depend on the global scene state, and a replayObservationLogic test failed. So the buttons read the count and format it with notesButtonLabel from the scene logic.
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 1be0808: 1 Should fix.
This is review round 5 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
| notes = Comment.objects.filter(team_id=instance.team_id, scope__in=OBSERVATION_COMMENT_SCOPES) | ||
| note_item_ids = set(notes.values_list("item_id", flat=True)) |
There was a problem hiding this comment.
P2 · Restrict note cleanup to the scanner in SQL
When the inline-scanner reaper deletes a batch, this receiver reloads every observation-note item ID in the team for each scanner, including childless scanners. Cleanup performs O(batch size × team notes) data transfer and UUID parsing inside its deletion transaction, exhausting the reaper budget as notes accumulate. Match the scanner's observations through a database subquery.
There was a problem hiding this comment.
Fixed in 27a0738. The receiver now runs one DELETE on comments, filtered by item_id IN (SELECT id::text FROM the scanner's observations). It no longer loads the team's notes or parses ids in Python, so a childless scanner from the reaper costs one cheap query. Note item_ids are stored as canonical UUID text, so the cast ids match them. test_scanner_delete_removes_the_notes_on_its_observations still covers it, including a note written with an uppercase id.
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
Re-add the stamphog label to request another review once you have addressed this.
stamphog can't act on this pull request because the prerequisites gate failed: the branch has merge conflicts. The other gates (deny-list, size, and tier) passed, so the conflicts are the only thing blocking it. The author should resolve the conflicts by merging or rebasing onto the base branch and pushing the result. If the author still wants this reviewed after that, they can ask a human reviewer, particularly since the change touches access control for comments across several areas.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✗ | merge conflicts present |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 479L, 25F substantive, 862L/33F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (862L, 33F, cross-cutting, feat) |
| stamphog 2.6.1 | .stamphog/policy.yml @ unknown · reviewed head f04cf20 |
Finished observations get an "Internal notes" section built on the generic comments system, under a new ReplayObservation scope. @-mentions notify the teammate, and the email and in-app notification links open the observation with its discussion panel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: f91fc37d-17fa-4339-acfd-dd1695556de6
- Comment reads, writes and detail actions in the ReplayObservation scope now need access to the observation, including scanner RBAC, session recording access and experiment access. Unscoped lists leave them out. - Mentions reach only members who can view the observation. - Note content is masked in the activity log. - The discussion panel is disabled until the scan succeeds. - The inline notes give way to the panel while it is open, so only one composer edits the thread. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: f91fc37d-17fa-4339-acfd-dd1695556de6
…ccess checks - Mention notifications and $discussion_mention_created events for observation notes carry a placeholder instead of the note text, because the notifications API and CDP destinations do not recheck observation access. - observation_viewer_user_ids loads the observation and memberships once for all mentioned users. - The scene logic stores null when an observation has no status, which fixes the replayObservationLogic Jest tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: f91fc37d-17fa-4339-acfd-dd1695556de6
…ead of an inline section - Remove the inline "Internal notes" section, so the page stays the same for teams that do not use notes. - Add a "Notes (N)" button next to Share, in the current and the Quill header. It opens and closes the discussion side panel. - On failed, ineligible, and running scans the button is disabled, and the discussion panel shows why instead of the generic "not supported" message (new optional discussions_disabled_reason on the side panel context). - Fix the mypy error in _mention_fanout_content. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: f91fc37d-17fa-4339-acfd-dd1695556de6
…was written on The thread endpoint authorizes only the root comment and returns replies by source_comment_id. A root moved to another observation would carry its replies across the observation access check. Updates that change the scope or item_id of an observation note, or move another comment into the observation scope, now return a validation error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: f91fc37d-17fa-4339-acfd-dd1695556de6
…ix typegen selector - The mention email for an observation note now carries the same placeholder as the in-app notification and the CDP event. The email task runs after the mention was authorized, so a recipient who loses access before delivery no longer receives the note. - Write the notesDisabledReason selector as an inline function. kea-typegen dropped the selector when it took a function by name, which failed the frontend type check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: f91fc37d-17fa-4339-acfd-dd1695556de6
- An observation note cannot change its thread: updates that change its source_comment are rejected, like changes to its scope or item_id. - Observation notes cannot be sent to Slack, and both Slack sync directions skip them. The frontend hides the Slack options for them. - The API accepts note writes only on observations whose scan succeeded. - Deleting a scanner deletes the notes on its observations. - Move the notes button state and toggle into the scene logic, and put the Quill button in its own file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: f91fc37d-17fa-4339-acfd-dd1695556de6
- Store observation note item_ids as canonical UUIDs. Any spelling resolves the observation, but lookups and the scanner-delete cleanup compare strings. - Ignore a late observation response in the scene logic when the user has moved to another observation, so it cannot change the notes state on screen. - Check note writes with one observation load, and read the detail action's comment target once for all scope gates. - Import the Replay Vision facade at module level in the comments API, and keep the scanner-delete receiver module import-light. Record the receiver in the setup baseline. - Cover /thread, /count, replies, and session recording denial in the access tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: f91fc37d-17fa-4339-acfd-dd1695556de6
f04cf20 to
fff91f2
Compare
|
No review started. stamphog started a review of this pull request less than 10 minutes ago, and a label can request at most one review per 10 minutes. The |
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
Independent assurance is reported for the current head, and the earlier access-control concerns are addressed, but @PostHog's substantive cleanup-performance concern remains in the code. Address that thread before re-requesting; request follow-up from @PostHog/team-replay, with Luke Belton or Hayne Ryu as suggested reviewers.
- Author wrote 0% of the modified lines and has 92 merged PRs in these paths (familiarity MODERATE).
- PostHog Review (Standard) reviewed the current head: no findings.
- @PostHog on products/replay_vision/backend/observation_notes.py: the cleanup performance concern remains unfixed. Lines 24–33 load every observation-note item ID in the team and parse those IDs for each deleted scanner, including scanners without observations; the scanner restriction is applied only afterward. Restrict the matching in SQL using the scanner's observations, then re-request review.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 479L, 25F substantive, 860L/33F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (860L, 33F, cross-cutting, feat) |
| stamphog 2.6.1 | .stamphog/policy.yml @ fff91f2 · reviewed head fff91f2 |
The scanner-delete receiver loaded every observation note id in the team and matched them in Python, once per deleted scanner, including the inline-scanner reaper's childless scanners. It now deletes with one query, matching notes against the scanner's observation ids through a subquery. Note item_ids are stored as canonical UUID text, so the ids cast to text match them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: f91fc37d-17fa-4339-acfd-dd1695556de6
|
✅ Visual changes approved by @ksvat — baseline updated in 8 changed. Install the Visual Review Chrome extension to see visual review results at the top of your pull requests. |
8 updated Run: 3bb4a1b9-fdcf-4de0-a806-8569bbbffda4 Co-authored-by: ksvat <147102038+ksvat@users.noreply.github.com>
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Approved.
The current code addresses the substantive concerns raised by @PostHog and @TueHaulund, including access boundaries, delayed mention disclosure, Slack synchronization, and scanner cleanup. The change enters risky territory and has no current-head review, but the author belongs to the owning Replay team; familiarity is MODERATE.
- Author wrote 0% of the modified lines and has 75 merged PRs in these paths (familiarity MODERATE).
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 480L, 26F substantive, 861L/34F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (861L, 34F, cross-cutting, feat) |
| stamphog 2.6.1 | .stamphog/policy.yml @ d0d57a6 · reviewed head d0d57a6 |
|
/trunk merge |


Problem
When a Replay Vision scan finds a bug, the team has no place on the observation to write down what to do next or to give the follow-up to someone. Users asked to add a note to an observation and assign it to a PM.
Changes
ReplayObservationcomment scope follows the observation's own access:replay_scanner:readandsession_recording:read.$discussion_mention_createdevents carry a placeholder instead of the note text, because they are read or sent later without a new observation access check.pre_deletereceiver onReplayScanner, recorded in the startup receiver baseline). Noteitem_ids are stored as canonical UUIDs so the cleanup finds every note.discussions_disabled_reasonon the side panel context. A retry deletes a failed row and the new scan gets a new id, so notes on it would be lost.Panel closed. The page has only the new button:
Panel open after a click on Notes:
Failed scan. The button is disabled and says why:
More states
Tooltip on the enabled button:
No notes yet:
Failed scan with the panel open:
Running scan:
1280 px window with the panel open:
The screenshots use invented local demo data. The recording is not shown because the demo has no recording.
How did you test this code?
I ran these locally:
pytest posthog/api/test/test_comments.py posthog/models/comment/test_utils.py products/replay_vision/backend/tests/test_facade.py products/replay_vision/backend/tests/test_observation_notes.pyhogli test products/replay_vision/frontend/observations/ReplayObservation.test.tsx products/replay_vision/frontend/observations/replayObservationLogic.test.tspnpm typescript:checkhogli lint:tachI took the screenshots from a local stack with Playwright. I did not take screenshots of the Quill header behind
today-rail-nav, and I did not check that the mention email or the in-app notification is delivered.Test rationale:
test_observation_notes.py(new): a member denied the scanner cannot list, read, or write notes, and a viewer can. A mention of a member without access sends nothing. A mention notification and event carry no note text. A mention email sent after the recipient lost access carries no note text. An update cannot move a note to another observation, or move another comment into the observation scope. Note bodies and reply bodies are masked in the activity log. No current test covers these gates for this scope. The ticket access tests are the closest, and they test a different scope.build_comment_item_urlcase: catches a wrong fallback link for API or MCP callers that send no slug.it.each: the Notes button and the discussion panel are enabled only on a finished scan. A new case checks that the page has no inline composer and that the button opens and closes the panel.Release status
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5
Skills used:
/django-startup-time(receiver wiring and the setup receiver baseline) and/code-review(an adversarial self-review after the reviewer's comments, which found the UUID spelling gap, the stale-response race, and the missing/thread,/count, and reply tests).The first commit added an inline notes section. Later commits handle the review findings (observation access checks, mention filtering, activity log masking, no note text in mention fan-out). After review, the inline section was replaced by a header button, because most teams will not use notes and the section added noise to every observation. The screenshots use invented demo data. Nothing from the agent session is in the diff.
Why: Users asked for a follow-up step after a scan finds a problem: add a note on the observation and tag a teammate, such as a PM.
🤖 Generated with Claude Code