Conversation
🦋 Changeset detectedLatest commit: cdd6a70 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds forwarding of transaction exchange rates to outgoing webhooks: an optional numeric Changes
Sequence Diagram(s)sequenceDiagram
participant TxnEvent as Transaction Event
participant ServerLogic as Server Logic
participant WebhookBuilder as Webhook Payload Builder
participant WebhookTarget as External Webhook
TxnEvent->>ServerLogic: emit created/completed (body.spend)
activate ServerLogic
alt body.spend.currency != body.spend.localCurrency
ServerLogic->>WebhookBuilder: include spend.exchangeRate
else
ServerLogic->>WebhookBuilder: omit spend.exchangeRate
end
deactivate ServerLogic
WebhookBuilder->>WebhookTarget: POST payload (with conditional exchangeRate)
activate WebhookTarget
WebhookTarget->>WebhookTarget: verify Signature HMAC and process payload
deactivate WebhookTarget
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the inclusion of exchange rates in webhook payloads when the transaction currency differs from the local currency. The implementation involves updating the transaction and webhook schemas, modifying the publishing logic to conditionally attach the exchange rate, and updating the documentation. Comprehensive tests have been added and updated to verify the new behavior. I have no feedback to provide.
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Prevent Tests Dashboard |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84402000-271a-4d76-8f8b-bd546d875628
📒 Files selected for processing (4)
.changeset/shy-foxes-trade.mddocs/src/content/docs/webhooks.mdserver/hooks/panda.tsserver/test/hooks/panda.test.ts
nfmelendez
force-pushed
the
exchange-rate
branch
from
cdd6a70 to
bc54ccf
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit what i can do to run a review if i get " Review rate limit exceeded" ? is there a manual way? |
|
Tip For best results, initiate chat on the files or code changes.
In short, the most straightforward path is to simply wait a short while and then retry with |
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed16abbe-f9f6-4d41-98b6-f61ecb01d30f
📒 Files selected for processing (5)
.changeset/shy-foxes-trade.mddocs/src/content/docs/webhooks.mdserver/hooks/panda.tsserver/test/database.tsserver/test/hooks/panda.test.ts
| | body.spend.authorizedAmount | integer | The authorized amount | 100 | | ||
| | body.spend.status | "pending" \| "declined" | Can be pending or declined. In case of declined, the field `declinedReason` has the reason | pending | | ||
| | body.spend.declinedReason? | string | Decline message | webhook declined | | ||
| | body.spend.exchangeRate? | number | Present when `currency` differs from `localCurrency`. The exchange rate applied to the transaction | 1.1806900825 | |
There was a problem hiding this comment.
Define the rate direction.
exchangeRate is still ambiguous here. Consumers need to know whether this is currency/localCurrency or the inverse to reconcile amount with localAmount.
Also applies to: 624-624
| ...BaseTransaction.entries.spend.entries, | ||
| status: v.picklist(["pending", "declined"]), | ||
| declinedReason: v.optional(v.string()), | ||
| exchangeRate: v.optional(v.number()), |
There was a problem hiding this comment.
The new webhook contract is not enforced.
exchangeRate is documented as present for cross-currency created/completed webhooks, but these schemas still accept it as optional and this branch only forwards it if Panda already supplied it. A cross-currency event without spend.exchangeRate will still pass validation and emit a payload that contradicts the docs. Either require it in those cases or relax the docs to “may be present”.
Also applies to: 160-160, 1322-1328, 1383-1383, 1422-1422
| } catch (error) { | ||
| warmupController.abort(); | ||
| await warmupLogWatcher.exit.catch(() => undefined); | ||
| await warmupLogWatcher.outputFlushed; | ||
| const warmupText = await readFile(warmupLog, "utf8").catch(() => ""); | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| throw new Error(`wait firehose warmup: ${message}\nfirehose:\n${warmupText}`, { cause: error }); | ||
| } finally { | ||
| warmupLogWatcher.stopWatchingOutput(); | ||
| warmupController.abort(); | ||
| await warmupLogWatcher.exit.catch(() => undefined); | ||
| await warmupLogWatcher.outputFlushed; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant cleanup between catch and finally blocks.
The catch block (lines 104-106) performs abort(), await exit, await outputFlushed, and then finally (lines 112-114) repeats the same operations. On the error path, both blocks execute sequentially, causing redundant calls.
Consider consolidating cleanup entirely in finally and capturing the error for deferred handling:
♻️ Proposed consolidation
- } catch (error) {
- warmupController.abort();
- await warmupLogWatcher.exit.catch(() => undefined);
- await warmupLogWatcher.outputFlushed;
- const warmupText = await readFile(warmupLog, "utf8").catch(() => "");
- const message = error instanceof Error ? error.message : String(error);
- throw new Error(`wait firehose warmup: ${message}\nfirehose:\n${warmupText}`, { cause: error });
+ } catch (error) {
+ warmupLogWatcher.stopWatchingOutput();
+ warmupController.abort();
+ await warmupLogWatcher.exit.catch(() => undefined);
+ await warmupLogWatcher.outputFlushed;
+ const warmupText = await readFile(warmupLog, "utf8").catch(() => "");
+ const message = error instanceof Error ? error.message : String(error);
+ throw new Error(`wait firehose warmup: ${message}\nfirehose:\n${warmupText}`, { cause: error });
} finally {
warmupLogWatcher.stopWatchingOutput();
warmupController.abort();
- await warmupLogWatcher.exit.catch(() => undefined);
- await warmupLogWatcher.outputFlushed;
}This moves stopWatchingOutput() into catch (where it's needed before reading logs) and removes redundant awaits from finally, matching the pattern used for firehoseLogWatcher at lines 139-148.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Chores