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:
📝 WalkthroughWalkthroughAdds a feature-flagged email-domain setup wizard with domain selection, DNS configuration, and verification screens. Adds backend SES and Maildev status checks, DNS record publication and provider detection, and an integration status endpoint with caching and refresh support. Adds verification-event reporting, setup-method tracking, Domain Connect integration, and an MCP status retrieval tool. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The wizard has bounded domain-selection and sender-editing concerns, and concurrent updates can lose verification or setup-method state. Address the config-write race and retry handling before rollout, or explicitly accept these risks with follow-up ownership. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new status check can change sender verification under read-only access. Project ownership and domain-verification checks constrain the impact, but the change expands who can complete sender setup. 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 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 (4)
products/workflows/frontend/Channels/EmailDomain/domainInput.ts-41-46 (1)
41-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
rootDomainOfandnameFromDomaingive wrong results for multi-part public suffixes.Both functions keep only the last two labels. For
acme.co.uk,rootDomainOfreturnsco.uk. As a result,inferDomainsgroups unrelated.co.ukhosts under one root and can recommend the wrong domain. For the same input,nameFromDomainreturnsCoas the sender name. UK, AU, and JP customers are affected. Use a public-suffix-aware helper, or at minimum handle common second-level suffixes such asco.uk,com.au, andco.jp.products/workflows/frontend/Channels/EmailDomain/emailDomainWizardLogic.ts-424-431 (1)
424-431: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRecord the analytics event even when
config.domainis missing.Line 430 falls back to
''when bothcreatedIntegration.config?.domainandvalues.sendingDomainare empty. With an empty domain,domainConnectCheck('')sends a request that is certain to fail. The failure is swallowed, so no data is lost. The only effect is one wasted request. The bigger concern is ordering:props.onCreatedruns first. In the modal,onCreatedswapsDomainStepforModalSettings, which unmounts this keyed logic while the listener is still awaiting. The capture still runs after the unmount, and readingvalues.sendingDomainafter the unmount can throw. Readvalues.sendingDomainbefore callingonCreated.Proposed fix
- actions.loadIntegrations() - props.onCreated(createdIntegration) - const domain = createdIntegration.config?.domain ?? values.sendingDomain ?? '' + const domain = createdIntegration.config?.domain ?? values.sendingDomain ?? '' + actions.loadIntegrations() + props.onCreated(createdIntegration) + if (!domain) { + return + }products/workflows/frontend/Channels/EmailDomain/components/FirstSenderCard.tsx-31-31 (1)
31-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBlock Enter from saving an empty sender name.
The Save button checks
!senderName.trim()and disables itself for a blank name.onPressEnterdoes not run this check. If the user clears the name and presses Enter,saveSenderpersists an emptyconfig.name.Proposed fix
- onPressEnter={nameChanged ? saveSender : undefined} + onPressEnter={nameChanged && senderName.trim() && !savedSenderLoading ? saveSender : undefined}frontend/src/lib/api.ts-6141-6147 (1)
6141-6147: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the generated client
The generated
integrationsEmailStatusRetrievefunction already supports this endpoint, including therefreshquery parameter. Use it with the generatedEmailDomainStatusApiresponse type instead of adding anApiRequestwrapper.Source: Coding guidelines
🧹 Nitpick comments (3)
products/workflows/frontend/Channels/EmailDomain/EmailDomainSetupModal.tsx (1)
66-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a kea logic for the modal's local state instead of
useState/useEffect.The modal stores
integrationIdwithuseStateand fires the analytics event fromuseEffect. The coding guidelines forbid both patterns for local state. MoveintegrationIdand the mount-timecapturecall into a small keyed logic, using a reducer andafterMount.As per coding guidelines: "Don't use
useStateoruseEffectto store local state."Source: Coding guidelines
frontend/src/lib/api.ts (1)
116-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a type-only import for
DnsHostTypeandEmailDomainStatusType.Both symbols are interfaces used only in type annotations. Remove them from the regular
~/typesimport.Suggested fix
import { - DnsHostType, - EmailDomainStatusType, EmailSenderDomainStatus, EndpointType, EndpointVersionType, ... } from '~/types' +import type { DnsHostType, EmailDomainStatusType } from '~/types'Source: Coding guidelines
posthog/models/integration/email.py (1)
192-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument or resolve the local-import exception.
These imports follow the existing model-to-workflows boundary, but the applicable rule still requires module-level imports. The inline
PLC0415comments do not document an approved exception. Add this boundary to the approved guidance, or route these calls through a lightweight module-level adapter that does not import the workflows facade during model initialization.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 49ea9d01-8698-4dbb-b195-00ea0922c34d
⛔ Files ignored due to path filters (4)
products/integrations/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/integrations/frontend/generated/api.tsis excluded by!**/generated/**services/mcp/src/generated/integrations/api.tsis excluded by!**/generated/**services/mcp/src/tools/generated/integrations.tsis excluded by!**/generated/**
📒 Files selected for processing (61)
frontend/src/lib/api.tsfrontend/src/lib/constants.tsxfrontend/src/lib/integrations/IntegrationEmailDomainView.tsxfrontend/src/productScenes.tsxfrontend/src/products.tsxfrontend/src/scenes/sceneTypes.tsfrontend/src/types.tsposthog/api/integration.pyposthog/api/integration_email_status_serializers.pyposthog/api/test/test_integration.pyposthog/dns_hosts.pyposthog/existing_email_tools.pyposthog/models/integration/email.pyposthog/test/test_dns_hosts.pyproducts/integrations/mcp/tools.yamlproducts/workflows/backend/facade/api.pyproducts/workflows/backend/facade/contracts.pyproducts/workflows/backend/facade/enums.pyproducts/workflows/backend/providers/__init__.pyproducts/workflows/backend/providers/maildev.pyproducts/workflows/backend/providers/ses.pyproducts/workflows/backend/services/email_domain_dns.pyproducts/workflows/backend/services/email_domain_status.pyproducts/workflows/backend/test/test_email_domain_status.pyproducts/workflows/frontend/Broadcasts/steps/BroadcastContentStep.tsxproducts/workflows/frontend/Channels/EmailDomain/EmailDomain.stories.tsxproducts/workflows/frontend/Channels/EmailDomain/EmailDomainScene.tsxproducts/workflows/frontend/Channels/EmailDomain/EmailDomainSetup.tsxproducts/workflows/frontend/Channels/EmailDomain/EmailDomainSetupModal.tsxproducts/workflows/frontend/Channels/EmailDomain/buildSetupPrompt.tsproducts/workflows/frontend/Channels/EmailDomain/components/AgentHandoffModal.tsxproducts/workflows/frontend/Channels/EmailDomain/components/AgentSetupButton.tsxproducts/workflows/frontend/Channels/EmailDomain/components/BigAction.tsxproducts/workflows/frontend/Channels/EmailDomain/components/DnsHostLink.tsxproducts/workflows/frontend/Channels/EmailDomain/components/DnsRecordRows.tsxproducts/workflows/frontend/Channels/EmailDomain/components/FirstSenderCard.tsxproducts/workflows/frontend/Channels/EmailDomain/components/NextWorkflowCard.tsxproducts/workflows/frontend/Channels/EmailDomain/components/StepHeading.tsxproducts/workflows/frontend/Channels/EmailDomain/components/StepIndicator.tsxproducts/workflows/frontend/Channels/EmailDomain/components/VerificationStepsStrip.tsxproducts/workflows/frontend/Channels/EmailDomain/dnsHosts.tsproducts/workflows/frontend/Channels/EmailDomain/domainInput.tsproducts/workflows/frontend/Channels/EmailDomain/emailDomainLogic.test.tsproducts/workflows/frontend/Channels/EmailDomain/emailDomainLogic.tsproducts/workflows/frontend/Channels/EmailDomain/emailDomainRoutes.test.tsproducts/workflows/frontend/Channels/EmailDomain/emailDomainWizardLogic.test.tsproducts/workflows/frontend/Channels/EmailDomain/emailDomainWizardLogic.tsproducts/workflows/frontend/Channels/EmailDomain/hoggies.tsproducts/workflows/frontend/Channels/EmailDomain/steps/DomainStep.tsxproducts/workflows/frontend/Channels/EmailDomain/steps/ReadyStep.tsxproducts/workflows/frontend/Channels/EmailDomain/steps/SettingsStep.tsxproducts/workflows/frontend/Channels/EmailDomain/steps/UnavailableStep.tsxproducts/workflows/frontend/Channels/EmailDomain/steps/VerifyingStep.tsxproducts/workflows/frontend/Channels/EmailDomain/useStartEmailDomainSetup.tsproducts/workflows/frontend/Channels/EmailSetup/EmailSetupModal.tsxproducts/workflows/frontend/Channels/MessageChannels.tsxproducts/workflows/frontend/MessagingTabActions.tsxproducts/workflows/manifest.tsxservices/mcp/schema/generated-tool-definitions.jsonservices/mcp/schema/tool-definitions-all.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; 11 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 (2)
products/workflows/frontend/Channels/EmailDomain/emailDomainLogic.ts-850-859 (1)
850-859: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReset
manualSetupRecordedwhen the update fails.
recordManualSetupsetscache.manualSetupRecorded = truebefore the request. The request swallows its error. If the PATCH fails, the logic never retries for that mount. The verification event then reportsmanualonly as the server default and loses an explicitmanualafter a previousauto. Reset the flag in the catch handler.Proposed fix
await api.integrations .updateEmailConfig(integrationId, { config: { ...integration.config, setup_method: 'manual' } }) - .catch(() => null) + .catch(() => { + cache.manualSetupRecorded = false + })posthog/api/integration.py-2581-2584 (1)
2581-2584: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPrevent stale email config writes.
email_updateloadsIntegrationbefore callingEmailIntegration.update_native_integration, which later saves the entire staleconfig. If_record_email_setup_methodor domain verification commits first, this save can removesetup_methodorverified.
email/statusandemail/verifyreload matching senders underselect_for_update(), so they do not have the same stale-save path. Use a database-side JSON-key merge for_record_email_setup_method, and apply the same merge pattern toEmailIntegration.update_native_integration; changing only this method does not prevent a later staleemail_updatesave.Suggested JSON-key update
+from django.contrib.postgres.fields import ArrayField from django.db import models, transaction @@ def _record_email_setup_method(self, integration_id: int, setup_method: str) -> None: - with transaction.atomic(): - instance = Integration.objects.select_for_update().get(id=integration_id, team_id=self.team_id) - instance.config["setup_method"] = setup_method - instance.save(update_fields=["config"]) + Integration.objects.filter(id=integration_id, team_id=self.team_id).update( + config=models.Func( + models.F("config"), + models.Value(["setup_method"], output_field=ArrayField(models.TextField())), + models.Value(setup_method, output_field=models.JSONField()), + function="jsonb_set", + ) + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 61b0d24a-1a8a-4a34-8a33-8d28a17ec3bf
📒 Files selected for processing (19)
posthog/api/integration.pyposthog/api/test/test_integration.pyposthog/domain_connect.pyposthog/models/integration/email.pyposthog/test/test_dns_hosts.pyproducts/workflows/backend/facade/api.pyproducts/workflows/backend/providers/ses.pyproducts/workflows/backend/services/email_domain_dns.pyproducts/workflows/backend/services/existing_email_tools.pyproducts/workflows/backend/test/test_email_domain_status.pyproducts/workflows/backend/test/test_email_integration_ses.pyproducts/workflows/backend/test/test_existing_email_tools.pyproducts/workflows/frontend/Channels/EmailDomain/EmailDomain.stories.tsxproducts/workflows/frontend/Channels/EmailDomain/components/FirstSenderCard.tsxproducts/workflows/frontend/Channels/EmailDomain/domainInput.tsproducts/workflows/frontend/Channels/EmailDomain/emailDomainLogic.test.tsproducts/workflows/frontend/Channels/EmailDomain/emailDomainLogic.tsproducts/workflows/frontend/Channels/EmailDomain/emailDomainWizardLogic.test.tsproducts/workflows/frontend/Channels/EmailDomain/steps/SettingsStep.tsx
💤 Files with no reviewable changes (2)
- posthog/test/test_dns_hosts.py
- products/workflows/backend/services/existing_email_tools.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
f14753a to
41cefba
Compare
Silthus
changed the title
41cefba to
707bfae
Compare
…email domain tests FakeDnsZone answers dnspython at the network edge, so the real resolver, zone_for_name and CNAME chaining run unchanged. FakeSes serves the SES, SESv2 and STS clients through boto3.client and flips each identity part to Success only when the fake zone holds its records. FakeDomainConnectProvider serves settings over HTTP, checks the apply URL signature, renders our real template and publishes its records. Every fake reads one FaultInjector keyed by operation. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Add GET integrations/{id}/email/status. It reads SES identity attributes and
the zone's own nameservers without changing SES, and returns the overall
status, three setup steps and every DNS record with pending, found or
verified. Records the zone's nameservers do not confirm, such as a DMARC
CNAME into another zone, are looked up again through a public resolver.
Responses are cached for 10 seconds; refresh=true skips the cache.
When SES reports the domain verified, the senders on that domain are marked
verified once and an "email domain verified" event reports the setup method
(manual or agent). The verify endpoint shares the same write.
The endpoint stays honest through provider and DNS failures:
- An SES or STS read error is a typed 503 with the code
email_provider_unavailable instead of an unhandled 500. Errors other
than throttling go to error tracking.
- A check where a DNS lookup got no answer neither verifies the domain nor
is cached, so the next poll decides once DNS answers again.
- A failed worker reload after the verified write no longer fails the
request. Workers refresh integrations on their own schedule.
- The SES MAIL FROM status counts only when SES holds this sender's MAIL
FROM domain, and the MAIL FROM SPF record shows verified only when it is
published.
- DNS checks stay off internal addresses, and the AWS account id is looked
up once instead of on every read.
Scenarios drive the endpoint end to end against the SES and DNS fakes.
The status tool is enabled for MCP behind workflows-email-domain-agent-setup.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…-click email setup domain-connect/check now returns the DNS host detected from the root domain's nameservers, with a link to its DNS settings, and the email tools that already send for the domain, detected from SPF and DKIM records. Its response gets a typed schema. The Domain Connect apply-url call records setup_method auto on the email integration and reports the "email domain verified" event when the domain verifies during that call. The sender update records setup_method manual and skips the SES MAIL FROM write when the bounce subdomain is unchanged. Lookups that got no answer are retried instead of cached: - A DNS timeout or SERVFAIL on the _domainconnect record, and a timeout, rate limit, 5xx or malformed answer from the provider settings, no longer hide one-click setup for an hour. Only a definitive answer is cached. - Existing email tool detection no longer caches an empty list for an hour after a lookup got no answer. Scenarios prove one-click email setup end to end: the check endpoint finds the provider and DNS host, the apply URL is signed, the fake provider renders our real template, publishes its records, and status polls reach verified with the auto setup method. They cover a person returning without approval, the sender update, and the provider failures. They replace the tests that patched generate_apply_url, the verify call and the private discovery helpers. The case for an existing DMARC record is skipped until Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…in connect check Adds the workflows-email-domain-wizard and workflows-email-domain-agent-setup flags. The Domain Connect check client and logic read the generated EmailDomainConnectCheckApi type instead of a hand-written copy, so they see the new dns_host and existing_email_tools keys. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ainst a fake backend The sending domain page state lives in six logics keyed by the integration id, each with one job: the sender record (load, name, setup method, test email), the status and its polling schedule, host detection and the Domain Connect round trip, the manual records path, the agent hand-off prompt, and a composer that derives the phase, the verification steps and the breadcrumbs from the others. The domain step logic infers likely sending domains from the project's event hosts. Domain parsing, domain inference, the agent prompt, the test email workflow and the page URL are pure modules. The status logic polls the read-only email status endpoint while the tab is visible, keeps polling through a 500 or an email_provider_unavailable 503, and stops on 400, 403 and 404. Both manual paths record setup_method manual on the integration. Tests talk to the HTTP boundary: a stateful fake backend typed with the generated API types holds senders, SES state and published DNS records, and a test advances it from pending to records_found to verified or injects a failure per endpoint. The polling schedule and the phase decisions are proven against it. The scene key and the urls.workflowsEmailDomain helper are registered here because the logics use them. The page itself, its route and the steps come in the next layer. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Adds a flag-gated page at /workflows/channels/email/:id that walks through picking a domain, adding the DNS settings at the detected host, and watching verification until the domain is ready to send. The page handles the Domain Connect return, offers an agent hand-off prompt behind a second flag, and shows an unavailable state with a Try again button when the sender or the status cannot be loaded. Every step and component reads from the logic that owns its state, bound once per integration id by BindEmailDomainLogics. The record rows tell people to merge include:amazonses.com into an SPF record that already exists and to keep an existing DMARC record. The Domain Connect return reads the kea router instead of window.location, so the same code path runs in the app and under the memory history in tests. The stories render the same fake backend the tests use, staged per state, so a story and a test cannot drift apart. Rendered flow tests drive the real scene outside in: the manual path from a typed domain to a verified first sender with a renamed sender and a test email; the one-click path through Cloudflare and the return with domain_connect=email; and recovery from a failed create, a failed sender load, a status outage after the records were found, a failed host detection, a failed restart and a return without approval. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e and broadcasts With workflows-email-domain-wizard on, the empty channels page, the new channel menu and configure on an unverified sender open the sending domain page, and the broadcast sender modal shows the wizard inline. The modal reports the new sender to the broadcast right after creating it and stays open for the DNS settings. With the flag off, every entry point keeps the old email sender modal. Rendered tests open the setup from each entry point with the flag off and on. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
707bfae to
2f97474
Compare
|
Note 🤖 GPT-6.1 Sol responding on behalf of Michael Replaced by the draft fork PR #341 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. |
Note
Merge risk: low (qa-swarm danger rubric)
Problem
Note
Stack (5 PRs, merge in order): 1. status endpoint #110516 → 2. Domain Connect and host detection #110517 → 3. wizard logics #110518 → 4. wizard UI #110519 → 5. entry points #110088
Every PR targets
masterbecause the branches live on a fork. Each one carries the commits of the PRs below it until those merge. Review only this layer: this layer's diff.Changes
workflows-email-domain-wizardon:email domain setup startedreports which entry point was used.How did you test this code?
Test rationale:
emailDomainEntryPoints.test.tsxrenders each entry point with the flag off and on. Off keeps the old modal; on goes to the page, or shows the wizard in the broadcast modal and hands the new sender to the broadcast.Release status
Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code. Backend by Opus 5.5 (
claude-opus-5-5), frontend by Fable 5.1 (claude-fable-5-1), stack split, integration and review fixes by Opus 5.5. Michael Reichenbach directed the work and picked the flow from three click-through prototypes.kea-disposablespauses keyed timers while the page is hidden, and a test covers it.🤖 Generated with Claude Code