feat(invoice): Invoice PDF generation & email delivery - #34
Conversation
Adds on-demand invoice PDF rendering via pdfkit, protected/public download endpoints, and merchant-triggered email delivery with the PDF attached. Nothing is persisted to disk or object storage — every request regenerates the PDF from live Prisma data. - generateInvoicePdf(invoice, merchant): pure, DB-free, unit-testable - GET /invoices/:id/pdf (merchant-owned) and GET /pay/:slug/pdf (public, reusing the existing 404/410 visibility rules) - email.service.ts: sendViaResend/sendViaSmtp accept Buffer attachments; sendInvoiceEmail() only sends when invoice.email is set - POST /invoices/:id/send (400 when no email on file) - storage/invoice-pdf.storage.ts: unused seam for a future persistence issue Closes ShadeProtocol#26 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds on-demand invoice PDF generation, authenticated and public PDF download routes, invoice email delivery with PDF attachments, shared invoice visibility checks, a mock storage seam, and integration/unit test coverage. ChangesInvoice PDF and Email Delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant InvoiceRoutes
participant InvoiceService
participant PdfService
participant EmailService
participant EmailProvider
Client->>InvoiceRoutes: Request invoice PDF or send invoice
InvoiceRoutes->>InvoiceService: Fetch invoice with merchant
InvoiceService-->>InvoiceRoutes: Invoice and merchant
InvoiceRoutes->>PdfService: Generate PDF
PdfService-->>InvoiceRoutes: PDF Buffer
InvoiceRoutes->>EmailService: Send invoice with PDF attachment
EmailService->>EmailProvider: Deliver email
EmailProvider-->>Client: Delivery response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
tests/unit/email.service.resend.test.tsParsing error: "parserOptions.project" has been provided for tests/unit/email.service.smtp.test.tsParsing error: "parserOptions.project" has been provided for tests/unit/email.service.test.tsParsing error: "parserOptions.project" has been provided for
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 |
|
This is ready for review. All acceptance criteria from #26 are implemented — PDF generation, protected/public download endpoints, email delivery with attachments, and the storage seam. Locally verified before pushing:
I noticed the CI workflows ( |
|
Re: the "Review failed" notice above — that's a transient error on CodeRabbit's own service ("An error occurred during the review process"), not related to this PR's content. Re-requesting it below. @coderabbitai review In the meantime, this PR is ready for maintainer review: PDF generation, download endpoints, email delivery with attachments, and the storage seam are all implemented per #26, with 221 passing tests and clean typecheck/lint locally. Still just waiting on a maintainer to approve the CI workflow run and on a review approval to unblock the merge. Thanks for taking a look! |
|
✅ Action performedReview finished.
|
|
Hey @codebestia, this one is ready whenever you get a chance to look at it. It closes #26 and covers the full spec: PDF generation, the protected and public download routes, email delivery with attachments, and the storage seam. 221 tests pass locally and CodeRabbit's review is running again after hitting a rate limit earlier. Two things need someone with write access on your end: approving the pending GitHub Actions run, and an approving review to unblock the merge. Happy to make changes if anything needs adjusting. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codebestia @Depo-dev could one of you review this when you get a chance? CI is green (tests, formatting, CodeRabbit all passing), just needs a review to unblock the merge. |
codebestia
left a comment
codebestia
left a comment
There was a problem hiding this comment.
Hello @angelraph
Please address the review changes.
Good job so far.
There was a problem hiding this comment.
The new assertInvoiceVisible helper was extracted but the original resolveInvoiceBySlug visibility check is missing the expiresAt range validation from the payload before extraction. Looking at the diff:
Lines 36-46 of the original had the full visibility check
The extracted assertInvoiceVisible (lines 14-27) correctly checks CANCELLED | PAID | REFUNDED and expiry
BUT resolveInvoiceBySlug at line 52 calls assertInvoiceVisible(invoice) after already having selected and returned a filtered view of the invoice
The issue: expiresAt is included in the Prisma select (line 22), so it's available—but confirmPayment (line 60 onward) still has the old inlined visibility check duplicated. This creates:
Logic duplication → maintenance burden and risk of divergence
Incomplete refactoring → confirmPayment doesn't use the new helper
Impact: While the specific routes work, the visibility rules are not consistently abstracted, risking future bugs when status/expiry logic changes.
Fix: Apply assertInvoiceVisible consistently in confirmPayment as well (lines 70-80 should be replaced with a call to assertInvoiceVisible(invoice)).
| const logoBuffer = decodeLogo(merchant.logo); | ||
| if (logoBuffer) { | ||
| try { | ||
| doc.image(logoBuffer, { fit: [80, 80] }); | ||
| doc.moveDown(); | ||
| } catch { | ||
| // Corrupt/undecodable image data — skip it rather than fail the render. | ||
| } |
There was a problem hiding this comment.
The try-catch silently swallows all errors from doc.image(), including:
Out-of-memory errors (if a malicious/oversized PNG is embedded)
File descriptor exhaustion
Other unexpected PDFKit failures
This masks real failures that should bubble up and be retried or logged.
Impact:
Silent failures make debugging production issues extremely difficult
No observability into PDF generation failures
Could mask security issues (e.g., DOS via malicious PNG payloads)
Fix: Catch only Error but inspect the message/type and only suppress image-specific errors:
There was a problem hiding this comment.
// invoice.routes.ts
router.get('/:id', getInvoiceController); // line 17
router.get('/:id/pdf', getInvoicePdfController); // line 18 (NEW)
// pay.routes.ts
router.get('/:slug', resolveInvoiceController); // line 10
router.get('/:slug/pdf', getInvoicePdfController); // line 11 (NEW)Express matches routes top-to-bottom, first-come-first-served. The route /:id on line 17 will intercept /:id/pdf on line 18 before it's evaluated, because Express splits on / and tries to match /:id first:
GET /api/v1/invoices/123/pdf → matches /:id with id=123, never reaches /:id/pdf
This means the PDF download will always 404 (invoice 123/pdf doesn't exist as an ID).
Impact: Public PDF download endpoint for authenticated merchants is completely broken.
Fix: Reorder routes in invoice.routes.ts to place more specific routes before generic ones:
router.get('/:id/pdf', getInvoicePdfController); // More specific first
router.post('/:id/send', sendInvoiceController); // More specific first
router.get('/:id', getInvoiceController); // Generic fallbackAddresses CodeRabbit review on PR ShadeProtocol#34: the bare catch around doc.image() hid all failures with no observability. Still skips embedding on a bad logo (so a corrupt merchant upload doesn't fail the whole invoice PDF), but now logs the error so real failures aren't invisible.
|
Addressed the review feedback:
Typecheck, lint, and the existing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/invoice-pdf.services.test.ts (1)
106-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a corrupt (but regex-matching) logo.
Current tests only cover a valid PNG and a non-data-URI logo. Add a case with well-formed base64 that isn't a valid image (e.g.
data:image/png;base64,YWJjZGVm) to exercise thedoc.image()catch branch and confirm it logs and continues instead of throwing — this is the exact path a past review flagged as a swallowed-error risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/invoice-pdf.services.test.ts` around lines 106 - 120, Add a unit test alongside the existing logo cases in generateInvoicePdf coverage using a data URI with valid base64 that is not a valid image, such as the specified abcdef payload. Assert the PDF remains valid and verify the image failure is logged, exercising the doc.image() catch path without allowing the error to escape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/email.service.ts`:
- Around line 122-146: Update the console-provider fallback in sendInvoiceEmail
to remove invoice.email from the stdout message; retain only the non-PII invoice
identifier and existing PDF size/context.
In `@tests/unit/email.service.test.ts`:
- Around line 1-6: Explicitly control EMAIL_PROVIDER across the affected test
modules before importing email.service: in tests/unit/email.service.test.ts,
ensure it is unset before the dynamic import and restore the prior value
afterward; in tests/unit/email.service.resend.test.ts and
tests/unit/email.service.smtp.test.ts, save and restore the environment value
around their module-load mutations. Keep sendInvoiceEmail’s existing test
behavior unchanged and apply the setup consistently to all three sites.
---
Nitpick comments:
In `@tests/unit/invoice-pdf.services.test.ts`:
- Around line 106-120: Add a unit test alongside the existing logo cases in
generateInvoicePdf coverage using a data URI with valid base64 that is not a
valid image, such as the specified abcdef payload. Assert the PDF remains valid
and verify the image failure is logged, exercising the doc.image() catch path
without allowing the error to escape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16ea8f20-cd24-485f-afeb-a274718281d6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
package.jsonsrc/controllers/invoice.controllers.tssrc/controllers/pay.controllers.tssrc/routes/invoice.routes.tssrc/routes/pay.routes.tssrc/services/email.service.tssrc/services/invoice-pdf.services.tssrc/services/invoice.services.tssrc/services/pay.services.tssrc/services/storage/invoice-pdf.storage.tstests/integration/auth.email-otp.test.tstests/integration/invoice.routes.test.tstests/integration/merchant.register.test.tstests/integration/pay.routes.test.tstests/unit/email.service.resend.test.tstests/unit/email.service.smtp.test.tstests/unit/email.service.test.tstests/unit/invoice-pdf.services.test.ts
…OVIDER across tests Addresses CodeRabbit review on ShadeProtocol#34: the console-provider fallback logged invoice.email directly; tests mutating process.env.EMAIL_PROVIDER at module scope could leak that value into sibling test files sharing a Jest worker. Also adds coverage for a corrupt-but-regex-matching logo data URI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codebestia Pushed a fix addressing the CodeRabbit review comments (PII in the console-provider log, EMAIL_PROVIDER isolation across test modules, and added logo-corruption test coverage). CI (test.yml / Code Formatting Check) is stuck in |
|
@codebestia Just a friendly nudge — the workflow run for the latest fix commit (909c75c) is still sitting in |
|
@codebestia All three review items are addressed in 909c75c:
Ready for another look whenever you have a chance. |
|
@angelraph Please fix the CI failure |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codebestia CI failure is fixed — pushed in 334b599 (prettier flagged the collapsed console.log line in The new workflow runs for this commit are stuck in |
codebestia
left a comment
codebestia
left a comment
There was a problem hiding this comment.
LGTM!
Nice Implementation.
Thank you for your contribution.
2 participants
Summary
Implements on-demand invoice PDF generation and email delivery as specified in #26 — no invoice document is ever persisted; every request/send regenerates the PDF from live Prisma data.
src/services/invoice-pdf.services.tsexportsgenerateInvoicePdf(invoice, merchant): Promise<Buffer>usingpdfkit. Pure function — no DB, filesystem, or network access. Renders merchant business name/logo (data-URI logos only, to keep the function side-effect free and deterministic — no fetch of remote URLs), description, amount + token (plus fiat breakdown forFIXED_FIATinvoices), status, payment slug, created/paid dates, and payer address when present.GET /invoices/:id/pdf(protected viaauthenticateMerchant, ownership-scoped — 404 for other merchants' invoices)GET /pay/:slug/pdf(public, reuses the same 404/410 visibility rules asGET /pay/:slugvia a sharedassertInvoiceVisiblehelper extracted inpay.services.ts)Content-Type: application/pdf,Content-Disposition: attachment) — nothing is written to disk or a bucket.email.service.ts'ssendViaResend/sendViaSmtpnow accept an optionalattachments: { filename, content: Buffer }[]. NewsendInvoiceEmail(invoice, merchant)generates the PDF fresh and sends it as an attachment — no-ops (no throw) wheninvoice.emailis unset.POST /invoices/:id/send(protected, ownership-scoped) triggers delivery; returns400with a clear error when the invoice has no email on file, rather than silently no-op-ing.src/services/storage/invoice-pdf.storage.tsadds theInvoicePdfStorageinterface +mockInvoicePdfStoragestub exactly as specified — compiles, but is not imported or called by any handler in this PR.No new Prisma models or fields were needed.
Test plan
generateInvoicePdfunit tests (tests/unit/invoice-pdf.services.test.ts) generate real PDFs from fixture data and assert valid%PDF-/%%EOFstructure, coveringFIXED_FIATpricing, paid invoices with payer/datePaid, data-URI logos, non-data-URI logos (gracefully skipped, no network call), and merchants with no branding.sendInvoiceEmailunit tests across all three providers:email.service.test.ts— console provider (default), no-op when no email, reflects live state per callemail.service.resend.test.ts— real attachment payload sent through a mocked Resend SDK (mocks only the third-party network client, not our logic), plus the Resend-error-propagates caseemail.service.smtp.test.ts— same via a mocked Nodemailer transportinvoice.routes.test.ts— added coverage forGET /invoices/:id/pdf(401/404/200 with real PDF body + headers) andPOST /invoices/:id/send(401/404/400-no-email/200-sends)pay.routes.test.ts(new) —GET /pay/:slug/pdfcovering 404, 410 (cancelled/paid/refunded), 410 expired, and 200 with a real PDF bodynpm test→ 28 suites / 221 tests passingtsc --noEmitclean (no new errors; pre-existing@stellar/stellar-sdktype noise unrelated to this change)eslint/prettierclean on all touched files🤖 Generated with Claude Code
Closes #26
Summary by CodeRabbit