Data foundations: close the >16K scan cliffs (counters + bounded ranges) - #26
Conversation
Deploying communique-site with
|
| Latest commit: |
8e57590
|
| Status: | 🚫 Build failed. |
|
Warning Review limit reached
More reviews will be available in 24 minutes and 31 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughReplaces full-table scans with denormalized per-org supporter/action counters, adds bounded helpers for district distinct counts and weekly growth, integrates deterministic (before, after) delta updates into supporter/action write flows, updates billing metering to use baselines with bounded self-heal, and adds tests and seed backfill. ChangesDenormalized Counters & Billing Metering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
convex/seed.ts (1)
2016-2076:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeed
verifiedActionsLifetimehere too.This path bypasses
campaigns.createCampaignAction, so it seeds verifiedcampaignActionswithout seeding the new org-level lifetime counter that billing meters against. AfterseedAll, orgs can have verified actions but still read as zero-usage to the lifetime-minus-baseline billing path.Suggested fix
// Accumulate the org-level engagement-tier histogram so dev dashboards // (getDashboardStats reads org.actionTierCounts) match the seeded actions. // Keyed by org index; each is a 5-slot [T0..T4] tally over ALL actions. const orgTierCounts = new Map<number, number[]>(); + const orgVerifiedLifetime = new Map<number, number>(); @@ actionCountTotal++; - if (isVerified) verifiedCountTotal++; + if (isVerified) { + verifiedCountTotal++; + orgVerifiedLifetime.set(lc.orgIdx, (orgVerifiedLifetime.get(lc.orgIdx) ?? 0) + 1); + } const counts = orgTierCounts.get(lc.orgIdx) ?? [0, 0, 0, 0, 0]; counts[tier] += 1; orgTierCounts.set(lc.orgIdx, counts); @@ // Persist the org-level tier histograms accumulated above. for (const [orgIdx, counts] of orgTierCounts) { - await ctx.db.patch(orgIds[orgIdx], { actionTierCounts: counts }); + await ctx.db.patch(orgIds[orgIdx], { + actionTierCounts: counts, + verifiedActionsLifetime: orgVerifiedLifetime.get(orgIdx) ?? 0, + }); }🤖 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 `@convex/seed.ts` around lines 2016 - 2076, Summary: Seed script inserts verified campaignActions but never seeds the org-level verifiedActionsLifetime counter. Fix: when accumulating orgTierCounts in the campaignActions insertion loop (the code that sets counts[tier] and orgTierCounts with lc.orgIdx), ensure the verified tally (tier 2) is persisted to the org record as verifiedActionsLifetime. Concretely, in the final persist loop that iterates "for (const [orgIdx, counts] of orgTierCounts)" patch each org (orgIds[orgIdx]) to include actionTierCounts: counts and set verifiedActionsLifetime to counts[2] (or counts[2] plus existing value if you need additive behavior); this ensures the seeded verified actions are reflected by the org-level lifetime counter.convex/supporters.ts (1)
1120-1150:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
updateSmsStatusnow drifts denormalizedsupporterStatscounters.At Line 1146,
smsStatusis patched on the supporter row, but org-level counters are not transitioned. SincegetSummaryStatsnow readsorg.supporterStats, SMS buckets will become stale.Suggested fix
await ctx.db.patch(args.supporterId, { smsStatus: args.smsStatus, updatedAt: Date.now() }); + if (args.smsStatus !== supporter.smsStatus) { + await applySupporterStatsDelta(ctx, supporter.orgId, supporter as CountableSupporter, { + ...(supporter as CountableSupporter), + smsStatus: args.smsStatus + }); + } return { updated: true }; } });🤖 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 `@convex/supporters.ts` around lines 1120 - 1150, The updateSmsStatus mutation updates supporter.smsStatus but doesn't adjust the denormalized org.supporterStats counters used by getSummaryStats; modify updateSmsStatus to read the supporter's previous smsStatus, return early if unchanged, then patch the org row (org._id) to decrement the counter for the old bucket and increment the counter for the new bucket (e.g., supporterStats.sms.none/subscribed/unsubscribed), also updating updatedAt; keep the guard that prevents overriding 'stopped' and ensure the org patch is applied after validating the supporter and before/after patching the supporter so counters remain consistent.
🧹 Nitpick comments (4)
convex/subscriptions.ts (1)
692-695: 💤 Low valueConsider using
MutationCtxinstead of inline type.The inline type definition for
ctxis functional but verbose and could drift from Convex's actual mutation context interface. UsingMutationCtxfrom"./_generated/server"would be more maintainable.♻️ Suggested simplification
+import type { MutationCtx } from "./_generated/server"; async function snapshotVerifiedActionBaseline( - ctx: { db: { get: (id: Id<"organizations">) => Promise<{ verifiedActionsLifetime?: number; verifiedActionsPeriodBaselineAt?: number } | null>; patch: (id: Id<"organizations">, patch: Record<string, unknown>) => Promise<unknown> } }, + ctx: MutationCtx, orgId: Id<"organizations">, periodStart: number, ): Promise<void> {🤖 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 `@convex/subscriptions.ts` around lines 692 - 695, The inline type for ctx in snapshotVerifiedActionBaseline is verbose and should be replaced with Convex's MutationCtx for maintainability: import MutationCtx from "./_generated/server" and change the function signature to async function snapshotVerifiedActionBaseline(ctx: MutationCtx, orgId: Id<"organizations">, periodStart: number) so the function uses the generated mutation context type; update or remove the custom db method typings accordingly (they will be provided by MutationCtx) and fix any resulting type errors by adjusting uses of ctx.db.get/patch to match the MutationCtx API.src/routes/org/[slug]/supporters/+page.server.ts (1)
157-174: ⚡ Quick winReuse parent-provided district count to avoid a second bounded scan on
/supporters.Line 167 fetches
getDistrictVerifiedCountagain even though the parent layout already resolves district verification. Pulling it fromparent()(with query fallback only when absent) reduces duplicate capped scans on this route.Also applies to: 216-216
🤖 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/routes/org/`[slug]/supporters/+page.server.ts around lines 157 - 174, The route is re-running getDistrictVerifiedCount unnecessarily; call parent() first to read the parent layout's district verification result and use it in the Promise.all (falling back to serverQuery(api.supporters.getDistrictVerifiedCount, ...) only when the parent value is absent). Concretely: await parent() to extract the parent's districtVerified (or similar key), then in the array passed to Promise.all replace the direct serverQuery(api.supporters.getDistrictVerifiedCount, ...) with parentDistrictVerified ? Promise.resolve(parentDistrictVerified) : serverQuery(api.supporters.getDistrictVerifiedCount, { orgSlug: org.slug }).catch(() => null) so you reuse the parent-provided count and only run the bounded scan if the parent did not provide it.src/routes/org/[slug]/+layout.server.ts (1)
200-209: ⚡ Quick winAvoid duplicate district-of-record scans in the same request path.
Line 207 adds
getDistrictVerifiedCount, but this layout already fetchesgetDashboardStats, which includesfunnel.districtVerified. Reusing one source (with fallback only if missing) would avoid an extra bounded scan per navigation.🤖 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/routes/org/`[slug]/+layout.server.ts around lines 200 - 209, The code is duplicating a district-of-record scan by always calling serverQuery(api.supporters.getDistrictVerifiedCount); instead, derive districtVerified from the already-fetched dashboard stats and only call the bounded getDistrictVerifiedCount as a fallback. Update the Promise.all usage so it does not include serverQuery(api.supporters.getDistrictVerifiedCount) — keep fetching supporterSummary, orgKeyResult, segmentsResult as before — then after awaiting getDashboardStats (or wherever dashboard stats are loaded), set districtVerifiedResult = dashboardStats?.funnel?.districtVerified ?? await serverQuery(api.supporters.getDistrictVerifiedCount, { orgSlug: slug }).catch(() => null). Use the same variable names (districtVerifiedResult, supporterSummary) and serverQuery/api.supporters.getDistrictVerifiedCount so the change is localized and avoids the extra scan.convex/schema.ts (1)
1850-1855: ⚡ Quick winConstrain
campaignActions.channelto a closed union to prevent attribution drift.At Line 1855,
v.optional(v.string())permits typo/rogue channel values, which can silently fragment results behindby_campaignId_channel.Suggested fix
- channel: v.optional(v.string()), + channel: v.optional( + v.union( + v.literal('email'), + v.literal('congressional'), + v.literal('sms'), + v.literal('web') + ) + ),🤖 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 `@convex/schema.ts` around lines 1850 - 1855, Replace the loose type on campaignActions.channel (currently v.optional(v.string())) with a closed union of allowed literals to prevent rogue values; change the schema in the campaignActions definition to use v.optional(v.union(v.literal('email'), v.literal('congressional'), v.literal('sms'), v.literal('web'))) so only those four channel values (or undefined) are permitted and will not fragment by_campaignId_channel.
🤖 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 `@convex/_dashboardStats.ts`:
- Around line 41-43: The index predicate on .withIndex('by_orgId_verified',
(idx) => idx.eq('orgId', orgId)) is missing the verified constraint so
unverified actions can be returned; update the predicate to also filter verified
=== true (e.g., .withIndex('by_orgId_verified', (idx) => idx.eq('orgId',
orgId).eq('verified', true))) so the district scan matches the function contract
and districtVerified only counts verified actions.
In `@convex/_supporterStats.ts`:
- Around line 86-105: The sourceCounts object must be a null-prototype map to
avoid inherited keys from caller-controlled source values; change the
initializer in emptySupporterStats so sourceCounts: Object.create(null) instead
of {} and update any other factories/initializers that create a fresh
sourceCounts (the other occurrences noted in the review) to use
Object.create(null) as well; keep all other fields unchanged and ensure any code
that reads/writes counts continues to treat missing keys as undefined or 0
(e.g., using (sourceCounts[key] ?? 0) will now be safe).
In `@convex/campaigns.ts`:
- Around line 1214-1246: The org-level counters actionTierCounts and
verifiedActionsLifetime are only incremented where new campaignActions are
inserted but never adjusted when campaignActions are hard-deleted by remove(),
causing drift; either (A) preserve "lifetime" semantics by updating the comments
and any query/display code (e.g., getDashboardStats) to reflect that these
counters are cumulative and should not be derived from live campaignActions, or
(B) make deletes decrement the same counters: modify remove() to aggregate the
deleted campaignActions' engagementTier and verified flags (or fetch affected
rows from campaignActions) and then patch the org document by subtracting counts
from actionTierCounts and decrementing verifiedActionsLifetime via the same
ctx.db.patch(orgId, patch) pattern used on insert, ensuring bounds-checking on
the 5-slot histogram and not letting counts go negative; pick one approach and
apply it consistently (update comments/code paths if you choose A, implement the
decrement logic in remove() and mirror the insert-side defensive array handling
if you choose B).
In `@tests/unit/org/dashboard-stats-bounded.test.ts`:
- Around line 93-94: Remove the explicit "as any" cast on the fake Convex ctx
and the eslint-disable comment; instead type the fake context via "unknown"
narrowed to a helper-derived type (e.g., create a type alias like "type FakeCtx
= Parameters<typeof yourTestHelper>[0]" or "type FakeCtx = ReturnType<typeof
makeFakeConvexContext>" and then cast the value as "unknown as FakeCtx"). Update
the variable declaration that currently ends with "as any" to use "unknown as
FakeCtx" (and remove the eslint-disable), so the test keeps strong typing
without using any.
---
Outside diff comments:
In `@convex/seed.ts`:
- Around line 2016-2076: Summary: Seed script inserts verified campaignActions
but never seeds the org-level verifiedActionsLifetime counter. Fix: when
accumulating orgTierCounts in the campaignActions insertion loop (the code that
sets counts[tier] and orgTierCounts with lc.orgIdx), ensure the verified tally
(tier 2) is persisted to the org record as verifiedActionsLifetime. Concretely,
in the final persist loop that iterates "for (const [orgIdx, counts] of
orgTierCounts)" patch each org (orgIds[orgIdx]) to include actionTierCounts:
counts and set verifiedActionsLifetime to counts[2] (or counts[2] plus existing
value if you need additive behavior); this ensures the seeded verified actions
are reflected by the org-level lifetime counter.
In `@convex/supporters.ts`:
- Around line 1120-1150: The updateSmsStatus mutation updates
supporter.smsStatus but doesn't adjust the denormalized org.supporterStats
counters used by getSummaryStats; modify updateSmsStatus to read the supporter's
previous smsStatus, return early if unchanged, then patch the org row (org._id)
to decrement the counter for the old bucket and increment the counter for the
new bucket (e.g., supporterStats.sms.none/subscribed/unsubscribed), also
updating updatedAt; keep the guard that prevents overriding 'stopped' and ensure
the org patch is applied after validating the supporter and before/after
patching the supporter so counters remain consistent.
---
Nitpick comments:
In `@convex/schema.ts`:
- Around line 1850-1855: Replace the loose type on campaignActions.channel
(currently v.optional(v.string())) with a closed union of allowed literals to
prevent rogue values; change the schema in the campaignActions definition to use
v.optional(v.union(v.literal('email'), v.literal('congressional'),
v.literal('sms'), v.literal('web'))) so only those four channel values (or
undefined) are permitted and will not fragment by_campaignId_channel.
In `@convex/subscriptions.ts`:
- Around line 692-695: The inline type for ctx in snapshotVerifiedActionBaseline
is verbose and should be replaced with Convex's MutationCtx for maintainability:
import MutationCtx from "./_generated/server" and change the function signature
to async function snapshotVerifiedActionBaseline(ctx: MutationCtx, orgId:
Id<"organizations">, periodStart: number) so the function uses the generated
mutation context type; update or remove the custom db method typings accordingly
(they will be provided by MutationCtx) and fix any resulting type errors by
adjusting uses of ctx.db.get/patch to match the MutationCtx API.
In `@src/routes/org/`[slug]/+layout.server.ts:
- Around line 200-209: The code is duplicating a district-of-record scan by
always calling serverQuery(api.supporters.getDistrictVerifiedCount); instead,
derive districtVerified from the already-fetched dashboard stats and only call
the bounded getDistrictVerifiedCount as a fallback. Update the Promise.all usage
so it does not include serverQuery(api.supporters.getDistrictVerifiedCount) —
keep fetching supporterSummary, orgKeyResult, segmentsResult as before — then
after awaiting getDashboardStats (or wherever dashboard stats are loaded), set
districtVerifiedResult = dashboardStats?.funnel?.districtVerified ?? await
serverQuery(api.supporters.getDistrictVerifiedCount, { orgSlug: slug }).catch(()
=> null). Use the same variable names (districtVerifiedResult, supporterSummary)
and serverQuery/api.supporters.getDistrictVerifiedCount so the change is
localized and avoids the extra scan.
In `@src/routes/org/`[slug]/supporters/+page.server.ts:
- Around line 157-174: The route is re-running getDistrictVerifiedCount
unnecessarily; call parent() first to read the parent layout's district
verification result and use it in the Promise.all (falling back to
serverQuery(api.supporters.getDistrictVerifiedCount, ...) only when the parent
value is absent). Concretely: await parent() to extract the parent's
districtVerified (or similar key), then in the array passed to Promise.all
replace the direct serverQuery(api.supporters.getDistrictVerifiedCount, ...)
with parentDistrictVerified ? Promise.resolve(parentDistrictVerified) :
serverQuery(api.supporters.getDistrictVerifiedCount, { orgSlug: org.slug
}).catch(() => null) so you reuse the parent-provided count and only run the
bounded scan if the parent did not provide it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78990809-106f-4821-b5f6-d477a84a24b3
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (18)
convex/_dashboardStats.tsconvex/_supporterStats.tsconvex/campaigns.tsconvex/email.tsconvex/organizations.tsconvex/schema.tsconvex/seed.tsconvex/subscriptions.tsconvex/supporters.tsconvex/v1api.tsconvex/webhooks.tssrc/routes/org/[slug]/+layout.server.tssrc/routes/org/[slug]/supporters/+page.server.tstests/unit/billing/verified-action-metering.test.tstests/unit/org/dashboard-stats-bounded.test.tstests/unit/org/supporter-stats-delta.test.tstests/unit/org/supporter-summary-counters.test.tstests/unit/people-import-consent-evidence.test.ts
| export function emptySupporterStats(): SupporterStats { | ||
| return { | ||
| identityVerified: 0, | ||
| postalResolved: 0, | ||
| phonePresent: 0, | ||
| emailSubscribed: 0, | ||
| emailUnsubscribed: 0, | ||
| emailBounced: 0, | ||
| emailComplained: 0, | ||
| smsSubscribed: 0, | ||
| smsUnsubscribed: 0, | ||
| smsStopped: 0, | ||
| smsNone: 0, | ||
| emailConsentEvidence: 0, | ||
| emailSubscribedConsentEvidence: 0, | ||
| smsConsentEvidence: 0, | ||
| smsSubscribedConsentEvidence: 0, | ||
| sourceCounts: {} | ||
| }; | ||
| } |
There was a problem hiding this comment.
Use a null-prototype map for sourceCounts.
source is effectively caller-controlled, so keys like constructor or __proto__ hit inherited properties here. On the first increment/decrement, next.sourceCounts[key] ?? 0 can read a function/object instead of undefined, which turns the persisted count into a string/NaN and poisons the org stats.
Suggested fix
+function emptySourceCounts(): Record<string, number> {
+ return Object.create(null) as Record<string, number>;
+}
+
export function emptySupporterStats(): SupporterStats {
return {
identityVerified: 0,
postalResolved: 0,
@@
emailConsentEvidence: 0,
emailSubscribedConsentEvidence: 0,
smsConsentEvidence: 0,
smsSubscribedConsentEvidence: 0,
- sourceCounts: {}
+ sourceCounts: emptySourceCounts()
};
}
@@
export function computeSupporterStats(
current: SupporterStats | undefined,
before: CountableSupporter | null,
after: CountableSupporter | null
): SupporterStats {
- const next = current ? { ...current, sourceCounts: { ...current.sourceCounts } } : emptySupporterStats();
+ const next = current
+ ? {
+ ...current,
+ sourceCounts: Object.assign(emptySourceCounts(), current.sourceCounts)
+ }
+ : emptySupporterStats();Also applies to: 161-161, 178-185
🤖 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 `@convex/_supporterStats.ts` around lines 86 - 105, The sourceCounts object
must be a null-prototype map to avoid inherited keys from caller-controlled
source values; change the initializer in emptySupporterStats so sourceCounts:
Object.create(null) instead of {} and update any other factories/initializers
that create a fresh sourceCounts (the other occurrences noted in the review) to
use Object.create(null) as well; keep all other fields unchanged and ensure any
code that reads/writes counts continues to treat missing keys as undefined or 0
(e.g., using (sourceCounts[key] ?? 0) will now be safe).
| // Monotonic org-level counters bumped on insert — the only-ever-increments | ||
| // bases for scale-safe dashboard + billing reads. Bumped here, next to the | ||
| // campaign counter, so there's exactly one new write site and they can | ||
| // never drift. | ||
| // - verifiedActionsLifetime: lifetime tally of VERIFIED actions, the base | ||
| // for billing metering (period usage = lifetime - period baseline, | ||
| // baseline snapshotted at billing-period rollover). Only verified | ||
| // actions count toward the metered quota. | ||
| // - actionTierCounts: engagement-tier histogram over ALL actions (matches | ||
| // the prior getDashboardStats loop, which counted every action's tier | ||
| // regardless of verified). engagementTier is immutable post-creation, so | ||
| // a monotonic counter is exact. Indexed 0-4; out-of-range tiers ignored. | ||
| if (orgId) { | ||
| const org = await ctx.db.get(orgId); | ||
| if (org) { | ||
| const patch: Record<string, unknown> = {}; | ||
| if (args.verified) { | ||
| patch.verifiedActionsLifetime = (org.verifiedActionsLifetime ?? 0) + 1; | ||
| } | ||
| const tier = args.engagementTier; | ||
| if (typeof tier === 'number' && tier >= 0 && tier <= 4) { | ||
| const counts = [...(org.actionTierCounts ?? [0, 0, 0, 0, 0])]; | ||
| // Defensive: pad/truncate to exactly 5 slots in case a legacy doc | ||
| // stored a shorter array. | ||
| while (counts.length < 5) counts.push(0); | ||
| counts[tier] = (counts[tier] ?? 0) + 1; | ||
| patch.actionTierCounts = counts.slice(0, 5); | ||
| } | ||
| if (Object.keys(patch).length > 0) { | ||
| await ctx.db.patch(orgId, patch); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
actionTierCounts will drift after campaign deletion.
These counters only increment here, but remove() still hard-deletes the backing campaignActions rows on Line 689-Line 697. Before this PR the dashboard histogram was derived from live rows, so deleting a campaign reduced the totals; with this change the org-level histogram stays inflated forever.
If the intended metric is still “current action rows,” this needs a matching decrement/rebuild on every delete path. If the intended metric is now “lifetime actions ever created,” the query-side semantics and comments need to be updated to match.
🤖 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 `@convex/campaigns.ts` around lines 1214 - 1246, The org-level counters
actionTierCounts and verifiedActionsLifetime are only incremented where new
campaignActions are inserted but never adjusted when campaignActions are
hard-deleted by remove(), causing drift; either (A) preserve "lifetime"
semantics by updating the comments and any query/display code (e.g.,
getDashboardStats) to reflect that these counters are cumulative and should not
be derived from live campaignActions, or (B) make deletes decrement the same
counters: modify remove() to aggregate the deleted campaignActions'
engagementTier and verified flags (or fetch affected rows from campaignActions)
and then patch the org document by subtracting counts from actionTierCounts and
decrementing verifiedActionsLifetime via the same ctx.db.patch(orgId, patch)
pattern used on insert, ensuring bounds-checking on the 5-slot histogram and not
letting counts go negative; pick one approach and apply it consistently (update
comments/code paths if you choose A, implement the decrement logic in remove()
and mirror the insert-side defensive array handling if you choose B).
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| } as any |
There was a problem hiding this comment.
Remove explicit any cast in the fake Convex ctx.
Line 94 disables the explicit-any rule and casts the test ctx to any, which weakens the type-contract checks these tests are validating. Prefer unknown + a narrowed helper type derived from the helper function parameter type.
As per coding guidelines, "Avoid using any whenever possible in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/org/dashboard-stats-bounded.test.ts` around lines 93 - 94, Remove
the explicit "as any" cast on the fake Convex ctx and the eslint-disable
comment; instead type the fake context via "unknown" narrowed to a
helper-derived type (e.g., create a type alias like "type FakeCtx =
Parameters<typeof yourTestHelper>[0]" or "type FakeCtx = ReturnType<typeof
makeFakeConvexContext>" and then cast the value as "unknown as FakeCtx"). Update
the variable declaration that currently ends with "as any" to use "unknown as
FakeCtx" (and remove the eslint-disable), so the test keeps strong typing
without using any.
Source: Coding guidelines
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🪓 Brutalist Review
Claude and Codex agree on the three headline bugs: computeDistrictVerified silently counts unverified actions (missing .eq('verified', true)), updateSmsStatus is not wired to applySupporterStatsDelta causing permanent SMS counter drift, and no backfill exists for the new supporterStats/verifiedActionsLifetime fields so every existing org shows all-zero stats on deploy. Codex additionally flags that the billing self-heal scan returns a plain integer with no truncation signal (so high-volume orgs can have enforcement silently collapse), that district.truncated is discarded by callers, and that sourceCounts accepts unbounded user-controlled key strings into the org document. The biggest disagreement is severity of the billing transition risk: Claude treats it as a rollout concern (self-heal path covers it) while Codex calls it critical; after fact-checking, the self-heal correctly handles the transition except when an org exceeds 16 K actions in the period, making it a medium risk rather than critical. The must-fix items before shipping are: (1) add .eq('verified', true) to computeDistrictVerified, (2) wire updateSmsStatus to applySupporterStatsDelta, and (3) write and run a backfill mutation for supporterStats and verifiedActionsLifetime on all pre-existing orgs.
Inline comments: 8 (4 🟠 high · 4 🟡 medium)
Per-CLI breakdown
✅ Claude (default, 276079ms)
Identified 6 findings: two correctness bugs (computeDistrictVerified missing verified filter; updateSmsStatus missing delta call), one critical rollout risk (no backfill for existing orgs), one perf issue (double district scan per /supporters load), one write-contention architectural concern, and a low-severity type-system weakness in the hand-rolled ctx type.
✅ Codex (default, 86123ms)
Identified 12 findings; after deduplication and fact-checking the most distinct actionable ones are: billing self-heal cap not surfaced as truncation, district truncated discarded in getDashboardStats, sourceCounts unbounded key space risk, Math.max clamp masking drift, billing transition risk for existing orgs, and tests mirroring production logic instead of importing it.
❌ agy (default, 900008ms)
Hit rate/usage limit and did not complete analysis.
Out-of-diff findings (2)
correctness
- 🟠 high
convex/supporters.ts— Claude [unanchored]: updateSmsStatus patches smsStatus without calling applySupporterStatsDelta — SMS breakdown counters permanently drift on every manual edit
maintainability
- 🔵 low
convex/subscriptions.ts— Claude [sub-threshold]: snapshotVerifiedActionBaseline uses a hand-rolled structural ctx type instead of MutationCtx
3 finding(s) dropped due to unverifiable verbatim quotes (likely fabrication).
Brutalist orchestrator schemaVersion=1 · context_id=43fef424-39ff-4ac7-bd14-9630c9ac5a1a
| ): Promise<DistrictVerifiedResult> { | ||
| const scanned = await ctx.db | ||
| .query('campaignActions') | ||
| .withIndex('by_orgId_verified', (idx) => idx.eq('orgId', orgId)) |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[Claude 🟠 high] correctness — computeDistrictVerified scans all org actions, not just verified — unverified districtHash entries inflate the count
The by_orgId_verified index is [orgId, verified], but the constraint only pins orgId. With .order('desc'), boolean true sorts before false, so verified actions rise to the top — but this is a sort bias, not a filter. Once the org's verified action count falls below DISTRICT_SCAN_CAP (10 K), the scan continues into verified=false rows and the in-memory accumulator at line 48 (if (action.supporterId && action.districtHash)) never checks action.verified. Any unverified action that carries a districtHash inflates the funnel metric. Compare computeGrowthWindow which correctly adds .eq('verified', true) to the index constraint. Fix: add .eq('verified', true) after the orgId constraint, exactly as the growth-window query does.
| .withIndex('by_orgId_verified', (idx) => idx.eq('orgId', orgId)) | |
| .withIndex('by_orgId_verified', (idx) => idx.eq('orgId', orgId).eq('verified', true)) |
| } | ||
|
|
||
| function sourceValue(s: CountableSupporter): string { | ||
| return typeof s.source === 'string' && s.source.trim() ? s.source.trim() : 'unknown'; |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[Codex 🟠 high] security — sourceCounts uses raw s.source.trim() as an org-document map key — unbounded key cardinality enables org document size attacks
sourceValue() uses the caller-supplied source field directly as a key in org.supporterStats.sourceCounts with no normalization whitelist, length cap, or cardinality limit. The schema allows v.record(v.string(), v.number()) with no constraint on key count or key length. An importer or v1 API caller that supplies a unique source string per supporter (e.g. a UUID, a URL, or a long freeform string) grows the sourceCounts map by one entry per supporter. Convex imposes a 1 MB document size limit; a large import with unique sources can exhaust it, causing subsequent org writes to fail. Since sourceCounts sits inside supporterStats which is patched on every supporter write, the blast radius is every mutation that touches a supporter in that org. Fix: normalize sources to an allowlist (e.g. 'csv', 'api', 'organic', 'import') and bucket unknowns to 'other', or add a cardinality cap before inserting a new key.
| .take(VERIFIED_ACTION_PERIOD_SCAN_CAP + 1); | ||
| // If the period's own volume somehow exceeds the cap, return the cap so | ||
| // enforcement over-counts (fails safe) rather than under-reporting. | ||
| return Math.min(rows.length, VERIFIED_ACTION_PERIOD_SCAN_CAP); |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[Codex 🟠 high] correctness — verifiedActionsThisPeriod self-heal path silently caps enforcement at 16 K with no truncation signal — high-volume orgs get wrong billing reads
When the O(1) baseline path isn't available (baseline missing or stale period), verifiedActionsThisPeriod falls back to a bounded sentAt-range scan and returns Math.min(rows.length, VERIFIED_ACTION_PERIOD_SCAN_CAP). Unlike _dashboardStats.ts, which surfaces truncated so callers can present a floor, the billing path returns a plain number with no indication that it hit the cap. An org with 17 K verified actions in a period during the self-heal window gets counted as exactly 16 K — which could either (a) block an org that should still have headroom if their limit is 20 K, or (b) let an org through whose true usage exceeds their limit. The self-heal branch is on the hot path for all existing orgs until their next Stripe webhook fires, so this isn't a corner case.
| } | ||
| } | ||
| const total = org.supporterCount ?? 0; | ||
| const stats = org.supporterStats ?? emptySupporterStats(); |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[Claude 🟠 high] correctness — No backfill mutation shipped — existing orgs fall back to emptySupporterStats() and show all-zero stats on deploy
Every org that existed before this deploy has no supporterStats field. getSummaryStats and getDashboardStats both read org.supporterStats ?? emptySupporterStats(), so every existing org serves the dashboard with zeroes for imported, postalResolved, identityVerified, emailSubscribed, and every other counter the moment this PR goes live. The same applies to org.supporterCount (falls back to 0), org.verifiedActionsLifetime, and org.actionTierCounts. The system will partially self-heal as future mutations flow through, but only for fields touched by those mutations. Historical data — everything before deploy — is permanently absent from the counters unless a backfill is run. A backfillSupporterStats internalMutation (paginate supporters, fold computeSupporterStats, patch the org once per page) is needed and must run before cutting over traffic.
| if (beforeC) delta -= beforeC[key]; | ||
| if (afterC) delta += afterC[key]; | ||
| if (delta !== 0) { | ||
| next[key] = Math.max(0, next[key] + delta); |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] correctness — Math.max(0) clamp silently absorbs double-decrement and missed-delta bugs — counter drift becomes invisible and irrecoverable without a rebuild
The defensive clamp in computeSupporterStats prevents negative display values, but it also absorbs any double-decrement or stale-before bug without any log, metric, or observable signal. If updateSmsStatus (which doesn't call the delta helper today) is fixed but the old path was already called 1 000 times, smsSubscribed is already 1 000 too low — the clamp keeps it from going negative but never corrects it. Without an invariant check or a periodic reconciliation job that re-scans supporters and re-derives the expected counts, operators have no way to know the counters are wrong until a visible downstream consequence surfaces. Consider asserting in dev/staging (process.env.NODE_ENV !== 'production' && delta < -1 && console.warn(...)) or surfacing a _derivedAt timestamp so a cron can spot-check counters.
| imported: total, | ||
| postalResolved: stats.postalResolved, | ||
| identityVerified: stats.identityVerified, | ||
| districtVerified: district.districtVerified |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] correctness — district.truncated discarded in getDashboardStats — capped approximate count is presented as an exact number
computeDistrictVerified returns { districtVerified, truncated, scanLimit }. The dashboard comment promises that when the cap saturates, consumers can present a floor (">= N"). But getDashboardStats returns only district.districtVerified and drops truncated. The same pattern repeats in +layout.server.ts where districtVerifiedResult?.districtVerified is extracted without ever reading truncated. The UI therefore has no signal to display ≥ notation for large orgs — it shows a capped floor as if it were exact, silently understating district reach.
| // District-of-record is set cardinality, served by a separate bounded | ||
| // query (not the always-on funnel summary). Null-safe so a failure or an | ||
| // org with no district signal just shows 0. | ||
| serverQuery(api.supporters.getDistrictVerifiedCount, { |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Claude 🟡 medium] perf — getDistrictVerifiedCount invoked independently in both layout and page server — two 10 K bounded scans per /supporters navigation
SvelteKit executes both the layout load and the page load on every navigation to /org/[slug]/supporters. Both call api.supporters.getDistrictVerifiedCount independently (layout at line 207, page at +page.server.ts:167). Each call is a bounded 10 K document scan. The result from the layout is not passed down through parent() in the page load, so there is no sharing. The bounded-scan optimization was introduced precisely to avoid repeated large reads; getting two per page navigation undermines that. Fix: either promote the district count to the layout only and consume it via parent() in the page loader, or move it exclusively to the page level and remove it from the layout.
| * Mirror of verifiedActionsThisPeriod. `scanThisPeriod()` stands in for the | ||
| * bounded sentAt-range index read used in the self-heal branch. | ||
| */ | ||
| function periodCount( |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] testing — Billing tests copy-paste local mirrors of production functions instead of importing them — production logic can diverge silently
The test file defines its own periodCount() and snapshot() — local mirrors of verifiedActionsThisPeriod and snapshotVerifiedActionBaseline in convex/subscriptions.ts. The comment acknowledges this ('convex-test isn't wired in this repo'), but the consequence is that these tests validate the mirror, not the production code. If someone edits the baseline comparison in verifiedActionsThisPeriod (e.g. from === to >=) the tests still pass. The highest-risk billing path — the one that determines whether an org gets blocked — has placeholder tests. Even without convex-test, pure computation helpers like snapshotVerifiedActionBaseline could be extracted into a testable utility module and imported directly. The current structure gives false confidence: green tests do not mean the billing logic is correct.
Three real bugs the review caught (counter completeness is the whole game): - computeDistrictVerified used the by_orgId_verified index but only constrained orgId, so it counted UNVERIFIED district actions too. Added eq verified true (computeGrowthWindow below it already filtered; this was the inconsistent miss). - updateSmsStatus (manual editor) patched smsStatus with no applySupporterStatsDelta, so the sms breakdown buckets drifted on every manual edit. Wired it (no-op guard when unchanged + delta on transition). - sourceCounts keyed on the user-controlled source label was unbounded, so the org doc could grow toward Convex's ~1MB cap. Bounded the key space (MAX_SOURCE_KEYS=32, overflow folds into other; display-only breakdown so the tail can be approximate). Plus: surface district truncated in the funnel (do not silently discard the floor); snapshotVerifiedActionBaseline uses MutationCtx instead of a hand-rolled struct. Tests: sms-transition + source-bound delta math, source pins for both wiring fixes. Deliberately NOT done: backfill for existing orgs (pre-launch, zero production data; counters start exact at 0, seed maintains them). Deferred: org-doc write-contention (acceptable at launch scale, Convex OCC retries; shard if it shows) and redundant bounded district scans per supporters load (perf, bounded).
2e2caaa to
8fa3275
Compare
There was a problem hiding this comment.
🪓 Brutalist Review
Both critics agree on the headline risk: the new denormalized counter system creates an honor-system invariant that every supporter writer must call applySupporterStatsDelta, and the PR misses at least two confirmed gaps — v1api.deleteSupporter and v1api.updateSupporter — that will silently inflate all funnel counters on every v1 API deletion and postal-code update, indefinitely. Claude additionally identified a subtle source-bucket corruption in the delete path (a pruned key is misidentified as folded-into-other), the OTHER_SOURCE_KEY = 'other' token collision with real user labels (Codex), and the unbounded .collect() for email/SMS blasts in billing — the exact failure mode this PR fixes for actions, still present on a longer time horizon. The two critics disagreed on two Codex claims (broken layout syntax, corrupted importBatch body) that were not present in actual files and were discarded. The truncated flag for district-of-record is wired in the API response but dropped at every server-load callsite, leaving large-org dashboards displaying a wrong exact count instead of a floor.
Inline comments: 6 (1 🟠 high · 5 🟡 medium)
Per-CLI breakdown
✅ Claude (default, 339790ms)
Identified 2 critical writer-coverage gaps in v1api.ts (deleteSupporter and updateSupporter missing delta calls), a high-severity source-bucket corruption path in the delete ambiguity logic, and several medium/low concerns around the double org read-write pattern, unbounded blast collects in billing, seed fixture accuracy, cap inconsistency, and missing truncated propagation.
✅ Codex (default, 82228ms)
Raised valid concerns about OTHER_SOURCE_KEY collision with user data and test fidelity (reimplement vs import). Two claimed critical findings — a broken layout syntax and a structurally corrupted importBatch body — were not present in the actual files and were discarded after grep verification.
❌ agy (default, 900010ms)
Hit rate/usage limit. No findings.
Out-of-diff findings (5)
correctness
- 🔴 critical
convex/v1api.ts— Claude [unanchored]:v1api.deleteSupporterbypassesapplySupporterStatsDelta— every v1 deletion inflates counters permanently - 🔴 critical
convex/v1api.ts— Claude [unanchored]:v1api.updateSupporterupdatespostalCodewithout callingapplySupporterStatsDelta—postalResolveddrifts
perf
- 🟠 high
convex/subscriptions.ts— Claude [unanchored]:emailBlastsandsmsBlastsremain unbounded.collect()in both billing queries — same cliff this PR fixes for actions
security
- 🔵 low
convex/_supporterStats.ts— Claude [sub-threshold]:sourcestring length is uncapped; large keys can bloat the org document
testing
- 🔵 low
tests/unit/billing/verified-action-metering.test.ts— Codex [sub-threshold]: Tests reimplement billing logic locally rather than importing the production module
Brutalist orchestrator schemaVersion=1 · context_id=61c9b46b-bd2e-423c-9819-f6cb083e3c6d
| let key = sourceValue(before); | ||
| // If this source was folded into 'other' on the way in (key space was | ||
| // full), its own key isn't present — decrement 'other' to stay consistent. | ||
| if (next.sourceCounts[key] === undefined && next.sourceCounts[OTHER_SOURCE_KEY] !== undefined) { |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[Claude 🟠 high] correctness — Source-folding delete ambiguity silently corrupts the other bucket
The guard at line 192 conflates two distinct states: (a) the source key was folded into other at create time because MAX_SOURCE_KEYS was full, and (b) the source key previously existed, its count reached 0, and it was pruned from the map (line 196). After pruning, sourceCounts[key] === undefined is true even though the supporter was never folded into other. If other also happens to exist, the delete wrongly decrements other instead of being a no-op. Net effect: other drifts toward zero (clamped), and source tallies diverge silently. Concrete path: fill the map to 32 + other; a slot-1 source reaches count 0 and is pruned; import and then delete another supporter with that same slot-1 source → misfires to other. Fix: track which keys were folded (e.g., a separate foldedSources: Set<string>) rather than inferring fold status from map presence.
| if (next.sourceCounts[key] === undefined && next.sourceCounts[OTHER_SOURCE_KEY] !== undefined) { | |
| // Instead of inferring fold from key absence, track folded keys explicitly: | |
| // Add to SupporterStats: foldedSources?: string[]; | |
| // On add: if key was new and map was full, push key to foldedSources and use OTHER_SOURCE_KEY. | |
| // On delete: if foldedSources includes key, decrement OTHER_SOURCE_KEY; else decrement key. |
| import type { Id } from './_generated/dataModel'; | ||
|
|
||
| /** Cap on the district-of-record scan. Bounded so it never hits the doc cap. */ | ||
| export const DISTRICT_SCAN_CAP = 10_000; |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Claude 🟡 medium] maintainability — Three scan caps with two values and no shared constant or documented rationale
DISTRICT_SCAN_CAP = 10_000 (line 22), GROWTH_WEEK_CAP = 10_000 (line 61), and VERIFIED_ACTION_PERIOD_SCAN_CAP = 16_000 (subscriptions.ts:57) are three separate constants. The billing cap is 1.6× larger than the others, but there is no comment explaining why, no shared base constant, and no link to Convex's actual per-query document limit (~16 K). The next engineer to add a fourth bounded scan will guess. Define a single CONVEX_QUERY_DOC_LIMIT constant in a shared module and derive all three from it (e.g. DISTRICT_SCAN_CAP = CONVEX_QUERY_DOC_LIMIT * 0.625), or at minimum add inline comments citing the Convex doc that defines the ceiling.
| * so an approximate tail is acceptable; the common handful of sources stay exact. | ||
| */ | ||
| const MAX_SOURCE_KEYS = 32; | ||
| const OTHER_SOURCE_KEY = 'other'; |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] correctness — OTHER_SOURCE_KEY = 'other' collides with real user-supplied import source labels
The overflow bucket uses the plain string 'other' as its key, which is indistinguishable from a real import where the operator labels the CSV source column as other. Once a real other and the folded other coexist in the map, delete/transition operations decrement the wrong conceptual bucket and the source breakdown becomes unrecoverably corrupted. Fix: use a reserved sentinel that cannot appear in user input, e.g. '__other__' or '\x00other', and document that the value is internal-only.
| const OTHER_SOURCE_KEY = 'other'; | |
| const OTHER_SOURCE_KEY = '__overflow__'; // sentinel — must not be a valid import source label |
|
|
||
| // Maintain the breakdown counters for the new row. Created subscribed/none | ||
| // with no identity/consent; postal/phone/source contribute when present. | ||
| await applySupporterStatsDelta(ctx, args.orgId, null, { |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Claude 🟡 medium] perf — Double org read+write on every new supporter creation in findOrCreateSupporter
On the create branch of findOrCreateSupporter, the mutation already reads the org doc at line 1086 and patches it (supporterCount + onboarding) at line 1097. The new applySupporterStatsDelta call at line 1106 then reads the org doc again inside the helper and issues a second patch (supporterStats). Result: every new action submission that creates a supporter incurs 2 db.get(orgId) + 2 db.patch(orgId) calls. This doubles OCC retry pressure on the hot org document at submission volume. The same pattern exists in supporters.create and v1api.createSupporter. Fix: either pass the already-fetched org into applySupporterStatsDelta to avoid the re-read, or merge both patches into a single db.patch call.
| updatedAt: s.updatedAt, | ||
| }); | ||
| ids.push(id); | ||
| stats = computeSupporterStats(stats, null, { |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Claude 🟡 medium] correctness — insertSupporterBatch omits phone, identity, and consent fields from computeSupporterStats
The seed loop calls computeSupporterStats but only passes emailStatus, smsStatus, source, postalCode, and verified. It omits encryptedPhone, phoneHash, identityCommitment, emailConsentSource/At/Text, and smsConsentSource/At/Text. Dev fixture orgs will therefore show phonePresent = 0 and emailConsentEvidence = 0 even when fixture supporters have phones and consent data. Every developer comparing the funnel breakdown to the supporter list will see a mismatch. The seed comment claims "same source of truth getSummaryStats reads" — it is not. Add the missing fields to match CountableSupporter fully.
| stats = computeSupporterStats(stats, null, { | |
| stats = computeSupporterStats(stats, null, { | |
| emailStatus: s.emailStatus, | |
| smsStatus: s.smsStatus, | |
| source: s.source, | |
| postalCode: s.postalCode, | |
| verified: s.verified, | |
| encryptedPhone: s.encryptedPhone, | |
| phoneHash: s.phoneHash, | |
| identityCommitment: s.identityCommitment, | |
| emailConsentSource: s.emailConsentSource, | |
| emailConsentedAt: s.emailConsentedAt, | |
| emailConsentText: s.emailConsentText, | |
| smsConsentSource: s.smsConsentSource, | |
| smsConsentedAt: s.smsConsentedAt, | |
| smsConsentText: s.smsConsentText, | |
| }); |
| sourceCounts: asNumberRecord(supporterSummary.sourceCounts), | ||
| postalResolved: asNumber(supporterSummary.postalResolved), | ||
| districtVerified: asNumber(supporterSummary.districtVerified), | ||
| districtVerified: asNumber(districtVerifiedResult?.districtVerified), |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Claude 🟡 medium] correctness — truncated flag from getDistrictVerifiedCount is silently dropped — large-org dashboards display a wrong exact count
getDistrictVerifiedCount returns { districtVerified, truncated, scanLimit }. The layout load extracts only districtVerifiedResult?.districtVerified and discards truncated. When an org has more than 10,000 verified actions the returned count is a floor, but the UI receives no signal to render it as ">= N" instead of an exact number. The same problem exists in supporters/+page.server.ts (districtVerifiedResult?.districtVerified ?? 0). getDashboardStats in organizations.ts does surface districtVerifiedTruncated, but that field is also never consumed in any frontend route. Fix: thread truncated through the server load's return value and handle it in the component.
…ket + completeness guard
There was a problem hiding this comment.
🧹 Nitpick comments (1)
convex/v1api.ts (1)
429-436: 💤 Low valueConsider using the read-back supporter for the delta instead of manually specifying fields.
The delta payload is manually constructed with a subset of fields (
emailStatus,smsStatus,source,postalCode,encryptedPhone,phoneHash), while the supporter is already read back at line 438. If future changes add new fields to both the insert andCountableSupporter, this manual list must also be updated or the counters will drift.Using the read-back supporter would keep the delta automatically in sync:
♻️ Suggested refactor
- await applySupporterStatsDelta(ctx, args.orgId, null, { - emailStatus: 'subscribed', - smsStatus: 'none', - source: args.source, - postalCode: args.postalCode, - encryptedPhone: args.encryptedPhone, - phoneHash: args.phoneHash - }); - const supporter = await ctx.db.get(id); + if (supporter) { + await applySupporterStatsDelta(ctx, args.orgId, null, supporter); + } return { duplicate: false, supporter };🤖 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 `@convex/v1api.ts` around lines 429 - 436, The delta passed to applySupporterStatsDelta is manually constructed and risks getting out of sync with the read-back CountableSupporter; instead build the delta from the already-read supporter object (the variable named supporter) and pass that to applySupporterStatsDelta so all present fields (emailStatus, smsStatus, source, postalCode, encryptedPhone, phoneHash, and any future additions) are included automatically; update the call site where applySupporterStatsDelta(ctx, args.orgId, null, { ... }) is used to pass the supporter-derived object (or a shallow pick from supporter) rather than the hard-coded literal.
🤖 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.
Nitpick comments:
In `@convex/v1api.ts`:
- Around line 429-436: The delta passed to applySupporterStatsDelta is manually
constructed and risks getting out of sync with the read-back CountableSupporter;
instead build the delta from the already-read supporter object (the variable
named supporter) and pass that to applySupporterStatsDelta so all present fields
(emailStatus, smsStatus, source, postalCode, encryptedPhone, phoneHash, and any
future additions) are included automatically; update the call site where
applySupporterStatsDelta(ctx, args.orgId, null, { ... }) is used to pass the
supporter-derived object (or a shallow pick from supporter) rather than the
hard-coded literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fde5659-f4d6-40b5-b64f-bca1fefce141
📒 Files selected for processing (12)
convex/_dashboardStats.tsconvex/_supporterStats.tsconvex/organizations.tsconvex/schema.tsconvex/subscriptions.tsconvex/supporters.tsconvex/v1api.tssrc/lib/components/org/os/spaces.tssrc/routes/org/[slug]/+layout.server.tssrc/routes/org/[slug]/supporters/+page.server.tstests/unit/org/supporter-stats-delta.test.tstests/unit/org/supporter-stats-writer-coverage.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- convex/_dashboardStats.ts
- src/routes/org/[slug]/supporters/+page.server.ts
- src/routes/org/[slug]/+layout.server.ts
- convex/organizations.ts
- convex/_supporterStats.ts
- convex/subscriptions.ts
- convex/supporters.ts
There was a problem hiding this comment.
🪓 Brutalist Review
Both critics converge on two pre-ship blockers: (1) there is no production backfill for supporterStats or actionTierCounts, so every existing org will show all-zero dashboard stats and supporter-page funnel data immediately after deploy — the gap grows worse as write activity accumulates atop wrong-zero baselines; and (2) blastSentThisPeriod retains a .collect() that is the same document-cap failure mode this PR is eliminating everywhere else. Claude uniquely caught that getDashboardStats discards thisWeekTruncated/lastWeekTruncated while correctly surfacing districtVerifiedTruncated in the same response object. Codex uniquely identified that the billing self-heal comment claims "over-counts (fails safe)" but Math.min(rows, cap) actually under-counts when period volume exceeds 16K — presently safe only because all plan limits (max 10K) sit below the cap, a fragile implicit invariant. Minor but real: sourceValue can collide with both the 'unknown' fallback (natural user label) and the '__other__' sentinel (not filtered by sourceValue), and the channel field needs a union validator to prevent bad values from becoming indexed data debt.
Inline comments: 6 (2 🟠 high · 4 🟡 medium)
Per-CLI breakdown
✅ Claude (default, 242268ms)
Identified the critical missing production backfill (all existing orgs show zeroed dashboards immediately post-deploy), the growth-truncation flag being silently dropped from getDashboardStats while districtVerifiedTruncated is correctly surfaced in the same response, and the blastSentThisPeriod .collect() that contradicts the PR's stated unbounded-scan removal goal. Also flagged sourceValue string-length and sentinel-collision issues, and the seed fixture gap for phone/identity fields.
✅ Codex (default, 98998ms)
Corroborated the backfill gap (anchored to supporters.ts read site), blastSentThisPeriod .collect(), and growth truncation drop. Added unique findings: the billing self-heal cap comment is directionally wrong (returns the cap, not the actual count, which under-counts large orgs — safe only because all plan limits sit below the cap today), the other overflow sentinel is not enforced by sourceValue so a literal source label 'other' collides with the fold bucket, and the channel field should be a union validator not a raw string. Prototype-pollution claim around sourceCounts bracket access is overstated and not confirmed given Convex's serialization model.
❌ agy (default, 900012ms)
Hit rate/usage limit — no output.
Out-of-diff findings (6)
maintainability
- 🔵 low
convex/_supporterStats.ts— Codex [unanchored]: Denormalized counter correctness enforced by convention, not architecture — any new writer can silently drift - ⚪ nit
convex/schema.ts— Codex [sub-threshold]:channelfield accepts arbitrary string — expected values only documented in a comment
correctness
- 🔵 low
convex/_supporterStats.ts— Claude [sub-threshold]: Source key string length is unbounded — crafted import label can bloat the org document - 🔵 low
convex/_supporterStats.ts— Claude [sub-threshold]:'unknown'fallback collides with literal user-supplied source label - 🔵 low
convex/_supporterStats.ts— Codex [sub-threshold]:__other__overflow sentinel is not actually reserved — a literal source label can collide with it - 🔵 low
convex/seed.ts— Claude [sub-threshold]: SeedcomputeSupporterStatscall omitsencryptedPhone,phoneHash, andidentityCommitment— dev fixtures show wrong funnel
Brutalist orchestrator schemaVersion=1 · context_id=b2083ea0-9ce0-452c-b5c6-d564713db255
| .collect(); | ||
| // Supporter funnel — O(1) denormalized counters (same as getSummaryStats). | ||
| const total = org.supporterCount ?? 0; | ||
| const stats = org.supporterStats ?? emptySupporterStats(); |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[Claude 🟠 high] correctness — No backfill for existing production orgs — all dashboards show zero post-deploy
supporterStats is v.optional(...) in schema, and every read site falls back to emptySupporterStats() (all zeros) when the field is absent. Every existing org has no supporterStats. The funnel dashboard and summary stats page will display zero across every bucket — postalResolved, identityVerified, emailSubscribed, etc. — until accumulated write activity brings the counters near reality. For an org with 50,000 subscribers and no pending imports, the counters stay wrong indefinitely. seed.ts bootstraps the counter for dev orgs only. There is no migration or one-shot backfill mutation for production. Fix: a paged backfill mutation that iterates supporters in bounded batches (e.g., 1,000/batch), folds each row's contribution into computeSupporterStats, and patches the org at the end. Do not ship without it.
| const stats = org.supporterStats ?? emptySupporterStats(); | |
| // Before shipping, run a one-shot backfill: | |
| // export const backfillSupporterStats = internalMutation({ | |
| // handler: async (ctx) => { | |
| // const orgs = await ctx.db.query('organizations').collect(); | |
| // for (const org of orgs) { | |
| // if (org.supporterStats !== undefined) continue; // already migrated | |
| // let stats = emptySupporterStats(); | |
| // let cursor = null; | |
| // do { | |
| // const page = await ctx.db.query('supporters') | |
| // .withIndex('by_orgId', idx => idx.eq('orgId', org._id)) | |
| // .paginate({ cursor, numItems: 1000 }); | |
| // for (const s of page.page) stats = computeSupporterStats(stats, null, s); | |
| // cursor = page.continueCursor; | |
| // } while (!page.isDone); | |
| // await ctx.db.patch(org._id, { supporterStats: stats }); | |
| // } | |
| // } | |
| // }); |
| } | ||
| } | ||
| const total = org.supporterCount ?? 0; | ||
| const stats = org.supporterStats ?? emptySupporterStats(); |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[Codex 🟠 high] correctness — Same zero-stats gap in getSummaryStats — the supporters page also shows all-zeroes for unmigrated orgs
Same root cause as the organizations.ts dashboard: org.supporterStats ?? emptySupporterStats() silently converts an absent historical field into zeros. Every existing org visiting the /supporters page immediately post-deploy will see postalResolved: 0, identityVerified: 0, empty source breakdown, and flat email/SMS health — none of it correct. Both read sites need the same backfill before the switch-over is live. The seed changes cover dev fixtures only.
| growth: { | ||
| thisWeek: verifiedThisWeek, | ||
| lastWeek: verifiedLastWeek | ||
| thisWeek: growth.thisWeek, |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Claude 🟡 medium] correctness — Growth truncation flags computed but silently dropped — high-volume orgs see a floor as an exact count
computeGrowthWindow returns thisWeekTruncated and lastWeekTruncated but getDashboardStats only threads thisWeek and lastWeek into its return object, discarding both flags. An org with >10,000 verified actions in a week gets a hard-clamped 10,000 with no ">=" indicator, while the same dashboard response correctly surfaces districtVerifiedTruncated for the district count. The inconsistency is in the same return object. At ~1,400 actions/day (60/hour) this is realistic for an org running a high-volume campaign. Fix: add thisWeekTruncated: growth.thisWeekTruncated, lastWeekTruncated: growth.lastWeekTruncated to the growth response, update spaces.ts types, and render N+ in the client.
| thisWeek: verifiedThisWeek, | ||
| lastWeek: verifiedLastWeek | ||
| thisWeek: growth.thisWeek, | ||
| lastWeek: growth.lastWeek |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] correctness — Growth window precision loss: thisWeekTruncated/lastWeekTruncated are computed and thrown away
computeGrowthWindow computes both truncation flags and returns them. The caller in getDashboardStats discards them, presenting a capped value as an exact metric. The code solved the Convex document-cap crash by replacing it with silent precision loss for high-volume orgs. districtVerifiedTruncated is already surfaced in the same return object — the growth widget needs the same treatment for consistency and honesty.
| .take(VERIFIED_ACTION_PERIOD_SCAN_CAP + 1); | ||
| // If the period's own volume somehow exceeds the cap, return the cap so | ||
| // enforcement over-counts (fails safe) rather than under-reporting. | ||
| return Math.min(rows.length, VERIFIED_ACTION_PERIOD_SCAN_CAP); |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] correctness — Billing self-heal cap comment claims "over-counts" but actually under-counts — latent enforcement gap
The comment says "enforcement over-counts (fails safe) rather than under-reporting" but Math.min(rows.length, VERIFIED_ACTION_PERIOD_SCAN_CAP) returns the cap (16K) when actual period usage exceeds it — that is under-counting relative to true usage. This is currently safe only because all plan maxVerifiedActions limits are ≤10K, which is below the 16K cap, so an org exceeding any plan limit would still read ≥10K and trigger enforcement. But the invariant is not expressed in code — if a plan ever raises its maxVerifiedActions above 16K, the self-heal path silently allows over-quota usage. The comment should be corrected and either the cap should be raised above all possible plan limits or a stricter guard should assert the invariant.
| .withIndex("by_orgId_sentAt", (idx) => | ||
| idx.eq("orgId", orgId).gte("sentAt", periodStart), | ||
| ) | ||
| .collect(); |
There was a problem hiding this comment.
🪓 Brutalist — 2 critics, rollup: 🟡 medium
[Claude 🟡 medium] correctness — blastSentThisPeriod still uses .collect() — the same unbounded-scan failure mode this PR is fixing
blastSentThisPeriod uses the new by_orgId_sentAt range index (good — it avoids the lifetime-history scan), but then calls .collect() instead of .take(cap + 1). One row per blast means a high-volume org (e.g. a PAC running daily sends across 50+ campaigns over the billing month) can still accumulate enough blasts in one period to hit the per-query document cap and crash the billing check on every plan-limit enforcement. The fix is .take(BLAST_PERIOD_SCAN_CAP + 1) with a cap constant set above the realistic period blast count but below Convex's 16K query limit.
[Codex 🟡 medium] correctness — blastSentThisPeriod retains an unbounded .collect() contradicting the PR's stated goal
This function was specifically introduced to replace unbounded blast scans, yet it still calls .collect() on the period-bounded result set. The range index bounds the historical horizon but not the within-period count. A high-volume sender accumulating thousands of blasts per billing period hits the same document-cap failure mode the PR is eliminating everywhere else. Consistency requires .take(cap + 1) here.
… + invariant clarity - reconcileOrgStats internalMutation: rebuild supporterStats/supporterCount/ actionTierCounts from rows (bounded by the scan cap) — the rebuild-from-truth path a denormalized-counter system needs. Pre-launch there is no production data to backfill; this corrects dev/seed orgs and repairs drift if a writer ever bypasses the delta despite the coverage guard. Leaves the single-writer, self-healing billing lifetime/baseline untouched. - blastSentThisPeriod: .collect() -> .take(cap+1) on the bounded range, fail-safe (block) on saturation — no .collect() left that can throw the doc cap. - getDashboardStats: surface growth thisWeekTruncated/lastWeekTruncated like the district floor (no silently-dropped truncation). - billing self-heal: make the cap-exceeds-max-plan-limit INVARIANT explicit and fix the misleading "over-counts" comment (it clamps; safe only because cap>limit). - sourceValue: remap a user label that equals the reserved '__other__' sentinel so it can never collide with the fold bucket. Held: production backfill (zero prod data; reconcile covers dev + drift). Deferred to D-06: tighten campaignActions.channel to a union once the emit defines the taxonomy.
|
1 participant
Solve the >16K-supporter ceiling at the foundations. Convex caps a single read at ~16,384 docs / 8MiB — an unbounded
.collect()over a per-org collection past that throws (page 500 / submit hard-lock). The list reader was already capped; this closes the three companion cliffs that were missed, using the codebase's existing denormalized-counter pattern (no@convex-dev/aggregate). Pre-launch zero-data ⇒ every counter starts at 0 and is exact from the first insert; no backfill.The three cliffs closed
getSummaryStats(every org page) — two unbounded collects (supporters + verified actions) → readsorg.supporterCount+org.supporterStats. Set-cardinality (districtVerified) moved off the always-on path into a separate bounded query.checkPlanLimitsbilling (every action/email/SMS submit — hard-locked the org at scale) → see below.getDashboardStats(ReturnSpace funnel) — same two collects → counters + bounded ranges.Counter discipline (
_supporterStats.ts)One delta helper wired into every supporter-status writer — create/update/remove/unsubscribe/importBatch,
findOrCreateSupporter,v1api.createSupporter, and the easy-to-miss ones: SES bounce + soft-bounce-threshold + inbound-SMS STOP/START (cross-org, per-row orgId). Status transitions read the pre-patch row. Also fixes pre-existingsupporterCountdrift in importBatch/v1api/placeholder-delete.Billing — scale-safe WITHOUT a period-reset counter
Honors the standing rule (
subscriptions.ts:153-155, "no denormalized counters for billing — avoids the never-reset bug"): a monotonicverifiedActionsLifetime(+1next to the existing verifiedActionCount bump — zero new write sites) minus a period baseline snapshotted at rollover (monotonic, can't rewind on duplicate webhooks). Read is O(1) when the snapshot matches the period; boundedsentAt-range self-heal (newby_orgId_verified_sentAtindex) when stale (late/missed webhook, free-tier calendar month); fail-safe over-count at the cap. A new period resets the effective count via the baseline, not a counter.Engagement-tier histogram
campaignActions.engagementTierverified immutable (only written at insert) → 5 monotonicorg.actionTierCounts, bumped once per insert. Growth (week-over-week) via two bounded range reads on the new index.Schema
campaignActions.channeldiscriminator added for the cross-channel attribution ledger (schema only; emit is a later wave).Evidence
tsc0 · svelte-check 0 errorsKnown, deliberately deferred (Wave 2)
Must-enumerate
.collect()sites that need pagination not counters (email/blast recipient resolution, packet rows, per-tag counts) — send/packet paths, not page-load.supporterStats.identityVerifiedis structurally 0 (no writer marks supporters verified; tracked viacampaignActions.trustTier) — counter correct, surfacing TBD.Summary by CodeRabbit
New Features
Tests