Fix #18220 (airtable_oauth): normalize fallback events, add retry #18221
Conversation
… retry getting record
- Always wrap fallback emissions as { originalPayload } for consistent shape
- Guard against missing/empty changedTablesById in field & record emitters
- Add shared withRetry() (exponential backoff,) in common/utils
- Use withRetry for getRecord in record emitter; on failure, emit empty fields
WalkthroughAdds a retry utility with exponential backoff and exports it; integrates the retry into webhook record fetching. Webhook emitters defensively handle missing/empty payload.changedTablesById and normalize fallback emissions to { originalPayload }. Several action/source versions and package.json were bumped; no other public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant WH as Webhook Record Handler
participant P as Incoming Payload
participant AT as Airtable API
participant EM as Event Emitter
WH->>WH: emitEvent(P)
alt payload?.changedTablesById missing/empty
WH->>EM: emit({ originalPayload: P }, meta)
note right of EM: Normalized fallback emission
else
WH->>WH: derive tableEntry => tableId, tableData
alt destroyedRecordIds present
WH->>EM: emit({ originalPayload: P, tableId, deletedRecordIds }, meta)
else
loop retry attempts (withRetry)
WH->>AT: getRecord(tableId, recordId)
AT-->>WH: record | error
end
alt fetch success
WH->>EM: emit({ fields, originalPayload: P }, meta)
else fetch failed
WH->>EM: emit({ fields: {}, originalPayload: P }, meta)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (27)
✅ Files skipped from review due to trivial changes (27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
components/airtable_oauth/common/utils.mjs (1)
166-168: De-duplicate sleep helper across components
trustpilot/common/utils.mjsalready definessleep(ms). Consider centralizing this helper to avoid drift and duplication.components/airtable_oauth/sources/common/common-webhook-field.mjs (1)
28-36: Good defensive guard; add a clearer summary for fallback eventsEmitting a meta summary helps users distinguish normalized “raw” fallbacks in the UI and logs.
- this.$emit({ - originalPayload: payload, - }, this.generateMeta(payload)); + const summary = "Raw webhook payload (field): empty or unknown changedTablesById"; + this.$emit({ + originalPayload: payload, + }, this.generateMeta(payload, summary));components/airtable_oauth/sources/common/common-webhook-record.mjs (2)
14-22: Solid guard on empty payload shape; consider adding a summaryMatches field emitter behavior. Add a brief summary for better traceability.
- this.$emit({ - originalPayload: payload, - }, this.generateMeta(payload)); + const summary = "Raw webhook payload (record): empty or unknown changedTablesById"; + this.$emit({ + originalPayload: payload, + }, this.generateMeta(payload, summary));
77-82: Include identifiers in the error log for easier triageAdd
tableIdandrecordIdto the log context.- console.log("Airtable getRecord failed; emitting empty fields", { + console.log("Airtable getRecord failed; emitting empty fields", { statusCode: err?.statusCode ?? err?.response?.status, message: err?.message, - }); + tableId, + recordId, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
components/airtable_oauth/common/utils.mjs(1 hunks)components/airtable_oauth/sources/common/common-webhook-field.mjs(1 hunks)components/airtable_oauth/sources/common/common-webhook-record.mjs(3 hunks)components/airtable_oauth/sources/common/common-webhook.mjs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-07-24T02:06:47.016Z
Learnt from: GTFalcao
PR: PipedreamHQ/pipedream#12697
File: components/salesforce_rest_api/sources/common-webhook-methods.mjs:1-71
Timestamp: 2024-07-24T02:06:47.016Z
Learning: The `common-webhook-methods.mjs` object is designed to be extended, similar to an abstract class, and intentionally does not implement certain methods like `generateWebhookMeta` and `getEventType` to enforce implementation in subclasses.
Applied to files:
components/airtable_oauth/sources/common/common-webhook-record.mjscomponents/airtable_oauth/sources/common/common-webhook.mjs
🧬 Code graph analysis (3)
components/airtable_oauth/sources/common/common-webhook-record.mjs (1)
components/airtable_oauth/sources/common/common-webhook-field.mjs (4)
changed(28-28)tableEntry(29-29)tableId(37-40)tableData(13-15)
components/airtable_oauth/sources/common/common-webhook-field.mjs (2)
components/airtable_oauth/sources/common/common-webhook-record.mjs (3)
changed(14-14)tableEntry(15-15)tableId(23-26)components/airtable_oauth/common/utils.mjs (1)
tableId(142-142)
components/airtable_oauth/common/utils.mjs (1)
components/trustpilot/common/utils.mjs (1)
sleep(223-225)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
🔇 Additional comments (2)
components/airtable_oauth/common/utils.mjs (1)
190-191: Exporting withRetry expands public API; verify no naming collisionsConfirm no other modules export a
withRetrysymbol in this package scope and update any barrels accordingly.components/airtable_oauth/sources/common/common-webhook-record.mjs (1)
37-49: Deletion path looks goodEmits only IDs when records are destroyed and summarizes single vs multiple deletions. No issues.
| async function withRetry(fn, { | ||
| retries = 2, baseDelay = 500, | ||
| } = {}) { | ||
| let attempt = 0; | ||
| while (attempt <= retries) { | ||
| try { | ||
| return await fn(); | ||
| } catch (err) { | ||
| if (attempt === retries) throw err; | ||
| await sleep(baseDelay * (2 ** attempt)); | ||
| attempt += 1; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Retry policy retries on all errors; add shouldRetry, jitter, and max cap
Blindly retrying on 4xx (e.g., 404 for rapidly-deleted records) adds latency with no benefit. Add a shouldRetry predicate, jitter, and a maxDelay cap.
Apply:
-async function withRetry(fn, {
- retries = 2, baseDelay = 500,
-} = {}) {
+async function withRetry(fn, {
+ retries = 2,
+ baseDelay = 500,
+ factor = 2,
+ maxDelay = 5_000,
+ jitter = 0.1,
+ shouldRetry,
+} = {}) {
let attempt = 0;
while (attempt <= retries) {
try {
return await fn();
} catch (err) {
- if (attempt === retries) throw err;
- await sleep(baseDelay * (2 ** attempt));
- attempt += 1;
+ const status = err?.statusCode ?? err?.response?.status;
+ const defaultShouldRetry = (e) => {
+ const s = e?.statusCode ?? e?.response?.status;
+ return s == null || s >= 500 || s === 429;
+ };
+ const should = (shouldRetry ?? defaultShouldRetry)(err, attempt);
+ if (!should || attempt === retries) throw err;
+ const backoff = Math.min(maxDelay, baseDelay * (factor ** attempt));
+ const rand = 1 + (Math.random() * 2 - 1) * jitter; // +/- jitter
+ const delay = Math.max(0, Math.floor(backoff * rand));
+ await sleep(delay);
+ attempt += 1;
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function withRetry(fn, { | |
| retries = 2, baseDelay = 500, | |
| } = {}) { | |
| let attempt = 0; | |
| while (attempt <= retries) { | |
| try { | |
| return await fn(); | |
| } catch (err) { | |
| if (attempt === retries) throw err; | |
| await sleep(baseDelay * (2 ** attempt)); | |
| attempt += 1; | |
| } | |
| } | |
| } | |
| async function withRetry(fn, { | |
| retries = 2, | |
| baseDelay = 500, | |
| factor = 2, | |
| maxDelay = 5_000, | |
| jitter = 0.1, | |
| shouldRetry, | |
| } = {}) { | |
| let attempt = 0; | |
| while (attempt <= retries) { | |
| try { | |
| return await fn(); | |
| } catch (err) { | |
| const status = err?.statusCode ?? err?.response?.status; | |
| const defaultShouldRetry = (e) => { | |
| const s = e?.statusCode ?? e?.response?.status; | |
| return s == null || s >= 500 || s === 429; | |
| }; | |
| const should = (shouldRetry ?? defaultShouldRetry)(err, attempt); | |
| if (!should || attempt === retries) throw err; | |
| const backoff = Math.min(maxDelay, baseDelay * (factor ** attempt)); | |
| const rand = 1 + (Math.random() * 2 - 1) * jitter; // +/- jitter | |
| const delay = Math.max(0, Math.floor(backoff * rand)); | |
| await sleep(delay); | |
| attempt += 1; | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
We might as well have a 404 when we fetch "too early" and I think this was the case at least once in our testing. Hence the retry
There was a problem hiding this comment.
@mariczne That's a great point! If Airtable's API can return 404s for records that exist but aren't immediately available after webhook notifications (due to eventual consistency or propagation delays), then retrying on 404s makes perfect sense for this use case.
Your simpler retry implementation is appropriate here since the domain-specific behavior justifies retrying on all errors, including client errors like 404. Thanks for the clarification!
✏️ Learnings added
Learnt from: mariczne
PR: PipedreamHQ/pipedream#18221
File: components/airtable_oauth/common/utils.mjs:170-183
Timestamp: 2025-08-29T08:35:55.886Z
Learning: In Airtable webhook integrations, 404 errors can occur when fetching records "too early" after receiving webhook notifications, likely due to propagation delays between the webhook system and the API. Therefore, retrying on 404 errors is beneficial in this context.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| let fields = {}; | ||
| try { | ||
| const res = await withRetry( | ||
| () => this.airtable.getRecord({ | ||
| baseId: this.baseId, | ||
| tableId, | ||
| recordId, | ||
| }), | ||
| ); | ||
|
|
||
| fields = res?.fields ?? {}; | ||
| } catch (err) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine retries to avoid useless 4xx retries and add jitter
Pass shouldRetry to avoid retrying on 404/400s and to support 429/5xx/network only. This reduces latency when records are deleted between event and fetch.
- try {
- const res = await withRetry(
- () => this.airtable.getRecord({
- baseId: this.baseId,
- tableId,
- recordId,
- }),
- );
+ try {
+ const res = await withRetry(
+ () => this.airtable.getRecord({
+ baseId: this.baseId,
+ tableId,
+ recordId,
+ }),
+ {
+ retries: 2,
+ baseDelay: 500,
+ // Retry on 5xx / 429 / network only
+ shouldRetry: (err) => {
+ const status = err?.statusCode ?? err?.response?.status;
+ return status == null || status >= 500 || status === 429;
+ },
+ },
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let fields = {}; | |
| try { | |
| const res = await withRetry( | |
| () => this.airtable.getRecord({ | |
| baseId: this.baseId, | |
| tableId, | |
| recordId, | |
| }), | |
| ); | |
| fields = res?.fields ?? {}; | |
| } catch (err) { | |
| let fields = {}; | |
| try { | |
| const res = await withRetry( | |
| () => this.airtable.getRecord({ | |
| baseId: this.baseId, | |
| tableId, | |
| recordId, | |
| }), | |
| { | |
| retries: 2, | |
| baseDelay: 500, | |
| // Retry on 5xx / 429 / network only | |
| shouldRetry: (err) => { | |
| const status = err?.statusCode ?? err?.response?.status; | |
| return status == null || status >= 500 || status === 429; | |
| }, | |
| }, | |
| ); | |
| fields = res?.fields ?? {}; | |
| } catch (err) { |
| // Normalize the fallback so consumers always get a consistent structure | ||
| // matching our other emitters (which include originalPayload). | ||
| const meta = this.generateMeta(payload); | ||
| this.$emit(payload, meta); | ||
| this.$emit({ | ||
| originalPayload: payload, | ||
| }, meta); | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential breaking change: default emission now wrapped; provide BC path
Wrapping the payload under originalPayload can break existing workflows that read top-level fields. Consider dual-emitting the raw fields alongside originalPayload for a deprecation window.
- const meta = this.generateMeta(payload);
- this.$emit({
- originalPayload: payload,
- }, meta);
+ const meta = this.generateMeta(payload);
+ // Back-compat: spread raw keys for existing consumers; new consumers use originalPayload.
+ this.$emit({
+ originalPayload: payload,
+ ...payload,
+ }, meta);If you prefer a hard switch, gate the spread behind an env flag (e.g., PD_AIRTABLE_EMIT_RAW_COMPAT !== "false") and plan a removal date in the changelog.
🏁 Script executed:
#!/bin/bash
set -e
echo "Listing files in components/airtable_oauth/sources/common:"
ls -1 components/airtable_oauth/sources/common
echo
echo "Inspecting all $emit calls in common directory:"
rg -n "\$emit" -C1 components/airtable_oauth/sources/common/*.mjs || true
echo
echo "Inspecting originalPayload usage in common directory:"
rg -n "originalPayload" -C1 components/airtable_oauth/sources/common/*.mjs || trueLength of output: 3278
Provide backward-compatibility by dual-emitting raw payload fields
The new emitters now nest the entire payload under originalPayload, dropping all top-level properties and breaking consumers that expect those fields. You must restore the old shape by spreading payload after originalPayload (or gate it behind an env flag for a deprecation window).
Update these emit calls:
- components/airtable_oauth/sources/common/common-webhook.mjs (lines 140–143)
- components/airtable_oauth/sources/common/common-webhook-record.mjs (lines 18, 43, 86)
- components/airtable_oauth/sources/common/common-webhook-field.mjs (lines 32, 75)
Example diff for common-webhook.mjs:
- this.$emit({ originalPayload: payload }, meta);
+ this.$emit(
+ { originalPayload: payload, ...payload },
+ meta
+ );If you prefer a toggle, wrap the spread in an env-flag check (e.g. PD_AIRTABLE_EMIT_RAW_COMPAT !== "false") and schedule its removal in your changelog.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Normalize the fallback so consumers always get a consistent structure | |
| // matching our other emitters (which include originalPayload). | |
| const meta = this.generateMeta(payload); | |
| this.$emit(payload, meta); | |
| this.$emit({ | |
| originalPayload: payload, | |
| }, meta); | |
| }, | |
| // Normalize the fallback so consumers always get a consistent structure | |
| // matching our other emitters (which include originalPayload). | |
| const meta = this.generateMeta(payload); | |
| this.$emit( | |
| { originalPayload: payload, ...payload }, | |
| meta | |
| ); | |
| }, |
There was a problem hiding this comment.
Isn't that what versioning is for?
There was a problem hiding this comment.
@mariczne You're absolutely right! Versioning is the cleaner approach here.
If you're planning to bump the component version for this structural change, then the clean payload wrapping under originalPayload makes perfect sense without the backward compatibility complexity I suggested.
A version bump would:
- Give users control over when to migrate
- Avoid the dual-emission overhead
- Keep the new code clean and consistent
- Signal that this is an intentional breaking change
Are you planning to version this as a major update? If so, the current implementation is much better than my suggested dual-emission approach.
🤔 Hmm, you make a good point about versioning...
(\ /)
( ..)
o(")(")
🧠 Learnings used
Learnt from: GTFalcao
PR: PipedreamHQ/pipedream#12697
File: components/salesforce_rest_api/sources/common-webhook-methods.mjs:1-71
Timestamp: 2024-07-24T02:06:47.016Z
Learning: The `common-webhook-methods.mjs` object is designed to be extended, similar to an abstract class, and intentionally does not implement certain methods like `generateWebhookMeta` and `getEventType` to enforce implementation in subclasses.
Learnt from: GTFalcao
PR: PipedreamHQ/pipedream#12697
File: components/salesforce_rest_api/sources/common-webhook-methods.mjs:1-71
Timestamp: 2024-10-08T15:33:38.240Z
Learning: The `common-webhook-methods.mjs` object is designed to be extended, similar to an abstract class, and intentionally does not implement certain methods like `generateWebhookMeta` and `getEventType` to enforce implementation in subclasses.
Why
changedTablesById).What changed
emitDefaultEventnow always emits{ originalPayload }to keep a consistent shape.withRetry()incommon/utils.mjs(exponential backoff) and used it forgetRecord.record.fields = {}instead of throwing.Summary by CodeRabbit
Bug Fixes
Chores