feat(apify): deterministic digest template — direct links to each new post, no LLM body#761
Conversation
… posts first The Instagram scrape alert fired whenever a scrape returned posts, with no comparison against posts already stored — every scrape re-announced the profile's recent feed as new (observed: 6 of 7 alerts in one day announced posts up to 10 days old). New reusable filterNewPostUrls diffs candidate URLs against posts BEFORE upsert; the alert is gated on a non-empty result. Persistence unchanged (recoupable/chat#1855, PR #3 of 6). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A roster scrape starts one Apify run per platform, each completing independently — extending per-platform alerts would mean 4+ emails per scrape. This registers every run under a batch_id at scrape start (apify_scraper_runs, columns from recoupable/database#41), records each webhook completion with its genuinely-new post URLs, and when the batch's last run completes sends ONE digest (per-platform sections, BCC-only) via the new digest module. Platforms with nothing new are omitted; a batch with nothing new sends nothing. Instagram's solo alert is suppressed for batch runs; legacy/non-batch runs keep today's behavior (recoupable/chat#1855, PR #4 of 6). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…new post Replaces the per-send LLM email body (nondeterministic branding, vendor jargon reaching customers, no reliable post links) with a shared deterministic renderer: stable subject/branding, one section per platform, a direct link to every genuinely-new post, chat CTA secondary. Used by both the batch digest and the legacy solo Instagram alert, which now also requires new-post URLs and is BCC-only (recoupable/chat#1855, PR #5 of 6; supersedes the interim body from PR #4 and, for this file, the standalone BCC fix in api#758 — same invariant, tests included). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. |
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR adds deterministic scrape-digest data extraction, run-level assembly, HTML rendering, and email delivery. Instagram and TikTok dataset items are converted into enriched posts, while missing enrichment falls back to URL-only entries. ChangesScrape digest delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ScraperResultHandler
participant sendApifyWebhookEmail
participant extractPostsFromDatasetItems
participant renderScrapeDigestHtml
participant Resend
ScraperResultHandler->>sendApifyWebhookEmail: pass profile, recipients, and new post URLs
sendApifyWebhookEmail->>extractPostsFromDatasetItems: extract Instagram post data
extractPostsFromDatasetItems-->>sendApifyWebhookEmail: return enriched posts or URL fallbacks
sendApifyWebhookEmail->>renderScrapeDigestHtml: render section and artist name
renderScrapeDigestHtml-->>sendApifyWebhookEmail: return subject and HTML
sendApifyWebhookEmail->>Resend: send BCC digest email
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 7 files
Confidence score: 2/5
lib/apify/digest/renderScrapeDigestHtml.tscurrently interpolatespostUrls,platform, andartistNamedirectly into HTML (includinghref), so crafted input could break email markup or inject unintended content for recipients; this is the main merge risk—escape/sanitize all dynamic fields (and URL-encode/validate link values) before merging.lib/apify/digest/__tests__/renderScrapeDigestHtml.test.tsdoes not cover singular wording paths (1 post / 1 platform), so a user-facing grammar regression could ship unnoticed in digest subjects—add a focused singular-case test before merging to lock this behavior.lib/apify/__tests__/sendApifyWebhookEmail.test.tsusesas neverin the PROFILE fixture, which bypasses type safety and can hide fixture/schema drift that later breaks real integrations—replace it with a properly typed fixture (or minimal explicit cast) to keep tests trustworthy.
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/apify/__tests__/sendApifyWebhookEmail.test.ts">
<violation number="1" location="lib/apify/__tests__/sendApifyWebhookEmail.test.ts:22">
P3: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
The PROFILE test fixture uses `as never` which completely erases type checking on this object. Since `ApifyInstagramProfileResult` has all-optional fields, the literal already satisfies the interface — no assertion is needed. Removing `as never` (or switching to `as ApifyInstagramProfileResult` if you want explicit annotation) would let TypeScript verify the fixture stays in sync with the type definition.</violation>
</file>
<file name="lib/apify/digest/__tests__/renderScrapeDigestHtml.test.ts">
<violation number="1" location="lib/apify/digest/__tests__/renderScrapeDigestHtml.test.ts:35">
P2: Singular subject form is untested: no case covers 1 post or 1 platform, where the pluralization code (`total === 1 ? "" : "s"`, `sections.length === 1 ? "" : "s"`) produces different output. Consider adding a case like `sections: [{platform: "instagram", postUrls: ["https://instagram.com/p/abc"]}]` and asserting subject `"Ashnikko: 1 new post across 1 platform"`.</violation>
</file>
<file name="lib/apify/digest/renderScrapeDigestHtml.ts">
<violation number="1" location="lib/apify/digest/renderScrapeDigestHtml.ts:37">
P1: The new renderer inserts `postUrls`, `platform`, and `artistName` into HTML without escaping, including inside the `href` attribute. A crafted value containing quotes or tags can break the email markup and inject unexpected links/content. Escaping dynamic HTML content (and safely encoding attribute values) before interpolation would avoid this injection path.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Webhook as Apify Webhook
participant Handler as Profile Scraper Handler
participant Upsert as Upsert Posts
participant Solo as Solo Alert (sendApifyWebhookEmail)
participant Batch as Batch Digest (sendScrapeDigestEmail)
participant Renderer as Deterministic Renderer
participant Email as Email Service (Resend)
Webhook->>Handler: Receive scrape results
Handler->>Upsert: upsertPosts(parsed)
Upsert-->>Handler: newPostUrls, profile
alt Solo scrape run
Handler->>Solo: sendApifyWebhookEmail(profile, emails, newPostUrls)
alt No recipients or no new posts
Solo-->>Handler: Return null (short circuit)
else Has recipients and new posts
Solo->>Renderer: renderScrapeDigestHtml({sections: [{platform:"instagram", postUrls: newPostUrls}], artistName})
Renderer-->>Solo: {subject, html}
Note over Solo,Email: BCC-only invariant: all recipients in bcc, to=RECOUP_FROM_EMAIL
Solo->>Email: sendEmailWithResend({to: [RECOUP_FROM_EMAIL], bcc: emails, subject, html})
Email-->>Solo: {id: "email-id"}
Solo-->>Handler: result
end
else Batch digest run
Handler->>Batch: sendScrapeDigestEmail(input)
Batch->>Renderer: renderScrapeDigestHtml({sections, artistName})
Renderer-->>Batch: {subject, html}
Batch->>Email: sendEmailWithResend({...}) (BCC)
Email-->>Batch: result
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const items = section.postUrls | ||
| .map(url => { | ||
| const href = url.startsWith("http") ? url : `https://${url}`; | ||
| return `<li style="margin:4px 0"><a href="${href}" style="color:#111;text-decoration:underline">${href}</a></li>`; |
There was a problem hiding this comment.
P1: The new renderer inserts postUrls, platform, and artistName into HTML without escaping, including inside the href attribute. A crafted value containing quotes or tags can break the email markup and inject unexpected links/content. Escaping dynamic HTML content (and safely encoding attribute values) before interpolation would avoid this injection path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/apify/digest/renderScrapeDigestHtml.ts, line 37:
<comment>The new renderer inserts `postUrls`, `platform`, and `artistName` into HTML without escaping, including inside the `href` attribute. A crafted value containing quotes or tags can break the email markup and inject unexpected links/content. Escaping dynamic HTML content (and safely encoding attribute values) before interpolation would avoid this injection path.</comment>
<file context>
@@ -0,0 +1,53 @@
+ const items = section.postUrls
+ .map(url => {
+ const href = url.startsWith("http") ? url : `https://${url}`;
+ return `<li style="margin:4px 0"><a href="${href}" style="color:#111;text-decoration:underline">${href}</a></li>`;
+ })
+ .join("");
</file context>
| expect((html + subject).toLowerCase()).not.toContain("dataset"); | ||
| }); | ||
|
|
||
| it("counts posts and platforms in the subject", () => { |
There was a problem hiding this comment.
P2: Singular subject form is untested: no case covers 1 post or 1 platform, where the pluralization code (total === 1 ? "" : "s", sections.length === 1 ? "" : "s") produces different output. Consider adding a case like sections: [{platform: "instagram", postUrls: ["https://instagram.com/p/abc"]}] and asserting subject "Ashnikko: 1 new post across 1 platform".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/apify/digest/__tests__/renderScrapeDigestHtml.test.ts, line 35:
<comment>Singular subject form is untested: no case covers 1 post or 1 platform, where the pluralization code (`total === 1 ? "" : "s"`, `sections.length === 1 ? "" : "s"`) produces different output. Consider adding a case like `sections: [{platform: "instagram", postUrls: ["https://instagram.com/p/abc"]}]` and asserting subject `"Ashnikko: 1 new post across 1 platform"`.</comment>
<file context>
@@ -0,0 +1,39 @@
+ expect((html + subject).toLowerCase()).not.toContain("dataset");
+ });
+
+ it("counts posts and platforms in the subject", () => {
+ const { subject } = renderScrapeDigestHtml({ sections: SECTIONS, artistName: "Ashnikko" });
+ expect(subject).toBe("Ashnikko: 3 new posts across 2 platforms");
</file context>
| followersCount: 100, | ||
| followsCount: 10, | ||
| latestPosts: [], | ||
| } as never; |
There was a problem hiding this comment.
P3: Custom agent: Enforce Clear Code Style and Maintainability Practices
The PROFILE test fixture uses as never which completely erases type checking on this object. Since ApifyInstagramProfileResult has all-optional fields, the literal already satisfies the interface — no assertion is needed. Removing as never (or switching to as ApifyInstagramProfileResult if you want explicit annotation) would let TypeScript verify the fixture stays in sync with the type definition.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/apify/__tests__/sendApifyWebhookEmail.test.ts, line 22:
<comment>The PROFILE test fixture uses `as never` which completely erases type checking on this object. Since `ApifyInstagramProfileResult` has all-optional fields, the literal already satisfies the interface — no assertion is needed. Removing `as never` (or switching to `as ApifyInstagramProfileResult` if you want explicit annotation) would let TypeScript verify the fixture stays in sync with the type definition.</comment>
<file context>
@@ -0,0 +1,62 @@
+ followersCount: 100,
+ followsCount: 10,
+ latestPosts: [],
+} as never;
+const NEW_URLS = ["https://instagram.com/p/new1"];
+
</file context>
…mplate # Conflicts: # lib/apify/__tests__/apifyWebhookHandler.test.ts # lib/apify/__tests__/sendApifyWebhookEmail.test.ts # lib/apify/apifyWebhookHandler.ts # lib/apify/digest/__tests__/maybeSendScrapeDigest.test.ts # lib/apify/digest/maybeSendScrapeDigest.ts # lib/apify/digest/sendScrapeDigestEmail.ts # lib/apify/instagram/handleInstagramProfileScraperResults.ts # lib/apify/sendApifyWebhookEmail.ts # lib/artist/__tests__/postArtistSocialsScrapeHandler.test.ts # lib/artist/postArtistSocialsScrapeHandler.ts # lib/supabase/apify_scraper_runs/selectApifyScraperRun.ts
|
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. |
…riant The branch imported renderScrapeDigestHtml in sendScrapeDigestEmail but never called it — the digest still rendered the old inline body (found during the main-sync conflict resolution; wired in that merge commit). This test fails against the unwired version: it asserts the send payload is byte-identical to the renderer's output, plus the BCC-only invariant and the empty-input no-send. 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. |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 4 unresolved issues from previous reviews.
Re-trigger cubic
Preview verification — 2026-07-09Branch prep: retargeted to Bug found and fixed during resolution: Preview:
Expected inbox (sweetmantech@gmail.com), for eyeball confirmation: Fixture fully cleaned (artist, 2 socials, 15 posts + social_posts + post_comments, batch rows; final sweep 0 remaining). Verdict: with the wiring fix, the deterministic template now actually ships for both the solo alert and the digest — ready to merge (last in sequence before api#762). |
…nd dates
The v1 deterministic template regressed visual quality vs the old LLM
emails (bare h3/ul list). This upgrades the renderer to the DESIGN.md
house style — achromatic chrome, card-per-post with 72px thumbnail,
escaped caption excerpt, date, and a direct link; black CTA button —
while staying deterministic and email-safe (tables + inline styles).
Media plumbing: extractPostsFromDatasetItems maps platform dataset items
(IG latestPosts, TikTok items) to {url, caption, thumbnailUrl, timestamp},
limited to the genuinely-new URLs. The solo alert enriches from the
dataset already in memory; the digest enriches via getRunDigestSection,
which re-reads each run's dataset from Apify (source of truth) and
degrades to URL-only links on any failure — enrichment never blocks a
send. Captions are HTML-escaped so scraped content can't inject markup.
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. |
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. |
![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 (3)
lib/apify/digest/getRunDigestSection.ts (1)
22-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd timeouts to external Apify calls.
Two external API calls (
apifyClient.run().get()andapifyClient.dataset().listItems()) execute without timeouts. SincemaybeSendScrapeDigestcalls this viaPromise.all, a single slow Apify response delays the entire digest. The fallback to URL-only posts is good, but it only triggers on errors — a hanging request won't reach the catch block.Consider wrapping these calls with
Promise.raceagainst a timeout, or using AbortController.⏱️ Proposed timeout wrapper
+const FETCH_TIMEOUT_MS = 10_000; + +async function withTimeout<T>(p: Promise<T>, ms: number): Promise<T> { + return Promise.race([ + p, + new Promise<T>((_, reject) => + setTimeout(() => reject(new Error("Apify fetch timed out")), ms), + ), + ]); +} + export async function getRunDigestSection( run: Tables<"apify_scraper_runs">, ): Promise<ScrapeDigestSection | null> { const urls = parseNewPostUrls(run.new_post_urls); if (!urls.length) return null; const platform = run.platform ?? "other"; try { - const runInfo = await apifyClient.run(run.run_id).get(); + const runInfo = await withTimeout(apifyClient.run(run.run_id).get(), FETCH_TIMEOUT_MS); const datasetId = runInfo?.defaultDatasetId; if (!datasetId) return { platform, posts: urls.map(url => ({ url })) }; - const { items } = await apifyClient.dataset(datasetId).listItems(); + const { items } = await withTimeout(apifyClient.dataset(datasetId).listItems(), FETCH_TIMEOUT_MS); return { platform, posts: extractPostsFromDatasetItems(platform, items, urls) }; } catch (error) { console.error("[WARN] digest enrichment failed; sending URL-only links:", error); return { platform, posts: urls.map(url => ({ url })) }; } }🤖 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/apify/digest/getRunDigestSection.ts` around lines 22 - 31, The Apify enrichment flow in getRunDigestSection currently waits indefinitely on apifyClient.run(run.run_id).get() and apifyClient.dataset(datasetId).listItems(), so a slow request can stall maybeSendScrapeDigest despite the URL-only fallback. Add explicit timeouts around both external calls, either by racing each request against a timer or by wiring an AbortController into the Apify client calls. Keep the existing fallback path in getRunDigestSection so timed-out requests return the URL-only posts instead of blocking the digest.lib/supabase/apify_scraper_runs/selectApifyScraperRunsByBatch.ts (1)
5-7: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueWire the digest flow to the new selector, or drop it.
lib/apify/digest/maybeSendScrapeDigest.tsstill callsselectApifyScraperRuns({ batchId }), soselectApifyScraperRunsByBatchis unused as-is. Either switch the consumer to this helper or remove the duplicate selector.🤖 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/apify_scraper_runs/selectApifyScraperRunsByBatch.ts` around lines 5 - 7, The new batch-specific selector is currently unused, while maybeSendScrapeDigest still calls selectApifyScraperRuns with a batchId filter. Update the digest flow in maybeSendScrapeDigest to use selectApifyScraperRunsByBatch, or remove selectApifyScraperRunsByBatch if the generic selector remains the intended path. Make sure the chosen selector and its consumer stay aligned so there is only one source of truth for fetching scraper runs by batch.lib/apify/digest/renderScrapeDigestHtml.ts (1)
25-31: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider a vetted encoder instead of hand-rolled
escapeHtml.The implementation is correct for
"-delimited attributes, but a vetted library (e.g.,escape-htmlorhe) would provide defense-in-depth and reduce the maintenance surface. The static analyzer also flagged this pattern (CWE-79). This can be deferred — the immediate gap is the missing escaping on URLs flagged above.🤖 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/apify/digest/renderScrapeDigestHtml.ts` around lines 25 - 31, The hand-rolled escapeHtml helper in renderScrapeDigestHtml should be replaced with a vetted HTML encoder (for example, escape-html or he) to reduce maintenance risk and align with the analyzer warning. Update the renderScrapeDigestHtml flow to use that library consistently wherever HTML text or attribute content is escaped, and keep the existing helper name or replace it with the library-backed equivalent so the call sites are easy to locate.Source: Linters/SAST tools
🤖 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/apify/digest/renderScrapeDigestHtml.ts`:
- Around line 69-75: The HTML rendering in renderScrapeDigestHtml is
interpolating external URL values directly into href and src attributes without
escaping, which allows attribute breakout from post.url and post.thumbnailUrl.
Update the URL handling around thumbCell and the post link so both values are
escaped for HTML attribute context before being inserted, while preserving the
existing http/https normalization for post.url.
In `@lib/apify/sendApifyWebhookEmail.ts`:
- Line 29: The inline object in the `sections` array within
`sendApifyWebhookEmail` needs to be reformatted to satisfy Prettier’s
line-length rules. Break the `platform: "instagram"` entry and its
`extractPostsFromDatasetItems` call across multiple lines in the `sections`
literal so the array item matches the surrounding formatting style and no line
exceeds the limit.
In `@lib/supabase/apify_scraper_runs/completeApifyScraperRun.ts`:
- Around line 1-24: The function in completeApifyScraperRun performs an update,
so it should follow the update* naming convention instead of complete*.
Consolidate this duplicated logic with the existing updateApifyScraperRun by
having that shared updater return ApifyScraperRunRow | null and handle
completed_at and new_post_urls there, then remove or rename this helper so the
database write lives in one place. Use the completeApifyScraperRun and
updateApifyScraperRun symbols to locate the duplicated update path.
In `@lib/supabase/apify_scraper_runs/selectApifyScraperRunsByBatch.ts`:
- Around line 8-16: The query in selectApifyScraperRunsByBatch currently filters
by batch_id but does not enforce a stable row order, which can make downstream
digest section ordering vary between runs. Update the Supabase query in
selectApifyScraperRunsByBatch to include an explicit ORDER BY using a
deterministic column such as created_at or platform before returning the
ApifyScraperRunRow[] results, so the HTML rendering order is consistent.
---
Nitpick comments:
In `@lib/apify/digest/getRunDigestSection.ts`:
- Around line 22-31: The Apify enrichment flow in getRunDigestSection currently
waits indefinitely on apifyClient.run(run.run_id).get() and
apifyClient.dataset(datasetId).listItems(), so a slow request can stall
maybeSendScrapeDigest despite the URL-only fallback. Add explicit timeouts
around both external calls, either by racing each request against a timer or by
wiring an AbortController into the Apify client calls. Keep the existing
fallback path in getRunDigestSection so timed-out requests return the URL-only
posts instead of blocking the digest.
In `@lib/apify/digest/renderScrapeDigestHtml.ts`:
- Around line 25-31: The hand-rolled escapeHtml helper in renderScrapeDigestHtml
should be replaced with a vetted HTML encoder (for example, escape-html or he)
to reduce maintenance risk and align with the analyzer warning. Update the
renderScrapeDigestHtml flow to use that library consistently wherever HTML text
or attribute content is escaped, and keep the existing helper name or replace it
with the library-backed equivalent so the call sites are easy to locate.
In `@lib/supabase/apify_scraper_runs/selectApifyScraperRunsByBatch.ts`:
- Around line 5-7: The new batch-specific selector is currently unused, while
maybeSendScrapeDigest still calls selectApifyScraperRuns with a batchId filter.
Update the digest flow in maybeSendScrapeDigest to use
selectApifyScraperRunsByBatch, or remove selectApifyScraperRunsByBatch if the
generic selector remains the intended path. Make sure the chosen selector and
its consumer stay aligned so there is only one source of truth for fetching
scraper runs by batch.
🪄 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: 18476c6d-5306-4018-b60d-b7efa79c2e86
⛔ Files ignored due to path filters (6)
lib/apify/__tests__/sendApifyWebhookEmail.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/apify/digest/__tests__/extractPostsFromDatasetItems.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/apify/digest/__tests__/maybeSendScrapeDigest.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/apify/digest/__tests__/renderScrapeDigestHtml.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/apify/digest/__tests__/sendScrapeDigestEmail.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/apify/instagram/__tests__/handleInstagramProfileScraperResults.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (11)
lib/apify/digest/extractPostsFromDatasetItems.tslib/apify/digest/getRunDigestSection.tslib/apify/digest/maybeSendScrapeDigest.tslib/apify/digest/renderScrapeDigestHtml.tslib/apify/digest/sendScrapeDigestEmail.tslib/apify/instagram/handleInstagramProfileScraperResults.tslib/apify/sendApifyWebhookEmail.tslib/supabase/apify_scraper_runs/completeApifyScraperRun.tslib/supabase/apify_scraper_runs/insertApifyScraperRuns.tslib/supabase/apify_scraper_runs/selectApifyScraperRunsByBatch.tslib/supabase/apify_scraper_runs/types.ts
| const { data, error } = await supabase | ||
| .from("apify_scraper_runs" as never) | ||
| .select("*") | ||
| .eq("batch_id", batchId); | ||
| if (error) { | ||
| console.error("[ERROR] selectApifyScraperRunsByBatch:", error); | ||
| return []; | ||
| } | ||
| return (data as ApifyScraperRunRow[] | null) ?? []; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add ORDER BY to ensure deterministic section ordering.
The PR objectives call for deterministic digest output, but this query has no ORDER BY clause. Without it, PostgreSQL may return rows in any order, making the section order in the rendered HTML non-deterministic. Add an explicit sort (e.g., by created_at or platform).
🔒 Proposed fix
const { data, error } = await supabase
.from("apify_scraper_runs" as never)
.select("*")
- .eq("batch_id", batchId);
+ .eq("batch_id", batchId)
+ .order("created_at", { ascending: true });📝 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 { data, error } = await supabase | |
| .from("apify_scraper_runs" as never) | |
| .select("*") | |
| .eq("batch_id", batchId); | |
| if (error) { | |
| console.error("[ERROR] selectApifyScraperRunsByBatch:", error); | |
| return []; | |
| } | |
| return (data as ApifyScraperRunRow[] | null) ?? []; | |
| const { data, error } = await supabase | |
| .from("apify_scraper_runs" as never) | |
| .select("*") | |
| .eq("batch_id", batchId) | |
| .order("created_at", { ascending: true }); | |
| if (error) { | |
| console.error("[ERROR] selectApifyScraperRunsByBatch:", error); | |
| return []; | |
| } | |
| return (data as ApifyScraperRunRow[] | null) ?? []; |
🤖 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/apify_scraper_runs/selectApifyScraperRunsByBatch.ts` around
lines 8 - 16, The query in selectApifyScraperRunsByBatch currently filters by
batch_id but does not enforce a stable row order, which can make downstream
digest section ordering vary between runs. Update the Supabase query in
selectApifyScraperRunsByBatch to include an explicit ORDER BY using a
deterministic column such as created_at or platform before returning the
ApifyScraperRunRow[] results, so the HTML rendering order is consistent.
Template upgrade: media cards + house styling — 2026-07-09Follow-up to the earlier verification after feedback that the v1 deterministic template regressed visual quality vs the old LLM emails (a bare What changed (commits
Visual sample (exact renderer output with placeholder images): https://claude.ai/code/artifact/b93ea147-9be3-4d37-bfa7-c63e35cdaaf6 Live verification on preview Known trade-off: thumbnail URLs are platform-CDN links with expiring signatures (IG Fixture fully cleaned (0 rows remaining). CI green on the final head after a prettier pass. |
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
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/apify/digest/renderScrapeDigestHtml.ts">
<violation number="1" location="lib/apify/digest/renderScrapeDigestHtml.ts:69">
P2: Post and thumbnail URLs from scrape results are interpolated directly into `href` and `src` attributes without HTML escaping. If a scraped URL contains a double-quote character it could break the attribute boundary. Consider applying `escapeHtml(post.url)` / `escapeHtml(post.thumbnailUrl)` for defense-in-depth, or at minimum ensure upstream `post.url` is validated.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const label = PLATFORM_LABELS[section.platform.toLowerCase()] ?? section.platform; | ||
| const cards = section.posts | ||
| .map(post => { | ||
| const href = post.url.startsWith("http") ? post.url : `https://${post.url}`; |
There was a problem hiding this comment.
P2: Post and thumbnail URLs from scrape results are interpolated directly into href and src attributes without HTML escaping. If a scraped URL contains a double-quote character it could break the attribute boundary. Consider applying escapeHtml(post.url) / escapeHtml(post.thumbnailUrl) for defense-in-depth, or at minimum ensure upstream post.url is validated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/apify/digest/renderScrapeDigestHtml.ts, line 69:
<comment>Post and thumbnail URLs from scrape results are interpolated directly into `href` and `src` attributes without HTML escaping. If a scraped URL contains a double-quote character it could break the attribute boundary. Consider applying `escapeHtml(post.url)` / `escapeHtml(post.thumbnailUrl)` for defense-in-depth, or at minimum ensure upstream `post.url` is validated.</comment>
<file context>
@@ -25,29 +58,39 @@ export function renderScrapeDigestHtml({
- return `<li style="margin:4px 0"><a href="${href}" style="color:#111;text-decoration:underline">${href}</a></li>`;
+ const cards = section.posts
+ .map(post => {
+ const href = post.url.startsWith("http") ? post.url : `https://${post.url}`;
+ const caption = post.caption ? escapeHtml(truncate(post.caption, 110)) : "";
+ const date = post.timestamp ? formatDate(post.timestamp) : "";
</file context>
…ats + fixed chat CTA Three feedback items on the template: - The digest header/subject said 'Your artist' — the assembler never passed a name. getRunDigestSection now also extracts the profile display name from the dataset (IG fullName, TikTok authorMeta.nickName) and maybeSendScrapeDigest addresses the email with the first platform's name. - Post cards now carry compact engagement stats (12.3K likes · 678 comments · 1.2M views · shares) mapped from platform counts (IG likesCount/commentsCount/videoViewCount, TikTok diggCount/commentCount/ playCount/shareCount); omitted entirely when the scraper returns none. - The CTA now always points at https://chat.recoupable.dev — previously it derived from the deployment base URL, which on previews is the API deployment itself. Funnel-tracking landing page tracked as follow-up. 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. |
There was a problem hiding this comment.
3 issues found across 9 files (changes from recent commits).
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/apify/digest/maybeSendScrapeDigest.ts">
<violation number="1" location="lib/apify/digest/maybeSendScrapeDigest.ts:29">
P2: The `artistName` used in the digest subject and heading is chosen from whichever platform section happens to come first, but `selectApifyScraperRuns` does not specify an `order` clause so the row order (and therefore the section order) is database-dependent. If platforms report different display names for the same artist, the same batch could produce different customer-visible output on different sends, which undermines the deterministic rendering goal. Consider either adding an explicit `order` (e.g., by platform or created_at) so the source of `artistName` is stable, or selecting a canonical name from a preferred platform instead of whichever happens to appear first.</violation>
</file>
<file name="lib/apify/digest/extractPostsFromDatasetItems.ts">
<violation number="1" location="lib/apify/digest/extractPostsFromDatasetItems.ts:13">
P2: The `num` helper accepts negative and fractional numbers from external Apify dataset items, and those values flow directly into customer-facing digest emails through `formatCount` in `renderScrapeDigestHtml.ts`. Engagement counts (`likes`, `views`, `comments`, `shares`) should be non-negative integers; a sentinel like `-1` or a malformed fractional value would render as "-1 likes" or "1.5 views" in the email body. Restrict the helper to the domain of valid engagement counts so bad data is silently dropped rather than shown to customers.</violation>
</file>
<file name="lib/apify/digest/renderScrapeDigestHtml.ts">
<violation number="1" location="lib/apify/digest/renderScrapeDigestHtml.ts:56">
P2: The compact count formatter can produce awkward customer-visible stats like `"1000K"` for values just below one million (e.g., 999,950) and `"2000M"` for billion-scale values. The unit is chosen before `toFixed(1)` rounds, so `999.95` rounds to `"1000.0"`, strips the `.0`, and becomes `"1000K"` instead of rolling over to `"1M"`. Consider switching the unit based on the rounded result, or using explicit thresholds that account for this carry.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ); | ||
| return await sendScrapeDigestEmail({ emails, sections }); | ||
| // A batch is one artist's scrape — any platform's profile name addresses it. | ||
| const artistName = sections.map(s => s.artistName).find(Boolean) ?? null; |
There was a problem hiding this comment.
P2: The artistName used in the digest subject and heading is chosen from whichever platform section happens to come first, but selectApifyScraperRuns does not specify an order clause so the row order (and therefore the section order) is database-dependent. If platforms report different display names for the same artist, the same batch could produce different customer-visible output on different sends, which undermines the deterministic rendering goal. Consider either adding an explicit order (e.g., by platform or created_at) so the source of artistName is stable, or selecting a canonical name from a preferred platform instead of whichever happens to appear first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/apify/digest/maybeSendScrapeDigest.ts, line 29:
<comment>The `artistName` used in the digest subject and heading is chosen from whichever platform section happens to come first, but `selectApifyScraperRuns` does not specify an `order` clause so the row order (and therefore the section order) is database-dependent. If platforms report different display names for the same artist, the same batch could produce different customer-visible output on different sends, which undermines the deterministic rendering goal. Consider either adding an explicit `order` (e.g., by platform or created_at) so the source of `artistName` is stable, or selecting a canonical name from a preferred platform instead of whichever happens to appear first.</comment>
<file context>
@@ -18,12 +18,14 @@ export async function maybeSendScrapeDigest(batchId: string | null | undefined)
);
- return await sendScrapeDigestEmail({ emails, sections });
+ // A batch is one artist's scrape — any platform's profile name addresses it.
+ const artistName = sections.map(s => s.artistName).find(Boolean) ?? null;
+ return await sendScrapeDigestEmail({ emails, sections, artistName });
}
</file context>
|
|
||
| const str = (v: unknown): string | null => (typeof v === "string" && v ? v : null); | ||
|
|
||
| const num = (v: unknown): number | null => (typeof v === "number" && isFinite(v) ? v : null); |
There was a problem hiding this comment.
P2: The num helper accepts negative and fractional numbers from external Apify dataset items, and those values flow directly into customer-facing digest emails through formatCount in renderScrapeDigestHtml.ts. Engagement counts (likes, views, comments, shares) should be non-negative integers; a sentinel like -1 or a malformed fractional value would render as "-1 likes" or "1.5 views" in the email body. Restrict the helper to the domain of valid engagement counts so bad data is silently dropped rather than shown to customers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/apify/digest/extractPostsFromDatasetItems.ts, line 13:
<comment>The `num` helper accepts negative and fractional numbers from external Apify dataset items, and those values flow directly into customer-facing digest emails through `formatCount` in `renderScrapeDigestHtml.ts`. Engagement counts (`likes`, `views`, `comments`, `shares`) should be non-negative integers; a sentinel like `-1` or a malformed fractional value would render as "-1 likes" or "1.5 views" in the email body. Restrict the helper to the domain of valid engagement counts so bad data is silently dropped rather than shown to customers.</comment>
<file context>
@@ -7,6 +10,14 @@ const asRecord = (v: unknown): Record<string, unknown> =>
const str = (v: unknown): string | null => (typeof v === "string" && v ? v : null);
+const num = (v: unknown): number | null => (typeof v === "number" && isFinite(v) ? v : null);
+
+/** Drops the stats object entirely when the scraper returned no counts. */
</file context>
| const num = (v: unknown): number | null => (typeof v === "number" && isFinite(v) ? v : null); | |
| const num = (v: unknown): number | null => (typeof v === "number" && Number.isInteger(v) && v >= 0 ? v : null); |
| return text.length <= max ? text : `${text.slice(0, max - 1).trimEnd()}…`; | ||
| } | ||
|
|
||
| /** Deterministic compact count: 678, 12.3K, 1.2M (trailing .0 stripped). */ |
There was a problem hiding this comment.
P2: The compact count formatter can produce awkward customer-visible stats like "1000K" for values just below one million (e.g., 999,950) and "2000M" for billion-scale values. The unit is chosen before toFixed(1) rounds, so 999.95 rounds to "1000.0", strips the .0, and becomes "1000K" instead of rolling over to "1M". Consider switching the unit based on the rounded result, or using explicit thresholds that account for this carry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/apify/digest/renderScrapeDigestHtml.ts, line 56:
<comment>The compact count formatter can produce awkward customer-visible stats like `"1000K"` for values just below one million (e.g., 999,950) and `"2000M"` for billion-scale values. The unit is chosen before `toFixed(1)` rounds, so `999.95` rounds to `"1000.0"`, strips the `.0`, and becomes `"1000K"` instead of rolling over to `"1M"`. Consider switching the unit based on the rounded result, or using explicit thresholds that account for this carry.</comment>
<file context>
@@ -41,6 +53,23 @@ function truncate(text: string, max: number): string {
return text.length <= max ? text : `${text.slice(0, max - 1).trimEnd()}…`;
}
+/** Deterministic compact count: 678, 12.3K, 1.2M (trailing .0 stripped). */
+function formatCount(n: number): string {
+ if (n < 1000) return String(n);
</file context>
…oter
Header is now a two-cell row with the brand icon top-right (hosted PNG —
email clients don't render SVG) linking to recoupable.com. Footer names
the artist: 'because {artist} is in your roster on Recoup', falling back
to 'this artist' when no profile name resolved.
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. |
|
Feedback round 3 ( |
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 9 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
SRP - each function definition needs a standalone lib file.
There was a problem hiding this comment.
Split in f3bcf90: extractInstagramPosts, extractTiktokPosts, buildPostStats, and the asRecord/asStringOrNull/asNumberOrNull coercers each in standalone files; this file keeps only extractPostsFromDatasetItems. 307 tests green, tsc + lint clean after the split.
| const CHAT_APP_URL = "https://chat.recoupable.dev"; | ||
| const WEBSITE_URL = "https://recoupable.com"; |
There was a problem hiding this comment.
DRY - do these consts already exist shared in a different part of the app we can reference to reduce duplication?
There was a problem hiding this comment.
Checked — no shared home existed for any of these. In f3bcf90: CHAT_APP_URL / WEBSITE_URL / RECOUP_LOGO_URL now live in lib/const.ts alongside DOCS_BASE_URL; platform labels had no prior shared source, so getPlatformLabel.ts is now the single owner.
| const MONTHS = ["Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"]; | ||
|
|
||
| /** Escapes scraped text so captions can never inject markup into the email. */ | ||
| function escapeHtml(text: string): string { |
There was a problem hiding this comment.
SRP - new lib file for escapeHtml
There was a problem hiding this comment.
Moved to lib/emails/escapeHtml.ts in f3bcf90 (checked repo-wide first — no existing escapeHtml to reuse; email-domain is its natural shared home).
| } | ||
|
|
||
| /** Deterministic "Jul 8" date label (UTC — no locale/timezone variance). */ | ||
| function formatDate(iso: string): string { |
There was a problem hiding this comment.
SRP - new lib file for any function defined in this file other than the function whose name matches the file name.
There was a problem hiding this comment.
Split in f3bcf90: formatUtcDateLabel, truncateText, formatCompactCount, formatPostStats, getPlatformLabel each in their own lib file; this file now contains only renderScrapeDigestHtml.
There was a problem hiding this comment.
KISS
- actual: lib/supabase/apify_scraper_runs/completeApifyScraperRun.ts
- required: lib/supabase/apify_scraper_runs/updateApifyScraperRun.ts
There was a problem hiding this comment.
Deleted in f3bcf90 — orphaned pre-rename duplicate; updateApifyScraperRun.ts from main is the wired one.
There was a problem hiding this comment.
KISS
- actual: lib/supabase/apify_scraper_runs/insertApifyScraperRuns.ts
- required: lib/supabase/apify_scraper_runs/upsertApifyScraperRuns.ts
There was a problem hiding this comment.
Deleted in f3bcf90 — orphaned pre-rename duplicate; upsertApifyScraperRuns.ts from main is the wired one.
There was a problem hiding this comment.
KISS
- actual: lib/supabase/apify_scraper_runs/selectApifyScraperRunsByBatch.ts
- required: lib/supabase/apify_scraper_runs/selectApifyScraperRuns.ts
There was a problem hiding this comment.
Deleted in f3bcf90 — orphaned pre-rename duplicate (see the types.ts thread); selectApifyScraperRuns.ts from main is the wired one.
There was a problem hiding this comment.
DRY - use the shared database schema instead of redefining existing types.
There was a problem hiding this comment.
Deleted in f3bcf90 — this whole file was an orphaned duplicate: the branch predates #760's renames, and merging main added the renamed set (which uses the generated Tables<"apify_scraper_runs"> types) alongside the branch's inherited old set with zero conflicts. The live code imports only the generated types.
…orphaned pre-rename supabase files Review feedback: - Every helper gets its own lib file: escapeHtml (lib/emails — repo had none), formatUtcDateLabel, truncateText, formatCompactCount, formatPostStats, getPlatformLabel, extractInstagramPosts, extractTiktokPosts, buildPostStats, asRecord/asStringOrNull/ asNumberOrNull coercers. renderScrapeDigestHtml and extractPostsFromDatasetItems now contain only their eponymous functions. - CHAT_APP_URL / WEBSITE_URL / RECOUP_LOGO_URL move to lib/const.ts as shared constants (no existing shared home found for platform labels — getPlatformLabel is the new one). - Deletes 4 orphaned pre-rename files under lib/supabase/apify_scraper_runs (completeApifyScraperRun, insertApifyScraperRuns, selectApifyScraperRunsByBatch, types): this branch predates #760's renames, so the merge of main added the renamed set alongside the branch's inherited old set — both landed with zero conflicts and zero references to the old files. Also confirms no legacy LLM email generation remains in lib/apify (generateText usage gone with the template rewrite). 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/apify/digest/renderScrapeDigestHtml.ts`:
- Around line 51-58: Escape both the normalized URL used by href attributes and
post.thumbnailUrl before interpolating them into HTML in the digest rendering
function. Preserve the existing protocol normalization and conditional thumbnail
behavior, but apply escapeHtml to each external URL specifically at the
attribute boundary to prevent quote-based markup injection.
🪄 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: b824eb9b-33ec-41f9-8b4e-22b8408aa2a2
⛔ Files ignored due to path filters (4)
lib/apify/digest/__tests__/extractArtistNameFromDatasetItems.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/apify/digest/__tests__/extractPostsFromDatasetItems.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/apify/digest/__tests__/maybeSendScrapeDigest.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/apify/digest/__tests__/renderScrapeDigestHtml.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (19)
lib/apify/digest/asNumberOrNull.tslib/apify/digest/asRecord.tslib/apify/digest/asStringOrNull.tslib/apify/digest/buildPostStats.tslib/apify/digest/extractArtistNameFromDatasetItems.tslib/apify/digest/extractInstagramPosts.tslib/apify/digest/extractPostsFromDatasetItems.tslib/apify/digest/extractTiktokPosts.tslib/apify/digest/formatCompactCount.tslib/apify/digest/formatPostStats.tslib/apify/digest/formatUtcDateLabel.tslib/apify/digest/getPlatformLabel.tslib/apify/digest/getRunDigestSection.tslib/apify/digest/maybeSendScrapeDigest.tslib/apify/digest/renderScrapeDigestHtml.tslib/apify/digest/truncateText.tslib/apify/sendApifyWebhookEmail.tslib/const.tslib/emails/escapeHtml.ts
✅ Files skipped from review due to trivial changes (3)
- lib/apify/digest/asNumberOrNull.ts
- lib/apify/digest/asStringOrNull.ts
- lib/apify/digest/truncateText.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/apify/digest/maybeSendScrapeDigest.ts
- lib/apify/digest/getRunDigestSection.ts
- lib/apify/sendApifyWebhookEmail.ts
| const href = post.url.startsWith("http") ? post.url : `https://${post.url}`; | ||
| const caption = post.caption ? escapeHtml(truncateText(post.caption, 110)) : ""; | ||
| const date = post.timestamp ? formatUtcDateLabel(post.timestamp) : ""; | ||
| const stats = post.stats ? formatPostStats(post.stats) : ""; | ||
| const thumbCell = post.thumbnailUrl | ||
| ? `<td width="72" valign="top" style="padding:0 14px 0 0"><a href="${href}"><img src="${post.thumbnailUrl}" width="72" height="72" alt="" style="display:block;width:72px;height:72px;object-fit:cover;border-radius:10px;border:1px solid #e8e8e8"/></a></td>` | ||
| : ""; | ||
| return `<table role="presentation" width="100%" cellpadding="0" cellspacing="0" style="border:1px solid #e8e8e8;border-radius:12px;margin:0 0 10px"><tr><td style="padding:14px"><table role="presentation" width="100%" cellpadding="0" cellspacing="0"><tr>${thumbCell}<td valign="top">${caption ? `<p style="margin:0 0 6px;font-size:14px;line-height:1.45;color:#0a0a0a">${caption}</p>` : ""}${stats ? `<p style="margin:0 0 6px;font-size:12px;color:#6b6b6b">${stats}</p>` : ""}<p style="margin:0;font-size:13px">${date ? `<span style="color:#6b6b6b">${date} · </span>` : ""}<a href="${href}" style="color:#0a0a0a;font-weight:600;text-decoration:underline">View post →</a></p></td></tr></table></td></tr></table>`; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
href and thumbnailUrl are still unescaped in attribute contexts — past fix appears to have regressed.
A previous review flagged this exact issue and it was marked addressed, but the current code still interpolates post.url and post.thumbnailUrl directly into href="..." and src="..." without escapeHtml. A URL containing " breaks out of the attribute, enabling markup injection. The startsWith("http") check blocks javascript: protocols but does not prevent attribute breakout. These are external scraped values from Apify datasets.
🔒 Proposed fix: escape URLs in attribute contexts
- const href = post.url.startsWith("http") ? post.url : `https://${post.url}`;
+ const href = escapeHtml(post.url.startsWith("http") ? post.url : `https://${post.url}`);
const caption = post.caption ? escapeHtml(truncateText(post.caption, 110)) : "";
const date = post.timestamp ? formatUtcDateLabel(post.timestamp) : "";
const stats = post.stats ? formatPostStats(post.stats) : "";
const thumbCell = post.thumbnailUrl
- ? `<td width="72" valign="top" style="padding:0 14px 0 0"><a href="${href}"><img src="${post.thumbnailUrl}" width="72" height="72" alt="" style="display:block;width:72px;height:72px;object-fit:cover;border-radius:10px;border:1px solid `#e8e8e8`"/></a></td>`
+ ? `<td width="72" valign="top" style="padding:0 14px 0 0"><a href="${href}"><img src="${escapeHtml(post.thumbnailUrl)}" width="72" height="72" alt="" style="display:block;width:72px;height:72px;object-fit:cover;border-radius:10px;border:1px solid `#e8e8e8`"/></a></td>`
: "";🤖 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/apify/digest/renderScrapeDigestHtml.ts` around lines 51 - 58, Escape both
the normalized URL used by href attributes and post.thumbnailUrl before
interpolating them into HTML in the digest rendering function. Preserve the
existing protocol normalization and conditional thumbnail behavior, but apply
escapeHtml to each external URL specifically at the attribute boundary to
prevent quote-based markup injection.
Source: Linters/SAST tools
There was a problem hiding this comment.
4 issues found across 19 files (changes from recent commits).
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/apify/digest/extractTiktokPosts.ts">
<violation number="1" location="lib/apify/digest/extractTiktokPosts.ts:24">
P2: TikTok timestamps should be normalized through `toIsoDate` before being assigned to the digest post, matching the existing database-side normalization in `handleTiktokProfileScraperResults.ts`. Raw `createTimeISO` strings may be garbage or non-ISO, and passing them through as-is defeats the unparseable-date fallback and the review guidance that calls for normalization. Consider replacing the raw `asStringOrNull` call with `toIsoDate` so invalid dates are dropped cleanly.</violation>
</file>
<file name="lib/apify/digest/truncateText.ts">
<violation number="1" location="lib/apify/digest/truncateText.ts:2">
P3: The `truncateText` helper doesn't guard against non-positive `max` values. When `max` is `0` (or negative), `text.slice(0, max - 1)` can return the entire string or most of it, producing output longer than the requested limit and breaking the truncation contract. Since this is an exported utility, adding a simple floor check (e.g., `if (max <= 0) return ""`) would make it safe for future callers without affecting the existing hardcoded usage.</violation>
</file>
<file name="lib/apify/digest/getPlatformLabel.ts">
<violation number="1" location="lib/apify/digest/getPlatformLabel.ts:13">
P2: The customer-facing platform label fallback returns the raw platform key unchanged when it is missing from the whitelist. This risks exposing internal/platform jargon to customers and is interpolated into HTML without escaping, while other customer-facing strings in the same renderer (`caption`, `artistName`, `who`) are all passed through `escapeHtml`. A safer fallback like `"Other"` (and wrapping the label in `escapeHtml` for consistency with the rest of the renderer) would better align with the PR goal of keeping internal terminology out of customer-facing output.</violation>
</file>
<file name="lib/apify/digest/formatCompactCount.ts">
<violation number="1" location="lib/apify/digest/formatCompactCount.ts:2">
P2: The formatter lacks a runtime guard for non-finite numbers, which means `NaN` or `Infinity` from scraped Apify data could leak into customer emails as gibberish like `"NaNM likes"` or `"InfinityM views"`. The `!= null` checks in `formatPostStats` do not block `NaN`, so adding a defensive `Number.isFinite` guard here prevents bad external data from reaching the digest.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| url, | ||
| caption: asStringOrNull(item.text), | ||
| thumbnailUrl: asStringOrNull(asRecord(item.videoMeta).coverUrl), | ||
| timestamp: asStringOrNull(item.createTimeISO), |
There was a problem hiding this comment.
P2: TikTok timestamps should be normalized through toIsoDate before being assigned to the digest post, matching the existing database-side normalization in handleTiktokProfileScraperResults.ts. Raw createTimeISO strings may be garbage or non-ISO, and passing them through as-is defeats the unparseable-date fallback and the review guidance that calls for normalization. Consider replacing the raw asStringOrNull call with toIsoDate so invalid dates are dropped cleanly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/apify/digest/extractTiktokPosts.ts, line 24:
<comment>TikTok timestamps should be normalized through `toIsoDate` before being assigned to the digest post, matching the existing database-side normalization in `handleTiktokProfileScraperResults.ts`. Raw `createTimeISO` strings may be garbage or non-ISO, and passing them through as-is defeats the unparseable-date fallback and the review guidance that calls for normalization. Consider replacing the raw `asStringOrNull` call with `toIsoDate` so invalid dates are dropped cleanly.</comment>
<file context>
@@ -0,0 +1,29 @@
+ url,
+ caption: asStringOrNull(item.text),
+ thumbnailUrl: asStringOrNull(asRecord(item.videoMeta).coverUrl),
+ timestamp: asStringOrNull(item.createTimeISO),
+ ...(stats && { stats }),
+ });
</file context>
|
|
||
| /** Customer-facing label for a scraper platform key. */ | ||
| export function getPlatformLabel(platform: string): string { | ||
| return PLATFORM_LABELS[platform.toLowerCase()] ?? platform; |
There was a problem hiding this comment.
P2: The customer-facing platform label fallback returns the raw platform key unchanged when it is missing from the whitelist. This risks exposing internal/platform jargon to customers and is interpolated into HTML without escaping, while other customer-facing strings in the same renderer (caption, artistName, who) are all passed through escapeHtml. A safer fallback like "Other" (and wrapping the label in escapeHtml for consistency with the rest of the renderer) would better align with the PR goal of keeping internal terminology out of customer-facing output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/apify/digest/getPlatformLabel.ts, line 13:
<comment>The customer-facing platform label fallback returns the raw platform key unchanged when it is missing from the whitelist. This risks exposing internal/platform jargon to customers and is interpolated into HTML without escaping, while other customer-facing strings in the same renderer (`caption`, `artistName`, `who`) are all passed through `escapeHtml`. A safer fallback like `"Other"` (and wrapping the label in `escapeHtml` for consistency with the rest of the renderer) would better align with the PR goal of keeping internal terminology out of customer-facing output.</comment>
<file context>
@@ -0,0 +1,14 @@
+
+/** Customer-facing label for a scraper platform key. */
+export function getPlatformLabel(platform: string): string {
+ return PLATFORM_LABELS[platform.toLowerCase()] ?? platform;
+}
</file context>
| @@ -0,0 +1,6 @@ | |||
| /** Deterministic compact count: 678, 12.3K, 1.2M (trailing .0 stripped). */ | |||
| export function formatCompactCount(n: number): string { | |||
There was a problem hiding this comment.
P2: The formatter lacks a runtime guard for non-finite numbers, which means NaN or Infinity from scraped Apify data could leak into customer emails as gibberish like "NaNM likes" or "InfinityM views". The != null checks in formatPostStats do not block NaN, so adding a defensive Number.isFinite guard here prevents bad external data from reaching the digest.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/apify/digest/formatCompactCount.ts, line 2:
<comment>The formatter lacks a runtime guard for non-finite numbers, which means `NaN` or `Infinity` from scraped Apify data could leak into customer emails as gibberish like `"NaNM likes"` or `"InfinityM views"`. The `!= null` checks in `formatPostStats` do not block `NaN`, so adding a defensive `Number.isFinite` guard here prevents bad external data from reaching the digest.</comment>
<file context>
@@ -0,0 +1,6 @@
+/** Deterministic compact count: 678, 12.3K, 1.2M (trailing .0 stripped). */
+export function formatCompactCount(n: number): string {
+ if (n < 1000) return String(n);
+ const [value, unit] = n < 1_000_000 ? [n / 1000, "K"] : [n / 1_000_000, "M"];
</file context>
| export function formatCompactCount(n: number): string { | |
| export function formatCompactCount(n: number): string { | |
| if (!Number.isFinite(n)) return "0"; | |
| if (n < 1000) return String(n); | |
| const [value, unit] = n < 1_000_000 ? [n / 1000, "K"] : [n / 1_000_000, "M"]; | |
| return `${value.toFixed(1).replace(/\.0$/, "")}${unit}`; | |
| } |
| @@ -0,0 +1,4 @@ | |||
| /** Truncates to `max` characters with a trailing ellipsis. */ | |||
| export function truncateText(text: string, max: number): string { | |||
There was a problem hiding this comment.
P3: The truncateText helper doesn't guard against non-positive max values. When max is 0 (or negative), text.slice(0, max - 1) can return the entire string or most of it, producing output longer than the requested limit and breaking the truncation contract. Since this is an exported utility, adding a simple floor check (e.g., if (max <= 0) return "") would make it safe for future callers without affecting the existing hardcoded usage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/apify/digest/truncateText.ts, line 2:
<comment>The `truncateText` helper doesn't guard against non-positive `max` values. When `max` is `0` (or negative), `text.slice(0, max - 1)` can return the entire string or most of it, producing output longer than the requested limit and breaking the truncation contract. Since this is an exported utility, adding a simple floor check (e.g., `if (max <= 0) return ""`) would make it safe for future callers without affecting the existing hardcoded usage.</comment>
<file context>
@@ -0,0 +1,4 @@
+/** Truncates to `max` characters with a trailing ellipsis. */
+export function truncateText(text: string, max: number): string {
+ return text.length <= max ? text : `${text.slice(0, max - 1).trimEnd()}…`;
+}
</file context>
| const EXTRACTORS: Record<string, (items: unknown[]) => Map<string, ScrapeDigestPost>> = { | ||
| instagram: extractInstagramPosts, | ||
| tiktok: extractTiktokPosts, | ||
| }; |
There was a problem hiding this comment.
Why aren't there extractors for Twitter, YouTube and LinkedIn?
There was a problem hiding this comment.
Good catch on Twitter — its results handler already computes newPostUrls (it feeds the digest today), so its sections were silently degrading to URL-only links. 8dc8ba3 adds extractTwitterPosts (apidojo fields: fullText, extendedEntities.media, createdAt via toIsoDate, like/reply/view/retweet counts), registered under both twitter and x, plus author-name extraction. YouTube and LinkedIn are deliberately omitted: their handlers persist only the socials row — no posts, no newPostUrls — so they never produce digest sections today; extractors would be dead code until post persistence ships for those platforms (chat#1833 territory). Verified against a real apidojo dataset shape; 311 tests green.
| const PLATFORM_LABELS: Record<string, string> = { | ||
| instagram: "Instagram", | ||
| tiktok: "TikTok", | ||
| x: "X", | ||
| twitter: "X", | ||
| youtube: "YouTube", | ||
| facebook: "Facebook", | ||
| threads: "Threads", | ||
| }; |
There was a problem hiding this comment.
Why isn't LinkedIn in this list?
There was a problem hiding this comment.
Added in 8dc8ba3 — LinkedIn is a registered scrape platform, so it belongs in the label map even though its digest sections can't appear yet (see the extractor thread).
Review feedback: Twitter's handler already feeds the digest (filterNewPostUrls -> newPostUrls) but had no extractor, so its sections degraded to URL-only links. extractTwitterPosts maps apidojo tweet items (fullText, extendedEntities media, createdAt via toIsoDate, like/reply/view/retweet counts), registered under both 'twitter' and 'x'. Artist-name extraction gains the same aliases (author.name ?? userName). LinkedIn added to platform labels. YouTube and LinkedIn deliberately have no extractors: their results handlers persist only the social row (no posts, no newPostUrls), so they never contribute digest sections today — extractors would be dead code until post persistence ships for them (chat#1833 territory). 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. |
There was a problem hiding this comment.
0 issues found across 7 files (changes from recent commits).
Requires human review: Auto-approval blocked by 11 unresolved issues from previous reviews.
Re-trigger cubic
…eets or replies Feedback from a real-account digest test: an all-retweet X section reported hundreds of likes the artist never earned (retweet items carry the ORIGINAL author's stats). isOriginalTweet keeps originals and quote tweets (the artist's own words and metrics), drops retweets and replies, applied at the persistence layer in handleTwitterProfileScraperResults so both stored posts and digest sections stay accurate. Items without flags pass (defensive default on schema drift). 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. |
|
Feedback fix ( |
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Requires human review: Auto-approval blocked by 11 unresolved issues from previous reviews.
Re-trigger cubic
…vives the retweet filter Real-account evidence (2026-07-09): the user's timeline had 7 retweets above their 2 originals and a quote tweet; a depth-1 (or 3) fetch returned only retweets, which isOriginalTweet now drops — so no authored post could ever reach the digest. Fetch deeper, filter after. 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. |
|
Real-account follow-up ( |
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 11 unresolved issues from previous reviews.
Re-trigger cubic
…dit onto the shipped template, add exact non-duplication Review question (sweetman): why a rate cap instead of properly enforcing non-duplication? Both, now, as separate concerns: - Completion claim: updateApifyScraperRun updates WHERE completed_at IS NULL — a webhook retry can't re-claim a completed run, re-trigger digest assembly, or overwrite the recorded diff (the 3->0 corruption observed live during #760 testing). - Batch idempotency: maybeSendScrapeDigest checks email_send_log for an existing scrape-digest:<batchId> row before sending — one digest per batch, ever, no time-window heuristics. - The 24h per-artist cap remains as the product frequency rule from the issue spec, reading the same audit rows. Every send is logged per watched artist (rawBody scrape-digest:<batchId>). Also deletes 4 orphaned pre-rename supabase files that rode in again via the merge (same both-added mechanism as #761) and supersedes the branch's selectRecentScrapeDigestLogs with filter-object selectScrapeDigestLogs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
The content half of the revamp on recoupable/chat#1855 (PR #5 of 6, stacked on api#760): a shared deterministic renderer replaces the per-send LLM body. Evidence on the issue showed the LLM body produced different branding on every send, leaked vendor jargon ("Apify Dataset Notification") to customers, and didn't reliably link the posts it claimed were new.
What changed
lib/apify/digest/renderScrapeDigestHtml.ts(new): deterministic subject + HTML — per-platform sections, a direct link to every genuinely-new post, friendly platform labels, chat CTA as secondary link. No LLM call.sendScrapeDigestEmail(batch digest) andsendApifyWebhookEmail(legacy solo alert) both use it. The solo alert now takesnewPostUrlsand sends nothing without them; its LLM path (generateText) is deleted.sendApifyWebhookEmail.tswholesale and embodies the same BCC invariant (tests recreated here) — merging fix(emails): BCC scrape-alert recipients — never a shared cross-tenant To: line #758 first then this stack resolves in favor of this version with no behavior change.Tests (RED→GREEN)
vitest run lib/apifyall green; tsc at baseline parity; eslint clean.🤖 Generated with Claude Code
Summary by cubic
Replaced the LLM email with a deterministic, house‑style digest that links to every genuinely new post with media, captions, UTC dates, and compact stats; emails are addressed by the artist name, show the Recoup logo, include a roster footer, and the CTA always points to the chat app. Also adds X/Twitter post extraction and a LinkedIn label, ignores retweets and replies to avoid misattributing engagement, and fetches 10 tweets by default so original posts surface after filtering.
Refactors
extractTwitterPosts,extractArtistNameFromDatasetItems,formatPostStats,formatUtcDateLabel,getPlatformLabel, andescapeHtml; platform labels cover X/Twitter and LinkedIn.getRunDigestSectionand updatedmaybeSendScrapeDigestto re-read run datasets for enrichment, degrade to URL‑only on failure, address the digest by the profile’s display name, and default X/Twitter scraping to 10 items to survive the retweet/reply filter.Migration
sendApifyWebhookEmail(profile, emails, newPostUrls);newPostUrlsis now required.emailsis empty ornewPostUrlsis empty; recipients are BCC‑only andtois set toRECOUP_FROM_EMAIL.Written for commit cd5761b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes