Conversation
🤖 CI report
|
| Function | Location | Complexity | Limit |
|---|---|---|---|
WorkflowSuggestionEvidence |
products/workflows/frontend/Workflows/suggestions/WorkflowSuggestionEvidence.tsx:13 |
21 | 10 |
WorkflowAppliedOutcome |
products/workflows/frontend/Workflows/suggestions/WorkflowAppliedOutcome.tsx:38 |
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 — 2% of added code lines are comments (11 of 483)
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/workflows/frontend/Workflows/suggestions/suggestionChanges.ts |
3 | 87 |
products/workflows/backend/presentation/views/hog_flow.py |
2 | 32 |
products/workflows/backend/tests/api/test_hog_flow.py |
2 | 94 |
products/workflows/frontend/Workflows/suggestions/suggestionEvidence.ts |
2 | 44 |
products/workflows/frontend/Workflows/hogflows/types.ts |
1 | 3 |
products/workflows/frontend/Workflows/suggestions/suggestionChanges.test.ts |
1 | 66 |
This check does not block merging. It updates on every push and clears when the share drops.
⚠️ Bundle size — 🔺 +3.6 KiB (+0.0%)
Uncompressed size of every built .js bundle, compared against the base branch.
Total: 68.97 MiB · 🔺 +3.6 KiB (+0.0%)
| File | Size | Δ vs base |
|---|---|---|
posthog-app/_parent/products/workflows/frontend/Workflows/WorkflowScene.js |
61.2 KiB | 🔺 +3.6 KiB (+6.2%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within 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.58 MiB · 22 files | no change | █████████░ 85.8% of 1.84 MiB |
logged-out boot: index + App + bootApp (preloaded by every page, including /login)src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts |
3.52 MiB · 629 files | no change | █████████░ 87.4% of 4.03 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
7.35 MiB · 2,339 files | no change | █████████░ 88.1% of 8.34 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/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/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
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts 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
🟢 [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/index.tsx |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 854 B | src/scenes/ChunkLoadErrorBoundary.tsx |
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
| Size | File |
|---|---|
| 301.8 KiB | ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 216.9 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 |
| 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 |
| 100.5 KiB | src/lib/api.ts |
| 88.5 KiB | src/products.tsx |
| 69.4 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 40.1 KiB | src/lib/utils/eventUsageLogic.ts |
| 38.7 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 |
| 33.9 KiB | ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js |
| 28.4 KiB | src/scenes/scenes.ts |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 301.8 KiB | ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 272.3 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 216.9 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 |
| 153.7 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 |
| 100.5 KiB | src/lib/api.ts |
| 98.8 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 |
| 88.5 KiB | src/products.tsx |
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.16 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.16 MiB · 19 files | no change | ████░░░░░░ 37.8% of 5.72 MiB |
| Deferred (lazy) | 2.10 MiB · 44 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 |
|---|---|
| 805.4 KiB | dist/toolbar/toolbar-app-5UT2PX3W.css |
| 651.7 KiB | dist/toolbar/chunk-chunk-FB7G53XJ.js |
| 259.4 KiB | dist/toolbar/chunk-chunk-VA2BZRYB.js |
| 138.3 KiB | dist/toolbar/chunk-chunk-TAQPY5O2.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 75.2 KiB | dist/toolbar/toolbar-app-VGDV3P3S.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-JGFHXRQQ.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-QYQSHABZ.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 — 🔺 +35.4 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 947.79 MiB · 🔺 +35.4 KiB (+0.0%)
ℹ️ MCP UI apps size — 33 app(s), 17633.1 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 597.9 KB | 199.2 KB |
| action | 454.1 KB | 199.2 KB |
| action-list | 564.2 KB | 199.2 KB |
| cohort | 453.1 KB | 199.2 KB |
| cohort-list | 563.2 KB | 199.2 KB |
| email-template | 452.9 KB | 199.2 KB |
| error-details | 469.6 KB | 199.2 KB |
| error-issue | 454.5 KB | 199.2 KB |
| error-issue-list | 564.8 KB | 199.2 KB |
| experiment | 561.3 KB | 199.2 KB |
| experiment-list | 564.9 KB | 199.2 KB |
| experiment-results | 566.3 KB | 199.2 KB |
| feature-flag | 566.8 KB | 199.2 KB |
| feature-flag-list | 570.5 KB | 199.2 KB |
| feature-flag-testing | 457.3 KB | 199.2 KB |
| inline-scan | 453.6 KB | 199.2 KB |
| insight-actors | 562.3 KB | 199.2 KB |
| invite-email-preview | 452.3 KB | 199.2 KB |
| llm-costs | 559.3 KB | 199.2 KB |
| session-recording | 455.3 KB | 199.2 KB |
| survey | 454.7 KB | 199.2 KB |
| survey-global-stats | 561.9 KB | 199.2 KB |
| survey-list | 564.9 KB | 199.2 KB |
| survey-stats | 561.9 KB | 199.2 KB |
| trace-span | 453.5 KB | 199.2 KB |
| trace-span-list | 564.1 KB | 199.2 KB |
| vision-observation-list | 563.3 KB | 199.2 KB |
| workflow | 453.4 KB | 199.2 KB |
| workflow-list | 563.5 KB | 199.2 KB |
| loops-review | 457.8 KB | 199.2 KB |
| query-results | 774.1 KB | 199.2 KB |
| render-ui | 858.1 KB | 199.2 KB |
| visual-review-snapshots | 457.9 KB | 199.2 KB |
⚠️ MCP snapshots — 6 updated (6 modified, 0 added, 0 deleted)
Snapshots: MCP unit test snapshots updated
Changes: 6 snapshots (6 modified, 0 added, 0 deleted)
What this means:
- Snapshots have been automatically updated to match current output
Next steps:
- Review the changes to ensure they're intentional
- If unexpected, investigate what caused the output to change
✅ Django migration risk — migration analysis complete
We've analyzed your migrations for potential risks.
Summary: 1 Safe | 1 Needs Review | 0 Blocked
⚠️ Needs Review
May have performance impact
workflows.0028_workflowproposal_uuid7
└─ #1 ⚠️ AlterField
Field alteration may cause table locks or data loss (check if changing type or constraints)
model: workflowproposal, field: id, field_type: UUIDField
✅ Safe
Brief or no lock, backwards compatible
workflows.0027_hogflowoptimization
└─ #1 ✅ CreateModel
Creating new table is safe
model: HogFlowOptimization
│
└──> ℹ️ INFO:
ℹ️ Skipped operations on newly created tables (empty tables
don't cause lock contention).
Last updated: 2026-09-29 16:39 UTC (7c87063)
|
[Medium risk] Adds metrics measurement to workflow suggestions. This PR is not safe to merge until suggestion authoring works with issued scopes and outcome readings preserve their provenance and version accounting. Reviews (1) · Last reviewed commit: "feat(workflows): measure what an applied..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProposal creation reads metrics for the evidence step at the specified base version and adds an available reading to the evidence. API declarations require Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to Suggestions can present misleading metric comparisons, and the MCP suggestion tool may be unable to file proposals. Resolve these issues before merging unless the affected behavior is explicitly accepted. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to A credential allowed to submit workflow suggestions can now receive workflow metric readings as part of submission. The reading is limited to the selected workflow, but it is unclear whether suggestion-only credentials are meant to have that access. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
products/workflows/backend/api/hog_flow.py-6330-6332 (1)
6330-6332: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
order_by("version")before slicing the revisions.
filter(...)[:OUTCOME_VERSION_LIMIT]has no explicit ordering. Once a workflow has more than 20 revisions afterapplied, the database can return any 20 of them. The loop then walks versions with gaps, socarryingandchange_ended_at_versioncan skip the version that actually ended the change.Proposed fix
- for revision in HogFlowRevision.objects.filter(hog_flow=hog_flow, version__gte=applied)[ - :OUTCOME_VERSION_LIMIT - ] + for revision in HogFlowRevision.objects.filter(hog_flow=hog_flow, version__gte=applied).order_by( + "version" + )[:OUTCOME_VERSION_LIMIT]products/workflows/backend/api/hog_flow.py-4364-4411 (1)
4364-4411: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
describe_version_changesmisses removed steps and removed keys, soother_changescan be wrong.The function walks only the paths that exist in
current. It never reports these cases:
- A step present in
previousbut absent fromcurrent.- A key that exists on the old step but was removed from the new step.
- A workflow field present in
previousbut absent fromcurrent.For example, a version that deletes an email step produces
changes == []andother_changes == False. The outcome card then reads that version's numbers as coming from the suggestion alone.Fix:
- Add a "step removed" entry for each id in
was_stepsthat is missing fromcurrent.- Also walk
_patch_paths(was)and compare those paths.- Loop over the union of
previousandcurrentkeys for workflow fields.products/workflows/frontend/Workflows/suggestions/suggestionChanges.ts-44-60 (1)
44-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDrop leaves whose suggested value already equals the live value.
proposal.contentis stored as the producer sent it. Only the backend'sproposal_changesreduces it to real changes. If a producer sends a whole step (the_proposetest helper sends the full trigger step and the full webhook step), the "What changes" table lists every unchanged field with the same value under "Now" and "Suggested". The "This suggestion changes nothing" message never appears for an echo-only proposal.Skip a leaf when
beforedeep-equalsafter, matching_changed_leaveson the server. Also drop a step whosefieldslist ends up empty.Proposed fix
+import { equal } from 'fast-equals' @@ - changes.push({ - path: segments.join('.'), - label: labelFor(segments), - before: readPath(live, segments), - after: value, - }) + const before = readPath(live, segments) + // A null patch leaf deletes the key, so it is no change when the key is already absent. + if (equal(before, value) || (value === null && before === undefined)) { + continue + } + changes.push({ path: segments.join('.'), label: labelFor(segments), before, after: value })
🧹 Nitpick comments (2)
products/workflows/backend/api/hog_flow.py (1)
6156-6163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the second draft assignment.
Line 6155 already sets
locked.draft = merged, andmergedis built fromchanges. Lines 6160-6163 build the same draft again and callbase_content_ofa second time. That is a second revision query while the row lock is held. It is also a second code path that can drift from the onevalidate_graphjust checked.Proposed fix
locked.draft = merged - - # The draft is always a full content snapshot (live content as the base, the proposal's - # changed fields merged in), so publish stays a plain copy with no merge logic. - - locked.draft = merge_proposal_content( - snapshot_flow_content(locked), - proposal_changes(locked_proposal, base_content_of(locked, locked_proposal)), - ) locked.draft_updated_at = timezone.now()products/workflows/mcp/tools.yaml (1)
350-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
workflows-list-versionsduplicatesworkflows-list-revisions.Both tools map to
hog_flows_revisions_listwith the same scope and the same input schema. Agents now see two tools for one endpoint and have to guess which one to call. Move the guidance about the live version's age into theworkflows-list-revisionsdescription and remove the new entry. The alternative is to disable the old entry. As per path instructions: "says everything once and only once, and has no superfluous parts".Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: d404c975-505b-4727-a8f1-496e11cea391
⛔ Files ignored due to path filters (5)
products/workflows/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/workflows/frontend/generated/api.tsis excluded by!**/generated/**products/workflows/frontend/generated/api.zod.tsis excluded by!**/generated/**services/mcp/src/generated/workflows/api.tsis excluded by!**/generated/**services/mcp/src/tools/generated/workflows.tsis excluded by!**/generated/**
📒 Files selected for processing (22)
posthog/api/app_metrics2.pyproducts/workflows/CONTRIBUTING.mdproducts/workflows/backend/api/hog_flow.pyproducts/workflows/backend/api/test/test_hog_flow.pyproducts/workflows/backend/api/test/test_workflow_proposals.pyproducts/workflows/backend/metrics.pyproducts/workflows/frontend/Workflows/hogflows/types.tsproducts/workflows/frontend/Workflows/suggestions/WorkflowAppliedOutcome.tsxproducts/workflows/frontend/Workflows/suggestions/WorkflowMetricReading.tsxproducts/workflows/frontend/Workflows/suggestions/WorkflowSuggestionCard.tsxproducts/workflows/frontend/Workflows/suggestions/WorkflowSuggestionDetails.tsxproducts/workflows/frontend/Workflows/suggestions/WorkflowSuggestionEvidence.tsxproducts/workflows/frontend/Workflows/suggestions/suggestionChanges.test.tsproducts/workflows/frontend/Workflows/suggestions/suggestionChanges.tsproducts/workflows/frontend/Workflows/suggestions/suggestionEvidence.test.tsproducts/workflows/frontend/Workflows/suggestions/suggestionEvidence.tsproducts/workflows/mcp/tools.yamlservices/mcp/schema/generated-tool-definitions.jsonservices/mcp/schema/tool-definitions-all.jsonservices/mcp/src/api/generated.tsservices/mcp/tests/unit/__snapshots__/tool-schemas/workflows-list-versions.jsonservices/mcp/tests/unit/__snapshots__/tool-schemas/workflows-stats.json
💤 Files with no reviewable changes (1)
- products/workflows/frontend/Workflows/suggestions/WorkflowMetricReading.tsx
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
| FROM app_metrics2 | ||
| WHERE team_id = %(team_id)s | ||
| AND app_source = %(app_source)s | ||
| {"AND app_source_id IN %(app_source_ids)s" if app_source_ids else ""} |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Return no rows for an empty app_source_ids list.
When app_source_ids=[], this condition removes the ID filter and queries every versioned workflow metric for the team. A caller requesting no IDs can receive unrelated totals and incur a full-team query. Return {} for an explicit empty list; reserve None for an unfiltered read.
| before_date, _, _ = relative_date_parse_with_delta_mapping(params["before"], team.timezone_info) | ||
|
|
||
| series = self._metric_series_for(obj, params.get("version")) | ||
| data = fetch_app_metric_totals( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Forward instance_id to versioned totals.
HogFlowMetricsRequestSerializer inherits the documented instance_id filter, but this call never passes it to fetch_app_metric_totals. If a version contains two email steps, ?version=1&instance_id=email_1 returns their combined totals instead of email_1 totals. Pass the validated instance filter, using the same instance-selection contract as /metrics.
| # Fields a proposal replaces wholesale. `actions` merges per step instead, since steps carry stable | ||
| # ids; edges have no id, and a variable list is short enough to carry whole. | ||
| PROPOSAL_WHOLE_LIST_FIELDS = ("edges", "variables") | ||
|
|
||
| # How far past the applied version the after side will look for versions that kept the change. | ||
| OUTCOME_VERSION_LIMIT = 20 | ||
|
|
||
| # Written by the serializer on publish, not by whoever edited the workflow. | ||
| DERIVED_STEP_KEYS = frozenset({"bytecode", "order", "transpiled"}) | ||
|
|
||
| PROPOSAL_MERGE_BY_ID_FIELDS = ("actions",) | ||
|
|
||
| # 1.0 is either every message or one of them; only the producer knows which. | ||
| EVIDENCE_UNITS = ("rate", "count") | ||
|
|
||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Delete the duplicated proposal constants.
PROPOSAL_WHOLE_LIST_FIELDS, PROPOSAL_MERGE_BY_ID_FIELDS and EVIDENCE_UNITS are already defined at lines 3835-3843. PROPOSAL_LIST_OF_OBJECT_FIELDS is already defined at line 3840. The second definitions silently rebind the names at module load. WorkflowProposalCreateSerializer reads the first binding at class creation, while merge_proposal_content and conflicting_parts read the later one. If one copy is edited, validation and merge disagree. Keep only OUTCOME_VERSION_LIMIT and DERIVED_STEP_KEYS here. As per path instructions: "says everything once and only once".
Also applies to: 4508-4509
Source: Path instructions
| # Listing is workflow-read. Authoring takes its own narrow scope, so a producer can suggest without | ||
| # being able to publish. | ||
| if request.method in ("GET", "HEAD", "OPTIONS"): | ||
| return ["hog_flow:read"] | ||
| return ["hog_flow:write"] | ||
| return ["hog_flow_proposal:write"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -n -C2 'hog_flow_proposal' --glob '!**/test/**'
rg -n -B2 -A10 'workflows-suggest:' products/workflows/mcp/tools.yamlRepository: PostHog/posthog
Length of output: 2873
Update the workflows-suggest scopes.
POST requests to proposals require hog_flow_proposal:write, but workflows-suggest declares only hog_flow:write. Tokens issued from the tool definition can therefore receive 403 when creating suggestions. Add hog_flow_proposal:write and hog_flow:read, then regenerate the MCP definitions.
| def _version_outcome( | ||
| self, hog_flow: HogFlow, versions: Sequence[Optional[int]], after: Any, step_id: Optional[str] = None | ||
| ) -> Optional[dict]: | ||
| read = [version for version in versions if version is not None] | ||
| if not read: | ||
| return None | ||
| totals: dict[str, float] = {} | ||
| for version in read: | ||
| # Scoped to the step the suggestion names; several email steps would otherwise share one denominator. | ||
| version_totals = fetch_app_metric_totals( | ||
| team_id=self.team_id, | ||
| app_source=HOG_FLOW_VERSION_APP_SOURCE, | ||
| app_source_id=f"{hog_flow.id}/{version}", | ||
| breakdown_by="name", | ||
| after=after, | ||
| instance_id=step_id or None, | ||
| name=[ | ||
| TARGET_SEND_METRIC, | ||
| TARGET_OPEN_METRIC, | ||
| TARGET_CLICK_METRIC, | ||
| TARGET_UNTRACKED_METRIC, | ||
| *GUARDRAIL_METRICS, | ||
| ], | ||
| ).totals | ||
| for name, count in version_totals.items(): | ||
| totals[name] = totals.get(name, 0) + count | ||
| return {**_outcome_from_totals(totals, read[0]), "versions": read} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
measured always reads opens, whatever metric the producer named.
_version_outcome calls _outcome_from_totals(totals, read[0]) without a target, so measured.target is always the open rate. proposal_outcome uses target_metric_of(proposal), but this filing-time read does not. Example: a producer files metric: "email_bounced", current_value: 0.07. The card shows "email open rate … Measured by PostHog". evidenceDisagrees in suggestionEvidence.ts then compares the claimed bounce rate with the measured open rate and shows the "not what PostHog measured" warning. The same false warning appears for any metric outside TARGET_METRICS, such as "failure rate".
Fix:
- Pass the target metric through to
_outcome_from_totals. - In the frontend, compare values only when
measured.target.metricmatches the metric the producer claimed.
Proposed fix
- reading = self._version_outcome(hog_flow, [base_version], after_date, step_id)
+ target = evidence.get("metric") if evidence.get("metric") in TARGET_METRICS else TARGET_OPEN_METRIC
+ reading = self._version_outcome(hog_flow, [base_version], after_date, step_id, target)
@@
def _version_outcome(
- self, hog_flow: HogFlow, versions: Sequence[Optional[int]], after: Any, step_id: Optional[str] = None
+ self,
+ hog_flow: HogFlow,
+ versions: Sequence[Optional[int]],
+ after: Any,
+ step_id: Optional[str] = None,
+ target: str = TARGET_OPEN_METRIC,
) -> Optional[dict]:
@@
- return {**_outcome_from_totals(totals, read[0]), "versions": read}
+ return {**_outcome_from_totals(totals, read[0], target), "versions": read}
9765937 to
c82b5bf
Compare
dmarchuk
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
products/workflows/backend/api/hog_flow.py (1)
6343-6345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the new signatures.
The new
_measure_evidencesignature uses bare dictionary annotations. The new test method has an untyped_mock_flagparameter and no return annotation. The implicitselfparameter does not need a separate annotation.Suggested typing fix
def _measure_evidence( - self, hog_flow: HogFlow, base_version: int, step_id: Optional[str], evidence: dict - ) -> Optional[dict]: + self, + hog_flow: HogFlow, + base_version: int, + step_id: Optional[str], + evidence: Mapping[str, object], + ) -> Optional[dict[str, object]]:- def test_a_suggestion_carries_what_posthog_measured_next_to_what_it_claimed(self, _mock_flag): + def test_a_suggestion_carries_what_posthog_measured_next_to_what_it_claimed( + self, _mock_flag: MagicMock + ) -> None:This is a localized typing and maintainability concern. It does not support a major-impact classification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: c03bf4fe-eddd-4581-93fc-24527f07d66a
⛔ Files ignored due to path filters (1)
products/workflows/frontend/generated/api.schemas.tsis excluded by!**/generated/**
📒 Files selected for processing (2)
products/workflows/backend/api/hog_flow.pyproducts/workflows/backend/api/test/test_hog_flow.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/workflows/frontend/Workflows/suggestions/suggestionChanges.ts-55-56 (1)
55-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDrop steps with no changed fields.
When
sameLeaffilters every field from a whole-step patch,describeSuggestedChangesstill pushes the step.WorkflowSuggestionDetailsthen renders an empty table and suppresses “This suggestion changes nothing on the workflow.” Only add steps with non-emptyfields.🐛 Suggested fix
const { id, ...patch } = step const liveStep = liveSteps.get(id) + const fields = leafChanges(patch, liveStep) + if (fields.length === 0) { + continue + } steps.push({ stepId: id, stepName: liveStep?.name ?? null, - fields: leafChanges(patch, liveStep), + fields, })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 1b109169-3c19-4cc6-9311-c113f058bcea
📒 Files selected for processing (2)
products/workflows/frontend/Workflows/suggestions/suggestionChanges.test.tsproducts/workflows/frontend/Workflows/suggestions/suggestionChanges.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 6 remain after this review.
be69494 to
7ae2099
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/workflows/backend/api/hog_flow.py-6331-6332 (1)
6331-6332: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not compare readings from different windows.
WorkflowProposalEvidenceFieldaccepts any JSON object, andvalidate_evidencedoes not validatewindow._measure_evidencereplaces unsupported values such as-2wor an ISO interval with-7d, while the originalevidence.windowremains stored.evidenceDisagreescompares only the rate and denominator, so it can flag readings from different windows as a disagreement.🐛 Suggested fix
export function evidenceDisagrees(evidence: Record<string, unknown>, measured: MeasuredEvidence): boolean { + if (evidence.window !== undefined && evidence.window !== measured.window) { + return false + } if ( readUnit(evidence.unit) !== 'rate' || typeof evidence.current_value !== 'number' ||
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 9bc8ca2c-28e1-4710-9f3b-d436e13a2ae1
⛔ Files ignored due to path filters (1)
products/workflows/frontend/generated/api.schemas.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
products/workflows/backend/api/hog_flow.pyproducts/workflows/backend/api/test/test_hog_flow.pyproducts/workflows/frontend/Workflows/suggestions/WorkflowAppliedOutcome.tsx
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
dmarchuk
added
run-ci-backend
7ae2099 to
a70d694
Compare
a70d694 to
f353805
Compare
…nto dmarchuk/wf-proposal-outcomes
…nto dmarchuk/wf-proposal-outcomes
This branch has not been deployed
1 participant
Problem
A suggestion arrives carrying whatever numbers its producer chose to send, and a reviewer has nothing to check them against. A producer that reads the wrong step, or the wrong window, files a confident number nobody can falsify.
Its details are no better: they name which fields changed, not what they changed to.
Changes
Unverified, and a reading under the minimum sample is marked rather than shown as a result.measuredkey is dropped before the server reads, so it can never be passed off as PostHog's.Not setandRemoved, and leave out fields a whole-step patch resent unchanged.How did you test this code?
These surfaces were exercised in the browser on the combined branch, before this layer was split out of #92252. Nothing was re-rendered after the split.
Measured by PostHogtag. Rewriting the producer's numbers to 31% on 40 showed the disagreement line.test_a_producer_cannot_pass_off_its_own_numbers_as_posthogsfails the metric read with a producer-suppliedmeasuredin the payload and asserts the key is gone.suggestionChanges.test.tscovers the change list, including a value removed versus never set, and a step sent whole with one field changed.Not done after the split: no browser pass. Sends and metrics are seeded, not produced by real traffic.
Release status
Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Opus 5
Split out of #92252, then again: #107879 carries the per-version outcome reading, this one the measured evidence and the change table.
describe_version_changesstayed in #107879 because the per-version outcome calls it directly.Skills invoked:
/writing-pr-descriptions,/writing-ui-components,/writing-tests,/stacking-prs.CodeRabbit CLI pass: skipped, the CLI is signed out.