OUT-3869: One Xero Item per product — webhook + sync services - #60
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Greptile SummarySwitches the Xero sync model from one item per price to one item per product, and moves the auto-sync webhook trigger from
Confidence Score: 5/5Safe to merge — the concurrent-create race is now handled end-to-end, the legacy-row migration path is correct, and the core idempotency guarantees hold. All three previously-raised concerns were addressed: orphaned Xero items from concurrent creates are cleaned up via onConflictDoNothing().returning() plus a best-effort delete; the product-not-found case now emits a warning log; and the legacy price.created row is deleted only after handleEvent returns successfully. No new correctness issues were found. No files require special attention. The most complex logic in SyncedItems.service.ts and RetryFailedSyncs.service.ts is well-guarded. Important Files Changed
Sequence DiagramsequenceDiagram
participant Copilot
participant WebhookService
participant SyncedItemsService
participant XeroAPI
participant DB
note over Copilot,DB: Live product.created webhook
Copilot->>WebhookService: handleEvent(product.created)
WebhookService->>WebhookService: checkAutomaticProductSyncEnabled()
WebhookService->>SyncedItemsService: createSyncedItemsForProducts([product])
SyncedItemsService->>DB: getSyncedItemsMapByProductIds([productId])
DB-->>SyncedItemsService: existingMappings
alt product already mapped
SyncedItemsService-->>WebhookService: [] (skip)
else product not yet mapped
SyncedItemsService->>XeroAPI: "createItems([{code, name, description}])"
XeroAPI-->>SyncedItemsService: newlyCreatedItems
SyncedItemsService->>DB: INSERT INTO synced_items ON CONFLICT DO NOTHING RETURNING
alt won the insert race
DB-->>SyncedItemsService: inserted row
SyncedItemsService-->>WebhookService: [item]
else lost the insert race
DB-->>SyncedItemsService: (empty)
SyncedItemsService->>XeroAPI: deleteItem(orphanedItemId)
SyncedItemsService-->>WebhookService: [] (orphan cleaned up)
end
end
note over Copilot,DB: Legacy price.created retry
DB-->>WebhookService: "legacyRow {payload: {productId}}"
WebhookService->>Copilot: getProductsMapById([productId])
Copilot-->>WebhookService: product
WebhookService->>WebhookService: handleEvent(product.created)
WebhookService->>DB: "DELETE FROM failed_syncs WHERE id=legacyRow.id"
Reviews (3): Last reviewed commit: "fix(OUT-3869): harden legacy price.creat..." | Re-trigger Greptile |
…t.created Auto-create the Xero item on product.created instead of price.created: - Replace PriceCreatedEvent/PriceCreatedWebhook schemas with ProductCreated equivalents (shared ProductEventSchema with product.updated) and swap the discriminated-union member. - ValidWebhookEvent keeps PriceCreated (marked legacy) for historical failed_syncs rows, but it is no longer in the WebhookEvent union or routed. - handlePriceCreated -> handleProductCreated; logs and returns early when the product is already mapped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Replace createSyncedItemsForPrices with createSyncedItemsForProducts: skip products that are already mapped, and create the Xero item with no salesDetails.unitPrice (invoice lines always supply the price). - createItems now takes a code -> productId map and uses onConflictDoNothing as race safety against the (portalId, tenantId, productId) unique index. - addSyncedItems/deleteSyncedItems: skip (continue) items missing an itemId instead of aborting the whole batch; correct the loop comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y price.created - failed_syncs_type enum: add product.created (additive migration), keep price.created so historical rows stay valid. - On retry, resolve legacy price.created records as product.created: look up the product by the payload's productId, dispatch product.created, and drop the legacy row (after the fetch, so transient failures keep it for retry). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two concurrent product.created events for the same product could each create a Xero item; onConflictDoNothing kept only one DB mapping, leaving the other Xero item orphaned and referenced by a stale sync log. Use returning() to detect which insert won, delete the losing request's Xero item, and skip its sync log. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8e07c45 to
6771f05
Compare
- Delete the legacy row only after a successful product.created dispatch (like every other event), so a failure in handleEvent can't lose the row. - Log when a legacy record is dropped because its product no longer exists or its payload has no productId. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6771f05 to
eaabd2e
Compare
priosshrsth
left a comment
There was a problem hiding this comment.
@SandipBajracharya lgtm. I have added few comments more related to code style.
|
|
||
| // Resolve legacy price.created records as product.created via the payload's productId | ||
| if (failedSync.type === ValidWebhookEvent.PriceCreated) { | ||
| const { productId } = (failedSync.payload ?? {}) as { productId?: string } |
There was a problem hiding this comment.
| const { productId } = (failedSync.payload ?? {}) as { productId?: string } | |
| const productId = failedSync.payload?.productId; |
II think this should simplfy if failedSync.payload is well typed. Otherwise your current approach is fine.
| 'WebhookService#handleProductCreated :: Product already mapped, nothing to do', | ||
| data.id, | ||
| ) | ||
| return |
There was a problem hiding this comment.
I don't like how we are relying on empty array => already mapped logic. But I think we are using upsert to create or update. So I am ok with this for now. But we should try to avoid using logic like this if possible.
| // One Xero Item per product: skip products that are already mapped | ||
| const existingMappings = await this.getSyncedItemsMapByProductIds(products.map((p) => p.id)) | ||
|
|
||
| for (const product of products) { |
There was a problem hiding this comment.
Not in the scope of this PR. But if there is too much processing to be done, We should extract these to a function. Fine for now.
Summary
Switches the sync flow from one-Xero-Item-per-price to one-per-product, and moves the auto-sync webhook trigger from
price.createdtoproduct.created. Part of OUT-3788.Commits, by concern:
switch product auto-sync from price.created to product.created— webhook event types:ProductCreatedEvent/ProductCreatedWebhook(sharedProductEventSchemawithproduct.updated), added toValidWebhookEvent+ discriminated union;handlePriceCreated→handleProductCreated(gated bysyncProductsAutomatically, skips when already mapped).PriceCreatedis kept inValidWebhookEvent(legacy) but dropped from theWebhookEventunion and routing.create one Xero item per product, idempotently—createSyncedItemsForProductsskips already-mapped products and creates the Xero Item with nosalesDetails.unitPrice;createItemstakes acode → productIdmap with.onConflictDoNothing();addSyncedItems/deleteSyncedItemsreturn→continue.add product.created to failed_syncs and resolve legacy price.created— additive enum migration (ALTER TYPE ... ADD VALUE 'product.created', keepsprice.created); on retry, legacyprice.createdrecords are resolved asproduct.created(look up product by the payload'sproductId, dispatch, then drop the legacy row).Acceptance criteria
product.createdcreates exactly one Xero Item per product (idempotent — no dupes if already mapped)product.updatedupdates the single item (unchanged)price.createdhandling remains (legacy enum value retained for historical rows only)productId, carrying the line'sunitAmountpnpm typecheckandpnpm lintpassTesting Criteria
https://www.loom.com/share/a794418fa3e74773acf2a328cd20ce68
Notes
ALTER TYPE ... ADD VALUE— fine on the Supabase Postgres version; it only adds the value (doesn't use it in the same transaction).🤖 Generated with Claude Code