fix: update supabase settings and locale metadata#2889
fix: update supabase settings and locale metadata#2889wwwillchen wants to merge 1 commit intodyad-sh:mainfrom
Conversation
|
@BugBot run |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Supabase integration by introducing support for self-hosted instances. Users can now provide custom API endpoints and secret keys, allowing for greater flexibility in how Supabase is connected and managed within the application. The changes include new UI elements for these settings, robust handling of sensitive credentials, and updates to the underlying API client to ensure seamless operation with both cloud and self-hosted Supabase environments. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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: 24c34378c6
ℹ️ 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 hasApiUrl = normalizedApiUrl !== ""; | ||
| const hasSecret = normalizedSecretKey !== ""; |
There was a problem hiding this comment.
normalizeSupabaseOverrideSettings computes hasApiUrl/hasSecret with normalizedX !== "", which evaluates to true when the value is undefined; for any existing user who has a supabase object but no self-hosted override, this makes hasSecret truthy and later dereferences supabase.selfHostedSupabaseSecretKey!.value, throwing at runtime during writeSettings. Because writeSettings catches and logs errors, this can silently block settings updates instead of persisting them.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const normalizedSecretKey = | ||
| supabase.selfHostedSupabaseSecretKey?.value?.trim(); | ||
| const hasApiUrl = normalizedApiUrl !== ""; | ||
| const hasSecret = normalizedSecretKey !== ""; |
There was a problem hiding this comment.
Undefined comparison causes crash in normalization function
High Severity
normalizedApiUrl and normalizedSecretKey can be undefined (from optional chaining), and undefined !== "" evaluates to true in JavaScript. This means hasApiUrl and hasSecret are true even when the fields don't exist. In writeSettings, normalizeSupabaseOverrideSettings is called unconditionally when supabase exists, so for any user with a Supabase config but no self-hosted overrides, the code reaches supabase.selfHostedSupabaseSecretKey!.value = normalizedSecretKey! and throws a TypeError because selfHostedSupabaseSecretKey is undefined.
Additional Locations (1)
| showSuccess(t("integrations.supabase.selfHostedOverridesCleared")); | ||
| } | ||
| } else { | ||
| showError(t("integrations.supabase.failedDisconnect")); |
There was a problem hiding this comment.
Wrong error message shown when saving overrides fails
Low Severity
The saveSelfHostedOverrides function shows t("integrations.supabase.failedDisconnect") ("Failed to disconnect from Supabase") when the save operation returns a falsy result. This is a copy-paste from the disconnect handler and displays a misleading error message for a save-override operation.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for self-hosted Supabase instances. My review identified several issues: a critical bug in the settings normalization logic that could cause a crash, multiple medium-severity security vulnerabilities related to URL path traversal due to unencoded variables in the Supabase management client, and medium-severity issues with error message handling in the UI. All identified comments are retained as they do not contradict any provided rules and highlight important areas for improvement.
| function normalizeSupabaseOverrideSettings( | ||
| supabase: Supabase | undefined, | ||
| ): void { | ||
| if (!supabase) { | ||
| return; | ||
| } | ||
|
|
||
| const normalizedApiUrl = supabase.selfHostedSupabaseApiUrl?.trim(); | ||
| const normalizedSecretKey = | ||
| supabase.selfHostedSupabaseSecretKey?.value?.trim(); | ||
| const hasApiUrl = normalizedApiUrl !== ""; | ||
| const hasSecret = normalizedSecretKey !== ""; | ||
|
|
||
| if (hasApiUrl) { | ||
| supabase.selfHostedSupabaseApiUrl = normalizedApiUrl; | ||
| } | ||
|
|
||
| if (hasSecret) { | ||
| supabase.selfHostedSupabaseSecretKey!.value = normalizedSecretKey!; | ||
| } | ||
|
|
||
| if (hasApiUrl !== hasSecret) { | ||
| delete supabase.selfHostedSupabaseApiUrl; | ||
| supabase.selfHostedSupabaseSecretKey = undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a potential crash in normalizeSupabaseOverrideSettings. If selfHostedSupabaseSecretKey is undefined, normalizedSecretKey will also be undefined, causing hasSecret to evaluate to true (undefined !== ""). This can lead to a crash on line 70 when trying to access supabase.selfHostedSupabaseSecretKey!.value.
The logic can be simplified to fix this bug and improve readability. The refactored version ensures that both URL and key are treated as empty strings if they are missing, and then checks for their presence before attempting to assign or clear them.
function normalizeSupabaseOverrideSettings(
supabase: Supabase | undefined,
): void {
if (!supabase) {
return;
}
const normalizedApiUrl = supabase.selfHostedSupabaseApiUrl?.trim() ?? "";
const normalizedSecretKey =
supabase.selfHostedSupabaseSecretKey?.value?.trim() ?? "";
if (normalizedApiUrl && normalizedSecretKey) {
supabase.selfHostedSupabaseApiUrl = normalizedApiUrl;
supabase.selfHostedSupabaseSecretKey!.value = normalizedSecretKey;
} else {
// Ensure both are cleared if one is missing
delete supabase.selfHostedSupabaseApiUrl;
supabase.selfHostedSupabaseSecretKey = undefined;
}
}|
|
||
| const response = await fetchWithRetry( | ||
| `https://api.supabase.com/v1/organizations/${organizationSlug}/members`, | ||
| getSupabaseManagementApiUrl(`/organizations/${organizationSlug}/members`), |
There was a problem hiding this comment.
The organizationSlug variable is inserted directly into the URL path without being URL-encoded. If the slug contains special characters (like /, ?, or #), it could lead to an incorrect URL structure and potential path traversal within the API. It is recommended to use encodeURIComponent(organizationSlug) here.
|
|
||
| const response = await fetchWithRetry( | ||
| `https://api.supabase.com/v1/organizations/${organizationSlug}`, | ||
| getSupabaseManagementApiUrl(`/organizations/${organizationSlug}`), |
There was a problem hiding this comment.
The organizationSlug variable is inserted directly into the URL path without being URL-encoded. This can lead to malformed URLs or unintended endpoint access if the slug contains special characters. Use encodeURIComponent(organizationSlug) to ensure the URL is correctly constructed.
| const url = getSupabaseManagementApiUrl( | ||
| `/projects/${projectId}/analytics/endpoints/logs.all?sql=${encodedSql}&iso_timestamp_start=${isoTimestampStart}&iso_timestamp_end=${isoTimestampEnd}`, | ||
| ); |
There was a problem hiding this comment.
The projectId variable is used in the URL path without URL encoding. While other parameters like sqlQuery are correctly encoded, the projectId should also be wrapped in encodeURIComponent(projectId) to prevent path traversal or malformed URL issues if the ID contains special characters.
|
|
||
| const response = await fetchWithRetry( | ||
| `https://api.supabase.com/v1/projects/${supabaseProjectId}/functions`, | ||
| getSupabaseManagementApiUrl(`/projects/${supabaseProjectId}/functions`), |
There was a problem hiding this comment.
The supabaseProjectId is used in the URL path without encoding. In other parts of this file (e.g., lines 885 and 933), this same variable is correctly encoded using encodeURIComponent. For consistency and security, please use encodeURIComponent(supabaseProjectId) here as well.
|
|
||
| const response = await fetchWithRetry( | ||
| `https://api.supabase.com/v1/projects/${supabaseProjectId}/branches`, | ||
| getSupabaseManagementApiUrl(`/projects/${supabaseProjectId}/branches`), |
There was a problem hiding this comment.
The supabaseProjectId is inserted into the URL path without URL encoding. This inconsistency (compared to other encoded usages in the same file) should be addressed by using encodeURIComponent(supabaseProjectId) to prevent potential URL manipulation or errors with special characters.
| showSuccess(t("integrations.supabase.selfHostedOverridesCleared")); | ||
| } | ||
| } else { | ||
| showError(t("integrations.supabase.failedDisconnect")); |
There was a problem hiding this comment.
The error message t("integrations.supabase.failedDisconnect") is incorrect for this context, as this operation is saving settings, not disconnecting. This could confuse the user. A more appropriate message like "Failed to save override settings" should be used. For internationalization, you could add a new translation key.
| showError(t("integrations.supabase.failedDisconnect")); | |
| showError(t("integrations.supabase.failedSaveOverrides", "Failed to save override settings")); |
| err.message || | ||
| t("integrations.supabase.selfHostedOverridesPairRequired"), |
There was a problem hiding this comment.
The fallback error message t("integrations.supabase.selfHostedOverridesPairRequired") in this catch block might not be accurate for all possible errors that could be caught here (e.g., network issues). A more generic fallback, such as "Failed to save override settings", would be more appropriate for unexpected errors.
| err.message || | |
| t("integrations.supabase.selfHostedOverridesPairRequired"), | |
| err.message || | |
| t("integrations.supabase.failedSaveOverrides", "Failed to save override settings") |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const hasApiUrl = normalizedApiUrl !== ""; | ||
| const hasSecret = normalizedSecretKey !== ""; |
There was a problem hiding this comment.
🔴 normalizeSupabaseOverrideSettings treats undefined as non-empty, causing crash on every writeSettings call for Supabase users
The comparisons normalizedApiUrl !== "" and normalizedSecretKey !== "" evaluate to true when the values are undefined (since undefined !== "" is true). This means hasApiUrl and hasSecret are both true even when neither field exists. The if (hasSecret) block then executes supabase.selfHostedSupabaseSecretKey!.value = normalizedSecretKey!, which at runtime is undefined.value = undefined — a TypeError.
In writeSettings (src/main/settings.ts:236-237), normalizeSupabaseOverrideSettings is called for ANY user with a supabase object (e.g., connected organizations), with no guard for whether self-hosted fields exist. The TypeError is caught by the outer try-catch (src/main/settings.ts:306), silently preventing the settings file from being written. This means all settings writes silently fail for existing Supabase users who haven't configured self-hosted overrides.
Fix: use Boolean() or explicit undefined checks
The hasApiUrl and hasSecret checks should be:
const hasApiUrl = Boolean(normalizedApiUrl);
const hasSecret = Boolean(normalizedSecretKey);This correctly treats both undefined and "" as falsy.
| const hasApiUrl = normalizedApiUrl !== ""; | |
| const hasSecret = normalizedSecretKey !== ""; | |
| const hasApiUrl = Boolean(normalizedApiUrl); | |
| const hasSecret = Boolean(normalizedSecretKey); |
Was this helpful? React with 👍 or 👎 to provide feedback.
Greptile SummaryThis PR adds self-hosted Supabase Management API support with custom API URL and secret key configuration, along with i18n updates and schema/search indexing changes. The implementation has solid architecture in most places, but contains a critical bug in Key verified issues:
Well-implemented aspects:
Confidence Score: 1/5
Last reviewed commit: 24c3437 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const hasApiUrl = normalizedApiUrl !== ""; | ||
| const hasSecret = normalizedSecretKey !== ""; |
There was a problem hiding this comment.
Undefined comparison logic causes runtime crash and mismatch detection failure.
The boolean checks at lines 62–63 use !== "" to test field presence, but since optional-chaining returns undefined (not empty string) when a field is absent, both expressions evaluate to true even when the fields don't exist.
Problem 1: Crash on any settings update by Supabase OAuth users
When writeSettings (line 237) calls this normalization function, if the credential field is absent, the boolean still evaluates to true. This causes line 70 to attempt property mutation on an undefined object, crashing with TypeError: Cannot set properties of undefined.
Problem 2: Mismatch detection disabled
When only one field is configured, both booleans become true, so the mismatch check at line 73 never triggers, leaving incomplete/inconsistent state.
Fix: Use truthiness checks instead: Boolean(normalizedValue) instead of normalizedValue !== "". Also add a null check before attempting property mutation at line 70.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main/settings.ts
Line: 62-63
Comment:
Undefined comparison logic causes runtime crash and mismatch detection failure.
The boolean checks at lines 62–63 use `!== ""` to test field presence, but since optional-chaining returns `undefined` (not empty string) when a field is absent, both expressions evaluate to `true` even when the fields don't exist.
**Problem 1: Crash on any settings update by Supabase OAuth users**
When `writeSettings` (line 237) calls this normalization function, if the credential field is absent, the boolean still evaluates to `true`. This causes line 70 to attempt property mutation on an `undefined` object, crashing with `TypeError: Cannot set properties of undefined`.
**Problem 2: Mismatch detection disabled**
When only one field is configured, both booleans become `true`, so the mismatch check at line 73 never triggers, leaving incomplete/inconsistent state.
**Fix:** Use truthiness checks instead: `Boolean(normalizedValue)` instead of `normalizedValue !== ""`. Also add a null check before attempting property mutation at line 70.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 9 files
Confidence score: 2/5
- There is a high-confidence regression risk in
src/main/settings.ts: the new presence checks can treatundefinedas present and dereferenceselfHostedSupabaseSecretKey, which may break settings writes for affected configurations. src/components/SupabaseIntegration.tsxhas a user-facing consistency issue where Clear updates the UI before persistence succeeds, so failed saves can leave the form appearing cleared when data was not actually saved.- The score is lowered because both findings are concrete and user-impacting (one severe backend write-path failure risk, one state/persistence mismatch), so this is not yet a low-risk merge.
- Pay close attention to
src/main/settings.ts,src/components/SupabaseIntegration.tsx- prevent undefined dereference in settings writes and align UI clearing with persistence outcomes.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/settings.ts">
<violation number="1" location="src/main/settings.ts:62">
P1: The new presence checks treat `undefined` as present, which can dereference `selfHostedSupabaseSecretKey` when it is missing and break settings writes.</violation>
</file>
<file name="src/components/SupabaseIntegration.tsx">
<violation number="1" location="src/components/SupabaseIntegration.tsx:175">
P2: Clear action updates the UI optimistically before persistence, which can leave inputs showing cleared values even when saving fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const hasApiUrl = normalizedApiUrl !== ""; | ||
| const hasSecret = normalizedSecretKey !== ""; |
There was a problem hiding this comment.
P1: The new presence checks treat undefined as present, which can dereference selfHostedSupabaseSecretKey when it is missing and break settings writes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/settings.ts, line 62:
<comment>The new presence checks treat `undefined` as present, which can dereference `selfHostedSupabaseSecretKey` when it is missing and break settings writes.</comment>
<file context>
@@ -48,6 +49,33 @@ const DEFAULT_SETTINGS: UserSettings = {
+ const normalizedApiUrl = supabase.selfHostedSupabaseApiUrl?.trim();
+ const normalizedSecretKey =
+ supabase.selfHostedSupabaseSecretKey?.value?.trim();
+ const hasApiUrl = normalizedApiUrl !== "";
+ const hasSecret = normalizedSecretKey !== "";
+
</file context>
| const hasApiUrl = normalizedApiUrl !== ""; | |
| const hasSecret = normalizedSecretKey !== ""; | |
| const hasApiUrl = normalizedApiUrl !== undefined && normalizedApiUrl !== ""; | |
| const hasSecret = normalizedSecretKey !== undefined && normalizedSecretKey !== ""; |
| setSelfHostedApiUrlInput(""); | ||
| setSelfHostedSecretKeyInput(""); | ||
| await saveSelfHostedOverrides("", ""); |
There was a problem hiding this comment.
P2: Clear action updates the UI optimistically before persistence, which can leave inputs showing cleared values even when saving fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/SupabaseIntegration.tsx, line 175:
<comment>Clear action updates the UI optimistically before persistence, which can leave inputs showing cleared values even when saving fails.</comment>
<file context>
@@ -15,42 +14,169 @@ import { useSettings } from "@/hooks/useSettings";
+ return;
+ }
+
+ setSelfHostedApiUrlInput("");
+ setSelfHostedSecretKeyInput("");
+ await saveSelfHostedOverrides("", "");
</file context>
| setSelfHostedApiUrlInput(""); | |
| setSelfHostedSecretKeyInput(""); | |
| await saveSelfHostedOverrides("", ""); | |
| await saveSelfHostedOverrides("", ""); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const normalizedSecretKey = | ||
| supabase.selfHostedSupabaseSecretKey?.value?.trim(); | ||
| const hasApiUrl = normalizedApiUrl !== ""; | ||
| const hasSecret = normalizedSecretKey !== ""; |
There was a problem hiding this comment.
🔴 HIGH | logic / crash
normalizeSupabaseOverrideSettings has undefined !== "" bug that causes crashes
When either field is absent (undefined), the check normalizedApiUrl !== "" evaluates to true because undefined !== "" is true in JavaScript. This means hasApiUrl and hasSecret are both true even when the fields don't exist.
Concrete crash scenario: if only the API URL is saved but the secret key is absent, hasSecret is true, so the code executes supabase.selfHostedSupabaseSecretKey!.value = normalizedSecretKey! — which tries to set .value on undefined, throwing a TypeError.
Additionally, because hasApiUrl !== hasSecret will be false when both are undefined (both evaluate to true), the cleanup branch that should delete incomplete pairs never fires.
💡 Suggestion: Use Boolean() checks:
const hasApiUrl = Boolean(normalizedApiUrl);
const hasSecret = Boolean(normalizedSecretKey);This correctly treats both undefined and empty string as absent.
| showError( | ||
| err.message || | ||
| t("integrations.supabase.selfHostedOverridesPairRequired"), | ||
| ); |
There was a problem hiding this comment.
🟡 MEDIUM | error-states
Wrong fallback error message in catch block
When saveSelfHostedOverrides throws an unexpected exception (e.g. network/persistence failure), the catch block falls back to t('integrations.supabase.selfHostedOverridesPairRequired') — a validation message ("Please provide both the API URL and secret key"). A user who hits a backend error will see a misleading message suggesting they need to fix their input.
💡 Suggestion: Use a generic save-failure message as the fallback, or add a new i18n key like selfHostedOverridesSaveFailed.
| {(overrideError || | ||
| (hasSelfHostedOverrideInput && !hasValidSelfHostedOverridePair)) && ( | ||
| <p className="text-xs text-red-600 dark:text-red-400"> | ||
| {overrideError || |
There was a problem hiding this comment.
🟡 MEDIUM | error-states / UX
Inline validation error shown before user finishes typing
The error paragraph renders whenever hasSelfHostedOverrideInput && !hasValidSelfHostedOverridePair is true, which fires the moment the user starts typing in the first field — before they've had a chance to fill in the second. This creates a jarring premature error message.
💡 Suggestion: Only show this inline error after the user has explicitly attempted to save (rely on the overrideError state set on submit), or introduce a touched/blurred flag so the error only appears after interacting with both fields.
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (2 items)
🚫 Dropped False Positives (7 items)
Generated by Dyadbot multi-agent code review |
github-actions
bot
added
the
needs-human:review-issue
🎭 Playwright Test Results❌ Some tests failed
Summary: 235 passed, 4 failed, 8 flaky, 6 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/mcp.spec.ts \
e2e-tests/supabase_branch.spec.ts \
e2e-tests/supabase_stale_ui.spec.ts
|
1 participant
(codex)