✨ server: support multiple cards in statement#920
Conversation
🦋 Changeset detectedLatest commit: aae69c9 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 |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughReworks activity API querying and PDF statement generation to support multiple cards per account, changes Statement component props/formatting, updates tests, adds a changeset for a patch release, and adds a firehose warmup phase to test DB startup. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/API caller
participant API as Activity API
participant DB as Database (Drizzle)
participant Processor as Statement Builder
participant Renderer as Statement.tsx (PDF renderer)
participant FS as File System / Response
Client->>API: GET /activity?maturity=...&pdf=true
API->>DB: query cards with with.transactions.where(arrayOverlaps(...))
DB-->>API: cards + matching transactions
API->>Processor: build purchases, cardLookup, payments, grouped cards
Processor-->>Renderer: render Statement({ account, cards, maturity, payments })
Renderer-->>FS: emit PDF bytes
FS-->>API: PDF buffer / path
API-->>Client: 200 + PDF
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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)
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 support for multiple cards within the generated PDF statements. Key changes include refactoring the activity API to group transactions by card using Map.groupBy, updating the Statement component with a professional table-based layout and SVG branding, and enhancing the test suite to cover multi-card scenarios and financial calculations like discounts and penalties. I have no feedback to provide.
|
✅ All tests passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/test/api/activity.test.ts (1)
257-295: 🧹 Nitpick | 🔵 TrivialAssert the PDF grouping, not just the MIME type.
The new
purchasesByCardlogic only runs in this PDF path, but these tests still stop atcontent-typeandbyteLength. A regression that drops one card or groups every purchase under the same card would still pass.Please assert both last-fours are present in the rendered output, or mock
Statementand inspect the props passed intorenderToBuffer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 141b917e-cca7-4de8-9649-498061fb41a4
📒 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
nfmelendez
force-pushed
the
statement-base
branch
from
3639b18 to
bd64f38
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
server/api/activity.ts (2)
268-272:⚠️ Potential issue | 🔴 CriticalReplace the iterator-helper chains with
Array.from()first.This reintroduces iterator helpers on
Mapiterators..entries()/.values()do not expose.filter(),.map(), or.toArray()on the current Node runtime this repo targets, so both paths will throw before the statement payload is built.Proposed fix
- const hashes = borrows - .entries() - .filter(([_, { events }]) => events.some(({ maturity: m }) => Number(m) === maturity)) - .map(([hash]) => hash) - .toArray(); + const hashes = Array.from(borrows.entries()) + .filter(([_, { events }]) => events.some(({ maturity: m }) => Number(m) === maturity)) + .map(([hash]) => hash); ... - const installments = item.operations - .reduce((accumulator, operation) => { + const installments = Array.from( + item.operations.reduce((accumulator, operation) => { if ("borrow" in operation) { if ("installments" in operation.borrow) { const events = borrows?.get(operation.transactionHash)?.events; if (!events) return accumulator; const sortedInstallments = events.toSorted((a, b) => Number(a.maturity) - Number(b.maturity)); @@ } return accumulator; - }, new Map<string, { amount: number; current: number; total: number }>()) - .values() - .toArray(); + }, new Map<string, { amount: number; current: number; total: number }>()).values(), + );Run this to confirm the repo runtime pin and the remaining iterator-helper chains:
#!/bin/bash set -euo pipefail echo "== runtime configuration ==" fd '^(package\.json|\.nvmrc|\.tool-versions)$' -x sh -c 'echo "--- $1"; sed -n "1,200p" "$1"' sh {} fd '.*\.(yml|yaml)$' .github/workflows -x sh -c 'echo "--- $1"; rg -n "setup-node|node-version" "$1" || true' sh {} echo echo "== iterator-helper usages in server/api/activity.ts ==" nl -ba server/api/activity.ts | sed -n '268,272p;462,463p'Also applies to: 462-463
517-523:⚠️ Potential issue | 🟡 MinorSort
statement.cardsbefore mapping.
findMany()has no explicit order, so multi-card PDFs can flip the summary and per-card section order between requests.Proposed fix
cards: purchases .filter(({ id }) => purchasesByCard.has(id)) + .toSorted((a, b) => a.lastFour.localeCompare(b.lastFour) || a.id.localeCompare(b.id)) .map(({ id, lastFour }) => ({ id, lastFour, purchases: (purchasesByCard.get(id) ?? []).map(({ cardId: _, ...rest }) => rest), })),
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 122cf4a5-cfe0-4b91-ae26-8c245661aaf7
📒 Files selected for processing (6)
.changeset/soft-trains-dig.mdserver/api/activity.tsserver/test/api/activity.test.tsserver/test/database.tsserver/test/utils/statement.test.tsserver/utils/Statement.tsx
nfmelendez
force-pushed
the
statement-base
branch
from
bd64f38 to
aae69c9
Compare
1 participant
Summary by CodeRabbit
New Features
Tests