Conversation
📝 WalkthroughWalkthroughThe pull request adds version deletion filtering to channel/device info queries, refactors the Supabase admin client initialization to explicitly handle the service role key, and expands test coverage with a new test for deleted bundle handling while refactoring existing update scenario tests using a shared helper function. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: b004950977
ℹ️ 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".
| .from(channelDevicesAlias) | ||
| .innerJoin(channelAlias, eq(channelDevicesAlias.channel_id, channelAlias.id)) | ||
| .innerJoin(versionAlias, eq(channelAlias.version, versionAlias.id)) | ||
| .innerJoin(versionAlias, and(eq(channelAlias.version, versionAlias.id), eq(versionAlias.deleted, false))) |
There was a problem hiding this comment.
Filtering channel_devices with eq(versionAlias.deleted, false) makes an override row disappear entirely when its channel points to a soft-deleted bundle. In the /updates flow, updateWithPG treats a missing override as "no override" and falls back to the default/public channel selection, so devices pinned to a private or staged channel can suddenly be routed to another channel’s release after bundle deletion. This is a behavior regression introduced by this join change; deleted override targets should produce an explicit non-update/no-channel result instead of implicit fallback routing.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/updates.test.ts (1)
689-718:⚠️ Potential issue | 🟡 MinorWrap the shared
productionchannel cleanup intry/finally.These cases restore
productiononly after the assertions pass. If either test exits early, later cases inherit the private/no-self-set state and start failing for the wrong reason.As per coding guidelines, "Create dedicated seed data for tests that modify shared resources; do not reuse seed data if your test modifies it."
Also applies to: 720-777
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/updates.test.ts` around lines 689 - 718, The test mutates the shared 'production' channel and currently restores it only after assertions, so wrap the channel state changes and assertions in a try/finally: call updateChannel('production', {...public:false...}) before the try, run postUpdate/getBaseData and all expects inside the try, and move the restoration updateChannel('production', {public:true, allow_device_self_set:true}) into the finally block to guarantee cleanup; apply the same try/finally pattern to the similar test block around lines 720-777 to ensure shared state is always restored.
🧹 Nitpick comments (1)
tests/updates.test.ts (1)
102-156: Consider achannel_devicesvariant of this regression too.This new case only protects the default-channel lookup. A future regression in the device-override join would still slip through unless that path is covered elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/updates.test.ts` around lines 102 - 156, Add a second test variant that covers the device-override path: in the existing test "does not resolve deleted bundles that are still referenced by a channel" create a device id (e.g., randomUUID()), insert a record into the channel_devices table linking that device_id to the same channelName (use the same fields as channels insert but just channel_id/name and device_id), then call postUpdate with the same baseData plus the device identifier field used by postUpdate (e.g., device_id or device) and assert the response.status is 200 and json.error === 'no_channel' and json.version is undefined; this ensures the device-override join path (channel_devices) is covered in addition to the default-channel lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/updates.test.ts`:
- Around line 689-718: The test mutates the shared 'production' channel and
currently restores it only after assertions, so wrap the channel state changes
and assertions in a try/finally: call updateChannel('production',
{...public:false...}) before the try, run postUpdate/getBaseData and all expects
inside the try, and move the restoration updateChannel('production',
{public:true, allow_device_self_set:true}) into the finally block to guarantee
cleanup; apply the same try/finally pattern to the similar test block around
lines 720-777 to ensure shared state is always restored.
---
Nitpick comments:
In `@tests/updates.test.ts`:
- Around line 102-156: Add a second test variant that covers the device-override
path: in the existing test "does not resolve deleted bundles that are still
referenced by a channel" create a device id (e.g., randomUUID()), insert a
record into the channel_devices table linking that device_id to the same
channelName (use the same fields as channels insert but just channel_id/name and
device_id), then call postUpdate with the same baseData plus the device
identifier field used by postUpdate (e.g., device_id or device) and assert the
response.status is 200 and json.error === 'no_channel' and json.version is
undefined; this ensures the device-override join path (channel_devices) is
covered in addition to the default-channel lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f46a18b7-fec9-4f3a-bb1b-9d3a8aab820f
📒 Files selected for processing (3)
supabase/functions/_backend/utils/pg.tssupabase/functions/_backend/utils/supabase.tstests/updates.test.ts
1 participant
Summary (AI generated)
Motivation (AI generated)
Deleting a bundle should immediately make it ineligible for OTA selection. Without the deleted filter, devices can still resolve a channel to a soft-deleted bundle as long as the channel still points at that version row.
Business Impact (AI generated)
This closes a stale-release security gap: when an operator deletes a bad or sensitive bundle, Capgo now stops offering it through /updates instead of continuing to route devices toward it.
Test Plan (AI generated)
bunx eslint "supabase/**/*.{ts,js}" tests/updates.test.tsbun run supabase:with-env -- bunx vitest run tests/updates.test.tsbun run test:allGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests