Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughEmailTemplater now reports the selected template ID through email input components to workflow configuration. For Priority: ➖ Normal Merge Risk: 🔵 Low · up to The workflow changes appear mergeable with a bounded test-coverage follow-up: confirm that refresh actually re-saves the flow, rather than only leaving its fields unchanged. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The editor now preserves a template link without letting that link replace the email a user wrote. No new path to another project's template content or to sending an unvalidated email was established. The link itself is not verified on web saves, so it should not be treated as proof of template ownership. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/hogflows/steps/StepFunction.test.tsx-184-185 (1)
184-185: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winOwn the single preflight mount in this test.
initKeaTests()mountspreflightLogicby default. ItsafterMounthandler can dispatchloadPreflightSuccesssynchronously whenwindow.POSTHOG_APP_CONTEXT.preflightexists. The laterpreflightLogic.mount()does not create a new lifecycle for the already-mounted logic, so the action can occur beforeexpectLogicstarts observing it. Disable common mounts and keep the explicit mount under test.Suggested fix
- initKeaTests() + initKeaTests(false) preflightLogic.mount()
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 51b97c4b-c0d6-447e-ab9b-ca7a922b56a4
📒 Files selected for processing (7)
frontend/src/lib/components/CyclotronJob/CyclotronJobInputs.tsxfrontend/src/scenes/hog-functions/email-templater/emailTemplaterLogic.test.tsproducts/workflows/backend/api/hog_flow.pyproducts/workflows/backend/api/test/test_hog_flow_action_email.pyproducts/workflows/frontend/Workflows/hogflows/steps/StepFunction.test.tsxproducts/workflows/frontend/Workflows/hogflows/steps/StepFunction.tsxproducts/workflows/frontend/Workflows/hogflows/steps/components/HogFlowFunctionConfiguration.tsx
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
products/workflows/backend/api/test/test_hog_flow_action_email.py (2)
563-563: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpecify the
body_editvalue type.
body_edit: dictleaves its keys and values implicitly typed asAny. Usedict[str, str | None]for the body fields passed by these tests.As per coding guidelines, “Write as if mypy
--strictwere on. Annotate every signature, avoidAny.”Proposed type annotation
- def _stage_linked_web_edit(self, body_edit: dict) -> tuple[str, MessageTemplate]: + def _stage_linked_web_edit(self, body_edit: dict[str, str | None]) -> tuple[str, MessageTemplate]:Source: Coding guidelines
623-624: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the refresh command re-saved the flow.
The command catches validation failures and reports them without raising. The test discards this output, so unchanged fields do not prove that the re-save succeeded. Capture the output and assert one update and zero errors.
Suggested assertion
+ output = StringIO() with patch("products.workflows.backend.models.hog_flow.hog_flow.reload_hog_flows_on_workers"): - call_command("refresh_hog_flows", hog_flow_id=flow_id, stdout=StringIO()) + call_command("refresh_hog_flows", hog_flow_id=flow_id, stdout=output) + assert "Updated: 1" in output.getvalue() + assert "Errors: 0" in output.getvalue()
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 4b0f8002-0089-4660-a162-347b8d2e4610
📒 Files selected for processing (3)
products/workflows/backend/api/hog_flow.pyproducts/workflows/backend/api/test/test_hog_flow_action_email.pyproducts/workflows/frontend/Workflows/hogflows/steps/StepFunction.test.tsx
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.
Inserting a Library template into a workflow email step copied the content but dropped which template it came from. The templater now reports the applied template id through an optional onTemplateApplied prop, and the workflow email step stores it in config.template_uuid. Later edits keep the link; inserting another template replaces it. Hosts that do not pass the prop behave as before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Now that the editor stores template_uuid, a web save whose email body the user cleared was refilled from the Library template, and a publish could fail on a template deleted since the insert. Web saves already carry the whole email, so they skip server-side materialization and the link stays provenance only. MCP and API saves are unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A generic function step whose destination has an email input (Mailgun) also rendered the templater, so inserting a Library template wrote a message template id into its template_uuid, a field that means the destination template version there. Only function_email steps link now. The step test now also covers a second insert replacing the link, keeps its saved bodies per case, and settles the onboarding team update so nothing logs after teardown. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Type each copy of the template-applied callback from the templater's own prop type, check that the host gets the content before the link, and drop the logic test that duplicated the step test's replace check. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
initKeaTests already mounts preflightLogic, so the test only waits for its load before rendering the email editor. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Enabling a workflow from a canvas and the refresh_hog_flows command build a serializer context with no request source, so the web-save guard did not fire and a cleared linked email step was refilled from its Library template, and could go live that way. Template bodies are now filled in only for programmatic (API key, MCP, CLI) requests. Those saves always validate strictly, so the lenient branch is gone. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Reopening the picker during its close transition made react-modal warn about registering the same instance twice. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Upstream replaced HogFlow's conversion.window_minutes with a duration string window field; adapt this PR's test fixture to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
d2e6714 to
36b00ff
Compare
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| template_cache[cache_key] = template | ||
| email_content = (template.content or {}).get("email") if template else None | ||
| if not isinstance(email_content, dict) or not any(email_content.get(key) for key in _TEMPLATE_EMAIL_BODY_KEYS): | ||
| if strict: |
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of Michael
Removed in c373eac because strict is always true here now. Only API, MCP and CLI saves get past the new early return, and _should_validate_strictly makes those strict, so an unknown template is always a 400.
Problem
config.template_uuid. Only steps built in the web editor lose it.Changes
config.template_uuid. Nothing looks different in the editor.function_emailsteps link. A generic destination step with an email input (for example Mailgun) keeps itstemplate_uuiduntouched, because there the field names the destination template version._apply_email_template_contentcopied the template body into any linked step whose body was empty, whoever saved it.refresh_hog_flowscommand had the same problem. They save with no request source, and the canvas enable could put the template content live.emailTemplaterLogicgains an optionalonTemplateApplied(templateId)prop. TheapplyTemplatelistener calls it after it sets the content.CyclotronJobInputsandHogFlowFunctionConfigurationpass an optionalonEmailTemplateApplieddown, typed from that one prop.StepFunctionwires it topartialSetWorkflowActionConfig. This prop threading is mechanical.How did you test this code?
StepFunction.test.tsx(new) renders the step against a realworkflowLogicwith MSW mocks and picks Library templates through the picker:function_emailguard going missing.emailTemplaterLogic.test.ts: applying a template reports the id after the content, and still applies the content when the host passes no callback. Catches a lost or reordered callback, and a crash in hosts without it.test_hog_flow_action_email.py, backend contract tests for saves of a linked step:template_uuidgoes through the staged draft and publish, and both reach live.refresh_hog_flowsleaves it empty. Catches internal re-saves refilling it.ed038b0fb49) with this PR (36b00ff1792), using invented Library templates and draft workflows. Each result comes from the workflow API after the action:template_uuidtemplate_uuidis the Spring templatetemplate_uuidkepttemplate_uuidis the Summer templatetemplate_uuidkepttemplate_uuidtemplate_uuidactions__1__template_uuidtemplate_uuidtest-new-events-schema: not needed. The diff touches no event ingestion, event reads, or SQL over events.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Automatic notifications
Docs update
None. No doc under
docs/covers the email step's template link.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code,
claude-opus-5-5[1m](an Opus 5.5 sub-agent, run by an orchestration conductor on the same model)/tdd,/writing-tests,/writing-kea-logics,/writing-ui-components,/writing-code-comments,/running-ci-preflight,/writing-pr-descriptions.function_emaillinks.StepFunction.test.tsx, duplicate logic test removed.describescope and a log after teardown: fixed.refresh_hog_flowsstill refilled a cleared body, because they carry no request source. Fixed by filling in bodies only for programmatic sources. The same review's nits (two stale comments, a renamed lenient-save test, a return type, and a React-Modal warning in the step test) are fixed too.template_uuidinto another team. Every lookup is team-scoped, so nothing leaks. The Broadcasts wizard does not link, which the spec leaves out of scope.claude-fable-5-1) sub-agent drove the local stack with Playwright after the rebase onto current master.🤖 Generated with Claude Code