✨ server: support many cards in the statement#893
Conversation
🦋 Changeset detectedLatest commit: 558ff97 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSwitches statement generation from a single-card, item-centric model to a purchases-by-card model: Activity API now queries credential cards, attaches cardId to items, groups purchases per card and separates payments; Statement component and tests updated to accept/render account, multiple cards with purchases, maturity, and payments for JSON/PDF output. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Activity API
participant DB as Database
participant Statement as Statement Component
participant Output as PDF/JSON Output
Client->>API: Request statement (account, format)
API->>DB: Query credential cards (id, lastFour)
DB-->>API: Cards list
API->>DB: Fetch transactions/items for account
DB-->>API: Transactions/items
API->>API: Build purchases (attach cardId, group by card)
API->>API: Extract payments
API->>Statement: Send account, cards[{lastFour,purchases[]}], maturity, payments
Statement->>Statement: Compute totals, format dates/currency
Statement->>Output: Render/serialize PDF or return JSON
Output-->>Client: Statement (PDF or JSON)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 unit tests (beta)
📝 Coding Plan
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 |
Summary of ChangesHello, 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 enables the generation of comprehensive user statements that can include transactions from multiple cards. The changes involve updating the data retrieval process in the API to fetch all relevant card data and refactoring the PDF rendering component to display this multi-card information in a structured and user-friendly format. This enhancement provides users with a more complete overview of their financial activity across all their associated cards. Highlights
Changelog
Activity
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-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #893 +/- ##
==========================================
+ Coverage 71.47% 71.92% +0.45%
==========================================
Files 212 212
Lines 8470 8582 +112
Branches 2776 2829 +53
==========================================
+ Hits 6054 6173 +119
+ Misses 2134 2132 -2
+ Partials 282 277 -5
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
server/utils/Statement.tsx (2)
99-111: 🧹 Nitpick | 🔵 TrivialDuplicate
cardTotalcalculation; consider extracting.The same
cardTotalreduction appears at lines 100-103 and 125-127. Pre-compute card totals once to avoid duplication and improve maintainability.♻️ Suggested refactor
+ const cardsWithTotals = cards.map((card) => ({ + ...card, + total: card.purchases.reduce((s, p) => s + p.installments.reduce((a, i) => a + i.amount, 0), 0), + })); + const totalSpent = cardsWithTotals.reduce((sum, card) => sum + card.total, 0); - const totalSpent = cards.reduce( - (sum, card) => - sum + - card.purchases.reduce((s, p) => s + p.installments.reduce((a, installment) => a + installment.amount, 0), 0), - 0, - );Then use
cardsWithTotalsin both the summary and per-card sections, referencingcard.totaldirectly.Also applies to: 124-128
105-105:⚠️ Potential issue | 🟡 MinorConsider a more unique React key.
Using
card.lastFouras the key could cause issues if two cards share the same last four digits. Since the component doesn't receive a unique card ID, consider using the array index as a fallback:-<View key={card.lastFour} style={styles.summaryRow}> +<View key={`${card.lastFour}-${index}`} style={styles.summaryRow}>Apply similarly at line 130.
Also applies to: 130-130
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85ea069e-c24d-4dbc-919a-71cb4c2744bf
📒 Files selected for processing (5)
.changeset/soft-trains-dig.mdserver/api/activity.tsserver/test/api/activity.test.tsserver/test/utils/statement.test.tsserver/utils/Statement.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/utils/Statement.tsx (1)
99-128: 🧹 Nitpick | 🔵 TrivialDuplicate
cardTotalcalculation.The same calculation appears at lines 100-103 (Summary section) and lines 125-128 (per-card section). Consider computing card totals once during initial data processing.
Additionally, using
card.lastFouras the React key (lines 105, 130) could cause issues if two cards share the same last four digits.♻️ Suggested refactor
+ const cardsWithTotals = cards.map((card) => ({ + ...card, + total: card.purchases.reduce((s, p) => s + p.installments.reduce((a, i) => a + i.amount, 0), 0), + })); + const totalSpent = cardsWithTotals.reduce((sum, card) => sum + card.total, 0); - const totalSpent = cards.reduce( - (sum, card) => - sum + - card.purchases.reduce((s, p) => s + p.installments.reduce((a, installment) => a + installment.amount, 0), 0), - 0, - );Then reference
cardsWithTotalsandcard.totaldirectly in both sections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17e1f6dc-adbd-449f-9f30-770174c13e19
📒 Files selected for processing (5)
.changeset/soft-trains-dig.mdserver/api/activity.tsserver/test/api/activity.test.tsserver/test/utils/statement.test.tsserver/utils/Statement.tsx
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d85aefd0f2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| cards: purchases.map(({ id, lastFour }) => ({ | ||
| lastFour, | ||
| purchases: (purchasesByCard.get(id) ?? []).map(({ cardId: _, ...rest }) => rest), |
There was a problem hiding this comment.
In the maturity/PDF path, purchases is populated from a card query whose relation filter only trims each card's transactions; it does not remove parent cards with no matching activity. Because this mapper renders every returned card, any account with multiple cards but purchases on only one of them gets extra Card **** … purchases sections and zero-dollar summary rows for the idle cards, which makes the statement misleading and unnecessarily long.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| 0, | ||
| ); | ||
| return ( | ||
| <View key={card.lastFour} style={styles.summaryRow}> |
There was a problem hiding this comment.
🟡 React key collision using non-unique card.lastFour in Statement component
The Statement component uses card.lastFour as the React key in two separate .map() calls (lines 105 and 130). The lastFour field is not guaranteed to be unique — two cards can share the same last four digits (e.g., a reissued card, or cards from different issuers). When keys collide, @react-pdf/renderer's reconciler will silently drop or merge sibling elements, causing card sections and summary rows to be missing or incorrect in the generated PDF.
The root cause is that server/api/activity.ts:516-518 has access to the unique card id but only passes lastFour to the Statement component, making it impossible to use a unique key downstream.
Prompt for agents
In server/api/activity.ts at line 516-519, pass through the card id alongside lastFour:
cards: purchases.map(({ id, lastFour }) => ({
id,
lastFour,
purchases: (purchasesByCard.get(id) ?? []).map(({ cardId: _, ...rest }) => rest),
})),
Then update server/utils/Statement.tsx to include `id: string` in the cards type definition (around line 13-21), and use `card.id` instead of `card.lastFour` as the React key at lines 105 and 130:
<View key={card.id} style={styles.summaryRow}> (line 105)
<View key={card.id} style={styles.section}> (line 130)
Was this helpful? React with 👍 or 👎 to provide feedback.
| cards: purchases.map(({ id, lastFour }) => ({ | ||
| lastFour, | ||
| purchases: (purchasesByCard.get(id) ?? []).map(({ cardId: _, ...rest }) => rest), | ||
| })), |
There was a problem hiding this comment.
🚩 Cards with zero purchases still rendered in the PDF statement
When purchases is built via the database query (server/api/activity.ts:273-282), ALL cards for the credential are returned regardless of whether they have matching transactions. Cards with no purchases for the maturity period will still appear in the PDF as empty sections with $0.00 totals in both the summary and detail views. The old code only tracked cards that actually had matching transactions (statementCards was derived from transaction card IDs). This is likely a deliberate design choice for the new multi-card layout, but could produce noisy PDFs if a user has many cards but only uses one.
Was this helpful? React with 👍 or 👎 to provide feedback.
| cards: purchases.map(({ id, lastFour }) => ({ | ||
| lastFour, | ||
| purchases: (purchasesByCard.get(id) ?? []).map(({ cardId: _, ...rest }) => rest), | ||
| })), |
There was a problem hiding this comment.
🟡 Cards with no purchases for the maturity period appear as empty $0.00 sections in the PDF statement
The query at server/api/activity.ts:273-282 fetches ALL cards for the credential via eq(cards.credentialId, credentialId). The nested where: arrayOverlaps(transactions.hashes, hashes) only filters the child transactions, not the parent cards. Cards that have no matching transactions are returned with transactions: []. These empty cards flow unfiltered into the statement at line 516-519 and appear in the PDF as empty card sections with $0.00 totals and table headers with no rows. The old code derived the card list from actual matching transactions (statementCards = [...new Set(statementTransactions.map(({ cardId }) => cardId))] at the old line 283), which naturally excluded cards without purchases.
| cards: purchases.map(({ id, lastFour }) => ({ | |
| lastFour, | |
| purchases: (purchasesByCard.get(id) ?? []).map(({ cardId: _, ...rest }) => rest), | |
| })), | |
| cards: purchases | |
| .filter(({ id }) => purchasesByCard.has(id)) | |
| .map(({ id, lastFour }) => ({ | |
| lastFour, | |
| purchases: (purchasesByCard.get(id) ?? []).map(({ cardId: _, ...rest }) => rest), | |
| })), |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary by CodeRabbit
New Features
UI
Tests