fix(stats): avoid extra apps query #1620
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ 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 |
![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: 71bd73592e
ℹ️ 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".
| return { success: true, isOnprem: true } | ||
| } | ||
|
|
||
| const allowDeviceCustomId = hasCustomId ? appOwner.allow_device_custom_id : true |
There was a problem hiding this comment.
appOwner.allow_device_custom_id is read here, but getAppOwnerPostgres does not currently select or type that field (it only returns owner/plan/channel/metadata fields in utils/pg.ts), so this value is always undefined on the /stats DB path. When a request includes custom_id, allowDeviceCustomId becomes falsey and the handler drops custom_id (and emits customIdBlocked) even for apps that actually allow custom IDs, which regresses production stats/device persistence behavior for any app sending custom_id.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR optimizes the plugin /stats path by eliminating an extra apps lookup for allow_device_custom_id, instead reusing existing ownership/plan checks and extending the app-status cache payload so the cancelled fast-path can enforce custom_id dropping without additional DB queries.
Changes:
- Extend app-status caching to store optional metadata (
allow_device_custom_id) and exposegetAppStatusPayload(). - Update
/stats,/channel_self, and update logic to writeallow_device_custom_idinto the app-status cache for cancelled/cloud states and adjust cancelled behavior to avoid emittingcustomIdBlocked. - Add tests covering cancelled-plan custom_id dropping and (optionally) the Cloudflare cancelled-cache fast-path.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/stats.test.ts | Adds regression tests for cancelled-plan behavior and cancelled-cache fast-path (Cloudflare-gated). |
| supabase/functions/_backend/utils/update.ts | Writes allow_device_custom_id into app-status cache when setting cancelled/cloud. |
| supabase/functions/_backend/utils/appStatus.ts | Introduces AppStatusPayload, getAppStatusPayload(), and payload-aware setAppStatus() with eager writes for cancelled/onprem. |
| supabase/functions/_backend/public/app/put.ts | Best-effort refresh of cached allow_device_custom_id for existing cancelled cache entries when app settings change. |
| supabase/functions/_backend/plugins/stats.ts | Removes the extra apps query and relies on cached payload / appOwner to enforce custom_id handling. |
| supabase/functions/_backend/plugins/channel_self.ts | Writes allow_device_custom_id into app-status cache when setting cancelled/cloud. |
| const allowDeviceCustomId = hasCustomId ? appOwner.allow_device_custom_id : true | ||
| 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 }) | ||
| cloudlog({ requestId: c.get('requestId'), message: 'Cannot update, upgrade plan to continue to update', id: app_id }) | ||
| const upgradeActions: StatsActions[] = [{ action: 'needPlanUpgrade' }] | ||
| if (!allowDeviceCustomId && device.custom_id !== undefined) { | ||
| if (!allowDeviceCustomId && hasCustomId) { | ||
| device.custom_id = undefined |
There was a problem hiding this comment.
appOwner.allow_device_custom_id is used here, but getAppOwnerPostgres() currently does not select/return that column (see utils/pg.ts around getAppOwnerPostgres). This will be a TypeScript error and at runtime the value will be undefined, breaking custom_id enforcement and the cancelled-cache payload population. Update getAppOwnerPostgres() to include allow_device_custom_id (and keep the replica-safe/backward-compatible selection behavior you previously had via to_jsonb(...)->>'allow_device_custom_id').
| @@ -96,7 +96,7 @@ export async function updateWithPG( | |||
| }, appOwner.owner_org, app_id, '0 0 * * 1', appOwner.orgs.management_email, drizzleClient)) // Weekly on Monday | |||
| return c.json({ error: 'on_premise_app', message: 'On-premise app detected' }, 429) | |||
| } | |||
| await setAppStatus(c, app_id, 'cloud') | |||
| await setAppStatus(c, app_id, 'cloud', { allow_device_custom_id: appOwner.allow_device_custom_id }) | |||
There was a problem hiding this comment.
appOwner.allow_device_custom_id is referenced when writing the app-status cache payload, but getAppOwnerPostgres() does not currently include that field in its select/return shape. This will not type-check and will write allow_device_custom_id: undefined to cache. Extend getAppOwnerPostgres() to return the column (preferably in a replica-safe way if schema rollout lag is a concern).
| @@ -120,7 +120,7 @@ async function assertChannelSelfAppOwnerPlanValid( | |||
| return { response: c.json({ error: 'on_premise_app', message: 'On-premise app detected' }, 429) } | |||
| } | |||
|
|
|||
| await setAppStatus(c, appId, 'cloud') | |||
| await setAppStatus(c, appId, 'cloud', { allow_device_custom_id: appOwner.allow_device_custom_id }) | |||
There was a problem hiding this comment.
appOwner.allow_device_custom_id is accessed here, but getAppOwnerPostgres() currently does not select/return that column (see utils/pg.ts). This should fail TypeScript and will make the cache payload/enforcement incorrect at runtime. Update getAppOwnerPostgres() to include allow_device_custom_id (ideally preserving the previous replica-safe fallback semantics).
|
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