fix(stats): avoid extra apps query #1618
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes the /stats plugin endpoint by removing a redundant apps table lookup for allow_device_custom_id, instead reusing the existing getAppOwnerPostgres data and extending the app-status cache payload to carry this flag for the cancelled fast-path.
Changes:
- Extend the app status cache payload to optionally include
allow_device_custom_idand addgetAppStatusPayload()to read it. - Remove the extra
appsquery in/statsby usingappOwner.allow_device_custom_id(and cached payload on the cancelled fast-path). - Update callers that set cached app status (
update,stats,channel_self) to includeallow_device_custom_idin the cached payload.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| supabase/functions/_backend/utils/update.ts | Include allow_device_custom_id when caching cloud/cancelled app status so downstream fast-paths can reuse it. |
| supabase/functions/_backend/utils/appStatus.ts | Introduce AppStatusPayload, add getAppStatusPayload(), and allow setAppStatus() to store optional metadata in cache. |
| supabase/functions/_backend/plugins/stats.ts | Stop querying apps solely for allow_device_custom_id; use cached payload for cancelled fast-path and appOwner elsewhere. |
| supabase/functions/_backend/plugins/channel_self.ts | Include allow_device_custom_id when caching cloud/cancelled status for consistency with the new cache payload shape. |
| if (cachedStatus === 'cancelled') { | ||
| const statsActions: StatsActions[] = [{ action: 'needPlanUpgrade' }] | ||
| // Keep behavior backward compatible (default allow=true), but allow owners to | ||
| // disable custom_id persistence from unauthenticated /stats traffic. | ||
| if (!allowDeviceCustomId && device.custom_id !== undefined) { | ||
| const allowDeviceCustomId = hasCustomId ? (cached?.allow_device_custom_id ?? true) : true | ||
| if (!allowDeviceCustomId && hasCustomId) { | ||
| device.custom_id = undefined | ||
| statsActions.push({ action: 'customIdBlocked' }) | ||
| } | ||
| await sendStatsAndDevice(c, device, statsActions) |
There was a problem hiding this comment.
The new cancelled fast-path changes behavior (drop device.custom_id when cached allow_device_custom_id is false, and no longer emits the customIdBlocked stat). There are existing tests around customIdBlocked emission, but none that cover the cancelled-cache path; please add/adjust tests to cover this specific behavior so regressions are caught (including verifying no customIdBlocked entry is written when status is cancelled).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6c6323f82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const allowDeviceCustomId = hasCustomId ? (cached?.allow_device_custom_id ?? true) : true | ||
| if (!allowDeviceCustomId && hasCustomId) { |
There was a problem hiding this comment.
In the cancelled fast-path, the custom ID gate now depends only on cached metadata (cached?.allow_device_custom_id ?? true) and never re-reads the app row, so a cancelled app that changes allow_device_custom_id can keep accepting/persisting custom_id until the app-status cache expires. This regresses the immediate-enforcement behavior /stats previously had for custom ID requests (it opened a primary DB connection for these requests), and during mixed-version rollout it also treats status-only cache entries as allowed by default.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughThe pull request refactors app status management to propagate metadata (specifically Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant stats.ts as Plugin (stats.ts)
participant appStatus as Utility (appStatus.ts)
participant Cache
participant DB
Note over Client,DB: Old Flow: Direct DB Query
Client->>stats.ts: POST /stats
stats.ts->>DB: allowDeviceCustomIdFromPg()
DB->>stats.ts: allow_device_custom_id value
stats.ts->>stats.ts: Apply allow_device_custom_id check
stats.ts-->>Client: 200/429 Response
Note over Client,DB: New Flow: Cached Payload with Override
Client->>stats.ts: POST /stats
stats.ts->>appStatus: getAppStatusPayload(appId)
appStatus->>Cache: Fetch cached payload
alt Cache Hit
Cache->>appStatus: Return { status, allow_device_custom_id }
Note over appStatus: Special case:<br/>status='cancelled' + no Stripe<br/>→ lift to 'cloud'
else Cache Miss
appStatus->>DB: Query appOwner.allow_device_custom_id
DB->>appStatus: Value or null
appStatus->>Cache: Write payload
end
appStatus->>stats.ts: AppStatusPayload
stats.ts->>stats.ts: Derive allow_device_custom_id<br/>from cache or appOwner
stats.ts->>appStatus: setAppStatus(c, appId, status, { allow_device_custom_id })
appStatus->>Cache: Write combined payload
stats.ts-->>Client: 200/429 Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
riderx
force-pushed
the
riderx/stats-app-flag
branch
from
2dafe08 to
71bd735
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/utils/update.ts`:
- Line 88: getAppOwnerPostgres is not selecting or returning the
apps.allow_device_custom_id field but update.ts calls
appOwner.allow_device_custom_id when calling setAppStatus; update
getAppOwnerPostgres to include allow_device_custom_id in the SELECT (and in its
TypeScript return type/interface) so the returned object has
allow_device_custom_id available for callers like update.ts and matches
setAppStatus's expected payload.
In `@tests/stats.test.ts`:
- Around line 14-16: The regex used to set STRIPE_CONFIGURED in
tests/stats.test.ts uses a capturing group; change it to a non-capturing group
so the linter warning goes away by replacing the pattern used when computing the
STRIPE_CONFIGURED constant (currently /(sk_|rk_)/ inside /^(...)/) with a
non-capturing group (?:sk_|rk_) so the line that defines STRIPE_CONFIGURED
continues to test the same prefixes but without creating an unused capture.
🧹 Nitpick comments (1)
supabase/functions/_backend/utils/appStatus.ts (1)
25-37: Dead-code guard:buildAppStatusRequestalways returns a truthy object.
buildAppStatusRequest(line 17-23) unconditionally returns{ helper, request }, so theif (!cacheEntry)check on line 27 can never be true. Not harmful, but misleading.
| } | ||
| if (!appOwner.plan_valid) { | ||
| await setAppStatus(c, app_id, 'cancelled') | ||
| await setAppStatus(c, app_id, 'cancelled', { allow_device_custom_id: appOwner.allow_device_custom_id }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find pg.ts file
find . -name "pg.ts" -path "*/supabase/functions/_backend/*"Repository: Cap-go/capgo
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Search for getAppOwnerPostgres function definition
rg -n "getAppOwnerPostgres" --type=ts -B2 -A15 -g "*/pg.ts"Repository: Cap-go/capgo
Length of output: 204
🏁 Script executed:
#!/bin/bash
# Check update.ts lines around 88 and 99
rg -n "setAppStatus" --type=ts -B2 -A2 supabase/functions/_backend/utils/update.ts | head -30Repository: Cap-go/capgo
Length of output: 1209
🏁 Script executed:
#!/bin/bash
# Check for the apps table schema to see if allow_device_custom_id exists
find . -name "*schema*" -o -name "*migrations*" | grep -E "(postgre|schema|migration)" | head -10Repository: Cap-go/capgo
Length of output: 564
🏁 Script executed:
#!/bin/bash
# Read pg.ts file and look for getAppOwnerPostgres
cat ./supabase/functions/_backend/utils/pg.ts | head -300Repository: Cap-go/capgo
Length of output: 11764
🏁 Script executed:
#!/bin/bash
# Search for getAppOwnerPostgres more carefully
grep -n "getAppOwnerPostgres" ./supabase/functions/_backend/utils/pg.ts -A 30Repository: Cap-go/capgo
Length of output: 2452
🏁 Script executed:
#!/bin/bash
# Check postgres_schema.ts for the apps table definition
cat ./supabase/functions/_backend/utils/postgres_schema.ts | grep -A 50 "apps"Repository: Cap-go/capgo
Length of output: 7806
🏁 Script executed:
#!/bin/bash
# Check for setAppStatus function in appStatus.ts to understand what parameters it accepts
find . -name "appStatus.ts" -path "*/supabase/functions/_backend/*" -exec cat {} \;Repository: Cap-go/capgo
Length of output: 2242
Fix getAppOwnerPostgres to include allow_device_custom_id in the return type.
The TypeScript build fails because getAppOwnerPostgres in supabase/functions/_backend/utils/pg.ts does not select or return allow_device_custom_id, yet update.ts (lines 88, 99) attempts to access this property from the returned object. The field exists in the apps table schema and setAppStatus accepts it in the payload; it just needs to be included in the query and return type of getAppOwnerPostgres.
🧰 Tools
🪛 GitHub Actions: Run tests
[error] 88-88: Property 'allow_device_custom_id' does not exist on type '{ owner_org: string; orgs: { created_by: string; id: string; management_email: string; }; plan_valid: boolean; channel_device_count: number; manifest_bundle_count: number; expose_metadata: boolean; }'.
🤖 Prompt for AI Agents
In `@supabase/functions/_backend/utils/update.ts` at line 88, getAppOwnerPostgres
is not selecting or returning the apps.allow_device_custom_id field but
update.ts calls appOwner.allow_device_custom_id when calling setAppStatus;
update getAppOwnerPostgres to include allow_device_custom_id in the SELECT (and
in its TypeScript return type/interface) so the returned object has
allow_device_custom_id available for callers like update.ts and matches
setAppStatus's expected payload.
| // Cancelled app-status cache is only honored when Stripe is configured in the worker env. | ||
| // CI/local Cloudflare tests typically don't configure Stripe, so we skip cache-specific tests there. | ||
| const STRIPE_CONFIGURED = /^(sk_|rk_)/.test(env.STRIPE_SECRET_KEY || '') |
There was a problem hiding this comment.
ESLint: use a non-capturing group in the regex.
Static analysis flags the unused capturing group. Since you only need to test membership, switch to (?:...).
🔧 Proposed fix
-const STRIPE_CONFIGURED = /^(sk_|rk_)/.test(env.STRIPE_SECRET_KEY || '')
+const STRIPE_CONFIGURED = /^(?:sk_|rk_)/.test(env.STRIPE_SECRET_KEY || '')📝 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.
| // Cancelled app-status cache is only honored when Stripe is configured in the worker env. | |
| // CI/local Cloudflare tests typically don't configure Stripe, so we skip cache-specific tests there. | |
| const STRIPE_CONFIGURED = /^(sk_|rk_)/.test(env.STRIPE_SECRET_KEY || '') | |
| // Cancelled app-status cache is only honored when Stripe is configured in the worker env. | |
| // CI/local Cloudflare tests typically don't configure Stripe, so we skip cache-specific tests there. | |
| const STRIPE_CONFIGURED = /^(?:sk_|rk_)/.test(env.STRIPE_SECRET_KEY || '') |
🧰 Tools
🪛 ESLint
[error] 16-16: Capturing group number 1 is defined but never used.
(regexp/no-unused-capturing-group)
🤖 Prompt for AI Agents
In `@tests/stats.test.ts` around lines 14 - 16, The regex used to set
STRIPE_CONFIGURED in tests/stats.test.ts uses a capturing group; change it to a
non-capturing group so the linter warning goes away by replacing the pattern
used when computing the STRIPE_CONFIGURED constant (currently /(sk_|rk_)/ inside
/^(...)/) with a non-capturing group (?:sk_|rk_) so the line that defines
STRIPE_CONFIGURED continues to test the same prefixes but without creating an
unused capture.
|
1 participant
Summary (AI generated)
/statsfrom queryingappsa second time forallow_device_custom_id; reuse the existinggetAppOwnerPostgreslookup.allow_device_custom_idfor the cancelled fast-path.customIdBlocked(still dropcustom_idif disabled).Test plan (AI generated)
bun lint:backendbun typecheckbun lintScreenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.Generated with AI
Summary by CodeRabbit
New Features
Bug Fixes
Tests