Conversation
📝 WalkthroughWalkthroughEnables active Cloudflare Workers testing and wiring, adds multiple Cloudflare API routes, refactors per-request rate-limit recording to synchronous guarded calls, updates worker startup to build and supply a runtime env (extracted from Supabase CLI), removes D1 sync from test flows, and adjusts routing for Cloudflare vs Supabase endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Script as start-cloudflare-workers.sh
participant SupabaseCLI as Supabase CLI / bunx
participant EnvFile as runtime .env.tmp
participant Wrangler as wrangler (workers)
participant Workers as CF Workers (API/Plugin/Files)
participant Tests as Test runner (bun run)
CI->>Script: run start script (background-action)
Script->>SupabaseCLI: supabase status -> extract SUPABASE_URL, KEYS
SupabaseCLI-->>Script: return keys
Script->>EnvFile: write SUPABASE_* and CF defaults
Script->>Wrangler: start workers with EnvFile (API/Plugin/Files)
Wrangler->>Workers: launch (listening ports)
CI->>Tests: run backend tests (USE_CLOUDFLARE true)
Tests->>Workers: invoke endpoints (files, webhooks, device APIs)
Workers->>SupabaseCLI: (runtime calls) Supabase endpoints / DB
Tests-->>CI: report results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: c44d7524a4
ℹ️ 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".
scripts/start-cloudflare-workers.sh
Outdated
| # Build a runtime env file with local Supabase keys so we don't commit secrets. | ||
| BASE_ENV_FILE="${ROOT_DIR}/cloudflare_workers/.env.local" | ||
| RUNTIME_ENV_FILE="$(mktemp)" | ||
| cp "${BASE_ENV_FILE}" "${RUNTIME_ENV_FILE}" |
There was a problem hiding this comment.
On macOS/BSD environments mktemp requires a template argument, so mktemp without parameters exits non‑zero. Because this script runs with set -e, that failure stops the worker boot before any processes start, breaking local Cloudflare testing on macOS. Consider using a portable template like mktemp "${TMPDIR:-/tmp}/capgo-cloudflare-env.XXXXXX" (or mktemp -t capgo-cloudflare-env.XXXXXX) so the script works across developer machines.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
supabase/functions/_backend/utils/stats.ts (1)
134-138:⚠️ Potential issue | 🟠 Major
readDeviceVersionCountsshould also checkgetRuntimeKey()when accessingDEVICE_INFO.At line 135,
readDeviceVersionCountschecksc.env.DEVICE_INFOwithout thegetRuntimeKey() === 'workerd'guard, unlikecountDevices(line 288) andreadDevices(line 298), which both include this check. The comments on those functions explain that DEVICE_INFO bindings are often absent in local Cloudflare testing, so the same guard should apply toreadDeviceVersionCountsto avoid binding unavailability errors in local environments.Change:
if (!c.env.DEVICE_INFO)to:
if (getRuntimeKey() !== 'workerd' || !c.env.DEVICE_INFO)tests/channel_self.test.ts (1)
1169-1243:⚠️ Potential issue | 🟠 MajorMixed tabs and spaces — ESLint will fail on these lines.
Lines 1169–1243 use tab characters mixed with spaces for indentation. The static analysis reports dozens of
no-tabs/no-mixed-spaces-and-tabs/indenterrors here. The PR checklist requires lint to pass (bun lint:backend), so this block needs to be reformatted to use spaces only (2-space indent).Fix: re-indent with spaces
Replace all leading tabs in the
it('should clean up old channel_devices entry…')test and itsfinallyblock with 2-space indentation. For example:- it('should clean up old channel_devices entry when migrating from old to new version', async () => { - const deviceId = randomUUID() - const data = getUniqueBaseData(APPNAME) + it('should clean up old channel_devices entry when migrating from old to new version', async () => { + const deviceId = randomUUID() + const data = getUniqueBaseData(APPNAME)Apply the same fix through the entire block (lines 1169–1243).
CLOUDFLARE_TESTING.md (1)
147-147:⚠️ Potential issue | 🟡 MinorStale path: still references
internal/cloudflare/.env.local.Line 147 in the Troubleshooting section still points to the old path. This should match the updated references on lines 33 and 116.
Fix
-- Verify `internal/cloudflare/.env.local` has correct database credentials +- Verify `cloudflare_workers/.env.local` has correct database credentials
🤖 Fix all issues with AI agents
In `@cloudflare_workers/api/index.ts`:
- Line 8: ESLint reports the import ordering for "app as accept_invitation" is
incorrect; move the import from
'../../supabase/functions/_backend/private/accept_invitation.ts' so it is
alphabetically before the import for
'../../supabase/functions/_backend/private/credits.ts' to satisfy
perfectionist/sort-imports; find the import statement that declares "import {
app as accept_invitation }" in cloudflare_workers/api/index.ts and reposition it
above the "credits" private import (preserving the exact import specifier
names).
In `@scripts/start-cloudflare-workers.sh`:
- Around line 30-33: The current extraction of env values (SUPA_ENV) uses awk
-F= and print $2 for SUPABASE_URL_FROM_STATUS,
SUPABASE_SERVICE_ROLE_KEY_FROM_STATUS, and SUPABASE_ANON_KEY_FROM_STATUS which
drops any '=' characters in the value (breaking JWTs); change the extraction to
split only on the first '=' (e.g., use cut -d'=' -f2- or a sed that strips the
leading "KEY=") when processing SUPA_ENV so SECRET_KEY and PUBLISHABLE_KEY
retain any trailing '=' padding; update the lines that set
SUPABASE_SERVICE_ROLE_KEY_FROM_STATUS and SUPABASE_ANON_KEY_FROM_STATUS (and
optionally SUPABASE_URL_FROM_STATUS) to use that method instead of awk -F=
'{print $2}'.
- Around line 26-28: The script blindly copies BASE_ENV_FILE into
RUNTIME_ENV_FILE which will trigger a cryptic failure under set -e if
cloudflare_workers/.env.local is missing; add an existence guard around
BASE_ENV_FILE (the variable BASE_ENV_FILE) before invoking mktemp/cp: if the
file does not exist, either create an empty file or emit a clear error and exit,
then proceed to create RUNTIME_ENV_FILE (the RUNTIME_ENV_FILE variable via
mktemp) and cp; update the flow around the cp invocation to only run when
BASE_ENV_FILE exists to avoid unexpected script termination.
In `@supabase/functions/_backend/files/files.ts`:
- Around line 594-604: The HEAD TUS routing forwards to uploadHandler but
uploadHandler currently uses c.req.method (which Hono/tiny rewrote to 'GET')
when proxying to the Durable Object; update uploadHandler so it uses
c.req.raw.method when constructing/forwarding the request to the Durable Object
(preserve HEAD and ensure no body is sent for HEAD) instead of c.req.method, so
the DO receives the original HTTP verb; locate references to c.req.method inside
uploadHandler and replace them with c.req.raw.method (and explicitly handle
omitting the body for HEAD).
In `@supabase/functions/_backend/plugins/channel_self.ts`:
- Around line 580-595: The finally block in the channel_self handlers now awaits
recordChannelSelfRequest and thus delays responses; revert to non-blocking
background execution by calling recordChannelSelfRequest and
recordChannelSelfIPRateLimit via the existing backgroundTask helper (or
c.executionCtx.waitUntil if available) instead of awaiting them in the finally
after closeClient, and add a short comment in the finally explaining the
trade-off for CF Worker environments if you intentionally choose to await
instead; update the POST/PUT/DELETE/GET handlers (the finally that wraps
post/put/delete/get + closeClient) to call backgroundTask(() =>
recordChannelSelfRequest(...)) and backgroundTask(() =>
recordChannelSelfIPRateLimit(...)) (or use c.executionCtx.waitUntil(Promise)) so
the response is not delayed.
🧹 Nitpick comments (7)
scripts/test-cloudflare-v2.sh (1)
20-25: Step numbering is inconsistent after removing step 2.Steps jump from "Step 1" (Line 21) to "Step 3" (Line 25). Renumber to keep them sequential now that the D1 sync step has been removed.
Note: The Shellcheck SC2209 warning on Line 22 is a false positive —
PAGER=catcorrectly sets the env var to the literal string"cat"to suppress interactive pagers.Proposed fix
-# 3. Start workers -echo -e "\n${YELLOW}Step 3: Starting Cloudflare Workers...${NC}" +# 2. Start workers +echo -e "\n${YELLOW}Step 2: Starting Cloudflare Workers...${NC}"-# 4. Run tests -echo -e "\n${YELLOW}Step 4: Running tests...${NC}" +# 3. Run tests +echo -e "\n${YELLOW}Step 3: Running tests...${NC}"tests/ok.test.ts (1)
27-34: Inconsistency: bypassesgetEndpointUrland targets API worker instead of plugin worker for/ok.
getEndpointUrl('/ok')routes toCLOUDFLARE_PLUGIN_URL(port 8788), but this test hitsCLOUDFLARE_API_URL(port 8787) directly. If/okis a health check on all workers this is fine as a smoke test, but it diverges from the routing convention used elsewhere. Consider usinggetEndpointUrl('/ok')for consistency, or add a comment explaining why the API worker is targeted specifically. As per coding guidelines, backend tests must use helpers fromtests/test-utils.tsincludinggetEndpointUrl(path)for correct worker routing.supabase/functions/_backend/triggers/queue_consumer.ts (1)
312-332: AbortController is instantiated but never wired — dead timeout timer.
controllerandtimeoutIdare created (Lines 313-314) butsignal: controller.signalis commented out at Line 323. ThesetTimeoutstill fires after 15s and callscontroller.abort()to no effect, andclearTimeoutruns infinally. This is harmless but wasteful.Either uncomment the signal to enable the timeout or remove the AbortController entirely to avoid confusion.
supabase/functions/_backend/public/device/index.ts (2)
23-28:assertDeviceIPRateLimitalways throws on limit — theif (ipLimit)guard is dead code.
simpleRateLimitis typed(): never, so it always throws.assertDeviceIPRateLimittherefore either returnsundefinedor throws — it never returns a truthy value. Theif (ipLimit) return ipLimitchecks on lines 55–57, 106–108, and 156–158 are unreachable in the rate-limited path.This works correctly (the throw propagates to Hono's error handler and the
finallyblock still runs), but the guard is misleading. Consider renaming to make the "assert" (throw) semantics explicit, or simply call without capturing the return:Proposed simplification
- const ipLimit = await assertDeviceIPRateLimit(c, body.app_id) - if (ipLimit) - return ipLimit + await assertDeviceIPRateLimit(c, body.app_id)Also applies to: 54-58, 105-108, 155-158
68-89: Consider extracting the duplicatedfinallyblocks into a shared helper.The rate-limit recording logic in the
finallyblocks for POST, GET, and DELETE is nearly identical — only the operation string ('set','get','delete') differs. A small helper would reduce ~57 lines to ~19 + 3 one-liners.Sketch
async function recordRateLimits( c: Context, appId: string | undefined, deviceId: string | undefined, operation: ChannelSelfOperation, ) { if (deviceId && appId) { try { await recordChannelSelfRequest(c, appId, deviceId, operation) } catch (error) { cloudlog({ requestId: c.get('requestId'), message: `Failed to record device ${operation} rate limit`, app_id: appId, device_id: deviceId, error }) } } if (appId) { try { await recordDeviceIPRateLimit(c, appId) } catch (error) { cloudlog({ requestId: c.get('requestId'), message: 'Failed to record device IP rate limit', app_id: appId, error }) } } }Also applies to: 118-139, 168-189
tests/channel_self.test.ts (1)
680-681: Nit:getUniqueBaseDataalready sets a uniquedevice_id— the subsequent override is redundant.Several tests call
getUniqueBaseData(APPNAME)(which assigns a freshrandomUUIDtodevice_id) and then immediately overwritedata.device_idwith anotherrandomUUID(). The second assignment is harmless but unnecessary.Also applies to: 733-734, 858-860, 956-957, 969-970
tests/channel-rate-limit.test.ts (1)
8-8: Hardcoded limit constant should stay in sync with the source of truth.
OP_LIMIT_PER_SECOND = 5mirrorsOP_RATE_LIMIT_PER_SECONDinchannelSelfRateLimit.ts. If the backend constant changes, this test will silently break. Consider importing or adding a comment referencing the source constant.
There was a problem hiding this comment.
Pull request overview
Updates the local Cloudflare Workers testing workflow so the Cloudflare-targeted Vitest suite can run reliably (locally and in CI) by aligning worker routing, runtime behavior, and test expectations.
Changes:
- Route test requests to the correct Cloudflare worker (API/Plugin/Files) and adjust a few tests for Cloudflare mode.
- Improve local Cloudflare runtime behavior (stats fallback when Analytics Engine bindings are missing; rate-limit recording timing; TUS HEAD routing).
- Add/refresh local testing scripts + documentation, and enable Cloudflare Workers backend tests in CI.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test-utils.ts | Routes “files” endpoints to the Files worker in Cloudflare mode. |
| tests/ok.test.ts | Uses Cloudflare worker /ok when running in Cloudflare mode. |
| tests/error-cases.test.ts | Uses getEndpointUrl() so plugin/files requests hit the right worker. |
| tests/channel_self.test.ts | Ensures unique device IDs in many cases; adjusts a migration test to avoid same-channel limiter. |
| tests/channel-rate-limit.test.ts | Updates tests to reflect the 5-ops/sec burst behavior and avoids same-channel limiter conflicts. |
| supabase/functions/_backend/utils/webhook.ts | Gates localhost webhook URL allowance behind CAPGO_ALLOW_LOCAL_WEBHOOK_URLS. |
| supabase/functions/_backend/utils/stats.ts | Falls back to Supabase/Postgres stats paths when Analytics Engine bindings aren’t present in local workerd. |
| supabase/functions/_backend/triggers/queue_consumer.ts | Normalizes function_type and improves Cloudflare URL selection/back-compat for older messages. |
| supabase/functions/_backend/public/device/index.ts | Makes rate-limit recording immediate/synchronous and adjusts IP limiter handling. |
| supabase/functions/_backend/plugins/channel_self.ts | Makes IP limiter return handling consistent; records rate limit synchronously with error logging. |
| supabase/functions/_backend/files/files.ts | Fixes HEAD detection in Hono/tiny and routes TUS HEAD correctly. |
| scripts/test-cloudflare-v2.sh | Updates script comments and removes the obsolete D1 sync step. |
| scripts/start-cloudflare-workers.sh | Generates a runtime env file from local Supabase status and starts the 3 workers. |
| cloudflare_workers/api/index.ts | Adds missing API/private routes and exposes /webhooks; adds triggers /ok. |
| CLOUDFLARE_TESTING.md | Updates env-file location and documents runtime overrides done by the start script. |
| .github/workflows/tests.yml | Starts local Cloudflare workers in CI and runs test:cloudflare:backend. |
scripts/start-cloudflare-workers.sh
Outdated
| SUPABASE_SERVICE_ROLE_KEY_FROM_STATUS="$(printf '%s\n' "${SUPA_ENV}" | awk -F= '/^SECRET_KEY=/{print $2}' | tr -d '"')" | ||
| SUPABASE_ANON_KEY_FROM_STATUS="$(printf '%s\n' "${SUPA_ENV}" | awk -F= '/^PUBLISHABLE_KEY=/{print $2}' | tr -d '"')" |
There was a problem hiding this comment.
The keys extracted from supabase status -o env appear to use the wrong variable names (SECRET_KEY / PUBLISHABLE_KEY). Supabase’s env output is typically SERVICE_ROLE_KEY and ANON_KEY, so this script may exit with “Missing Supabase keys…” even when Supabase is running. Update the awk patterns (or switch to supabase status -o json and parse deterministically) so SUPABASE_SERVICE_ROLE_KEY and SUPABASE_ANON_KEY are reliably populated in CI/local runs.
| SUPABASE_SERVICE_ROLE_KEY_FROM_STATUS="$(printf '%s\n' "${SUPA_ENV}" | awk -F= '/^SECRET_KEY=/{print $2}' | tr -d '"')" | |
| SUPABASE_ANON_KEY_FROM_STATUS="$(printf '%s\n' "${SUPA_ENV}" | awk -F= '/^PUBLISHABLE_KEY=/{print $2}' | tr -d '"')" | |
| SUPABASE_SERVICE_ROLE_KEY_FROM_STATUS="$(printf '%s\n' "${SUPA_ENV}" | awk -F= '/^(SERVICE_ROLE_KEY|SECRET_KEY)=/{print $2}' | tr -d '"')" | |
| SUPABASE_ANON_KEY_FROM_STATUS="$(printf '%s\n' "${SUPA_ENV}" | awk -F= '/^(ANON_KEY|PUBLISHABLE_KEY)=/{print $2}' | tr -d '"')" |
tests/channel_self.test.ts
Outdated
| it('should clean up old channel_devices entry when migrating from old to new version', async () => { | ||
| const deviceId = randomUUID() | ||
| const data = getUniqueBaseData(APPNAME) | ||
| data.device_id = deviceId |
There was a problem hiding this comment.
These lines contain tab indentation (\t), which is inconsistent with the surrounding file and is likely to fail formatting/lint rules. Replace the tabs with spaces to keep indentation consistent and avoid CI lint failures.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/files/files.ts (1)
26-26:⚠️ Potential issue | 🟡 MinorStale comment: "20 minutes" but the value is 30 minutes.
The constant is
1000 * 60 * 30(30 minutes), but the comment says "20 minutes."Suggested fix
-const DO_CALL_TIMEOUT = 1000 * 60 * 30 // 20 minutes +const DO_CALL_TIMEOUT = 1000 * 60 * 30 // 30 minutes
🧹 Nitpick comments (5)
tests/channel_self.test.ts (2)
680-681: Redundantdevice_idoverride aftergetUniqueBaseData.
getUniqueBaseDataalready assigns a freshrandomUUID()todevice_id(line 21). The immediate re-assignment on line 681 is unnecessary. The same pattern repeats in several other tests (lines 733–734, 786–787, 858–860, 928, 943, 957, 970).Not urgent, but cleaning these up would make the intent of
getUniqueBaseDataclearer.
88-263: Tests useit()instead ofit.concurrent()— pre-existing but worth noting.Per coding guidelines, tests should use
it.concurrent()for parallel execution. However, many tests here mutate shared channel state (togglingallow_device_self_set,allow_emulator, etc.) and depend on sequential ordering, so switching isn't trivial. This is pre-existing and not introduced by this PR, but worth tracking for a future refactor that isolates state per test. As per coding guidelines, "Design tests for parallel execution; useit.concurrent()instead ofit()to run tests in parallel within the same file; create dedicated seed data when tests modify shared resources".scripts/start-cloudflare-workers.sh (2)
70-77: Duplicate keys possible when.env.localalready defines Supabase vars.
cat >>appends to the runtime env file, which already contains the contents of.env.local. If that file already definesSUPABASE_URL,SUPABASE_SERVICE_ROLE_KEY, etc., the runtime file will have duplicate entries. Depending on howwrangler --env-fileresolves duplicates (first-wins vs last-wins), this could either silently work or cause confusion.Consider either stripping the matching keys from the base file before appending, or documenting that the appended values intentionally override.
46-46: Unquoted${SUPABASE_CLI}undergoes word-splitting — intentional but fragile.When
SUPABASE_CLI="bunx supabase", the unquoted expansion inside$(...)works because word splitting produces two arguments. This is correct but relies on the value never containing spaces in unexpected places. A brief comment would help future maintainers understand the intent.supabase/functions/_backend/plugins/channel_self.ts (1)
606-616:resvariable is now dead — simplify to direct return.Since
return resmoved inside thetryblock (Line 609), the outerlet resdeclaration on Line 606 is unused. The same pattern repeats in PUT (Line 646), DELETE (Line 686), and GET (Line 727).♻️ Simplify by removing the intermediate variable (POST example)
- let res try { - res = await post(c, getDrizzleClient(pgClient), bodyParsed) - return res + return await post(c, getDrizzleClient(pgClient), bodyParsed) }Apply the same simplification to the PUT, DELETE, and GET handlers.
|
1 participant
Summary (AI generated)
channel_selfanddevicehandlers (aimed at Sonar duplication gate).Test plan (AI generated)
bun lint && bun lint:backendbun typecheck./scripts/test-cloudflare-v2.shScreenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.accordingly.
my tests
Generated with AI