Skip to content

Deduplicate referral invite emails - #124

Closed
beepwork wants to merge 1 commit into
profullstack:masterfrom
beepwork:fix-referral-invite-dedupe
Closed

Deduplicate referral invite emails#124
beepwork wants to merge 1 commit into
profullstack:masterfrom
beepwork:fix-referral-invite-dedupe

Conversation

@beepwork

Copy link
Copy Markdown
Contributor

Summary

  • normalize and deduplicate referral invite email batches before duplicate checks, rate-limit math, inserts, and email sends
  • add regression coverage for repeated mixed-case/whitespace emails in one invite request

Why

A single /api/referrals POST could include the same recipient more than once, bypassing the existing database-only duplicate-invite filter. That could create duplicate referral rows and send duplicate invite emails in one batch.

Validation

  • npm run test:run -- src/app/api/referrals/route.test.ts
  • npx eslint src/app/api/referrals/route.ts src/app/api/referrals/route.test.ts

@ralyodio ralyodio closed this May 23, 2026
@ralyodio ralyodio reopened this May 23, 2026
@greptile-apps

greptile-apps Bot commented May 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a batch-level duplicate invite bug by hoisting email normalization (trim().toLowerCase()) and deduplication (new Set) before the rate-limit and database-duplicate checks, so a single request containing repeated or differently-cased copies of the same address no longer creates multiple referral rows or triggers multiple email sends.

  • route.ts: normalizedEmails is now built before rate-limit guards; hourly and daily count checks correctly use normalizedEmails.length instead of the raw emails.length. The emails.length > 20 batch-size cap was not moved, so it still reads the pre-dedup raw array — a minor inconsistency noted in comments.
  • route.test.ts: Adds a focused regression test that submits [\"Friend@Test.com\", \" friend@test.com \"] and asserts that exactly one row is inserted and one email is sent to the normalized address.

Confidence Score: 4/5

Safe to merge; the deduplication fix is correct and the new test covers the target scenario.

The core change is straightforward and well-targeted: normalization now happens before every guard that needs it. The only leftover is the emails.length > 20 batch-size cap, which still reads the raw pre-dedup array while all other limits use the normalized count — a minor inconsistency rather than a harmful gap. The new test also carries a latent order-dependency on mock state from a different describe block.

route.ts warrants a quick look at the emails.length > 20 cap and the redundant .trim().toLowerCase() on line 151.

Important Files Changed

Filename Overview
src/app/api/referrals/route.ts Normalization + Set-dedup is hoisted before rate-limit checks; rate-limit guards now correctly use deduplicated count. The emails.length > 20 batch cap still runs on the pre-dedup raw array, creating a minor inconsistency. A redundant .trim().toLowerCase() in the insert mapping is also left over from the old placement.
src/app/api/referrals/route.test.ts Adds a regression test for the dedupe scenario with mixed-case and whitespace emails. The test correctly captures inserted rows and verifies a single send, but its email-content assertion depends on mock state carried over from the GET describe block's beforeEach rather than being self-contained.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[POST /api/referrals] --> B{Auth check}
    B -- fail --> Z1[401 Unauthorized]
    B -- pass --> C{emails array valid & non-empty?}
    C -- fail --> Z2[400 Bad Request]
    C -- pass --> D{emails.length > 20?}
    D -- yes --> Z3[400 Max 20]
    D -- no --> E["normalizedEmails = Set(emails.trim().toLowerCase())"]
    E --> F{hourlyCount + normalizedEmails.length > 10?}
    F -- yes --> Z4[429 Hourly limit]
    F -- no --> G{dailyCount + normalizedEmails.length > 50?}
    G -- yes --> Z5[429 Daily limit]
    G -- no --> H["Query existing invites for normalizedEmails"]
    H --> I["newEmails = normalizedEmails − alreadyInvited"]
    I --> J{newEmails.length == 0?}
    J -- yes --> Z6[400 All already invited]
    J -- no --> K[Filter valid email format]
    K --> L{validEmails.length == 0?}
    L -- yes --> Z7[400 No valid emails]
    L -- no --> M[Insert referral rows]
    M --> N[Send invite emails]
    N --> O[200 Created & sent]
Loading

Comments Outside Diff (2)

  1. src/app/api/referrals/route.ts, line 65-74 (link)

    P2 The emails.length > 20 batch-size cap is checked against the raw (pre-dedup) input while every downstream limit uses normalizedEmails.length. A caller who sends 20 copies of the same address passes this guard, then normalization collapses them to 1, making the cap effectively dead for duplicated payloads. Moving normalization before this check keeps all size/rate decisions consistent.

  2. src/app/api/referrals/route.ts, line 151 (link)

    P2 .trim().toLowerCase() is now redundant here — validEmails is a subset of normalizedEmails, which already applies both transforms. The double-normalisation is harmless but adds noise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "Deduplicate referral invite emails" | Re-trigger Greptile

Comment on lines +244 to +250
expect(mockSendEmail).toHaveBeenCalledTimes(1);
expect(mockSendEmail).toHaveBeenCalledWith({
to: "friend@test.com",
subject: "Join ugig.net",
html: "<p>Join</p>",
text: "Join",
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test relies on cross-describe mock state

The assertion that mockSendEmail was called with subject, html, and text only works because mockReferralInviteEmail.mockReturnValue(...) is set in the GET describe's beforeEach and vi.clearAllMocks() (used in the POST describe's beforeEach) does not clear mock return values — only call records. If the GET block is ever removed, reordered, or the POST beforeEach is upgraded to vi.resetAllMocks(), this assertion will silently pass with mockSendEmail receiving { to: "friend@test.com" } (no subject/html/text). Add mockReferralInviteEmail.mockReturnValue({ subject: "Join ugig.net", html: "<p>Join</p>", text: "Join" }) inside this test or the POST beforeEach.

@ralyodio ralyodio closed this May 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants