fix: stop /updates from serving deleted bundles#1879
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughExcludes deleted app-version rows from channel→version JOINs via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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".
![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
|
@copilot fix comments also ensure this new query will not destroy our perf because it's most critical one, revert change on supabaseAdmin unless you justify it's necessity |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03710e9308
ℹ️ 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".
Agent-Logs-Url: https://github.com/Cap-go/capgo/sessions/0cab38d8-3162-4685-be93-c0977a0d11e9 Co-authored-by: riderx <4084527+riderx@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Cap-go/capgo/sessions/0cab38d8-3162-4685-be93-c0977a0d11e9 Co-authored-by: riderx <4084527+riderx@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Cap-go/capgo/sessions/0cab38d8-3162-4685-be93-c0977a0d11e9 Co-authored-by: riderx <4084527+riderx@users.noreply.github.com>
Addressed in 2aeb652 and 9947504. I reverted the Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/updates.test.ts`:
- Line 5: Replace hardcoded BASE_URL usage in the test fetches with
getEndpointUrl(): update the import in tests/updates.test.ts to include
getEndpointUrl (and remove BASE_URL if no longer used), then change the fetch
calls that currently concatenate BASE_URL to instead call getEndpointUrl() when
building the request URL (e.g., replace BASE_URL + '/path' with getEndpointUrl()
+ '/path') so the test harness routing (USE_CLOUDFLARE_WORKERS) is respected for
the affected requests.
🪄 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: 417b7441-c347-4d75-ba8e-21409bf30c76
📒 Files selected for processing (3)
supabase/functions/_backend/utils/pg.tssupabase/functions/_backend/utils/supabase.tstests/updates.test.ts
✅ Files skipped from review due to trivial changes (1)
- supabase/functions/_backend/utils/supabase.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/functions/_backend/utils/pg.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9f5c1a752
ℹ️ 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".
|
Summary
(AI generated)
app_versionsfrom both/updateschannel-selection joins so soft-deleted bundles are no longer returnedsupabaseAdminAuthorization-header override so shared service-role client behavior stays unchanged/updates:Test plan
(AI generated)
supabase/functions/_backend/utils/pg.tsto confirm the deleted filter remains on the existing join pathsupabase/functions/_backend/utils/supabase.tsto confirm thesupabaseAdminAuthorization override was revertedtests/updates.test.tsonce dependencies are availablenpm.jsr.ioScreenshots
(AI generated)
Checklist
(AI generated)
bun run lint:backend && bun run lint.accordingly.
my tests
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests