refactor(polling): consolidate polling services into provider handler pattern#4035
Conversation
… pattern Eliminate self-POST anti-pattern and extract shared boilerplate from 4 polling services into a clean handler registry mirroring lib/webhooks/providers/. - Add processPolledWebhookEvent() to processor.ts for direct in-process webhook execution, removing HTTP round-trips that caused Lambda 403/timeout errors - Extract shared utilities (markWebhookFailed/Success, fetchActiveWebhooks, runWithConcurrency, resolveOAuthCredential, updateWebhookProviderConfig) - Create PollingProviderHandler interface with per-provider implementations - Consolidate 4 identical route files into single dynamic [provider] route - Standardize concurrency to 10 across all providers - No infra changes needed — Helm cron paths resolve via dynamic route Co-Authored-By: Claude Opus 4.6 <[email protected]>
PR SummaryMedium Risk Overview Consolidates API endpoints by replacing separate Eliminates the polling self-POST pattern by adding Reviewed by Cursor Bugbot for commit 4a1000c. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR is a well-scoped structural refactor that consolidates four near-identical polling services (Gmail, Outlook, IMAP, RSS) into a unified handler pattern under Key changes:
Confidence Score: 5/5Safe to merge — all prior P1 concerns are resolved, no new correctness or security issues introduced. All previously raised P1 findings (archived webhook filtering, auth-before-enumeration, idempotency import path) are addressed. Remaining findings are P2: a discarded preprocessing error detail, duplicated local interfaces in processor.ts, and verbose logger type annotations in private helpers. None affect runtime correctness. apps/sim/lib/webhooks/processor.ts — minor observability gap in preprocessResult error handling and interface duplication.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/webhooks/poll/[provider]/route.ts | Consolidates 4 identical route files into one dynamic route; auth check correctly precedes provider validation; lock key and TTL preserved correctly. |
| apps/sim/lib/webhooks/polling/utils.ts | Extracts shared boilerplate (markWebhookFailed/Success, fetchActiveWebhooks with archivedAt guards, runWithConcurrency, resolveOAuthCredential, updateWebhookProviderConfig) that was previously duplicated across all 4 providers. |
| apps/sim/lib/webhooks/polling/orchestrator.ts | Clean entry point delegating to handler registry and shared concurrency runner; correct per-webhook requestId scoping. |
| apps/sim/lib/webhooks/processor.ts | Adds processPolledWebhookEvent() to bypass self-POST; preprocessResult.error is discarded without logging; PolledWebhookRecord/PolledWorkflowRecord duplicate types in polling/types.ts. |
| apps/sim/lib/webhooks/polling/gmail.ts | Ports Gmail polling logic faithfully; uses ReturnType<typeof import('@sim/logger').createLogger> instead of Logger type in private helpers — minor style inconsistency. |
| apps/sim/lib/webhooks/polling/types.ts | Clean strategy interface for PollingProviderHandler; WebhookRecord/WorkflowRecord types well-defined but not reused in processor.ts. |
| apps/sim/lib/webhooks/polling/registry.ts | Simple handler lookup with VALID_POLLING_PROVIDERS derived from registry keys — adding a new provider only requires one entry here. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
CRON["Helm Cron\n/api/webhooks/poll/{provider}"]
ROUTE["[provider]/route.ts\nverifyCronAuth → VALID_POLLING_PROVIDERS\nacquireLock"]
ORCHESTRATOR["orchestrator.ts\npollProvider()"]
REGISTRY["registry.ts\ngetPollingHandler()"]
UTILS_FETCH["utils.ts\nfetchActiveWebhooks()"]
CONCURRENCY["utils.ts\nrunWithConcurrency(CONCURRENCY=10)"]
GMAIL["gmail.ts\ngmailPollingHandler"]
OUTLOOK["outlook.ts\noutlookPollingHandler"]
IMAP["imap.ts\nimapPollingHandler"]
RSS["rss.ts\nrssPollingHandler"]
IDEMPOTENCY["pollingIdempotency\n(per item)"]
PROCESSOR["processor.ts\nprocessPolledWebhookEvent()"]
ADMIT["tryAdmit() ticket"]
PREPROCESS["checkWebhookPreprocessing()"]
DISPATCH["BullMQ / inline job queue\nwebhook-execution"]
CRON --> ROUTE
ROUTE --> ORCHESTRATOR
ORCHESTRATOR --> REGISTRY
ORCHESTRATOR --> UTILS_FETCH
UTILS_FETCH --> CONCURRENCY
CONCURRENCY --> GMAIL & OUTLOOK & IMAP & RSS
GMAIL & OUTLOOK & IMAP & RSS --> IDEMPOTENCY
IDEMPOTENCY --> PROCESSOR
PROCESSOR --> ADMIT
ADMIT --> PREPROCESS
PREPROCESS --> DISPATCH
Reviews (2): Last reviewed commit: "fix(polling): use literal for maxDuratio..." | Re-trigger Greptile
…e casts - Widen processPolledWebhookEvent body param to accept object, eliminating `as unknown as Record<string, unknown>` double casts in all 4 handlers - Extract LOCK_TTL_SECONDS constant in route, tying maxDuration and lock TTL to a single value Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Add archivedAt filters to fetchActiveWebhooks query, matching findWebhookAndWorkflow in processor.ts to prevent polling archived webhooks/workflows - Move provider validation after auth check to prevent provider enumeration by unauthenticated callers - Fix inconsistent pollingIdempotency import path in outlook.ts to match other handlers Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
@greptile |
|
@cursor review |
Next.js requires segment config exports to be statically analyzable literals. Using a variable reference caused build failure. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 4a1000c. Configure here.
Summary
processPolledWebhookEvent()toprocessor.tsfor direct in-process webhook execution, removing HTTP round-trips that caused Lambda 403/timeout errors under loadmarkWebhookFailed/Success,fetchActiveWebhooks,runWithConcurrency,resolveOAuthCredential,updateWebhookProviderConfig— previously duplicated across all 4 services (~30% of each file)[provider]route. Helm cron paths (/api/webhooks/poll/gmail, etc.) resolve unchanged via Next.js dynamic routingCONCURRENCY = 10(IMAP was previously 5)New structure (mirrors
lib/webhooks/providers/)No infra changes needed
/api/webhooks/poll/{gmail,outlook,imap,rss}— dynamic route matches identically${provider}-polling-lock) produce the same strings as beforemaxDuration = 180,dynamic = 'force-dynamic',verifyCronAuth— all preservedTest plan
bun run type-checkpasses (only pre-existing Athena errors)grep -r "polling-service" apps/sim/returns nothing/api/webhooks/poll/gmailetc. and returns 200