Sync Fathom recordings from webhooks - #25208
Conversation
|
🚀 Preview Environment Ready! Your preview environment is available at: https://gate-subcommittee-step-parents.trycloudflare.com This environment will automatically shut down after 5 hours. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Greptile SummaryThe PR adds signed, account-scoped Fathom webhook registration and ingestion, synchronizes delivered recordings into Call Recordings, and adds disconnect and uninstall lifecycle handling.
Confidence Score: 4/5The PR is not yet safe to merge because uninstall still loses the cleanup identifier when Fathom transiently rejects webhook deletion. A non-404 Fathom deletion failure is caught, but the corresponding registration is then deleted unconditionally, leaving an active remote webhook without recoverable cleanup state. Files Needing Attention: packages/twenty-apps/public/fathom/src/logic-functions/fathom-uninstall.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Twenty
participant KV
participant Fathom
participant CRM
User->>Twenty: Connect Fathom account
Twenty->>Fathom: Register signed webhook
Fathom-->>Twenty: Webhook ID and signing secret
Twenty->>KV: Store account-scoped registration
Fathom->>Twenty: Deliver recording webhook
Twenty->>KV: Load registration secret
Twenty->>Twenty: Verify signature and parse meeting
Twenty->>CRM: Match calendar event and upsert Call Recording
User->>Twenty: Uninstall application
Twenty->>Fathom: Delete each registered webhook
Twenty->>KV: Delete registration state
Reviews (2): Last reviewed commit: "Use isDefined for the call recording exi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🤖 PR Review
🛡️ Security Review✅ No high-severity vulnerabilities detected. 🚦 Auto-approve🙋 Manual review recommended for the following reason(s):
Automated pre-review — human approval still required. |
✅ Quality review · no findings
High-level — Clean additive single-directory Fathom app — product context documented, no migration/flag/public surface, disconnect-deletion deferral tracked behind #25215. Reviewed against the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 37 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-apps/public/fathom/src/logic-functions/utils/upsert-call-recording.util.ts">
<violation number="1" location="packages/twenty-apps/public/fathom/src/logic-functions/utils/upsert-call-recording.util.ts:34">
P2: When `createCallRecording` resolves without a returned record, this function reports `created: true` and the webhook succeeds even though creation was not confirmed. Validate `createResult.createCallRecording?.id` before returning so the delivery is retried or the failure is surfaced.</violation>
</file>
<file name="packages/twenty-apps/public/fathom/src/logic-functions/fathom-uninstall.ts">
<violation number="1" location="packages/twenty-apps/public/fathom/src/logic-functions/fathom-uninstall.ts:13">
P1: When a Fathom account was disconnected before app uninstall, this loop cannot delete its webhook. `fathomDisconnectHandler` retains the registration, but `listConnections` only returns remaining connected accounts, so the retained Fathom subscription is orphaned. Persist registrations in a durable uninstall-visible registry or delete them during disconnect with a valid credential.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
031499b to
c165ade
Compare
![twenty-ci-bot-public[bot]](https://avatars.githubusercontent.com/u/119600397?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 40 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 4 unresolved issues already reported by Cubic.
Re-trigger cubic
c165ade to
d36fcfb
Compare
There was a problem hiding this comment.
All reported issues were addressed across 40 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 7 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- filter isCanceled out of the calendar matching window and strip a leading www. when comparing meeting URLs - extract the stale-webhook delete and the store-or-rollback step out of the register hook so it reads top to bottom - use isDefined and isNonEmptyString guards instead of truthiness checks - lock the calendar page cap in the existing matcher test
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1 participant
Summary
TWENTY_FUNCTIONS_URL, including self-hosted/spathsValidation
yarn typecheck,yarn twenty dev:typecheck,yarn twenty dev:buildyarn lint,oxfmt --check src/Stack
3 of 4, now based on
mainsince the OAuth PR merged in #25197. Replaces #24902, which could not be reopened after its base branch was deleted. Review findings from that PR are resolved here: paginated calendar matching, compensating delete for the webhook registration write, uninstall hook, string literals over enum-like constant objects, and theascasts removed.Discovery and backfill are deferred to the next PR.