feat(emails): valuation-report email on valuation complete (chat#1867) - #773
Conversation
When a playcount snapshot run reaches its terminal done state, email the owning account a deterministic valuation summary (recoupable/chat#1867): headline valuation band (same computeValuationBand model as the catalog measurements endpoint), lifetime Spotify streams, songs/albums measured, and a deep link to chat.recoupable.dev/catalogs/{id} (app root when no catalog was claimed). House-style template mirroring the scrape digest (DESIGN.md achromatic chrome, tables + inline styles). Idempotent per snapshot run: a snapshot_id marker in email_send_log raw_body guards re-invocations, plus a Resend idempotency key (valuation-report/<id>) for racing retries within 24h. The send step is best-effort and never fails a completed run; accounts with no email are skipped and every attempt is logged to email_send_log. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a valuation report email pipeline with release-level valuation data, escaped HTML rendering, idempotent sending, catalog enrichment, and deferred delivery after valuation completion. ChangesValuation email delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 11 files
Confidence score: 2/5
- In
lib/emails/valuationReport/sendValuationReportEmail.ts, sending multipleaccount_emailsin a singleto:header can expose recipients’ email addresses to each other, creating a concrete privacy/compliance risk in production — switch to the project’s established per-recipient/BCC multi-recipient pattern. - In
lib/emails/valuationReport/formatCompactNumber.ts, values near unit boundaries can render as1000K/1000Mbecause rounding happens after tier selection, which can misstate reported metrics in customer emails — re-evaluate the tier after rounding (or cap pre-round values) to prevent cross-tier artifacts. - In
lib/emails/valuationReport/renderValuationReportHtml.ts, the static subject ("Your catalog valuation is ready") is also used for degraded link-only emails with no valuation block, which can mislead users and increase confusion/support churn — branch the subject based on whetherparams.valuationis present. - In
lib/emails/valuationReport/__tests__/sendValuationReportEmail.test.ts, the zero-streams degraded path is untested, so regressions in the “no valuation / no stream-song metrics” rendering could ship unnoticed — add a fixture asserting thetotalStreams: 0output contract.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/emails/valuationReport/formatCompactNumber.ts">
<violation number="1" location="lib/emails/valuationReport/formatCompactNumber.ts:9">
P2: Values just under a tier boundary (e.g. 999,950–999,999 or 999,950,000–999,999,999) round to "1000.0" after `toFixed(1)` but the unit was already fixed to the lower tier, producing display bugs like "1000K" or "1000M" instead of "1M"/"1B" in the valuation email. Since this formatter is used for both lifetime stream counts and dollar valuations, this narrow-but-plausible input range would show an inconsistent number format to recipients.</violation>
</file>
<file name="lib/emails/valuationReport/renderValuationReportHtml.ts">
<violation number="1" location="lib/emails/valuationReport/renderValuationReportHtml.ts:15">
P3: The email subject `"Your catalog valuation is ready"` is static and always used, even on the degraded link-only path where `params.valuation` is undefined and no valuation is shown in the body — this creates a mismatch between subject and content for zero-stream catalogs.</violation>
</file>
<file name="lib/emails/valuationReport/sendValuationReportEmail.ts">
<violation number="1" location="lib/emails/valuationReport/sendValuationReportEmail.ts:84">
P2: When an account has multiple rows in account_emails, all addresses are placed in the same `to:` header, so each recipient can see every other recipient's email address. The codebase's established pattern for multi-recipient sends (chat#1855) is `to: [RECOUP_FROM_EMAIL], bcc: emails` — worth applying the same pattern here.</violation>
</file>
<file name="lib/emails/valuationReport/__tests__/sendValuationReportEmail.test.ts">
<violation number="1" location="lib/emails/valuationReport/__tests__/sendValuationReportEmail.test.ts:45">
P2: Add a test for the zero-streams degrade case. When `selectCatalogMeasurementsAggregate` returns `totalStreams: 0` with a valid catalog, the email should render without a valuation block and without stream/song metrics — effectively a link-only email similar to the no-catalog fallback but with the catalog name present. This ensures the `> 0` guard in `sendValuationReportEmail.ts` stays exercised and the degrade behavior is pinned.</violation>
</file>
Architecture diagram
sequenceDiagram
participant WF as playcountSnapshotWorkflow
participant Step as sendValuationEmailStep
participant Send as sendValuationReportEmail
participant DB as Supabase
participant Resend as Resend API
participant Email as User Inbox
Note over WF,Email: Happy path: snapshot completes, email sent
WF->>Step: await sendValuationEmailStep(snapshot)
Step->>Send: sendValuationReportEmail(snapshot)
Send->>DB: selectValuationEmailSendLog(snapshot.id)
DB-->>Send: null (no prior send)
Send->>DB: selectAccountEmails(accountIds: snapshot.account)
DB-->>Send: [{email: "digital@epitaph.com"}]
alt catalog is not null
Send->>DB: selectCatalogById(snapshot.catalog)
DB-->>Send: {name: "Epitaph Catalog"}
Send->>DB: selectCatalogMeasurementsAggregate(catalogId)
DB-->>Send: {totalStreams: 3_480_000, measuredSongCount: 112}
Send->>DB: getCatalogEarliestReleaseDate(catalogId)
DB-->>Send: "2016-01-01"
Send->>Send: computeValuationBand({totalStreams, earliestReleaseDate})
Note over Send: Returns {valuation: {low,mid,high}, catalogAgeYears}
else catalog is null or enrichment fails
Note over Send: Degrade to link-only email<br/>(no valuation/streams rows)
end
Send->>Send: renderValuationReportHtml(params)
Note over Send: Deterministic HTML template
Send->>Resend: POST /emails (idempotencyKey: "valuation-report/snap_1")
Resend-->>Send: {id: "resend_1"}
Send->>DB: logEmailAttempt({status: "sent", rawBody: '{...snapshot_id:"snap_1"...}'})
DB-->>Send: OK
Send-->>Step: {sent: true, resendId: "resend_1"}
Step-->>WF: void (never throws)
Note over WF,Email: Unhappy path: already sent (idempotency)
WF->>Step: sendValuationEmailStep(snapshot) [retry]
Step->>Send: sendValuationReportEmail(snapshot)
Send->>DB: selectValuationEmailSendLog(snapshot.id)
DB-->>Send: {id: "log_1"} (prior sent row found)
Send-->>Step: {sent: false, skipped: "already_sent"}
Step-->>WF: void
Note over WF,Email: Degraded path: Resend failure
Send->>Resend: POST /emails
Resend-->>Send: NextResponse 502 {error: "Failed to send"}
Send->>DB: logEmailAttempt({status: "send_failed"})
Send-->>Step: {sent: false, error: "Failed to send email"}
Step->>Step: console.error (logged, no throw)
Step-->>WF: void (snapshot remains "done")
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| export function formatCompactNumber(n: number): string { | ||
| const rounded = Math.round(n); | ||
| if (rounded < 1000) return String(rounded); | ||
| const [value, unit] = |
There was a problem hiding this comment.
P2: Values just under a tier boundary (e.g. 999,950–999,999 or 999,950,000–999,999,999) round to "1000.0" after toFixed(1) but the unit was already fixed to the lower tier, producing display bugs like "1000K" or "1000M" instead of "1M"/"1B" in the valuation email. Since this formatter is used for both lifetime stream counts and dollar valuations, this narrow-but-plausible input range would show an inconsistent number format to recipients.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/valuationReport/formatCompactNumber.ts, line 9:
<comment>Values just under a tier boundary (e.g. 999,950–999,999 or 999,950,000–999,999,999) round to "1000.0" after `toFixed(1)` but the unit was already fixed to the lower tier, producing display bugs like "1000K" or "1000M" instead of "1M"/"1B" in the valuation email. Since this formatter is used for both lifetime stream counts and dollar valuations, this narrow-but-plausible input range would show an inconsistent number format to recipients.</comment>
<file context>
@@ -0,0 +1,16 @@
+export function formatCompactNumber(n: number): string {
+ const rounded = Math.round(n);
+ if (rounded < 1000) return String(rounded);
+ const [value, unit] =
+ rounded < 1_000_000
+ ? [rounded / 1000, "K"]
</file context>
| const result = await sendEmailWithResend( | ||
| { from: RECOUP_FROM_EMAIL, to: emails, subject, html }, | ||
| { idempotencyKey: `valuation-report/${snapshot.id}` }, | ||
| ); |
There was a problem hiding this comment.
P2: When an account has multiple rows in account_emails, all addresses are placed in the same to: header, so each recipient can see every other recipient's email address. The codebase's established pattern for multi-recipient sends (chat#1855) is to: [RECOUP_FROM_EMAIL], bcc: emails — worth applying the same pattern here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/valuationReport/sendValuationReportEmail.ts, line 84:
<comment>When an account has multiple rows in account_emails, all addresses are placed in the same `to:` header, so each recipient can see every other recipient's email address. The codebase's established pattern for multi-recipient sends (chat#1855) is `to: [RECOUP_FROM_EMAIL], bcc: emails` — worth applying the same pattern here.</comment>
<file context>
@@ -0,0 +1,103 @@
+ subject,
+ });
+
+ const result = await sendEmailWithResend(
+ { from: RECOUP_FROM_EMAIL, to: emails, subject, html },
+ { idempotencyKey: `valuation-report/${snapshot.id}` },
</file context>
| const result = await sendEmailWithResend( | |
| { from: RECOUP_FROM_EMAIL, to: emails, subject, html }, | |
| { idempotencyKey: `valuation-report/${snapshot.id}` }, | |
| ); | |
| const result = await sendEmailWithResend( | |
| { from: RECOUP_FROM_EMAIL, to: [RECOUP_FROM_EMAIL], bcc: emails, subject, html }, | |
| { idempotencyKey: `valuation-report/${snapshot.id}` }, | |
| ); |
| id: "cat_1", | ||
| name: "Epitaph Catalog", | ||
| } as Tables<"catalogs">); | ||
| vi.mocked(selectCatalogMeasurementsAggregate).mockResolvedValue({ |
There was a problem hiding this comment.
P2: Add a test for the zero-streams degrade case. When selectCatalogMeasurementsAggregate returns totalStreams: 0 with a valid catalog, the email should render without a valuation block and without stream/song metrics — effectively a link-only email similar to the no-catalog fallback but with the catalog name present. This ensures the > 0 guard in sendValuationReportEmail.ts stays exercised and the degrade behavior is pinned.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/valuationReport/__tests__/sendValuationReportEmail.test.ts, line 45:
<comment>Add a test for the zero-streams degrade case. When `selectCatalogMeasurementsAggregate` returns `totalStreams: 0` with a valid catalog, the email should render without a valuation block and without stream/song metrics — effectively a link-only email similar to the no-catalog fallback but with the catalog name present. This ensures the `> 0` guard in `sendValuationReportEmail.ts` stays exercised and the degrade behavior is pinned.</comment>
<file context>
@@ -0,0 +1,125 @@
+ id: "cat_1",
+ name: "Epitaph Catalog",
+ } as Tables<"catalogs">);
+ vi.mocked(selectCatalogMeasurementsAggregate).mockResolvedValue({
+ measuredSongCount: 112,
+ totalStreams: 3_480_000,
</file context>
| catalogAgeYears?: number; | ||
| }; | ||
|
|
||
| const SUBJECT = "Your catalog valuation is ready"; |
There was a problem hiding this comment.
P3: The email subject "Your catalog valuation is ready" is static and always used, even on the degraded link-only path where params.valuation is undefined and no valuation is shown in the body — this creates a mismatch between subject and content for zero-stream catalogs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/valuationReport/renderValuationReportHtml.ts, line 15:
<comment>The email subject `"Your catalog valuation is ready"` is static and always used, even on the degraded link-only path where `params.valuation` is undefined and no valuation is shown in the body — this creates a mismatch between subject and content for zero-stream catalogs.</comment>
<file context>
@@ -0,0 +1,83 @@
+ catalogAgeYears?: number;
+};
+
+const SUBJECT = "Your catalog valuation is ready";
+
+/**
</file context>
There was a problem hiding this comment.
KISS
- actual: lib/supabase/email_send_log/selectValuationEmailSendLog.ts
- required: lib/supabase/email_send_log/selectEmailSendLog.ts (you should be able to use a generic supabase lib and pass in the params you want to filter the query on).
There was a problem hiding this comment.
Done in de095912. Replaced selectValuationEmailSendLog with a generic selectEmailSendLog({ status, rawBodyLike, limit }) — the dedup guard now passes the snapshot_id marker as a filter. Reusable for any email-type idempotency check. Tests: generic select 4/4, sendValuationReportEmail 5/5. Also updated the branch onto main.
…selectEmailSendLog (KISS, review feedback)
Per PR review: instead of a valuation-specific email_send_log select, use one
generic selectEmailSendLog({ status, rawBodyLike, limit }) and pass the filters.
sendValuationReportEmail's dedup guard now calls it with the snapshot_id marker.
Reusable for other email-type idempotency checks. Tests updated (generic select
4/4, sendValuationReportEmail 5/5).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
lib/emails/valuationReport/sendValuationReportEmail.ts (1)
33-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFunction combines several responsibilities across ~78 lines.
Dedup check, recipient discovery, catalog enrichment, rendering, sending, and logging are all inlined into one function. The enrichment block (Lines 59-80) is a self-contained, independently testable unit and a natural extraction point, per the "single responsibility per function" and "under 50 lines" guidance for
lib/**/*.ts.As per path instructions,
lib/**/*.tsshould keep "Single responsibility per function" and functions "under 50 lines."🤖 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 `@lib/emails/valuationReport/sendValuationReportEmail.ts` around lines 33 - 110, Extract the catalog enrichment logic from sendValuationReportEmail into a dedicated helper, including the selectCatalogById, selectCatalogMeasurementsAggregate, computeValuationBand, and error-handling behavior. Have the main function invoke that helper to populate ValuationReportEmailParams while preserving existing defaults and failure-tolerant logging, reducing sendValuationReportEmail below 50 lines.lib/supabase/email_send_log/selectEmailSendLog.ts (1)
25-29: 🚀 Performance & Scalability | 🔵 TrivialLeading-wildcard LIKE scan on a growing shared table.
raw_body LIKE '%...%'can't use a standard B-tree index, so every idempotency check (now reused across email types per the docstring) does a full scan ofemail_send_log. Worth considering a dedicated indexed idempotency-key column, or at minimum a trigram (pg_trgm) GIN index onraw_body, as this table grows.🤖 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 `@lib/supabase/email_send_log/selectEmailSendLog.ts` around lines 25 - 29, Update the idempotency lookup in selectEmailSendLog to avoid relying on the leading-wildcard rawBodyLike filter over raw_body; prefer a dedicated indexed idempotency-key column and query it directly, or add and use the appropriate pg_trgm GIN index when raw-body matching must remain supported. Preserve the existing status and limit filtering behavior.
🤖 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 `@lib/emails/valuationReport/formatCompactNumber.ts`:
- Around line 6-16: Update formatCompactNumber to re-check tier thresholds after
rounding the scaled value, so values that format to 1000K or 1000M roll over to
the next unit and render as 1M or 1B. Preserve the existing formatting for
values below each rollover boundary and avoid emitting a 1000-prefixed
lower-tier result.
In `@lib/emails/valuationReport/sendValuationReportEmail.ts`:
- Around line 83-108: Remove the recipient list from the rawBody payload
constructed in sendValuationReportEmail, while preserving snapshot_id and the
other fields used for email logging or deduplication. Keep both logEmailAttempt
calls unchanged so accountId remains available for recipient lookup without
persisting email addresses.
- Around line 91-94: Add an HTTP-level timeout to the sendEmailWithResend call
in the valuation report email workflow, passing an abort signal through its
request options (using the appropriate type assertion if required). Keep the
existing idempotency key and email payload unchanged, and choose a bounded
timeout that prevents a hung Resend request from blocking task completion.
In `@lib/supabase/email_send_log/selectEmailSendLog.ts`:
- Around line 22-29: Update selectEmailSendLog so rawBodyLike is escaped before
constructing the LIKE pattern, treating caller-supplied % and _ characters as
literals while preserving the surrounding substring matching behavior. Apply the
escaping in the filters.rawBodyLike branch before calling query.like.
---
Nitpick comments:
In `@lib/emails/valuationReport/sendValuationReportEmail.ts`:
- Around line 33-110: Extract the catalog enrichment logic from
sendValuationReportEmail into a dedicated helper, including the
selectCatalogById, selectCatalogMeasurementsAggregate, computeValuationBand, and
error-handling behavior. Have the main function invoke that helper to populate
ValuationReportEmailParams while preserving existing defaults and
failure-tolerant logging, reducing sendValuationReportEmail below 50 lines.
In `@lib/supabase/email_send_log/selectEmailSendLog.ts`:
- Around line 25-29: Update the idempotency lookup in selectEmailSendLog to
avoid relying on the leading-wildcard rawBodyLike filter over raw_body; prefer a
dedicated indexed idempotency-key column and query it directly, or add and use
the appropriate pg_trgm GIN index when raw-body matching must remain supported.
Preserve the existing status and limit filtering behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8628020-8262-4365-a6f4-9349cda540e9
⛔ Files ignored due to path filters (5)
app/workflows/__tests__/sendValuationEmailStep.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included byapp/**lib/emails/valuationReport/__tests__/formatCompactNumber.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/valuationReport/__tests__/renderValuationReportHtml.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/valuationReport/__tests__/sendValuationReportEmail.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/supabase/email_send_log/__tests__/selectEmailSendLog.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (6)
app/workflows/playcountSnapshotWorkflow.tsapp/workflows/sendValuationEmailStep.tslib/emails/valuationReport/formatCompactNumber.tslib/emails/valuationReport/renderValuationReportHtml.tslib/emails/valuationReport/sendValuationReportEmail.tslib/supabase/email_send_log/selectEmailSendLog.ts
| export function formatCompactNumber(n: number): string { | ||
| const rounded = Math.round(n); | ||
| if (rounded < 1000) return String(rounded); | ||
| const [value, unit] = | ||
| rounded < 1_000_000 | ||
| ? [rounded / 1000, "K"] | ||
| : rounded < 1_000_000_000 | ||
| ? [rounded / 1_000_000, "M"] | ||
| : [rounded / 1_000_000_000, "B"]; | ||
| return `${value.toFixed(1).replace(/\.0$/, "")}${unit}`; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Boundary rounding can leak "1000" into the wrong tier.
For rounded just under a tier threshold (e.g. 999,950–999,999), rounded / 1000 is 999.95–999.999; .toFixed(1) rounds this up to "1000.0", which after the trailing-.0 strip renders as "1000K" instead of rolling over to "1M". The same happens at the M→B boundary.
🐛 Proposed fix to re-check for tier rollover
export function formatCompactNumber(n: number): string {
const rounded = Math.round(n);
if (rounded < 1000) return String(rounded);
- const [value, unit] =
- rounded < 1_000_000
- ? [rounded / 1000, "K"]
- : rounded < 1_000_000_000
- ? [rounded / 1_000_000, "M"]
- : [rounded / 1_000_000_000, "B"];
- return `${value.toFixed(1).replace(/\.0$/, "")}${unit}`;
+ const tiers: Array<[number, string]> = [
+ [1_000_000_000, "B"],
+ [1_000_000, "M"],
+ [1_000, "K"],
+ ];
+ for (const [divisor, unit] of tiers) {
+ if (rounded >= divisor) {
+ const formatted = (rounded / divisor).toFixed(1).replace(/\.0$/, "");
+ // toFixed can round e.g. 999.999K up to "1000.0" — defer to the next tier up.
+ if (formatted !== "1000") return `${formatted}${unit}`;
+ }
+ }
+ return `${(rounded / 1_000_000_000).toFixed(1).replace(/\.0$/, "")}B`;
}🤖 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 `@lib/emails/valuationReport/formatCompactNumber.ts` around lines 6 - 16,
Update formatCompactNumber to re-check tier thresholds after rounding the scaled
value, so values that format to 1000K or 1000M roll over to the next unit and
render as 1M or 1B. Preserve the existing formatting for values below each
rollover boundary and avoid emitting a 1000-prefixed lower-tier result.
| const result = await sendEmailWithResend( | ||
| { from: RECOUP_FROM_EMAIL, to: emails, subject, html }, | ||
| { idempotencyKey: `valuation-report/${snapshot.id}` }, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'sendEmail.ts' lib/emails
cat -n lib/emails/sendEmail.tsRepository: recoupable/api
Length of output: 1496
🏁 Script executed:
#!/bin/bash
set -e
echo "== files =="
fd -t f 'sendValuationReportEmail.ts|playcountSnapshotWorkflow.ts|client.ts' lib
echo "== outlines =="
for f in lib/emails/sendValuationReportEmail.ts lib/emails/client.ts lib/workflows/playcountSnapshotWorkflow.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
ast-grep outline "$f" || true
fi
done
echo "== relevant sections =="
cat -n lib/emails/sendValuationReportEmail.ts | sed -n '1,140p'
echo "---"
cat -n lib/emails/client.ts | sed -n '1,160p'
echo "---"
cat -n lib/workflows/playcountSnapshotWorkflow.ts | sed -n '1,90p'Repository: recoupable/api
Length of output: 1291
🌐 Web query:
Resend SDK resend.emails.send timeout options CreateEmailRequestOptions Node client
💡 Result:
In the Resend Node.js SDK, the resend.emails.send method accepts a second argument, options, which is of type CreateEmailRequestOptions [1]. While CreateEmailRequestOptions is used to configure aspects of the HTTP request, it does not officially include an explicit timeout property in its TypeScript type definition [2]. However, because the SDK forwards these options to the underlying fetch call, you can pass an AbortSignal at runtime to implement request timeouts [2]. To implement a timeout, you can use AbortSignal.timeout(ms) within the options object [2]: javascript await resend.emails.send( { from: 'Acme <onboarding@resend.dev>', to: ['delivered@resend.dev'], subject: 'hello world', html: '<p>it works!</p>', }, { // `@ts-ignore`: signal may not be in official CreateEmailRequestOptions yet signal: AbortSignal.timeout(5000), // 5 second timeout } ); As noted in community discussions, although the SDK performs this pass-through correctly at runtime, you may need to use a type assertion or @ts-ignore because the signal property is not explicitly defined in the current CreateEmailRequestOptions interface [2]. Always ensure you are using a modern Node.js environment that supports AbortSignal [2].
Citations:
- 1: https://cdn.jsdelivr.net/npm/resend@4.3.0/dist/index.d.ts
- 2: Support `AbortSignal` in request options resend/resend-node#958
Add a timeout around the Resend email send.
sendEmailWithResend waits on resend.emails.send() without a bound, and this workflow awaits the call before considering the task complete. Add an HTTP-level timeout, for example via options: { signal: AbortSignal.timeout(ms) } (with a type assertion if needed), so a hung Resend request cannot stall workflow completion.
🤖 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 `@lib/emails/valuationReport/sendValuationReportEmail.ts` around lines 91 - 94,
Add an HTTP-level timeout to the sendEmailWithResend call in the valuation
report email workflow, passing an abort signal through its request options
(using the appropriate type assertion if required). Keep the existing
idempotency key and email payload unchanged, and choose a bounded timeout that
prevents a hung Resend request from blocking task completion.
| export async function selectEmailSendLog( | ||
| filters: SelectEmailSendLogFilters = {}, | ||
| ): Promise<Tables<"email_send_log">[]> { | ||
| let query = supabase.from("email_send_log").select("*"); | ||
|
|
||
| if (filters.status) query = query.eq("status", filters.status); | ||
| if (filters.rawBodyLike) query = query.like("raw_body", `%${filters.rawBodyLike}%`); | ||
| if (filters.limit) query = query.limit(filters.limit); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unescaped LIKE wildcards in rawBodyLike.
%/_ in the caller-supplied substring are passed straight into like() without escaping. The current caller's marker already contains a literal _ ("snapshot_id"), which SQL LIKE treats as a single-char wildcard — harmless today only because no other row is likely to collide, but this function is explicitly designed to be "reused for any email-type idempotency/dedup check," so future markers containing _/% risk false-positive dedup matches.
🛡️ Proposed fix to escape LIKE special characters
if (filters.status) query = query.eq("status", filters.status);
- if (filters.rawBodyLike) query = query.like("raw_body", `%${filters.rawBodyLike}%`);
+ if (filters.rawBodyLike) {
+ const escaped = filters.rawBodyLike.replace(/[\\%_]/g, "\\$&");
+ query = query.like("raw_body", `%${escaped}%`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function selectEmailSendLog( | |
| filters: SelectEmailSendLogFilters = {}, | |
| ): Promise<Tables<"email_send_log">[]> { | |
| let query = supabase.from("email_send_log").select("*"); | |
| if (filters.status) query = query.eq("status", filters.status); | |
| if (filters.rawBodyLike) query = query.like("raw_body", `%${filters.rawBodyLike}%`); | |
| if (filters.limit) query = query.limit(filters.limit); | |
| export async function selectEmailSendLog( | |
| filters: SelectEmailSendLogFilters = {}, | |
| ): Promise<Tables<"email_send_log">[]> { | |
| let query = supabase.from("email_send_log").select("*"); | |
| if (filters.status) query = query.eq("status", filters.status); | |
| if (filters.rawBodyLike) { | |
| const escaped = filters.rawBodyLike.replace(/[\\%_]/g, "\\$&"); | |
| query = query.like("raw_body", `%${escaped}%`); | |
| } | |
| if (filters.limit) query = query.limit(filters.limit); |
🤖 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 `@lib/supabase/email_send_log/selectEmailSendLog.ts` around lines 22 - 29,
Update selectEmailSendLog so rawBodyLike is escaped before constructing the LIKE
pattern, treating caller-supplied % and _ characters as literals while
preserving the surrounding substring matching behavior. Apply the escaping in
the filters.rawBodyLike branch before calling query.like.
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Confidence score: 4/5
- In
lib/supabase/email_send_log/selectEmailSendLog.ts,rawBodyLikeis used in a SQLLIKEpattern without escaping%/_, so marker values containing wildcards can return broader matches than intended and make this reusable helper produce misleading results—escape wildcard characters (and set an escape clause) before building the pattern. - In
lib/supabase/email_send_log/selectEmailSendLog.ts,if (filters.limit)ignoreslimit: 0, which can silently drop the intended "no rows" guard and return unbounded rows if a caller starts using zero—switch to an explicit undefined/null check so0is handled correctly.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/supabase/email_send_log/selectEmailSendLog.ts">
<violation number="1" location="lib/supabase/email_send_log/selectEmailSendLog.ts:28">
P3: `rawBodyLike` is inserted into the LIKE pattern unescaped, so a marker value containing `%` or `_` (SQL LIKE wildcards) would match more broadly than intended. Since this is now a generic, reusable helper for any email-type idempotency check, consider escaping those characters before building the pattern to avoid subtle false-positive dedup matches for future callers.</violation>
<violation number="2" location="lib/supabase/email_send_log/selectEmailSendLog.ts:29">
P3: `if (filters.limit)` is falsy for `limit: 0`, so passing `0` (meaning "no rows") would be silently ignored and the query would return unbounded rows instead. Current callers pass `limit: 1` so this isn't hit today, but it's a latent bug in a generic/reusable helper.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| if (filters.status) query = query.eq("status", filters.status); | ||
| if (filters.rawBodyLike) query = query.like("raw_body", `%${filters.rawBodyLike}%`); | ||
| if (filters.limit) query = query.limit(filters.limit); |
There was a problem hiding this comment.
P3: if (filters.limit) is falsy for limit: 0, so passing 0 (meaning "no rows") would be silently ignored and the query would return unbounded rows instead. Current callers pass limit: 1 so this isn't hit today, but it's a latent bug in a generic/reusable helper.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/email_send_log/selectEmailSendLog.ts, line 29:
<comment>`if (filters.limit)` is falsy for `limit: 0`, so passing `0` (meaning "no rows") would be silently ignored and the query would return unbounded rows instead. Current callers pass `limit: 1` so this isn't hit today, but it's a latent bug in a generic/reusable helper.</comment>
<file context>
@@ -0,0 +1,37 @@
+
+ if (filters.status) query = query.eq("status", filters.status);
+ if (filters.rawBodyLike) query = query.like("raw_body", `%${filters.rawBodyLike}%`);
+ if (filters.limit) query = query.limit(filters.limit);
+
+ const { data, error } = await query;
</file context>
| if (filters.limit) query = query.limit(filters.limit); | |
| if (filters.limit !== undefined) query = query.limit(filters.limit); |
| let query = supabase.from("email_send_log").select("*"); | ||
|
|
||
| if (filters.status) query = query.eq("status", filters.status); | ||
| if (filters.rawBodyLike) query = query.like("raw_body", `%${filters.rawBodyLike}%`); |
There was a problem hiding this comment.
P3: rawBodyLike is inserted into the LIKE pattern unescaped, so a marker value containing % or _ (SQL LIKE wildcards) would match more broadly than intended. Since this is now a generic, reusable helper for any email-type idempotency check, consider escaping those characters before building the pattern to avoid subtle false-positive dedup matches for future callers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/email_send_log/selectEmailSendLog.ts, line 28:
<comment>`rawBodyLike` is inserted into the LIKE pattern unescaped, so a marker value containing `%` or `_` (SQL LIKE wildcards) would match more broadly than intended. Since this is now a generic, reusable helper for any email-type idempotency check, consider escaping those characters before building the pattern to avoid subtle false-positive dedup matches for future callers.</comment>
<file context>
@@ -0,0 +1,37 @@
+ let query = supabase.from("email_send_log").select("*");
+
+ if (filters.status) query = query.eq("status", filters.status);
+ if (filters.rawBodyLike) query = query.like("raw_body", `%${filters.rawBodyLike}%`);
+ if (filters.limit) query = query.limit(filters.limit);
+
</file context>
…rrors chat report) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t + proportional value) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ndValuationReportEmail Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…email) Move the send out of playcountSnapshotWorkflow (where snapshot.catalog is still null) into runValuationHandler via next/server after(), pointed at the freshly-materialized catalog. Deletes sendValuationEmailStep. Fixes the nearly-empty email root cause (chat#1881). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
6 issues found across 13 files (changes from recent commits).
Confidence score: 3/5
- In
lib/emails/valuationReport/sendValuationReportEmail.ts, fetching only the first page of measurements (and relying on implicit song query limits) can silently truncate large catalogs, producing per-release totals that don’t reconcile with headline valuation and misleading recipients — paginate through all results and set explicit, documented limits/looping behavior. - In
lib/emails/valuationReport/renderValuationReportHtml.ts, rendering an unbounded per-release table can push email size into client clipping thresholds (like Gmail), which can hide the CTA/footer and reduce report usability — cap rows and summarize overflow. - In
lib/emails/valuationReport/buildValuationReleaseRows.ts, matching album art by lowercased album title can assign incorrect artwork when titles collide (e.g., self-titled/deluxe variants), which undermines trust in the report output — key artwork mapping by a stable release identifier. - In
lib/valuation/runValuationHandler.ts, the newafter()email side effect was added to an already large handler without targeted coverage, so regressions like wrong catalog/artist payloads or unhandled async behavior could slip through — extract the side effect into a focused helper and add tests for payload correctness and rejected-promise handling.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/valuation/runValuationHandler.ts">
<violation number="1" location="lib/valuation/runValuationHandler.ts:148">
P1: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
`runValuationHandler.ts` is 164 lines, well over the 100-line limit in Rule 3. The newly added `after()` block further bloats an already overloaded handler that currently mixes auth/validation, Spotify resolution, measurement orchestration, catalog materialization, artist linking, valuation computation, and email sending. Consider splitting the numbered workflow steps into focused helper functions or separate modules so the handler stays under 100 lines and each unit has a single responsibility.</violation>
<violation number="2" location="lib/valuation/runValuationHandler.ts:148">
P3: The new email-trigger side effect wired into runValuationHandler (via after()) has no accompanying test verifying it fires with the corrected catalog id and searched artist, or that a rejected promise is swallowed as intended. Per this repo's TDD convention for lib/valuation/**, consider adding a test that asserts sendValuationReportEmail is invoked with `{ ...snapshot, catalog: catalog.id }` and that a thrown/rejected email send doesn't affect the handler's response.</violation>
</file>
<file name="lib/emails/valuationReport/buildValuationReleaseRows.ts">
<violation number="1" location="lib/emails/valuationReport/buildValuationReleaseRows.ts:11">
P3: Album art is joined to release rows purely by lowercased album name, so two different releases that share a title (self-titled record, deluxe edition, compilation) will silently get the wrong artwork since the map only keeps the first match. Consider keying by a more unique identifier (e.g. Spotify album id matched against the rollup) if you have one available, or documenting this as an accepted limitation.</violation>
</file>
<file name="lib/emails/valuationReport/sendValuationReportEmail.ts">
<violation number="1" location="lib/emails/valuationReport/sendValuationReportEmail.ts:32">
P2: Fetching only page 1 (limit 1000) of catalog measurements means large catalogs (>1000 measured tracks) will silently get an incomplete/understated per-release breakdown that no longer reconciles with the headline valuation band. Consider looping pages until exhausted, or explicitly document/guard the cutoff so it's not silently wrong for big catalogs.</violation>
<violation number="2" location="lib/emails/valuationReport/sendValuationReportEmail.ts:47">
P2: This song fetch has no explicit limit, so it silently relies on Supabase's default row cap for large catalogs instead of an intentional, documented limit — compounding the same truncation risk as the paginated measurements fetch above and making the release rollup incomplete for big catalogs without any signal that rows were dropped.</violation>
</file>
<file name="lib/emails/valuationReport/renderValuationReportHtml.ts">
<violation number="1" location="lib/emails/valuationReport/renderValuationReportHtml.ts:96">
P2: The per-release table renders every rollup row with no cap; for catalogs with many albums this can bloat the email past clients' clipping limits (e.g. Gmail clips messages over ~102KB), hiding the CTA button and footer below the fold. Since rollups already arrive sorted by streams descending, consider capping to the top N releases (e.g. 20-25) and adding a 'and N more releases' row.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // the local `snapshot` predates createSnapshotCatalog, so point it at the | ||
| // fresh catalog id. Deferred with `after` so it never blocks the response, | ||
| // and self-guarded (dedup + idempotency key) inside sendValuationReportEmail. | ||
| after(() => |
There was a problem hiding this comment.
P1: Custom agent: Enforce Clear Code Style and Maintainability Practices
runValuationHandler.ts is 164 lines, well over the 100-line limit in Rule 3. The newly added after() block further bloats an already overloaded handler that currently mixes auth/validation, Spotify resolution, measurement orchestration, catalog materialization, artist linking, valuation computation, and email sending. Consider splitting the numbered workflow steps into focused helper functions or separate modules so the handler stays under 100 lines and each unit has a single responsibility.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/valuation/runValuationHandler.ts, line 148:
<comment>`runValuationHandler.ts` is 164 lines, well over the 100-line limit in Rule 3. The newly added `after()` block further bloats an already overloaded handler that currently mixes auth/validation, Spotify resolution, measurement orchestration, catalog materialization, artist linking, valuation computation, and email sending. Consider splitting the numbered workflow steps into focused helper functions or separate modules so the handler stays under 100 lines and each unit has a single responsibility.</comment>
<file context>
@@ -140,6 +141,17 @@ export async function runValuationHandler(request: NextRequest): Promise<NextRes
+ // the local `snapshot` predates createSnapshotCatalog, so point it at the
+ // fresh catalog id. Deferred with `after` so it never blocks the response,
+ // and self-guarded (dedup + idempotency key) inside sendValuationReportEmail.
+ after(() =>
+ sendValuationReportEmail(
+ { ...snapshot, catalog: catalog.id },
</file context>
| function renderReleasesTable(releases: ValuationReleaseRow[] | undefined): string { | ||
| if (!releases || releases.length === 0) return ""; | ||
| const header = `<tr> | ||
| <td colspan="2" style="padding:0 0 8px;font-size:11px;font-weight:600;letter-spacing:0.06em;text-transform:uppercase;color:#6b6b6b">Release</td> | ||
| <td align="right" style="padding:0 12px 8px 0;font-size:11px;font-weight:600;letter-spacing:0.06em;text-transform:uppercase;color:#6b6b6b">Streams</td> | ||
| <td align="right" style="padding:0 0 8px;font-size:11px;font-weight:600;letter-spacing:0.06em;text-transform:uppercase;color:#6b6b6b">Value</td> | ||
| </tr>`; | ||
| const rows = releases.map(renderReleaseRow).join(""); | ||
| return `<table role="presentation" width="100%" cellpadding="0" cellspacing="0" style="margin:0 0 24px">${header}${rows}</table>`; |
There was a problem hiding this comment.
P2: The per-release table renders every rollup row with no cap; for catalogs with many albums this can bloat the email past clients' clipping limits (e.g. Gmail clips messages over ~102KB), hiding the CTA button and footer below the fold. Since rollups already arrive sorted by streams descending, consider capping to the top N releases (e.g. 20-25) and adding a 'and N more releases' row.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/valuationReport/renderValuationReportHtml.ts, line 96:
<comment>The per-release table renders every rollup row with no cap; for catalogs with many albums this can bloat the email past clients' clipping limits (e.g. Gmail clips messages over ~102KB), hiding the CTA button and footer below the fold. Since rollups already arrive sorted by streams descending, consider capping to the top N releases (e.g. 20-25) and adding a 'and N more releases' row.</comment>
<file context>
@@ -2,80 +2,148 @@ import { RECOUP_LOGO_URL, WEBSITE_URL } from "@/lib/const";
+</tr>`;
+}
+
+function renderReleasesTable(releases: ValuationReleaseRow[] | undefined): string {
+ if (!releases || releases.length === 0) return "";
+ const header = `<tr>
</file context>
| function renderReleasesTable(releases: ValuationReleaseRow[] | undefined): string { | |
| if (!releases || releases.length === 0) return ""; | |
| const header = `<tr> | |
| <td colspan="2" style="padding:0 0 8px;font-size:11px;font-weight:600;letter-spacing:0.06em;text-transform:uppercase;color:#6b6b6b">Release</td> | |
| <td align="right" style="padding:0 12px 8px 0;font-size:11px;font-weight:600;letter-spacing:0.06em;text-transform:uppercase;color:#6b6b6b">Streams</td> | |
| <td align="right" style="padding:0 0 8px;font-size:11px;font-weight:600;letter-spacing:0.06em;text-transform:uppercase;color:#6b6b6b">Value</td> | |
| </tr>`; | |
| const rows = releases.map(renderReleaseRow).join(""); | |
| return `<table role="presentation" width="100%" cellpadding="0" cellspacing="0" style="margin:0 0 24px">${header}${rows}</table>`; | |
| function renderReleasesTable(releases: ValuationReleaseRow[] | undefined): string { | |
| if (!releases || releases.length === 0) return ""; | |
| const MAX_ROWS = 25; | |
| const visible = releases.slice(0, MAX_ROWS); | |
| const remaining = releases.length - visible.length; | |
| const header = `<tr> | |
| <td colspan="2" style="padding:0 0 8px;font-size:11px;font-weight:600;letter-spacing:0.06em;text-transform:uppercase;color:#6b6b6b">Release</td> | |
| <td align="right" style="padding:0 12px 8px 0;font-size:11px;font-weight:600;letter-spacing:0.06em;text-transform:uppercase;color:#6b6b6b">Streams</td> | |
| <td align="right" style="padding:0 0 8px;font-size:11px;font-weight:600;letter-spacing:0.06em;text-transform:uppercase;color:#6b6b6b">Value</td> | |
| </tr>`; | |
| const rows = visible.map(renderReleaseRow).join(""); | |
| const more = | |
| remaining > 0 | |
| ? `<tr><td colspan="4" style="padding:10px 0 0;font-size:12px;color:#6b6b6b">and ${remaining} more release${remaining === 1 ? "" : "s"}</td></tr>` | |
| : ""; | |
| return `<table role="presentation" width="100%" cellpadding="0" cellspacing="0" style="margin:0 0 24px">${header}${rows}${more}</table>`; | |
| } |
| ): Promise<ValuationReportEmailParams["releases"]> { | ||
| try { | ||
| const [{ songs }, measurements, tokenResult] = await Promise.all([ | ||
| selectCatalogSongsWithArtists({ catalogId }), |
There was a problem hiding this comment.
P2: This song fetch has no explicit limit, so it silently relies on Supabase's default row cap for large catalogs instead of an intentional, documented limit — compounding the same truncation risk as the paginated measurements fetch above and making the release rollup incomplete for big catalogs without any signal that rows were dropped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/valuationReport/sendValuationReportEmail.ts, line 47:
<comment>This song fetch has no explicit limit, so it silently relies on Supabase's default row cap for large catalogs instead of an intentional, documented limit — compounding the same truncation risk as the paginated measurements fetch above and making the release rollup incomplete for big catalogs without any signal that rows were dropped.</comment>
<file context>
@@ -3,35 +3,87 @@ import { CHAT_APP_URL, RECOUP_FROM_EMAIL } from "@/lib/const";
+): Promise<ValuationReportEmailParams["releases"]> {
+ try {
+ const [{ songs }, measurements, tokenResult] = await Promise.all([
+ selectCatalogSongsWithArtists({ catalogId }),
+ selectCatalogMeasurementsPage({ catalogId, page: 1, limit: MEASUREMENTS_LIMIT }),
+ generateAccessToken(),
</file context>
|
|
||
| // One page is enough for the report: valuation-claimed catalogs are well under | ||
| // this, and the release table only needs the measured tracks. | ||
| const MEASUREMENTS_LIMIT = 1000; |
There was a problem hiding this comment.
P2: Fetching only page 1 (limit 1000) of catalog measurements means large catalogs (>1000 measured tracks) will silently get an incomplete/understated per-release breakdown that no longer reconciles with the headline valuation band. Consider looping pages until exhausted, or explicitly document/guard the cutoff so it's not silently wrong for big catalogs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/valuationReport/sendValuationReportEmail.ts, line 32:
<comment>Fetching only page 1 (limit 1000) of catalog measurements means large catalogs (>1000 measured tracks) will silently get an incomplete/understated per-release breakdown that no longer reconciles with the headline valuation band. Consider looping pages until exhausted, or explicitly document/guard the cutoff so it's not silently wrong for big catalogs.</comment>
<file context>
@@ -3,35 +3,87 @@ import { CHAT_APP_URL, RECOUP_FROM_EMAIL } from "@/lib/const";
+// One page is enough for the report: valuation-claimed catalogs are well under
+// this, and the release table only needs the measured tracks.
+const MEASUREMENTS_LIMIT = 1000;
+
+/**
</file context>
| * last entry is the ~64px thumbnail best suited to an email row. | ||
| */ | ||
| export function buildAlbumArtMap(albums: SpotifyAlbum[]): Map<string, string> { | ||
| const map = new Map<string, string>(); |
There was a problem hiding this comment.
P3: Album art is joined to release rows purely by lowercased album name, so two different releases that share a title (self-titled record, deluxe edition, compilation) will silently get the wrong artwork since the map only keeps the first match. Consider keying by a more unique identifier (e.g. Spotify album id matched against the rollup) if you have one available, or documenting this as an accepted limitation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/valuationReport/buildValuationReleaseRows.ts, line 11:
<comment>Album art is joined to release rows purely by lowercased album name, so two different releases that share a title (self-titled record, deluxe edition, compilation) will silently get the wrong artwork since the map only keeps the first match. Consider keying by a more unique identifier (e.g. Spotify album id matched against the rollup) if you have one available, or documenting this as an accepted limitation.</comment>
<file context>
@@ -0,0 +1,41 @@
+ * last entry is the ~64px thumbnail best suited to an email row.
+ */
+export function buildAlbumArtMap(albums: SpotifyAlbum[]): Map<string, string> {
+ const map = new Map<string, string>();
+ for (const album of albums ?? []) {
+ const name = album?.name?.trim().toLowerCase();
</file context>
| // the local `snapshot` predates createSnapshotCatalog, so point it at the | ||
| // fresh catalog id. Deferred with `after` so it never blocks the response, | ||
| // and self-guarded (dedup + idempotency key) inside sendValuationReportEmail. | ||
| after(() => |
There was a problem hiding this comment.
P3: The new email-trigger side effect wired into runValuationHandler (via after()) has no accompanying test verifying it fires with the corrected catalog id and searched artist, or that a rejected promise is swallowed as intended. Per this repo's TDD convention for lib/valuation/**, consider adding a test that asserts sendValuationReportEmail is invoked with { ...snapshot, catalog: catalog.id } and that a thrown/rejected email send doesn't affect the handler's response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/valuation/runValuationHandler.ts, line 148:
<comment>The new email-trigger side effect wired into runValuationHandler (via after()) has no accompanying test verifying it fires with the corrected catalog id and searched artist, or that a rejected promise is swallowed as intended. Per this repo's TDD convention for lib/valuation/**, consider adding a test that asserts sendValuationReportEmail is invoked with `{ ...snapshot, catalog: catalog.id }` and that a thrown/rejected email send doesn't affect the handler's response.</comment>
<file context>
@@ -140,6 +141,17 @@ export async function runValuationHandler(request: NextRequest): Promise<NextRes
+ // the local `snapshot` predates createSnapshotCatalog, so point it at the
+ // fresh catalog id. Deferred with `after` so it never blocks the response,
+ // and self-guarded (dedup + idempotency key) inside sendValuationReportEmail.
+ after(() =>
+ sendValuationReportEmail(
+ { ...snapshot, catalog: catalog.id },
</file context>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/emails/valuationReport/buildValuationReleaseRows.ts (1)
13-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize album-name normalization.
trim().toLowerCase()is duplicated in the art-map builder and row lookup. Extract a sharednormalizeAlbumNamehelper so both sides cannot drift.As per coding guidelines, avoid duplication by extracting shared logic; as per path instructions, consolidate similar logic into shared utilities.
Also applies to: 39-39
🤖 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 `@lib/emails/valuationReport/buildValuationReleaseRows.ts` around lines 13 - 16, Extract the duplicated trim-and-lowercase logic into a shared normalizeAlbumName helper, then use it both when building the album art map and when performing row lookups. Preserve the existing handling of missing or empty album names and ensure both paths use the same normalization behavior.Sources: Coding guidelines, Path instructions
lib/valuation/runValuationHandler.ts (1)
148-153: 🩺 Stability & Availability | 🔵 TrivialVerify that the deployment keeps
after()work alive.This is valid for a Route Handler, but serverless adapters require a
waitUntilimplementation; without it, the deferred email can be dropped after the response. Confirm the deployed runtime supports this and monitor callback execution. (nextjs.org)🤖 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 `@lib/valuation/runValuationHandler.ts` around lines 148 - 153, Update the deferred email handling around sendValuationReportEmail and after() so it uses the deployed runtime’s supported waitUntil mechanism, or otherwise ensures the callback remains alive after the response; verify the deployment adapter provides this capability and monitor callback execution. Preserve the existing error logging for failed email delivery.Source: MCP tools
lib/catalog/buildReleaseRollups.ts (1)
36-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit the oversized functions into focused helpers.
lib/catalog/buildReleaseRollups.ts#L36-L71: extract playcount indexing and rollup/artist aggregation helpers.lib/valuation/runValuationHandler.ts#L144-L153: move deferred email scheduling into a focused orchestration helper.As per coding guidelines, “Flag functions longer than 20 lines” and “Keep functions small and focused”; as per path instructions, domain functions in
lib/**/*.tsshould remain under 50 lines.🤖 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 `@lib/catalog/buildReleaseRollups.ts` around lines 36 - 71, Split buildReleaseRollups in lib/catalog/buildReleaseRollups.ts (lines 36-71) into focused helpers for building the playcount index and aggregating rollups/artists, preserving the existing grouping, measurement, and sorting behavior. In lib/valuation/runValuationHandler.ts (lines 144-153), extract deferred email scheduling into a dedicated orchestration helper and have the handler delegate to it.Sources: Coding guidelines, Path instructions
🤖 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 `@lib/emails/valuationReport/buildValuationReleaseRows.ts`:
- Around line 10-18: Move the primary function buildAlbumArtMap into a dedicated
buildAlbumArtMap.ts module, preserving its current album normalization, image
selection, and deduplication behavior. Remove its definition from
buildValuationReleaseRows.ts and import the function there for existing callers.
---
Nitpick comments:
In `@lib/catalog/buildReleaseRollups.ts`:
- Around line 36-71: Split buildReleaseRollups in
lib/catalog/buildReleaseRollups.ts (lines 36-71) into focused helpers for
building the playcount index and aggregating rollups/artists, preserving the
existing grouping, measurement, and sorting behavior. In
lib/valuation/runValuationHandler.ts (lines 144-153), extract deferred email
scheduling into a dedicated orchestration helper and have the handler delegate
to it.
In `@lib/emails/valuationReport/buildValuationReleaseRows.ts`:
- Around line 13-16: Extract the duplicated trim-and-lowercase logic into a
shared normalizeAlbumName helper, then use it both when building the album art
map and when performing row lookups. Preserve the existing handling of missing
or empty album names and ensure both paths use the same normalization behavior.
In `@lib/valuation/runValuationHandler.ts`:
- Around line 148-153: Update the deferred email handling around
sendValuationReportEmail and after() so it uses the deployed runtime’s supported
waitUntil mechanism, or otherwise ensures the callback remains alive after the
response; verify the deployment adapter provides this capability and monitor
callback execution. Preserve the existing error logging for failed email
delivery.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 331208aa-9db9-4943-8de7-618089e4af22
⛔ Files ignored due to path filters (4)
lib/catalog/__tests__/buildReleaseRollups.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/valuationReport/__tests__/buildValuationReleaseRows.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/valuationReport/__tests__/renderValuationReportHtml.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/valuationReport/__tests__/sendValuationReportEmail.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (6)
lib/catalog/buildReleaseRollups.tslib/emails/valuationReport/buildValuationReleaseRows.tslib/emails/valuationReport/renderValuationReportHtml.tslib/emails/valuationReport/sendValuationReportEmail.tslib/spotify/getAlbums.tslib/valuation/runValuationHandler.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/emails/valuationReport/renderValuationReportHtml.ts
- lib/emails/valuationReport/sendValuationReportEmail.ts
| export function buildAlbumArtMap(albums: SpotifyAlbum[]): Map<string, string> { | ||
| const map = new Map<string, string>(); | ||
| for (const album of albums ?? []) { | ||
| const name = album?.name?.trim().toLowerCase(); | ||
| const images = album?.images ?? []; | ||
| const url = images[images.length - 1]?.url; | ||
| if (name && url && !map.has(name)) map.set(name, url); | ||
| } | ||
| return map; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move buildAlbumArtMap into its own module.
Create lib/emails/valuationReport/buildAlbumArtMap.ts and import it here. This file currently exports two primary functions, and buildAlbumArtMap does not match buildValuationReleaseRows.ts.
As per path instructions, “The file name MUST match the exported function name”; as per coding guidelines, each TypeScript file must export one primary function and remain focused on one responsibility.
Also applies to: 27-41
🤖 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 `@lib/emails/valuationReport/buildValuationReleaseRows.ts` around lines 10 -
18, Move the primary function buildAlbumArtMap into a dedicated
buildAlbumArtMap.ts module, preserving its current album normalization, image
selection, and deduplication behavior. Remove its definition from
buildValuationReleaseRows.ts and import the function there for existing callers.
Sources: Coding guidelines, Path instructions
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 13 unresolved issues from previous reviews.
Re-trigger cubic
| * a Spotify /v1/albums response. Spotify returns images largest-first, so the | ||
| * last entry is the ~64px thumbnail best suited to an email row. | ||
| */ | ||
| export function buildAlbumArtMap(albums: SpotifyAlbum[]): Map<string, string> { |
There was a problem hiding this comment.
SRP - new lib file for buildAlbumArtMap.
There was a problem hiding this comment.
Done in 405f4321 — extracted to lib/emails/valuationReport/buildAlbumArtMap.ts (its unit test moved to __tests__/buildAlbumArtMap.test.ts).
|
|
||
| const usd = (n: number) => `$${formatCompactNumber(n)}`; | ||
|
|
||
| function renderArtistHeader(artist: ValuationReportEmailParams["artist"]): string { |
There was a problem hiding this comment.
SRP - new lib file for renderArtistHeader
There was a problem hiding this comment.
Done in 405f4321 — lib/emails/valuationReport/renderArtistHeader.ts.
| </tr></table>`; | ||
| } | ||
|
|
||
| function renderValuationBlock(valuation: ValuationReportEmailParams["valuation"]): string { |
There was a problem hiding this comment.
SRP - new lib file for renderValuationBlock
There was a problem hiding this comment.
Done in 405f4321 — lib/emails/valuationReport/renderValuationBlock.ts (shared usd() → formatUsd.ts).
| </td></tr></table>`; | ||
| } | ||
|
|
||
| function renderStatRow(params: ValuationReportEmailParams): string { |
There was a problem hiding this comment.
SRP - new lib file for renderStatRow
There was a problem hiding this comment.
Done in 405f4321 — lib/emails/valuationReport/renderStatRow.ts.
| return `<table role="presentation" width="100%" cellpadding="0" cellspacing="0" style="margin:0 0 24px"><tr>${cells}</tr></table>`; | ||
| } | ||
|
|
||
| function renderReleaseRow(release: ValuationReleaseRow): string { |
There was a problem hiding this comment.
SRP - new lib file for renderReleaseRow
There was a problem hiding this comment.
Done in 405f4321 — lib/emails/valuationReport/renderReleaseRow.ts.
| </tr>`; | ||
| } | ||
|
|
||
| function renderReleasesTable(releases: ValuationReleaseRow[] | undefined): string { |
There was a problem hiding this comment.
SRP - new lib file for renderReleasesTable
There was a problem hiding this comment.
Done in 405f4321 — lib/emails/valuationReport/renderReleasesTable.ts.
| * valued catalog. Best-effort: any read/Spotify failure returns [] so the email | ||
| * still sends with the headline band, just without the breakdown. | ||
| */ | ||
| async function buildReleaseRows( |
There was a problem hiding this comment.
SRP - new lib file for buildReleaseRows
There was a problem hiding this comment.
Done in 405f4321 — lib/emails/valuationReport/buildReleaseRows.ts. renderValuationReportHtml.ts 151→56 LOC, sendValuationReportEmail.ts 179→134; row/param types moved to valuationReportTypes.ts to break the render↔types cycle. 257 domain tests green, tsc + lint + format clean.
Preview verification —
|
| Field | Documented | Actual | ✓ |
|---|---|---|---|
status |
sent |
sent |
✅ |
type |
valuation_report |
valuation_report |
✅ |
catalog |
fresh catalog id (fires after createSnapshotCatalog) |
7822518c-…-630312 — matches the response catalog, NOT null |
✅ |
snapshot_id |
present (idempotency marker) | bd533bd8-… |
✅ |
resend_id |
present (Resend accepted) | 3a547672-… |
✅ |
The non-null catalog matching the response is the proof this fired from runValuationHandler's after() after materialization — not the pre-rework workflow path that sent a nearly-empty email before the catalog existed (that stale 2026-07-22 23:19 row still shows catalog: null).
Error codes (deliberate bad input)
| Case | Documented | Actual |
|---|---|---|
No Authorization header |
401 | 401 ✅ |
Missing spotify_artist_id |
400 | 400 {"status":"error","missing_fields":["spotify_artist_id"],…} ✅ |
| Unresolvable artist id | 5xx | 502 {"error":"Couldn't resolve the artist's releases"} ✅ |
No secret/env value echoed in any response.
Still pending before merge
- Inbox render check —
raw_bodystores only the JSON idempotency metadata (by design), not the HTML, so the rich render (artist header + headline band + per-release table with real album art + per-album$summing to the band) is verified by eyeballing the delivered email atsweetmantech@gmail.com. Will confirm and check this box.
|
Triage — CodeRabbit "Solid & Clean Code" warning (won't-fix, with reasoning). The warning flags |
…(SRP) Addresses PR review: split each render/build helper into its own lib file. - renderArtistHeader / renderValuationBlock / renderStatRow / renderReleaseRow / renderReleasesTable out of renderValuationReportHtml.ts (151->56 LOC) - buildAlbumArtMap out of buildValuationReleaseRows.ts - buildReleaseRows out of sendValuationReportEmail.ts (179->134 LOC) - shared usd() -> formatUsd.ts; row/param types -> valuationReportTypes.ts (breaks the render<->types cycle) - buildAlbumArtMap test moved to its own file No behavior change; 257 domain tests green, tsc + lint + format clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Update to my earlier SRP triage (above): overruled by @sweetmantech's review — done in |
There was a problem hiding this comment.
3 issues found across 14 files (changes from recent commits).
Confidence score: 3/5
- In
lib/emails/valuationReport/buildReleaseRows.ts, proportional values are calculated from a paginated subset (first 1000 rows) but divided by unpaginatedtotalStreams, which can understate per-release valuation in larger catalogs—align numerator/denominator scope (or page through all measurements) before relying on these figures. lib/emails/valuationReport/buildReleaseRows.tscurrently lacks targeted tests for the catch fallback (catch -> []) and the Spotify art branch (albumIds.length > 0with token), so regressions here could silently produce empty release tables or missing artwork in emails—add focused unit tests for both control paths.- In
lib/emails/valuationReport/buildAlbumArtMap.ts, deduping by lowercased album name can map different releases with the same title to one cover image, causing incorrect art in valuation emails—key the map by stable album ID (with a name-based fallback only when ID is absent).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/emails/valuationReport/buildAlbumArtMap.ts">
<violation number="1" location="lib/emails/valuationReport/buildAlbumArtMap.ts:10">
P3: buildAlbumArtMap dedupes by lowercased album name only, so two different releases sharing a name will incorrectly show the same cover art in the valuation email (first-seen wins). Consider keying by album id and having buildValuationReleaseRows join on id instead of name, or at least accept this as a known cosmetic limitation.</violation>
</file>
<file name="lib/emails/valuationReport/buildReleaseRows.ts">
<violation number="1" location="lib/emails/valuationReport/buildReleaseRows.ts:12">
P2: The release-table rows are built from only the first 1000 measurement rows (MEASUREMENTS_LIMIT) while the proportional value calculation divides by the full-catalog `totalStreams` from an unpaginated aggregate. For catalogs with >1000 measured songs, the release table's values will no longer sum to the headline valuation band, undermining the documented invariant in buildValuationReleaseRows.ts.</violation>
<violation number="2" location="lib/emails/valuationReport/buildReleaseRows.ts:19">
P2: Unlike its sibling helpers, buildReleaseRows.ts has no dedicated unit test, so its error-fallback path (catch → []) and the conditional Spotify art-lookup branch (albumIds.length > 0 && access_token present) aren't covered by an automated test.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| * valued catalog. Best-effort: any read/Spotify failure returns [] so the email | ||
| * still sends with the headline band, just without the breakdown. | ||
| */ | ||
| export async function buildReleaseRows( |
There was a problem hiding this comment.
P2: Unlike its sibling helpers, buildReleaseRows.ts has no dedicated unit test, so its error-fallback path (catch → []) and the conditional Spotify art-lookup branch (albumIds.length > 0 && access_token present) aren't covered by an automated test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/valuationReport/buildReleaseRows.ts, line 19:
<comment>Unlike its sibling helpers, buildReleaseRows.ts has no dedicated unit test, so its error-fallback path (catch → []) and the conditional Spotify art-lookup branch (albumIds.length > 0 && access_token present) aren't covered by an automated test.</comment>
<file context>
@@ -0,0 +1,45 @@
+ * valued catalog. Best-effort: any read/Spotify failure returns [] so the email
+ * still sends with the headline band, just without the breakdown.
+ */
+export async function buildReleaseRows(
+ catalogId: string,
+ albumIds: string[],
</file context>
|
|
||
| // One page is enough for the report: valuation-claimed catalogs are well under | ||
| // this, and the release table only needs the measured tracks. | ||
| const MEASUREMENTS_LIMIT = 1000; |
There was a problem hiding this comment.
P2: The release-table rows are built from only the first 1000 measurement rows (MEASUREMENTS_LIMIT) while the proportional value calculation divides by the full-catalog totalStreams from an unpaginated aggregate. For catalogs with >1000 measured songs, the release table's values will no longer sum to the headline valuation band, undermining the documented invariant in buildValuationReleaseRows.ts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/valuationReport/buildReleaseRows.ts, line 12:
<comment>The release-table rows are built from only the first 1000 measurement rows (MEASUREMENTS_LIMIT) while the proportional value calculation divides by the full-catalog `totalStreams` from an unpaginated aggregate. For catalogs with >1000 measured songs, the release table's values will no longer sum to the headline valuation band, undermining the documented invariant in buildValuationReleaseRows.ts.</comment>
<file context>
@@ -0,0 +1,45 @@
+
+// One page is enough for the report: valuation-claimed catalogs are well under
+// this, and the release table only needs the measured tracks.
+const MEASUREMENTS_LIMIT = 1000;
+
+/**
</file context>
| */ | ||
| export function buildAlbumArtMap(albums: SpotifyAlbum[]): Map<string, string> { | ||
| const map = new Map<string, string>(); | ||
| for (const album of albums ?? []) { |
There was a problem hiding this comment.
P3: buildAlbumArtMap dedupes by lowercased album name only, so two different releases sharing a name will incorrectly show the same cover art in the valuation email (first-seen wins). Consider keying by album id and having buildValuationReleaseRows join on id instead of name, or at least accept this as a known cosmetic limitation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/valuationReport/buildAlbumArtMap.ts, line 10:
<comment>buildAlbumArtMap dedupes by lowercased album name only, so two different releases sharing a name will incorrectly show the same cover art in the valuation email (first-seen wins). Consider keying by album id and having buildValuationReleaseRows join on id instead of name, or at least accept this as a known cosmetic limitation.</comment>
<file context>
@@ -0,0 +1,17 @@
+ */
+export function buildAlbumArtMap(albums: SpotifyAlbum[]): Map<string, string> {
+ const map = new Map<string, string>();
+ for (const album of albums ?? []) {
+ const name = album?.name?.trim().toLowerCase();
+ const images = album?.images ?? [];
</file context>
Rendered email — inbox render ✅The delivered email (to
Full-length render (all 47 releases): valuation-email-773-full.png With this, every Done when criterion for the PR is met: CI green on |
…from lib/stripe/formatUsd) lib/stripe/formatUsd takes cents and renders exact $99.00 for billing; the valuation email's helper takes whole dollars and compacts ($3.6M). Same name, different unit + rounding -> rename to make intent explicit and remove the collision. No behavior change; 23 tests + tsc + lint + format clean.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/emails/valuationReport/renderValuationReportHtml.ts (1)
50-50: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winEscape
deepLinkUrlbefore interpolating into thehrefattribute.Every other URL in this pipeline (
artist.imageUrlinrenderArtistHeader,release.artUrlinrenderReleaseRow) is passed throughescapeHtmlbefore being embedded.params.deepLinkUrlhere is not, breaking that consistency and leaving an HTML-attribute-injection opening if its construction ever incorporates less-trusted input.🔒️ Proposed fix
-<table role="presentation" cellpadding="0" cellspacing="0" style="margin:0 0 8px"><tr><td style="background:`#0a0a0a`;border-radius:8px"><a href="${params.deepLinkUrl}" style="display:inline-block;padding:12px 22px;font-size:14px;font-weight:600;color:`#ffffff`;text-decoration:none">Get the full report with Recoup →</a></td></tr></table> +<table role="presentation" cellpadding="0" cellspacing="0" style="margin:0 0 8px"><tr><td style="background:`#0a0a0a`;border-radius:8px"><a href="${escapeHtml(params.deepLinkUrl)}" style="display:inline-block;padding:12px 22px;font-size:14px;font-weight:600;color:`#ffffff`;text-decoration:none">Get the full report with Recoup →</a></td></tr></table>🤖 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 `@lib/emails/valuationReport/renderValuationReportHtml.ts` at line 50, Update the href interpolation in renderValuationReportHtml to pass params.deepLinkUrl through the existing escapeHtml helper before embedding it in the HTML attribute, matching renderArtistHeader and renderReleaseRow.
🤖 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 `@lib/emails/valuationReport/buildReleaseRows.ts`:
- Around line 26-38: Decouple generateAccessToken from the Promise.all
containing selectCatalogSongsWithArtists and selectCatalogMeasurementsPage in
the release-row builder. Let the Supabase reads and buildReleaseRollups complete
even when token generation fails, and only attempt getAlbums when a successful
token is available, preserving art-less rows as the fallback.
---
Outside diff comments:
In `@lib/emails/valuationReport/renderValuationReportHtml.ts`:
- Line 50: Update the href interpolation in renderValuationReportHtml to pass
params.deepLinkUrl through the existing escapeHtml helper before embedding it in
the HTML attribute, matching renderArtistHeader and renderReleaseRow.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 41ac4ba6-3b9a-4d31-bd12-75703bc12354
⛔ Files ignored due to path filters (2)
lib/emails/valuationReport/__tests__/buildAlbumArtMap.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/valuationReport/__tests__/buildValuationReleaseRows.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (12)
lib/emails/valuationReport/buildAlbumArtMap.tslib/emails/valuationReport/buildReleaseRows.tslib/emails/valuationReport/buildValuationReleaseRows.tslib/emails/valuationReport/formatCompactUsd.tslib/emails/valuationReport/renderArtistHeader.tslib/emails/valuationReport/renderReleaseRow.tslib/emails/valuationReport/renderReleasesTable.tslib/emails/valuationReport/renderStatRow.tslib/emails/valuationReport/renderValuationBlock.tslib/emails/valuationReport/renderValuationReportHtml.tslib/emails/valuationReport/sendValuationReportEmail.tslib/emails/valuationReport/valuationReportTypes.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/emails/valuationReport/sendValuationReportEmail.ts
| const [{ songs }, measurements, tokenResult] = await Promise.all([ | ||
| selectCatalogSongsWithArtists({ catalogId }), | ||
| selectCatalogMeasurementsPage({ catalogId, page: 1, limit: MEASUREMENTS_LIMIT }), | ||
| generateAccessToken(), | ||
| ]); | ||
|
|
||
| const rollups = buildReleaseRollups(songs, measurements ?? []); | ||
|
|
||
| let artByAlbum = new Map<string, string>(); | ||
| if (albumIds.length > 0 && tokenResult.access_token) { | ||
| const { albums } = await getAlbums({ ids: albumIds, accessToken: tokenResult.access_token }); | ||
| if (albums) artByAlbum = buildAlbumArtMap(albums); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Spotify token failure discards an otherwise-successful release breakdown.
generateAccessToken() is bundled into the same Promise.all as the two Supabase reads (Line 26-30). If it rejects, the whole function falls into the outer catch (Line 41-44) and returns [] — even though songs/measurements may have resolved fine and rollups could still be built without album art. This turns a Spotify-only failure into a total loss of the release table, when it could gracefully degrade to art-less rows.
🛡️ Proposed fix: decouple the Spotify call from the Supabase reads
- const [{ songs }, measurements, tokenResult] = await Promise.all([
- selectCatalogSongsWithArtists({ catalogId }),
- selectCatalogMeasurementsPage({ catalogId, page: 1, limit: MEASUREMENTS_LIMIT }),
- generateAccessToken(),
- ]);
+ const [{ songs }, measurements] = await Promise.all([
+ selectCatalogSongsWithArtists({ catalogId }),
+ selectCatalogMeasurementsPage({ catalogId, page: 1, limit: MEASUREMENTS_LIMIT }),
+ ]);
const rollups = buildReleaseRollups(songs, measurements ?? []);
let artByAlbum = new Map<string, string>();
- if (albumIds.length > 0 && tokenResult.access_token) {
- const { albums } = await getAlbums({ ids: albumIds, accessToken: tokenResult.access_token });
- if (albums) artByAlbum = buildAlbumArtMap(albums);
+ if (albumIds.length > 0) {
+ try {
+ const tokenResult = await generateAccessToken();
+ if (tokenResult.access_token) {
+ const { albums } = await getAlbums({ ids: albumIds, accessToken: tokenResult.access_token });
+ if (albums) artByAlbum = buildAlbumArtMap(albums);
+ }
+ } catch (artError) {
+ console.error(`Valuation email album-art lookup failed for catalog ${catalogId}:`, artError);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [{ songs }, measurements, tokenResult] = await Promise.all([ | |
| selectCatalogSongsWithArtists({ catalogId }), | |
| selectCatalogMeasurementsPage({ catalogId, page: 1, limit: MEASUREMENTS_LIMIT }), | |
| generateAccessToken(), | |
| ]); | |
| const rollups = buildReleaseRollups(songs, measurements ?? []); | |
| let artByAlbum = new Map<string, string>(); | |
| if (albumIds.length > 0 && tokenResult.access_token) { | |
| const { albums } = await getAlbums({ ids: albumIds, accessToken: tokenResult.access_token }); | |
| if (albums) artByAlbum = buildAlbumArtMap(albums); | |
| } | |
| const [{ songs }, measurements] = await Promise.all([ | |
| selectCatalogSongsWithArtists({ catalogId }), | |
| selectCatalogMeasurementsPage({ catalogId, page: 1, limit: MEASUREMENTS_LIMIT }), | |
| ]); | |
| const rollups = buildReleaseRollups(songs, measurements ?? []); | |
| let artByAlbum = new Map<string, string>(); | |
| if (albumIds.length > 0) { | |
| try { | |
| const tokenResult = await generateAccessToken(); | |
| if (tokenResult.access_token) { | |
| const { albums } = await getAlbums({ ids: albumIds, accessToken: tokenResult.access_token }); | |
| if (albums) artByAlbum = buildAlbumArtMap(albums); | |
| } | |
| } catch (artError) { | |
| console.error(`Valuation email album-art lookup failed for catalog ${catalogId}:`, artError); | |
| } | |
| } |
🤖 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 `@lib/emails/valuationReport/buildReleaseRows.ts` around lines 26 - 38,
Decouple generateAccessToken from the Promise.all containing
selectCatalogSongsWithArtists and selectCatalogMeasurementsPage in the
release-row builder. Let the Supabase reads and buildReleaseRollups complete
even when token generation fails, and only attempt getAlbums when a successful
token is available, preserving art-less rows as the fallback.
Re-verified on
|
1 participant
Implements the "Valuation-report email on valuation complete" item from recoupable/chat#1867.
Problem
The valuation $ number lives only in the user's open browser tab. digital@epitaph.com ran a 10-album measurement on 2026-07-13 and had nothing to return to;
email_send_loghas never contained a non-task email for a new signup.What this does
When
playcountSnapshotWorkflowreaches its terminal success state (markSnapshotStep(..., { state: "done" })), a newsendValuationEmailStepemails every address on the owning account:https://chat.recoupable.dev/catalogs/{id}(app roothttps://chat.recoupable.devwhen the run has no claimed catalog).lib/catalog/computeValuationBand.ts— the exact functionGET /api/catalogs/{id}/measurementsalready uses (which itself mirrors the marketing card model). No formula was copied.renderScrapeDigestHtml— DESIGN.md achromatic chrome (#0a0a0aon#ffffff,#e8e8e8borders,#6b6b6bmuted), tables + inline styles, system font stack, black CTA button, fixedCHAT_APP_URL(never a derived deployment URL). Range copy uses "to", no em/en dashes.Idempotency (no duplicate sends on retries)
Two independent guards:
email_send_logmarker — the send is logged withraw_bodyJSON containing"snapshot_id":"<id>";selectValuationEmailSendLogchecks it before any send. Unbounded window.valuation-report/<snapshot_id>passed toresend.emails.send, covering racing retries within Resend's 24h window.The step is best-effort: it never throws, so an email failure can never flip a completed measurement run to
failed. Failed sends are logged with statussend_failed; accounts with noaccount_emailsrow are skipped silently.Files
app/workflows/sendValuationEmailStep.ts— new step, wired intoplaycountSnapshotWorkflowafter thedonetransitionlib/emails/valuationReport/sendValuationReportEmail.ts— orchestrator (guards → enrichment → send → log)lib/emails/valuationReport/renderValuationReportHtml.ts— deterministic templatelib/emails/valuationReport/formatCompactNumber.ts— K/M/B formatter (digest's formatter stops at M)lib/supabase/email_send_log/selectValuationEmailSendLog.ts— idempotency lookupMeasurement enrichment (
selectCatalogById+selectCatalogMeasurementsAggregate+getCatalogEarliestReleaseDate) is wrapped: a read failure degrades to the link-only email rather than dropping the send. Zero measured streams also degrades to link-only (no $0 valuations).Verification
pnpm testpnpm exec tsc --noEmitpnpm lint+pnpm format:checkHow to test on preview
POST /api/research/snapshotswith a small catalog (an account whoseaccount_emailsrow is a test inbox — previews share the prod DB, so use your own account, not a customer's).playcount_snapshots.stateuntildone)./catalogs/{id}deep link.email_send_logrow withstatus='sent'andraw_bodycontaining thesnapshot_idmarker.sentrow.Internal-only change: no public API contract changes, so no docs PR. Uses the existing
RESEND_API_KEYenv var; no new env vars.🤖 Generated with Claude Code
Summary by cubic
Send a valuation report email after a snapshot completes so users can return to their results. The email mirrors the chat report with an artist header and a releases table, and sends after the catalog is materialized (recoupable/chat#1867, chat#1881).
New Features
lib/catalog/computeValuationBand.ts, lifetime Spotify streams, measured tracks, releases, catalog age, and a deep link. Adds artist header (image and followers) and a releases table with album art, streams, and per-release value that sums to the headline.buildReleaseRollupsaggregates per-album streams and joins measurements; album art uses the smallest image from SpotifygetAlbums;buildValuationReleaseRowscomputes proportional values. Safe fallbacks send a link-only email if enrichment fails or streams are zero.selectEmailSendLogwith a"snapshot_id":"<id>"marker and Resend keyvaluation-report/<snapshot_id>. Logssentorsend_failed. Skips when no account email.Refactors
runValuationHandlerusingnext/serverafter()so it runs post-materialization and never delays the response.formatCompactUsd(distinct fromlib/stripe/formatUsd).Written for commit 82161fc. Summary will update on new commits.
Summary by CodeRabbit