ποΈ server: collect regardless of card status#593
Conversation
π¦ Changeset detectedLatest commit: 566a1f0 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 |
Summary of ChangesHello @cruzdanilo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a critical fix to the server's Panda hook, enabling the system to collect transaction data for cards regardless of their current status, particularly for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with π and π on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
![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 correctly implements the logic to allow transaction collection regardless of card status for non-authorization events, while keeping stricter checks for authorizations. The removal of the findCardById helper and inlining the queries makes the logic in each case clearer. The new test case over-captures frozen debit effectively validates the changes for frozen cards.
I've added a couple of suggestions to improve error handling consistency by returning specific HTTP error responses instead of throwing generic errors when a card is not found. This will make the API behavior more predictable for clients.
server/hooks/panda.ts
Outdated
There was a problem hiding this comment.
For better error handling and consistency within the file, consider returning a specific HTTP error response instead of throwing a generic Error. For example, a 404 Not Found would be appropriate here. This aligns with how other error conditions, like a mutex timeout, are handled in this endpoint.
| if (!card) throw new Error("card not found"); | |
| if (!card) return c.json({ code: "card not found" }, 404); |
There was a problem hiding this comment.
throwing an error is appropiate because if the card doens't exist in the db is because we didn't save it at the moment of the card creation and is not a user input error that can fix
server/hooks/panda.ts
Outdated
There was a problem hiding this comment.
Similar to my other comment, it's better to return a specific HTTP error response here instead of throwing a generic Error. This improves consistency in error handling across the different transaction actions.
| if (!card) throw new Error("card not found"); | |
| if (!card) return c.json({ code: "card not found" }, 404); |
There was a problem hiding this comment.
throwing an error is appropiate because if the card doens't exist in the db is because we didn't save it at the moment of the card creation and is not a user input error that can fix
There was a problem hiding this comment.
Code Review
This pull request enables transaction collection to proceed even if a card's status changes to frozen or locked after authorization. This is achieved by removing the status === 'ACTIVE' check when fetching cards for created and completed transaction events. The findCardById helper has been removed in favor of inlining the database queries. A new test case has been added to verify that a capture on a frozen card is successful.
My review focuses on improving error handling and code maintainability. I've identified that 'card not found' scenarios are not handled gracefully and will lead to unhandled exceptions. I've also pointed out that inlining the queries has introduced code duplication and suggested an alternative approach using a more flexible helper function, which would also help address the error handling concerns.
I am having trouble creating individual review comments. Click here to see my feedback.
server/hooks/panda.ts (219)
Throwing a generic Error here results in an unhandled exception within this handler, leading to a generic 500 response. For a 'card not found' scenario, it's better to return a specific error response (e.g., a 404 or a custom 5xx status code) to provide clearer feedback to the API consumer.
if (!card) return c.json({ code: "card not found or not active" }, 404);
server/hooks/panda.ts (444)
Similar to the requested case, throwing a generic Error here results in an unhandled exception. It's better to handle the 'card not found' case explicitly and return a specific error response to improve robustness and provide clear feedback.
if (!card) return c.json({ code: "card not found" }, 404);
server/hooks/panda.ts (816-824)
While inlining the database queries makes the logic in each case handler explicit, it introduces code duplication. Consider creating a new, more flexible helper function instead of removing this one entirely. This would centralize the card fetching logic and improve maintainability.
A parameterized helper function could also return null if a card isn't found, allowing callers to handle this case gracefully and fix the unhandled exception issues. For example:
async function findCard(cardId: string, options?: { activeOnly: boolean }) {
return database.query.cards.findFirst({
columns: { mode: true },
where: options?.activeOnly
? and(eq(cards.id, cardId), eq(cards.status, "ACTIVE"))
: eq(cards.id, cardId),
with: { credential: { columns: { account: true } } },
});
}
Codecov Reportβ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #593 +/- ##
==========================================
- Coverage 50.91% 46.02% -4.90%
==========================================
Files 47 40 -7
Lines 7259 6584 -675
Branches 580 478 -102
==========================================
- Hits 3696 3030 -666
+ Misses 3548 3537 -11
- Partials 15 17 +2
Flags with carried forward coverage won't be shown. Click here to find out more. β View full report in Codecov by Sentry. π New features to boost your workflow:
|
2 participants
Note
Enables collection to proceed irrespective of card status on non-authorization events, while retaining stricter checks for authorization.
transaction.requested, fetch card withstatus === "ACTIVE"and acquire mutex as beforetransaction.created/completed, fetch card byidwithout status constraint to permit collection when cards are frozen/lockedfindCardByIdhelper; inline Drizzle queries for cardsover-captures frozen debitto verify capture works when card becomesFROZENafter authorizationWritten by Cursor Bugbot for commit 3922e00. This will update automatically on new commits. Configure here.