Repository navigation
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 |
|
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:
📝 WalkthroughWalkthroughThe pull request adds a managed sandbox email sender with feature-flagged provisioning, organization-member recipient checks, sandbox-specific SES delivery, and workflow UI support. It also adds a feature-flagged workflow first-run gallery with template start metadata, email editing, test sending, workflow creation, and setup-task completion. Tests and Storybook stories cover the new flows. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The sandbox sender and first-run onboarding are behind feature flags and are mostly well guarded. Two issues remain. A partially configured sandbox sender produces failed sends rather than a clean skip. One gallery template describes a start condition that differs from its actual trigger. Both are bounded and simple to fix. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Server-side recipient checks and fixed sender identity substantially constrain sandbox email use. The main risk is enabling the shared sender before independent volume and pause controls are available: one project's permitted traffic could affect other projects. Sending is disabled by default, which limits immediate exposure. 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❌ Failed checks (1 warning)
Full details: Description checkExplanation The description is well structured and explains the test-email completion change, but it does not match the broader changes in the supplied file summaries. Those summaries include sandbox sender provisioning and delivery restrictions, plus first-run workflow setup and gallery features. The statement that sender or recipient rules did not change conflicts with those summaries. The release-status section also keeps all three checkbox options instead of using one plain-bullet line. Resolution Clarify which changes belong to this PR versus its stacked parent. If the sandbox sender and first-run features are part of this PR, describe their user-visible behavior and testing in Problem, Changes, and How did you test this code?, and correct the claim that sender or recipient rules did not change. If they are inherited from the parent, state the exact scope of this PR. Replace the three checkbox lines under Release status with exactly one plain-bullet line. ✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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.
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/templates/onboarding_started_but_not_completed_template.json-4-8 (1)
4-8: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrigger URL filter and
starts_ondetail disagree.
starts_on.detailsayson /onboarding. The trigger filter at lines 35-43 matches$current_urlwithicontainsonmessaging/campaigns. The gallery will show a start condition that the workflow does not use.Align the two. Either change
detailto describe the actual filter, or change the filter value to/onboarding.products/workflows/frontend/firstRun/WorkflowsFirstRunMakeItYours.stories.tsx-83-88 (1)
83-88: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the global access level after the
ViewerAccessstory.The
ViewerAccessrender function changes the sharedwindow.POSTHOG_APP_CONTEXTobject. It setsresource_access_control.hog_flowto'viewer'and never sets it back. If another story renders after this one in the same runtime, that story can show viewer-disabled buttons in its snapshot. Put this state in a small wrapper that restores the previous value on unmount. As an alternative, use a story parameter or decorator that sets the app context for this story only.Proposed fix
export const ViewerAccess: Story = { render: function ViewerAccess(): JSX.Element { - ;(window as any).POSTHOG_APP_CONTEXT.resource_access_control.hog_flow = 'viewer' + const accessControl = (window as any).POSTHOG_APP_CONTEXT.resource_access_control + const previous = accessControl.hog_flow + accessControl.hog_flow = 'viewer' + useEffect(() => () => { + accessControl.hog_flow = previous + }, []) return <App /> }, }nodejs/src/cdp/cdp-services.ts-464-472 (1)
464-472: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTurn the sandbox sender off when its routing config is incomplete.
enabledcomes only fromWORKFLOWS_SANDBOX_SENDER_ENABLED. The tenant name, configuration set and From address all default to''. If the flag is on and any of these is unset,EmailServicepasses the gate atisSandbox && !this.sandboxSender?.config.enabled:
resolveFromSenderreturnsemail: ''.sesRouteForreturns an emptyTenantNameorConfigurationSetName.Each sandbox send then reaches SES and fails as
email_failed. It is not the cleanswitch_offskip.The Node From address (
SES_SANDBOX_FROM_ADDRESS) is also separate from the DjangoWORKFLOWS_SANDBOX_SENDER_FROM_ADDRESSstored in the integration row. If the two drift, the UI shows one address and the worker sends from another.Derive
enabledfrom all four values. Also log the misconfiguration at startup.Proposed fix
new SandboxEmailSender( { - enabled: config.WORKFLOWS_SANDBOX_SENDER_ENABLED, + enabled: + config.WORKFLOWS_SANDBOX_SENDER_ENABLED && + !!config.SES_SANDBOX_TENANT_NAME && + !!config.SES_SANDBOX_CONFIGURATION_SET && + !!config.SES_SANDBOX_FROM_ADDRESS, tenantName: config.SES_SANDBOX_TENANT_NAME,
🧹 Nitpick comments (4)
frontend/src/lib/components/ProductSetup/productSetupLogic.test.ts (1)
180-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStory and test guidelines forbid
featureFlagLogic.actions.setFeatureFlagsin stories only. Flag state leaks across tests here.The test sets
WORKFLOWS_FIRST_RUNthroughfeatureFlagLogic.actions.setFeatureFlags. ThefeatureFlagsreducer is persisted (persist: true). The test never resets it. The flag value from the lastit.eachcase (false) stays in the reducer for later tests in the file. It also persists to local storage in jsdom. Reset the flags in thefinallyblock.Proposed fix
} finally { + featureFlagLogic.actions.setFeatureFlags([], {}) workflowsLogic.unmount() }products/workflows/package.json (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin
@testing-library/domto a version range instead of*.
*accepts any version. The rootfrontend/package.jsondeclares>=9.3.4. Use the same range to keep the workspace resolution stable.Proposed fix
- "@testing-library/dom": "*", + "@testing-library/dom": ">=9.3.4",products/workflows/backend/tests/api/test_hog_flow_template.py (2)
22-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParameterize the per-template checks.
The
subTestloop hides which template failed from the runner. The path instructions prefer parameterised tests over near-duplicate test functions. Useparameterized.expandover template names, or keep the loop if you want one DB-free test.Source: Path instructions
54-61: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe trigger check is vacuous for templates with empty
trigger.filters.Templates such as
celebrate-milestone.jsonandtrial_started_upgrade_nudge_template.jsonhave"filters": {}on the trigger. For them, the loop asserts nothing. Their declared events (milestone_reached,trial_started) are never compared with a trigger. Assert that, forkind == "event", the trigger either has no event filters or has a first event that matchesstarts_on["events"][0]. Then confirm these templates are meant to trigger on any event.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
b3f0b0dd-8f4a-41f4-b932-04c427ab40c8
⛔ Files ignored due to path filters (4)
frontend/src/generated/core/api.schemas.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlproducts/integrations/frontend/generated/api.tsis excluded by!**/generated/**products/workflows/frontend/generated/api.schemas.tsis excluded by!**/generated/**
📒 Files selected for processing (112)
docs/internal/workflows-email-sending-tiers.mdfrontend/src/lib/components/CyclotronJob/CyclotronJobInputs.tsxfrontend/src/lib/components/ProductSetup/ProductSetupPopover.tsxfrontend/src/lib/components/ProductSetup/productSetupLogic.test.tsfrontend/src/lib/components/ProductSetup/productSetupLogic.tsfrontend/src/lib/components/ProductSetup/productSetupRegistry.tsfrontend/src/lib/components/ProductSetup/types.tsfrontend/src/lib/constants.tsxfrontend/src/lib/integrations/IntegrationsList.tsxfrontend/src/lib/integrations/integrationsLogic.test.tsfrontend/src/lib/integrations/integrationsLogic.tsfrontend/src/lib/integrations/utils.tsfrontend/src/scenes/hog-functions/email-templater/EmailTemplater.test.tsxfrontend/src/scenes/hog-functions/email-templater/EmailTemplater.tsxfrontend/src/scenes/hog-functions/email-templater/NativeEmailIntegrationChoice.stories.tsxfrontend/src/scenes/hog-functions/email-templater/emailTemplaterLogic.test.tsfrontend/src/scenes/hog-functions/email-templater/emailTemplaterLogic.tsxfrontend/src/scenes/hog-functions/email-templater/selectSenders.test.tsfrontend/src/scenes/hog-functions/email-templater/selectSenders.tsnodejs/package.jsonnodejs/src/cdp/cdp-services.tsnodejs/src/cdp/config.tsnodejs/src/cdp/services/managers/organization-members.service.tsnodejs/src/cdp/services/messaging/email.service.test.tsnodejs/src/cdp/services/messaging/email.service.tsnodejs/src/cdp/services/messaging/sandbox-email-sender.tsnodejs/src/cdp/types.tsnodejs/src/cdp/utils.tsposthog/api/integration.pyposthog/cdp/test/test_validation.pyposthog/cdp/validation.pyposthog/models/integration/__init__.pyposthog/models/integration/email.pyposthog/models/team/setup_tasks.pyposthog/settings/ses.pyproducts/integrations/mcp/tools.yamlproducts/workflows/backend/facade/api.pyproducts/workflows/backend/facade/contracts.pyproducts/workflows/backend/presentation/views/hog_flow.pyproducts/workflows/backend/presentation/views/hog_flow_template.pyproducts/workflows/backend/services/sandbox_sender.pyproducts/workflows/backend/templates/announce_a_new_feature_template.jsonproducts/workflows/backend/templates/celebrate-milestone.jsonproducts/workflows/backend/templates/feature-adoption-tips.jsonproducts/workflows/backend/templates/heavy-usage-detected.jsonproducts/workflows/backend/templates/high-intent-notification.jsonproducts/workflows/backend/templates/negative-survey-response.jsonproducts/workflows/backend/templates/onboarding_started_but_not_completed_template.jsonproducts/workflows/backend/templates/re-engagement-workflow.jsonproducts/workflows/backend/templates/repeated-failure-alert.jsonproducts/workflows/backend/templates/support-ticket-notification.jsonproducts/workflows/backend/templates/ticket-unresolved-alert.jsonproducts/workflows/backend/templates/trial-ending-reminder.jsonproducts/workflows/backend/templates/trial_started_upgrade_nudge_template.jsonproducts/workflows/backend/templates/unlocking_advanced_features.jsonproducts/workflows/backend/templates/unused-features-education.jsonproducts/workflows/backend/templates/user-stuck-onboarding.jsonproducts/workflows/backend/templates/user-upgrade-webhook.jsonproducts/workflows/backend/templates/welcome_email_sequence_template.jsonproducts/workflows/backend/test/test_sandbox_sender_lifecycle.pyproducts/workflows/backend/tests/api/test_email_reputation.pyproducts/workflows/backend/tests/api/test_hog_flow_template.pyproducts/workflows/backend/tests/api/test_sandbox_sender.pyproducts/workflows/backend/tests/api/test_sandbox_sender_validation.pyproducts/workflows/frontend/Broadcasts/broadcastTestSendLogic.tsproducts/workflows/frontend/Broadcasts/steps/BroadcastContentStep.tsxproducts/workflows/frontend/Channels/EmailSetup/emailSetupModalLogic.test.tsproducts/workflows/frontend/Channels/EmailSetup/emailSetupModalLogic.tsproducts/workflows/frontend/Channels/MessageChannels.stories.tsxproducts/workflows/frontend/Channels/MessageChannels.tsxproducts/workflows/frontend/Channels/SandboxEmailSenderRow.tsxproducts/workflows/frontend/Channels/ownSenderQuickStartLogic.test.tsproducts/workflows/frontend/Channels/ownSenderQuickStartLogic.tsproducts/workflows/frontend/TemplateLibrary/SendTestEmailModal.stories.tsxproducts/workflows/frontend/TemplateLibrary/SendTestEmailModal.test.tsxproducts/workflows/frontend/TemplateLibrary/SendTestEmailModal.tsxproducts/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.test.tsproducts/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.tsproducts/workflows/frontend/Workflows/hogflows/findTestSendSkipReason.test.tsproducts/workflows/frontend/Workflows/hogflows/findTestSendSkipReason.tsproducts/workflows/frontend/Workflows/hogflows/panel/testing/hogFlowEditorNotificationTestLogic.tsproducts/workflows/frontend/Workflows/hogflows/steps/components/HogFlowFunctionConfiguration.tsxproducts/workflows/frontend/Workflows/templates/WorkflowTemplateCard.tsxproducts/workflows/frontend/WorkflowsScene.tsxproducts/workflows/frontend/completeTestEmailSetupTask.tsproducts/workflows/frontend/emptyState/workflowsEmptyState.tsxproducts/workflows/frontend/firstRun/GalleryTemplateCard.tsxproducts/workflows/frontend/firstRun/MakeItYoursActions.tsxproducts/workflows/frontend/firstRun/MakeItYoursEmailList.tsxproducts/workflows/frontend/firstRun/TemplateStartsOnLine.tsxproducts/workflows/frontend/firstRun/WorkflowsFirstRun.stories.tsxproducts/workflows/frontend/firstRun/WorkflowsFirstRunGallery.tsxproducts/workflows/frontend/firstRun/WorkflowsFirstRunMakeItYours.stories.tsxproducts/workflows/frontend/firstRun/WorkflowsFirstRunMakeItYours.tsxproducts/workflows/frontend/firstRun/WorkflowsFirstRunTab.test.tsxproducts/workflows/frontend/firstRun/WorkflowsFirstRunTab.tsxproducts/workflows/frontend/firstRun/buildFirstRunTestSend.tsproducts/workflows/frontend/firstRun/buildFirstRunWorkflow.tsproducts/workflows/frontend/firstRun/firstRunGalleryLogic.test.tsproducts/workflows/frontend/firstRun/firstRunGalleryLogic.tsproducts/workflows/frontend/firstRun/firstRunMakeItYoursLogic.test.tsproducts/workflows/frontend/firstRun/firstRunMakeItYoursLogic.tsproducts/workflows/frontend/firstRun/firstRunSenderLogic.test.tsproducts/workflows/frontend/firstRun/firstRunSenderLogic.tsproducts/workflows/frontend/firstRun/firstRunStoryFixtures.tsproducts/workflows/frontend/firstRun/firstRunWorkflowStorage.tsproducts/workflows/frontend/firstRun/templateEmails.tsproducts/workflows/frontend/firstRun/withFirstRunSender.test.tsproducts/workflows/frontend/firstRun/withFirstRunSender.tsproducts/workflows/frontend/testEmailSetupTask.test.tsproducts/workflows/package.jsonservices/mcp/src/api/generated.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.
dd87076 to
fed9695
Compare
|
Note 🤖 GPT-6.1 Sol responding on behalf of Michael Replaced by the draft fork PR #334 in its native fork stack. @meikelmosby will start migrating the stack from the fork into PostHog/posthog for upstream review and CI. Closing this master-targeted copy to keep the handoff clear. |
Problem
Workflows users can receive a test email from a template, broadcast, or workflow email step while "Send yourself a test email" stays incomplete.
Stacked on #112068. Tracks Silthus/posthog#290.
Changes
Storybook, before and after the delivered test-send result, with fixture data only.
1280px screenshots
Before:
flowchart LR A[First-run delivered email] --> B[Quick-start task complete] C[Other delivered test emails] --> D[Task stays incomplete] classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; class A,C phBlue; class B,D phYellow;After:
flowchart LR A[Any real delivered test email] --> B[Shared completion check] --> C[Quick-start task complete] D[Skipped, failed, or mocked send] --> E[Task stays incomplete] classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; class A,B,D phBlue; class C,E phYellow;How did you test this code?
Red/green and local gate evidence. The touched Jest suites, full frontend typecheck, lint, and format check pass. The full pre-push hook passes.
Test rationale: The HTTP-mocked send logics verify delivered-only completion and persisted onboarding tasks for each send path. Existing first-run tests verify helper reuse. Existing WorkflowsFirstRunTab tests cover gallery/list selection. The gallery highlight stays intact.
Storybook screenshots exercise the template send logic with an HTTP-mocked success response. No live email provider delivery was exercised.
Release status
Automatic notifications
Docs update
No existing document covers this first-run setup task. This description records the behavior.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: T3 Code, Codex harness, GPT-6.1 Sol (
gpt-6.1-sol, high reasoning effort).Skills:
tdd,writing-tests,writing-kea-logics,writing-ui-components,adopting-generated-api-types,placing-product-frontend-code,writing-user-facing-copy,setting-feature-flags-in-storybook,hogli,writing-pr-descriptions,running-ci-preflight,merge-risk,share-preview, andunslop.The duplicate search found #112068, which completes only the first-run send. This increment wires the other existing send paths. All test addresses use example.com fixtures. CodeRabbit CLI is signed out, so its local pass was skipped. Merge risk is low; no sender or recipient rules changed. The read-only Claude Opus 5.5 review found one defect: lost gallery guidance. Commit
dd8707653eb19restores it; the same review session confirmed the fix. No findings remain.The fork-master staleness advisory concerns fork-only agent docs. The specified stack parent stays unchanged; inherited OpenAPI/Python advisories do not originate in this frontend increment.
Assignment to the verified handle Silthus failed because the upstream API denied the assignee update. The PostHog asset uploader also denied access; screenshots use the authorized fork asset branch.