Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces direct Supabase CLI calls with Bun-run package scripts, adds per-worktree Supabase configuration and orchestration (deterministic ports per worktree), adjusts URL rewriting to honor forwarded headers, updates scripts/package.json/tests/docs, and adds a migration granting EXECUTE on top_up_usage_credits. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CLI as "bun run (package scripts)"
participant WT as "supabase-worktree.ts"
participant Config as "supabase-worktree-config.ts"
participant SB as "Supabase CLI/Daemon"
participant Cmd as "Target Command"
Dev->>CLI: bun run supabase:with-env <command...>
CLI->>WT: invoke "with-env" subcommand
WT->>Config: getSupabaseWorktreeConfig(cwd)
Config-->>WT: { repoRoot, worktreeHash, projectId, ports }
WT->>WT: prepare per-worktree supabase dir & rewrite config.toml
WT->>SB: runSupabase('status') in worktree dir
SB-->>WT: status output (text/JSON)
WT->>WT: parse status -> derive env (SUPABASE_URL, KEYS, DB_URL...)
WT->>Cmd: exec <command...> with derived env
Cmd-->>Dev: output / result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
383-385:⚠️ Potential issue | 🟡 MinorInconsistent command:
supabase db resetnot updated in the "Supabase DB Reset" section.Line 384 still references the raw
supabase db resetwhile the earlier "Seed Supabase DB locally" section (line 349) was updated tobun run supabase:db:reset. This section should be updated for consistency.Proposed fix
```bash -supabase db reset +bun run supabase:db:reset</details> </blockquote></details> <details> <summary>supabase/migration_guide.md (1)</summary><blockquote> `64-68`: _⚠️ Potential issue_ | _🟡 Minor_ **Step 7 still references the old `supabase db reset` command.** For consistency with the rest of this PR (and Step 4 above), this should also be updated to the new worktree-aware command. <details> <summary>📝 Proposed fix</summary> ```diff -supabase db reset +bun run supabase:db:resetBased on learnings: "Backend tests modify local database; always reset database with
supabase db resetbefore running tests to ensure clean state" — the guide should reflect the new command so developers follow the correct workflow.
🤖 Fix all issues with AI agents
In `@playwright.config.ts`:
- Line 14: Call getSupabaseWorktreeConfig without passing process.cwd() so it
uses its internal default and avoids referencing the global process; change the
expression const { ports: supabasePorts } =
getSupabaseWorktreeConfig(process.cwd()) to const { ports: supabasePorts } =
getSupabaseWorktreeConfig() and remove any now-unnecessary direct use of
process.cwd() in this statement.
In `@supabase/functions/_backend/files/files.ts`:
- Around line 32-37: getExternalBaseUrl currently uses the raw X-Forwarded-Proto
header which can be a comma-separated list when multiple proxies are involved;
update the logic that computes proto (from protoHeader) to split on ',' and use
the first non-empty trimmed value (e.g., protoHeader.split(',')[0].trim()), then
fall back to new URL(c.req.url).protocol.replace(':','') or 'http' as before;
keep references to protoHeader and proto so you modify the existing assignment
for proto inside getExternalBaseUrl without changing other behavior.
In `@supabase/functions/_backend/utils/stripe.ts`:
- Around line 31-41: The call to Number.parseInt in the isLocalSupabase branch
should include an explicit radix to avoid ambiguity; update the parsing of
api.port in the function handling supabaseUrl (the block using
isLocalSupabase(c), supabaseUrl and customerId) to call Number.parseInt(api.port
|| '54321', 10) so the API port is parsed as a decimal before computing
studioPort.
🧹 Nitpick comments (4)
supabase/functions/_backend/utils/downloadUrl.ts (1)
41-55: Duplicated edge-runtime URL rewriting — extract a shared helper.The forwarded-header URL fixup logic at lines 41–55 and 68–80 is identical. Consider extracting it to reduce duplication, similar to the
getExternalBaseUrlhelper infiles.ts.Suggested helper
function resolveEdgeRuntimeUrl(c: Context, url: URL): string { if (url.host.endsWith(':8081') && url.hostname.startsWith('supabase_edge_runtime_')) { const forwardedHost = c.req.header('X-Forwarded-Host') || c.req.header('Host') const forwardedProto = c.req.header('X-Forwarded-Proto')?.split(',')[0]?.trim() if (forwardedHost) { url.host = forwardedHost if (forwardedProto) url.protocol = `${forwardedProto}:` } else { url.host = 'localhost:54321' } return `functions/v1/${BASE_PATH}` } return BASE_PATH }Then both call sites become:
const finalPath = resolveEdgeRuntimeUrl(c, url)Also applies to: 68-80
scripts/supabase-worktree-config.ts (1)
43-43: Port offset design is sound but sensitive to the* 10factor.The
offset / 10on Line 68 relies onoffsetalways being a multiple of 10 (guaranteed by the* 10here). If this factor is ever changed,edgeInspectorwould get a fractional port number. Consider addingMath.floor()defensively on Line 68, or a comment here noting the coupling.🔧 Defensive fix for edgeInspector
- edgeInspector: base.edgeInspector + (offset / 10), + edgeInspector: base.edgeInspector + Math.floor(offset / 10),scripts/supabase-worktree.ts (2)
100-107:parseStatusJsonmay fail if JSON is followed by trailing text.
JSON.parserejects trailing non-whitespace characters. Ifsupabase status -o jsonever emits text after the JSON object,mixed.slice(idx)will include it and parsing will throw. ThecatchingetStatusJsonhandles this gracefully today, but you could make parsing more robust by extracting only the JSON object.🔧 Trim trailing content after the JSON object
function parseStatusJson(mixed: string): any { - // `supabase status -o json` can print non-JSON lines like: - // "Stopped services: [...]" const idx = mixed.indexOf('{') if (idx < 0) throw new Error('Failed to parse supabase status output (no JSON object found).') - return JSON.parse(mixed.slice(idx)) + const candidate = mixed.slice(idx) + // Find the matching closing brace to tolerate trailing output + let depth = 0 + for (let i = 0; i < candidate.length; i++) { + if (candidate[i] === '{') depth++ + else if (candidate[i] === '}') depth-- + if (depth === 0) + return JSON.parse(candidate.slice(0, i + 1)) + } + return JSON.parse(candidate) }
83-91: Hardcoded symlink list may become stale.If new entries (e.g., new directories or files) are added to
supabase/in the future, they won't be symlinked automatically. Consider a brief comment noting this needs updating when the supabase directory structure changes, or dynamically listing entries and excluding onlyconfig.toml.
There was a problem hiding this comment.
Pull request overview
This PR introduces a worktree-aware local Supabase wrapper and updates dev/test tooling to avoid port/Docker collisions when using multiple git worktrees.
Changes:
- Add
scripts/supabase-worktree.ts+scripts/supabase-worktree-config.tsto generate per-worktreeproject_idand port offsets, and to exportSUPABASE_*env vars for child commands. - Route
bun backend,bun reset, and mostbun test:*scripts throughsupabase:with-envso tests automatically target the correct worktree’s Supabase instance. - Update various local URL/host assumptions (seed secret, Studio link, file URL rewriting, docs) to better support non-default ports and container/internal hosts.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test-utils.ts | Read Postgres URL from SUPABASE_DB_URL/DB_URL and normalize localhost → IPv4 |
| supabase/seed.sql | Use http://kong:8000 for DB-side job callback base in local docker network |
| supabase/migration_guide.md | Update docs to use bun run supabase:start |
| supabase/functions/_backend/utils/stripe.ts | Compute Studio port from Supabase API port (+2) for local instances |
| supabase/functions/_backend/utils/downloadUrl.ts | Prefer forwarded headers when building URLs under Edge Runtime container hostnames |
| supabase/functions/_backend/files/supabaseTusProxy.ts | Improve forwarded host/proto handling to preserve worktree ports |
| supabase/functions/_backend/files/files.ts | Replace hardcoded localhost base with forwarded-derived base for public storage URLs |
| scripts/supabase-worktree.ts | New wrapper: symlink supabase dir, rewrite config.toml ports/project_id, run commands with exported env |
| scripts/supabase-worktree-config.ts | New helper: derive stable worktree hash + per-service port offsets |
| scripts/start-cloudflare-workers.sh | Prefer wrapper-based supabase:status when available |
| playwright.config.ts | Use derived Supabase API port for backend webServer port |
| package.json | Add supabase:* scripts and route backend/reset/tests through wrapper |
| README.md | Document new supabase:* commands and status usage |
| CONTRIBUTING.md | Update “running supabase” instruction to wrapper command |
| AGENTS.md | Update local Supabase start/reset instructions to wrapper commands |
scripts/supabase-worktree.ts
Outdated
| const res = spawnSync(cmdArgs[0], cmdArgs.slice(1), { | ||
| stdio: 'inherit', | ||
| env: childEnv, | ||
| shell: false, | ||
| }) |
There was a problem hiding this comment.
runWithEnv() spawns the passed command with shell: false. On Windows this commonly fails for package-bin shims like cross-env/vitest (which are .cmd files), and can break the updated test:* scripts. Consider enabling shell on win32 or resolving the actual executable path before spawning.
| function getExternalBaseUrl(c: Context): string { | ||
| const protoHeader = c.req.header('X-Forwarded-Proto') | ||
| const proto = protoHeader || new URL(c.req.url).protocol.replace(':', '') || 'http' | ||
| const host = c.req.header('X-Forwarded-Host') || c.req.header('Host') || 'localhost:54321' |
There was a problem hiding this comment.
In getExternalBaseUrl(), X-Forwarded-Host is preferred over Host, but X-Forwarded-Host can omit the port (you handled this in supabaseTusProxy). That can generate redirect URLs like http://localhost/... (missing :), breaking local/worktree setups. Consider appending X-Forwarded-Port when X-Forwarded-Host has no port, or falling back to Host when it includes a port.
| const host = c.req.header('X-Forwarded-Host') || c.req.header('Host') || 'localhost:54321' | |
| const forwardedHost = c.req.header('X-Forwarded-Host') | |
| const forwardedPort = c.req.header('X-Forwarded-Port') | |
| const hostHeader = c.req.header('Host') || 'localhost:54321' | |
| let host: string | |
| if (forwardedHost) { | |
| // If X-Forwarded-Host already has a port, use it directly. | |
| // A simple check is sufficient here; we only need to avoid the obvious "no-port" case. | |
| const hasPort = forwardedHost.includes(':') | |
| if (hasPort) { | |
| host = forwardedHost | |
| } | |
| else if (forwardedPort) { | |
| host = `${forwardedHost}:${forwardedPort}` | |
| } | |
| else { | |
| // Fallback to Host header (which typically includes the port in local/worktree setups) | |
| host = hostHeader | |
| } | |
| } | |
| else { | |
| host = hostHeader | |
| } |
scripts/supabase-worktree.ts
Outdated
| function which(cmd: string): boolean { | ||
| const res = spawnSync('command', ['-v', cmd], { shell: true, stdio: 'ignore' }) | ||
| return res.status === 0 |
There was a problem hiding this comment.
which() uses spawnSync('command', ['-v', ...], { shell: true }), relying on the POSIX command builtin. This will fail on non-POSIX shells (notably Windows), making the wrapper unable to detect whether supabase is installed. Consider using a cross-platform check (e.g. process.platform === 'win32' ? where : command -v, or attempting spawnSync(cmd, ['--version']) and checking the exit code).
![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: d77551b4fa
ℹ️ 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".
| const forwardedHost = c.req.header('X-Forwarded-Host') || c.req.header('Host') | ||
| const forwardedProto = c.req.header('X-Forwarded-Proto') | ||
| if (forwardedHost) { | ||
| url.host = forwardedHost |
There was a problem hiding this comment.
In the supabase_edge_runtime_*:8081 rewrite branch, forwardedHost now falls back to Host, so when X-Forwarded-Host is absent the code can re-use the same internal container host that triggered this branch and emit non-routable download URLs. This is a regression from the previous unconditional localhost fallback and can break local/dev bundle and manifest downloads in setups where the proxy does not forward X-Forwarded-Host.
Useful? React with 👍 / 👎.
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/copilot-instructions.md (1)
72-73:⚠️ Potential issue | 🟡 MinorStale port number in documentation.
Line 73 still references
:54321as the backend port, but with the worktree changes the API port is now dynamic (base 54321 + offset). Consider updating to note it's dynamic or removing the specific port number.📝 Suggested fix
-bun backend # Supabase functions on :54321 +bun backend # Supabase functions (port varies per worktree)
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/triggers/on_version_update.ts`:
- Around line 44-49: The v2 branch currently logs and returns early from the
inner flow but does not exit updateIt, causing handleManifest to still run; make
the v2 path behavior consistent with the non-v2 branch by returning from
updateIt the same way (call and return c.json(BRES) after logging) when
resolveOwnerOrg(c, record) is falsy in v2PathSize, so both branches either abort
before handleManifest or both continue—adjust the v2 block around
resolveOwnerOrg and ensure it returns c.json(BRES) just like the non-v2 else
branch to prevent manifest processing.
In `@supabase/functions/_backend/utils/downloadUrl.ts`:
- Around line 23-28: The code reads X-Forwarded-Host raw into forwardedHost but
uses firstForwardedHeaderValue for X-Forwarded-Proto, causing invalid hosts when
headers are comma-separated; update the handling in downloadUrl.ts to parse
X-Forwarded-Host (and optionally X-Forwarded-Port) with
firstForwardedHeaderValue before using them, then set url.host using the
sanitized host (and port fallback logic) so url.host is never built from a
comma-separated string; modify the variables forwardedHost/forwardedPort usage
where url.host is assigned to reference the parsed values.
🧹 Nitpick comments (3)
supabase/functions/_backend/utils/downloadUrl.ts (1)
17-43: URL mutation side-effect is implicit — consider documenting or returning a new URL.
rewriteLocalEdgeRuntimeUrlmutatesurlin place but also returns it. Callers (getBundleUrlline 75,getManifestUrlline 87) destructure onlyfinalPathand then referenceurldirectly. This works because of reference semantics, but the side-effect is easy to miss during future maintenance.A brief JSDoc or an inline comment on the function noting the mutation would help readability.
scripts/supabase-worktree.ts (2)
128-135:parseStatusJsonmay fail on trailing non-JSON output.
JSON.parse(mixed.slice(idx))parses from the first{to end-of-string. Ifsupabase statusemits any text after the JSON object, parsing will throw. The error is caught upstream ingetStatusJson(returnsok: false), so it won't crash — but it will incorrectly report Supabase as not running.A safer approach would be to extract just the JSON object:
♻️ Suggested improvement
function parseStatusJson(mixed: string): any { const idx = mixed.indexOf('{') if (idx < 0) throw new Error('Failed to parse supabase status output (no JSON object found).') - return JSON.parse(mixed.slice(idx)) + const lastBrace = mixed.lastIndexOf('}') + if (lastBrace < idx) + throw new Error('Failed to parse supabase status output (no closing brace found).') + return JSON.parse(mixed.slice(idx, lastBrace + 1)) }
243-246: Allowlist restrictswith-envtobun/bunxonly — consider documenting the rationale.This is a reasonable security boundary, but if someone needs to run e.g.
nodeornpxthroughwith-env, they'll get a cryptic error. The error message is good, but a brief code comment explaining why this restriction exists would help future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 19-20: The package.json has a missing comma after the
"readreplicate:add-table" script entry causing JSON parse/CI failures; edit
package.json and add a trailing comma after the "readreplicate:add-table": "bash
read_replicate/replicate_add_table.sh" line so the subsequent "preview": "vite
preview" entry is valid JSON (verify the scripts block remains well-formed).
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@messages/es.json`:
- Around line 496-498: Update the "credits-only-info-link" string to use the
plural term "créditos" for consistency with other Spanish entries; change its
value from "Gestiona tu uso del crédito" to "Gestiona tu uso de créditos" (edit
the JSON value for the key "credits-only-info-link") and verify it matches the
style of "credits-only-info-title" and other credit-related keys.
In `@messages/pt-br.json`:
- Around line 496-498: The link text "credits-only-info-link" uses singular
"crédito" while the rest of the pt-BR messages use plural "créditos"; update the
value of the "credits-only-info-link" key to a plural form (for example
"Gerencie seu uso de créditos" or "Gerencie seus créditos") so it matches the
other keys "credits-only-info-description" and "credits-only-info-title".
In `@package.json`:
- Line 4: Revert the manual change to the "version" field in package.json (the
line setting "version": "12.110.6") so the file returns to its prior state;
remove any committed version bump and ensure no manual edits to package.json's
version remain, since CI/CD/semantic-release manages versioning, CHANGELOG.md,
and deployments automatically.
In `@scripts/update_cloudsql_authorized_networks.sh`:
- Around line 48-69: The option parsing loop currently does blind "shift 2" for
--project, --instance, and --cloudflare-url which fails under set -euo pipefail
when a value is missing; update the case branches for these flags to first
verify that "$2" exists and does not begin with "-" (e.g., [[ -n "${2:-}" &&
${2:0:1} != "-" ]]) and if the check fails print a clear error (including the
flag name) and exit non‑zero, otherwise assign PROJECT_ID, INSTANCE_NAME, or
CF_IPS_URL and then shift 2 as before so missing values are handled gracefully.
In `@src/components/CreditsCta.vue`:
- Around line 31-66: Add the DaisyUI d-btn class to both clickable <button>
elements in the CreditsCta component: the button with v-if="props.creditsOnly"
(the credits-only CTA that includes IconInformationCircle) and the fallback
v-else button (the default "Don't want to upgrade?" CTA). Update each button's
class attribute to include "d-btn" alongside the existing Tailwind classes so
both interactive elements comply with the UI guidelines.
In
`@supabase/migrations/20260214054927_restore_top_up_usage_credits_for_service_role.sql`:
- Around line 16-18: The inline comment mentioning caller `#3` using
supabaseClient is stale; update or remove it in the migration header so it
accurately reflects that the /complete-top-up endpoint in credits.ts now uses
supabaseAdmin (service role) instead of supabaseClient — either change the text
to state "now uses supabaseAdmin (service role)" for caller `#3` or delete the
caller `#3` bullet entirely so the comment no longer misstates the caller
authentication method.
- Around line 23-30: The migration currently grants EXECUTE on the function
top_up_usage_credits to "service_role" only; update the migration to also GRANT
EXECUTE on FUNCTION "public"."top_up_usage_credits"( "p_org_id" "uuid",
"p_amount" numeric, "p_expires_at" timestamp with time zone, "p_source" "text",
"p_source_ref" "jsonb", "p_notes" "text" ) TO "authenticated" so the frontend
caller via supabaseClient (the authenticated role) can execute it; ensure the
GRANT uses the exact function signature used in the file to avoid mismatched
permission statements.
🧹 Nitpick comments (3)
src/stores/organization.ts (1)
396-403: Pre-existing:stripeEnabledcheck is missing infetchOrganizations.The watcher on line 188 includes
!stripeEnabledValueas a condition to skip the failure check, but thisfetchOrganizationsblock (line 398) does not account forstripeEnabled. This is a pre-existing inconsistency (not introduced by this PR), but worth noting since you're touching adjacent logic — a future change could unify both paths.supabase/functions/_backend/private/credits.ts (1)
450-452: Consider granting EXECUTE toauthenticatedinstead of switching to admin SDK.The coding guidelines state that the admin SDK should only be used for internal operations, triggers, and CRON jobs — not user-facing APIs. The
/complete-top-upendpoint is user-facing, so switching tosupabaseAdminhere is a guideline deviation.An alternative approach: grant EXECUTE on
top_up_usage_creditstoauthenticatedas well (in the migration), and keep usingsupabaseClienthere. The function itself should enforce any necessary authorization internally, and the upstream checks (JWT + RBAC + Stripe verification) already guard this code path.That said, if the RPC function performs privileged writes that RLS would otherwise block (e.g., inserting into tables the user shouldn't directly access), then
supabaseAdminis the pragmatic choice — just be aware this bypasses RLS for the entire call.As per coding guidelines: "Never use the Supabase admin SDK (with service key) for user-facing APIs; always use the client SDK with user authentication to enforce RLS policies; admin SDK should only be used for internal operations, triggers, and CRON jobs."
scripts/update_cloudsql_authorized_networks.sh (1)
141-141: Drop unusedrowlocal.It is declared but never used.
♻️ Suggested cleanup
- local row
package.json
Outdated
| "name": "capgo-app", | ||
| "type": "module", | ||
| "version": "12.110.1", | ||
| "version": "12.110.6", |
There was a problem hiding this comment.
Avoid manual version bumps in package.json.
Line 4 updates the version to 12.110.6, but versioning is CI/CD-managed. Please revert the manual bump.
🐛 Suggested fix
- "version": "12.110.6",
+ "version": "12.110.1",Based on learnings: Do not manually deploy or commit version bumps; CI/CD handles version bumping in package.json, CHANGELOG.md generation via semantic-release, and deployment after merge to main.
📝 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.
| "version": "12.110.6", | |
| "version": "12.110.1", |
🤖 Prompt for AI Agents
In `@package.json` at line 4, Revert the manual change to the "version" field in
package.json (the line setting "version": "12.110.6") so the file returns to its
prior state; remove any committed version bump and ensure no manual edits to
package.json's version remain, since CI/CD/semantic-release manages versioning,
CHANGELOG.md, and deployments automatically.
supabase/migrations/20260214054927_restore_top_up_usage_credits_for_service_role.sql
Outdated
Show resolved
Hide resolved
| GRANT EXECUTE ON FUNCTION "public"."top_up_usage_credits"( | ||
| "p_org_id" "uuid", | ||
| "p_amount" numeric, | ||
| "p_expires_at" timestamp with time zone, | ||
| "p_source" "text", | ||
| "p_source_ref" "jsonb", | ||
| "p_notes" "text" | ||
| ) TO "service_role"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and examine the specific migration file
find . -name "20260214054927_restore_top_up_usage_credits_for_service_role.sql" -type f | head -1 | xargs cat -nRepository: Cap-go/capgo
Length of output: 1648
🏁 Script executed:
#!/bin/bash
# Search for all references to top_up_usage_credits in migrations and code
rg 'top_up_usage_credits' --glob '*.sql' -B 2 -A 2Repository: Cap-go/capgo
Length of output: 8487
🏁 Script executed:
#!/bin/bash
# Check for any GRANT statements on this function to understand historical context
rg 'GRANT.*top_up_usage_credits' --glob '*.sql' -B 1 -A 1Repository: Cap-go/capgo
Length of output: 697
Grant authenticated role as well — migration comment documents authenticated caller that will still fail.
The migration comment explicitly states that top_up_usage_credits is called via supabaseClient (authenticated role) from supabase/functions/_backend/private/credits.ts (line ~450), the frontend complete-top-up endpoint. The comment also states that without this fix, "all three callers fail with 42501: permission denied for function top_up_usage_credits". However, the migration only grants EXECUTE to service_role.
You must also GRANT EXECUTE to authenticated role, or the frontend endpoint will still receive a 42501 error.
🤖 Prompt for AI Agents
In
`@supabase/migrations/20260214054927_restore_top_up_usage_credits_for_service_role.sql`
around lines 23 - 30, The migration currently grants EXECUTE on the function
top_up_usage_credits to "service_role" only; update the migration to also GRANT
EXECUTE on FUNCTION "public"."top_up_usage_credits"( "p_org_id" "uuid",
"p_amount" numeric, "p_expires_at" timestamp with time zone, "p_source" "text",
"p_source_ref" "jsonb", "p_notes" "text" ) TO "authenticated" so the frontend
caller via supabaseClient (the authenticated role) can execute it; ensure the
GRANT uses the exact function signature used in the file to avoid mismatched
permission statements.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
supabase/functions/_backend/triggers/on_version_update.ts (2)
213-253:⚠️ Potential issue | 🟠 MajorManifest rows without
s3_pathare never deleted — orphaned data.The
if (entry.s3_path)guard on line 215 means entries wheres3_pathis falsy are completely skipped: their DB rows survive while the counters are zeroed out (line 260), leaving stale data.Proposed fix: always delete the manifest row, only conditionally delete from S3
const promisesDeleteS3 = [] for (const entry of manifestEntries) { - if (entry.s3_path) { - promisesDeleteS3.push( - // First delete the manifest row from database - supabaseAdmin(c) - .from('manifest') - .delete() - .eq('id', entry.id) - .then(({ error: deleteError }) => { + promisesDeleteS3.push( + // First delete the manifest row from database + supabaseAdmin(c) + .from('manifest') + .delete() + .eq('id', entry.id) + .then(({ error: deleteError }) => { + if (deleteError) { + cloudlog({ requestId: c.get('requestId'), message: 'error deleting manifest row', id: entry.id, error: deleteError }) + return null // Signal to skip S3 cleanup + } + if (!entry.s3_path) + return null // No S3 asset to clean up + // After deleting, check if any other rows still reference this file + return supabaseAdmin(c) + .from('manifest') + .select('*', { count: 'exact', head: true }) + .eq('file_name', entry.file_name) + .eq('file_hash', entry.file_hash) + }) + .then((v) => { + if (!v) + return + if (v.error) { + cloudlog({ requestId: c.get('requestId'), message: 'error checking manifest references', error: v.error }) + return + } + const count = v.count ?? 0 + if (count) + return + cloudlog({ requestId: c.get('requestId'), message: 'deleted manifest file from S3', s3_path: entry.s3_path }) + return s3.deleteObject(c, entry.s3_path!) + }), + ) - if (deleteError) { - cloudlog({ requestId: c.get('requestId'), message: 'error deleting manifest row', id: entry.id, error: deleteError }) - return null // Signal to skip S3 cleanup - } ...
300-306:⚠️ Potential issue | 🟡 MinorS3 delete failure prevents manifest cleanup.
If
s3.deleteObjectthrows for the main bundle (line 301),deleteManifestat line 328 is never reached. This means a transient S3 error leaves manifest rows and their S3 assets permanently orphaned. Consider whether manifest cleanup should run regardless of the main bundle's S3 outcome.
🤖 Fix all issues with AI agents
In `@scripts/update_cloudsql_authorized_networks.sh`:
- Around line 152-157: The variable declaration "local row" is unused and should
be removed; update the block that declares INSTANCE_NAMES and the while loop
(the "while IFS=$'\t' read -r name region state db_version; do" loop) by
deleting the unused "row" declaration so only the used variables and
INSTANCE_NAMES remain, ensuring no other references to "row" exist elsewhere in
the script.
🧹 Nitpick comments (1)
supabase/functions/_backend/utils/downloadUrl.ts (1)
23-49: Side-effect mutation of theurlparameter may surprise future callers.The function mutates the
URLobject in place while also returning it. Current callers destructure onlyfinalPathand rely on the implicit mutation of their localurlreference. This works but is easy to misuse — a future caller could pass a URL they don't expect to be modified.Consider either documenting the mutation clearly in the JSDoc, or cloning the URL internally (
new URL(url.href)) and returning the copy. Given this is internal local-dev-only code, it's low risk.
| INSTANCE_NAMES=() | ||
| local row | ||
| while IFS=$'\t' read -r name region state db_version; do | ||
| [[ -z "$name" ]] && continue | ||
| INSTANCE_NAMES+=("$name") | ||
| printf " %2d) %-36s region=%-16s state=%-12s db=%s\n" "${#INSTANCE_NAMES[@]}" "$name" "$region" "$state" "$db_version" |
There was a problem hiding this comment.
Remove unused row variable.
row is declared on line 153 but never read; the while read on line 154 destructures directly into name, region, state, db_version.
Proposed fix
local instances_json
instances_json="$(gcloud sql instances list --project="$PROJECT_ID" --format=json)"
INSTANCE_NAMES=()
- local row
while IFS=$'\t' read -r name region state db_version; do📝 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.
| INSTANCE_NAMES=() | |
| local row | |
| while IFS=$'\t' read -r name region state db_version; do | |
| [[ -z "$name" ]] && continue | |
| INSTANCE_NAMES+=("$name") | |
| printf " %2d) %-36s region=%-16s state=%-12s db=%s\n" "${#INSTANCE_NAMES[@]}" "$name" "$region" "$state" "$db_version" | |
| INSTANCE_NAMES=() | |
| while IFS=$'\t' read -r name region state db_version; do | |
| [[ -z "$name" ]] && continue | |
| INSTANCE_NAMES+=("$name") | |
| printf " %2d) %-36s region=%-16s state=%-12s db=%s\n" "${`#INSTANCE_NAMES`[@]}" "$name" "$region" "$state" "$db_version" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 153-153: row appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
In `@scripts/update_cloudsql_authorized_networks.sh` around lines 152 - 157, The
variable declaration "local row" is unused and should be removed; update the
block that declares INSTANCE_NAMES and the while loop (the "while IFS=$'\t' read
-r name region state db_version; do" loop) by deleting the unused "row"
declaration so only the used variables and INSTANCE_NAMES remain, ensuring no
other references to "row" exist elsewhere in the script.
|
1 participant
Summary (AI generated)
project_idand port set under.context/to avoid Docker/port collisions.bun backend,bun reset, andbun test:*through the wrapper so each worktree uses its own local Supabase instance.Test plan (AI generated)
bun run supabase:startbun run supabase:status(confirm ports differ across worktrees)bun run test:allScreenshots (AI generated)
N/A
Checklist (AI generated)
bun run lint:backend && bun run lint.accordingly.
my tests
Generated with AI
Summary by CodeRabbit
Chores
New Features
Bug Fixes / Improvements
UI / i18n
Tests