fix(supabase): drop wrong table cast in CRM send-email usage_count + nightly brief 2026-06-07 - #237
Closed
njrini99-code wants to merge 1 commit into
Closed
fix(supabase): drop wrong table cast in CRM send-email usage_count + nightly brief 2026-06-07#237njrini99-code wants to merge 1 commit into
njrini99-code wants to merge 1 commit into
Conversation
The SELECT for crm_email_templates.usage_count was casting the table name as 'crm_contact_log' to suppress type-check errors. Same line chain on the .update() then needed an 'as Record<string, unknown>' cast on the payload. Both casts are semantically wrong (different tables) and the update cast started failing the build under stricter postgrest-js typing (Vercel deploy dpl_2hdwVGBRQfrfa2F7rGQZT8D3jqjN on Dependabot PR #229). Route both SELECT and UPDATE through the existing fromUntyped() escape hatch — same runtime behavior, no misleading type casts, build-safe against the next supabase-js bump.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
njrini99-code has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
WalkthroughEmail send endpoint refactors template usage counter increment to use ChangesEmail template usage counter refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (9 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/app/api/admin/crm/send-email/route.ts`:
- Around line 276-284: The current read-then-update in route handler using
fromUntyped against crm_email_templates with templateId (the tpl read and
subsequent update) causes a race; replace it with an atomic increment operation
instead of reading usage_count first—either call a DB-side RPC/function (e.g.,
increment_template_usage(template_id)) that runs UPDATE crm_email_templates SET
usage_count = COALESCE(usage_count,0)+1 WHERE id = template_id, or execute a
single atomic UPDATE query via fromUntyped to increment usage_count directly;
update the code that currently uses the tpl variable and the two-step
select+update to call the atomic updater using templateId.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e5ea28d6-e535-4e3d-8855-8d8c12549fa2
📒 Files selected for processing (1)
src/app/api/admin/crm/send-email/route.ts
Comment on lines
+276
to
284
| const { data: tpl } = (await fromUntyped(supabase, 'crm_email_templates') | ||
| .select('usage_count') | ||
| .eq('id', templateId) | ||
| .single() as { data: { usage_count: number } | null }; | ||
| .single()) as { data: { usage_count: number | null } | null }; | ||
|
|
||
| if (tpl) { | ||
| await fromUntyped(supabase, 'crm_email_templates') | ||
| .update({ usage_count: (tpl.usage_count ?? 0) + 1 } as Record<string, unknown>) | ||
| .update({ usage_count: (tpl.usage_count ?? 0) + 1 }) | ||
| .eq('id', templateId); |
There was a problem hiding this comment.
Race condition: usage count increment is not atomic.
src/app/api/admin/crm/send-email/route.ts:276-284
The current read-modify-write pattern can lose increments under concurrent usage. If two requests send emails with the same templateId simultaneously:
- Both read
usage_count = N - Both write
usage_count = N + 1 - Result: count incremented by 1 instead of 2
Replace with an atomic increment to guarantee data integrity:
🔒 Proposed fix: atomic increment
// ── Increment template usage count ──
if (templateId && sent > 0) {
try {
- const { data: tpl } = (await fromUntyped(supabase, 'crm_email_templates')
- .select('usage_count')
- .eq('id', templateId)
- .single()) as { data: { usage_count: number | null } | null };
-
- if (tpl) {
- await fromUntyped(supabase, 'crm_email_templates')
- .update({ usage_count: (tpl.usage_count ?? 0) + 1 })
- .eq('id', templateId);
- }
+ // Atomic increment using RPC or raw SQL
+ await supabase.rpc('increment_template_usage', { template_id: templateId });
} catch {
// Non-critical — don't fail the response
}
}If the RPC doesn't exist yet, add this migration:
CREATE OR REPLACE FUNCTION increment_template_usage(template_id uuid)
RETURNS void AS $$
BEGIN
UPDATE crm_email_templates
SET usage_count = COALESCE(usage_count, 0) + 1
WHERE id = template_id;
END;
$$ LANGUAGE plpgsql SECURITY DEFINER;🤖 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 `@src/app/api/admin/crm/send-email/route.ts` around lines 276 - 284, The
current read-then-update in route handler using fromUntyped against
crm_email_templates with templateId (the tpl read and subsequent update) causes
a race; replace it with an atomic increment operation instead of reading
usage_count first—either call a DB-side RPC/function (e.g.,
increment_template_usage(template_id)) that runs UPDATE crm_email_templates SET
usage_count = COALESCE(usage_count,0)+1 WHERE id = template_id, or execute a
single atomic UPDATE query via fromUntyped to increment usage_count directly;
update the code that currently uses the tpl variable and the two-step
select+update to call the atomic updater using templateId.
njrini99-code
changed the title
Owner
Author
|
Folded into #304 (feat/coachhelm-stats-roundup) — merged clean, combined gates green. Branch intact + reopenable. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Daily Brief — 2026-06-07
24h Totals
6d07386)error_logs/admin_events— see "Couldn't verify" below)error_logsnot directly queryable from this session (see below)Top patterns (24h)
src/app/api/admin/crm/send-email/route.ts:284—Argument of type 'Record<string, unknown>' is not assignable to … RejectExcessProperties<…>main)'crm_email_templates' as 'crm_contact_log'to satisfy the typechecker. The matching.update()then carries anas Record<string, unknown>cast that fails the new postgrest-js strict-insert constraint introduced in supabase-js > 2.107. Main still builds because it pins 2.107 — but the bad cast is on the wrong table either way (different schema entirely), so it's a latent correctness footgun even before the version bump lands.[insights.triggerPlayerInsightsAfterRound] philosophy gate filtered N tier-1 insight(s)(cron/api/cron/coachhelm-roster-sweep)docs/daily-briefs/2026-05-27.mdas "Hot but ambiguous — not safe to silence autonomously". Skipped per task rules.[cron.v3.causality] unknown metric '…'on/api/cron/v3/causality-attributeinfoseverity. Registry-drift diagnostic, not a bug. Skipped per task rules.Performance: top slow queries
Not collected — Supabase MCP could not auto-authenticate in this remote session (OAuth flow blocks on a localhost redirect that the sandbox can't reach).
pg_stat_statements,admin_events, anderror_logsare all behind that gate. Please run a manualdb-auditif you want the perf top-N; the cron paths in the runtime log all completed in well under their 300smaxDurationbudget, so nothing obviously regressed.Deployment health
dpl_2hdwVGBRQfrfa2F7rGQZT8D3jqjN6d07386(Dependabot PR #229 — production-dependencies group, 40 updates)Type error: Argument of type 'Record<string, unknown>' is not assignable to parameter of type 'RejectExcessProperties<{ coach_id?…contact_type?: "email" | "call" | "demo" | "meeting" | "note"… subject?…}, Record<…>>'atsrc/app/api/admin/crm/send-email/route.ts:284:21dpl_BtgmhVgNzn1LQNBkJMcrX5e2DQBk(current production, commit19fd32a)All other recent deploys are READY (Dependabot dev-dependencies group and 6 individual major-bump PRs all built clean once the production-dependencies group was split out).
Fixes applied
src/app/api/admin/crm/send-email/route.ts.from('crm_email_templates' as 'crm_contact_log')and the redundant.update(… as Record<string, unknown>)cast with calls through the existingfromUntyped()escape hatch (the documented project-wide helper for tables not in generated types). Same runtime behavior; loses the misleading cross-table cast that was silently assertingcrm_email_templatesrows havecrm_contact_logcolumns; survives the strict-insert /RejectExcessPropertiesconstraint that supabase-js > 2.107 introduces.npx tsc --noEmitis clean.Couldn't fix / verify (handoff)
Host not in allowlist, soadmin_events,error_logs, andpg_stat_statementswere not queried this run. The 2 client-side errors POSTed to/api/log-error(18:20:02 and 19:26:31 UTC) wrote to those tables; their payloads are still readable in the Supabase dashboard but were not visible to triage. If they're high-impact, the daily brief is missing them — recommend a quick spot-check..from('X' as 'Y')casts still in the tree (src/app/golf/(dashboard)/dashboard/team-hub/page.tsx:84,…/hub/page.tsx:105,src/app/golf/actions/player-notifications.ts:99) — all three cast'golf_task_assignments' as 'golf_shots'. Same anti-pattern, called out indocs/architectural-review-golfhelm-2026-02-22.mdFinding 12. Not touched here to keep the PR minimal and focused on the build-blocker, but worth a follow-up sweep when someone gets to that file area.Generated by Claude Code