Conversation
📝 WalkthroughWalkthroughThis PR refactors app status management to include Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as App PUT API
participant Cache as Cache/Cloudflare
participant DB as Database
participant Stats as Stats Plugin
Client->>API: PUT /app (allow_device_custom_id)
activate API
API->>DB: Update app record
API->>Cache: Get current app status payload
activate Cache
Cache-->>API: Return cached status
deactivate Cache
alt Cached status is 'cancelled'
API->>Cache: Update cached payload with allow_device_custom_id
activate Cache
Cache-->>API: Cache updated (sync)
deactivate Cache
end
API-->>Client: Response
deactivate API
Note over Stats: Next request with stats
Stats->>Cache: Get app status payload
activate Cache
Cache-->>Stats: Return { status, allow_device_custom_id }
deactivate Cache
Stats->>Stats: Use cached allow_device_custom_id for custom_id handling
Stats-->>Stats: No DB query needed (payload-based)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![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).
![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
🤖 Fix all issues with AI agents
In `@tests/stats.test.ts`:
- Line 16: The regex used to define the STRIPE_CONFIGURED constant contains a
capturing group which triggers an ESLint warning; update the pattern in the
STRIPE_CONFIGURED declaration (the const STRIPE_CONFIGURED assignment) to use a
non-capturing group (?:sk_|rk_) instead of (sk_|rk_) so the group is not
captured while preserving the same match behavior.
| const USE_CLOUDFLARE = env.USE_CLOUDFLARE_WORKERS === 'true' | ||
| // 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.
Use a non-capturing group to fix the ESLint warning.
The static analysis tool flags an unused capturing group in the regex.
🔧 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.
| const STRIPE_CONFIGURED = /^(sk_|rk_)/.test(env.STRIPE_SECRET_KEY || '') | |
| 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` at line 16, The regex used to define the
STRIPE_CONFIGURED constant contains a capturing group which triggers an ESLint
warning; update the pattern in the STRIPE_CONFIGURED declaration (the const
STRIPE_CONFIGURED assignment) to use a non-capturing group (?:sk_|rk_) instead
of (sk_|rk_) so the group is not captured while preserving the same match
behavior.
|
|
trash code I made it myself |
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
Bug Fixes
Performance
Tests