Implement computed-airtable-fields (framework for pushing aggregate fields back into Airtable)#2614
Conversation
|
Warning Review limit reached
More reviews will be available in 56 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a new package Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Will-Howard
force-pushed
the
wh-2582-computed-airtable-fields-2026-06
branch
from
cbffb14 to
b8a9f39
Compare
Greptile SummaryThis PR introduces
Confidence Score: 5/5Safe to merge — the new library is additive and read-path failures are fully isolated; writes only happen for rows where the value actually changed. The computation engine is well-tested across all meaningful edge cases (idempotency, null transitions, multi-chunk, error isolation), and the cron integration correctly guards against concurrent runs while sharing the existing rate-limiter. The two suggestions are minor observability and API-surface nits that do not affect correctness. No files require special attention. The SQL patterns in definitions.ts (unnest + arrayOverlaps) are the most novel part and are covered by dedicated integration tests. Important Files Changed
Sequence DiagramsequenceDiagram
participant Cron as recomputeComputedAirtableFieldsCron
participant Core as recomputeValues
participant PG as PostgreSQL
participant RL as RateLimiter
participant AT as Airtable (via db.update)
Cron->>Core: "recomputeValues({ db, definition, beforeWrite })"
Core->>PG: "SELECT id, field WHERE id > cursor LIMIT 500"
PG-->>Core: chunk of rows
Core->>Core: "compute(db, chunkIds) → { id: newValue }"
Core->>Core: diff newValue vs currentValue
loop for each changed row
Core->>RL: beforeWrite() → rateLimiter.acquire()
RL-->>Core: slot acquired
Core->>AT: "db.update(table, { id, field: newValue })"
AT-->>Core: ok / error (caught, counted as failed)
end
Core->>PG: next chunk (cursor advances)
Core-->>Cron: "{ checked, updated, failed, errors }"
Cron->>Cron: logger.info(counts)
Reviews (2): Last reviewed commit: "Make compute functions more query-effici..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libraries/computed-airtable-fields/src/core.ts (1)
55-59: 💤 Low valueConsider validating that
computereturns values for all input ids.The diff loop iterates
Object.entries(computed), so if the compute function returns fewer ids than requested, those missing ids are silently skipped (their stored values remain unchanged). Whilst all current definitions follow the contract of returning a value for every input id, adding validation would catch future bugs earlier.🛡️ Proposed validation
const processChunk = async (rows: { id: string; current: ComputedAirtableFieldValue }[]) => { let computed: Record<string, ComputedAirtableFieldValue>; try { computed = await definition.compute(db, rows.map((r) => r.id)); } catch (err) { // Whole chunk fails — we couldn't determine fresh values, so count every row as failed. errors.push(err); failed += rows.length; return; } + + // Validate contract: compute must return a value for every input id + for (const row of rows) { + if (!(row.id in computed)) { + const err = new Error(`Compute function did not return value for id: ${row.id}`); + errors.push(err); + failed += rows.length; + return; + } + } const currentById = Object.fromEntries(rows.map((r) => [r.id, r.current]));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libraries/computed-airtable-fields/src/core.ts` around lines 55 - 59, The loop over Object.entries(computed) can silently ignore missing ids from the compute result; after calling compute (the function that produces `computed`) validate that every expected id (use the keys from `currentById` or the original requested ids array) exists in `computed`, and if any are missing throw or return a clear error specifying the missing ids (include the id list in the message) so you fail-fast; reference the `computed`, `currentById` and the compute call to locate where to add this validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/pg-sync-service/src/lib/cron.ts`:
- Around line 64-76: The recompute loop should not abort on a single field
error: wrap the await recomputeValues call inside a per-field try/catch so each
{table, fields} -> [field, compute] iteration catches its own errors, logs them
(use logger.error with context including getTableName(table.pg) and field), and
continues to the next field; keep beforeWrite: () => rateLimiter.acquire() as-is
and still log successful runs with logger.info when recomputeValues returns
checked/updated/failed.
---
Nitpick comments:
In `@libraries/computed-airtable-fields/src/core.ts`:
- Around line 55-59: The loop over Object.entries(computed) can silently ignore
missing ids from the compute result; after calling compute (the function that
produces `computed`) validate that every expected id (use the keys from
`currentById` or the original requested ids array) exists in `computed`, and if
any are missing throw or return a clear error specifying the missing ids
(include the id list in the message) so you fail-fast; reference the `computed`,
`currentById` and the compute call to locate where to add this validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5caac56-b0d5-41f7-9b7a-73c0c2281427
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
apps/pg-sync-service/package.jsonapps/pg-sync-service/src/lib/cron.tslibraries/computed-airtable-fields/.env.testlibraries/computed-airtable-fields/README.mdlibraries/computed-airtable-fields/eslint.config.mjslibraries/computed-airtable-fields/package.jsonlibraries/computed-airtable-fields/src/core.test.tslibraries/computed-airtable-fields/src/core.tslibraries/computed-airtable-fields/src/definitions.test.tslibraries/computed-airtable-fields/src/definitions.tslibraries/computed-airtable-fields/src/index.tslibraries/computed-airtable-fields/tsconfig.jsonlibraries/computed-airtable-fields/vitest.config.mjs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/pg-sync-service/src/lib/cron.ts`:
- Around line 68-73: The current destructuring from recomputeValues ignores its
returned errors array, so when failed > 0 we only log counts and lose diagnostic
details; update the call to destructure errors (e.g., const { checked, updated,
failed, errors } = await recomputeValues(...)) and after the existing
logger.info add a logger.error branch: if (failed > 0) log an error-level
message that includes the key `${getTableName(table.pg)}.${field}` and at least
one item from errors (errors[0] or a summarized errors[0].message) so callers
can see an example failure and stack/message for debugging.
- Around line 85-88: The three cron registrations are currently executed at
module import; move the COMPUTED_AIRTABLE_FIELDS_RECOMPUTE_SCHEDULE registration
(the call that schedules recomputeComputedAirtableFieldsCron) out of
apps/pg-sync-service/src/lib/cron.ts so it is not started at import time and
instead register it from the service post-start path (e.g., after the optional
initial sync in apps/pg-sync-service/src/index.ts post-start hook). Concretely:
stop calling cron.schedule for COMPUTED_AIRTABLE_FIELDS_RECOMPUTE_SCHEDULE in
the module top-level, expose a function like
registerComputedAirtableRecomputeSchedule or startComputedAirtableCron that
performs cron.schedule(recomputeComputedAirtableFieldsCron,
COMPUTED_AIRTABLE_FIELDS_RECOMPUTE_SCHEDULE), and invoke that function from the
index.ts post-start/after-initial-sync code path so the schedule only starts
after bootstrap completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c3d4a0e0-64dd-468d-b4a1-4b978d095e87
📒 Files selected for processing (2)
apps/pg-sync-service/src/lib/cron.test.tsapps/pg-sync-service/src/lib/cron.ts
marn-in-prod
left a comment
marn-in-prod
left a comment
There was a problem hiding this comment.
Looks good. Just the edge case that we should probably make a decision on
| .where(and( | ||
| arrayOverlaps(resourceCompletionTable.pg.resourceId, ids), | ||
| // != NO_RESPONSE also excludes NULL (NULL != 0 → UNKNOWN, filtered out). | ||
| ne(resourceCompletionTable.pg.resourceFeedback, RESOURCE_FEEDBACK.NO_RESPONSE), |
There was a problem hiding this comment.
computedNumCompletions filters on isCompleted = true but this average doesn't, so a row with isCompleted: false and an actual rating still moves the average while not counting as a completion.
This is currently possible in the UI, rate a resource and then uncomplete it.
The data state already exists, but since this field is new we should probably consciously decide what we intend here and add a test to make the decision obvious (or a comment)
There was a problem hiding this comment.
Seems reasonable 👍. I think I'll go with isCompleted = true everywhere to keep things simple (so we can keep the mental model that there's not much different between a row having isCompleted = false vs simply not existing)
Will-Howard
temporarily deployed
to
wh-2582-computed-airtable-fields-2026-06 - bluedot-preview PR #2614
Render
Destroyed
Will-Howard
temporarily deployed
to
wh-2582-computed-airtable-fields-2026-06 - bluedot-storybook-preview PR #2614
Render
Destroyed
2 participants
Description
Creates a new library,
computed-airtable-fields:libraries/computed-airtable-fields/src/definitions.ts, analogous toschema.tsrecomputeComputedAirtableFieldsCronin pg-sync-service runs every 2 hours to recalculate the fieldsdb.updatecalls for changed rowsIssue
Fixes #2582
Developer checklist