Conversation
|
@BugBot run |
Summary of ChangesHello @princeaden1, 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 introduces a database viewer feature, enabling users to directly interact with their Supabase database within the application. It includes UI components for browsing tables, viewing schemas, and displaying paginated data, along with necessary backend integrations and e2e tests. Highlights
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
|
Greptile OverviewGreptile SummaryThis PR adds a comprehensive database viewer feature for Supabase projects, allowing users to browse tables, inspect schemas, query rows with pagination, and execute custom SQL queries. Key Changes
Architecture HighlightsThe implementation follows Electron IPC best practices with validation at the IPC boundary. Table names are validated with a restrictive regex before being used in queries. The Previous thread concerns about SQL injection in Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant DatabasePanel
participant useSupabaseTables
participant IpcClient
participant supabase_handlers
participant executeSupabaseSql
participant SupabaseAPI
User->>DatabasePanel: Select "Database" mode
DatabasePanel->>useSupabaseTables: Fetch tables list
useSupabaseTables->>IpcClient: listTables(projectId, orgSlug)
IpcClient->>supabase_handlers: supabase:list-tables
alt Test Mode
supabase_handlers->>supabase_handlers: Return fake tables
else Production
supabase_handlers->>executeSupabaseSql: Query information_schema
executeSupabaseSql->>SupabaseAPI: POST /sql (SELECT table_name...)
SupabaseAPI-->>executeSupabaseSql: JSON result
executeSupabaseSql-->>supabase_handlers: Parsed table names
end
supabase_handlers-->>IpcClient: Array of table names
IpcClient-->>useSupabaseTables: Table list
useSupabaseTables-->>DatabasePanel: Display tables
User->>DatabasePanel: Click on "users" table
DatabasePanel->>useSupabaseTables: Fetch schema + rows
par Parallel Queries
useSupabaseTables->>IpcClient: getTableSchema(table)
IpcClient->>supabase_handlers: supabase:get-table-schema
supabase_handlers->>supabase_handlers: Validate table name
supabase_handlers->>executeSupabaseSql: Query columns
executeSupabaseSql-->>supabase_handlers: Column definitions
supabase_handlers-->>IpcClient: Schema array
and
useSupabaseTables->>IpcClient: queryTableRows(table, limit, offset)
IpcClient->>supabase_handlers: supabase:query-table-rows
supabase_handlers->>supabase_handlers: Validate + sanitize params
supabase_handlers->>executeSupabaseSql: SELECT * FROM "table" ORDER BY ctid
executeSupabaseSql-->>supabase_handlers: Row data + total count
supabase_handlers-->>IpcClient: {rows, total}
end
IpcClient-->>DatabasePanel: Schema + Rows
DatabasePanel-->>User: Display table details
User->>DatabasePanel: Switch to SQL tab
User->>DatabasePanel: Type and run custom query
DatabasePanel->>useSqlQuery: Execute query
useSqlQuery->>IpcClient: executeSql(query)
IpcClient->>supabase_handlers: supabase:execute-sql
supabase_handlers->>executeSupabaseSql: Execute arbitrary SQL
alt Query Success
executeSupabaseSql-->>supabase_handlers: Query results
supabase_handlers-->>IpcClient: {columns, rows, rowCount, error: null}
else Query Error
executeSupabaseSql-->>supabase_handlers: Error thrown
supabase_handlers-->>IpcClient: {columns: [], rows: [], rowCount: 0, error: msg}
end
IpcClient-->>DatabasePanel: Display results
|
![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 a significant and well-implemented Database Viewer feature. The changes are comprehensive, spanning from frontend components and hooks to backend IPC handlers and tests. The code is generally clean and follows good practices. I've identified a few opportunities for improvement, primarily concerning code duplication in helper functions and the test mock implementation. Addressing these points will enhance the maintainability and testability of this new feature. Overall, great work on adding this valuable functionality.
| <tbody> | ||
| {rows.map((row, rowIdx) => ( | ||
| <tr | ||
| key={rowIdx} |
There was a problem hiding this comment.
Using the array index as a key is not recommended as it can lead to unpredictable UI behavior and performance issues if the list is re-ordered. Since these are database rows, they should have a primary key. It would be better to use a unique identifier from the row, like an id column, for the key prop.
| key={rowIdx} | |
| key={row.id ? String(row.id) : rowIdx} |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/ipc/handlers/supabase_handlers.ts
Line: 298:354
Comment:
[P1] Test-only handler sends a real Supabase OAuth URL, which can make E2E flaky/offline-dependent.
`supabase:fake-connect-and-set-project` simulates `deep-link-received` using `url: "https://supabase-oauth.dyad.sh/api/connect-supabase/login"`. If any renderer logic reacts by opening/navigating to this URL (or validating it), E2E may unexpectedly depend on network availability. Using a clearly fake URL (or a `dyad://` deep link) would keep tests hermetic.
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.
3 issues found across 16 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/components/preview_panel/PaginationControls.tsx">
<violation number="1" location="src/components/preview_panel/PaginationControls.tsx:22">
P3: Empty tables (total=0) render an invalid range like “Showing 1-0 of 0 rows” because start is always offset+1 and end uses total=0.</violation>
</file>
<file name="src/components/preview_panel/DatabasePanel.tsx">
<violation number="1" location="src/components/preview_panel/DatabasePanel.tsx:15">
P2: selectedTable is never cleared when projectId/app changes, so switching apps can leave a stale table name that TableDetails still queries against the new project.</violation>
</file>
<file name="src/__tests__/supabase_table_helpers.test.ts">
<violation number="1" location="src/__tests__/supabase_table_helpers.test.ts:11">
P2: Test duplicates the production `formatCellValue` helper instead of importing it, so the tests can pass even if the real helper regresses.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: ea02480442
ℹ️ 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".
🔍 Multi-Agent Code ReviewReviewed by 3 independent AI agents with different file orderings and focus areas. Agents:
SummaryMost significant issues have already been identified by previous reviewers. After consensus voting (issues flagged by 2+ agents where at least one rated it MEDIUM or higher):
New Finding
Recommendation: Wrap 🟢 Low Priority Issues (not commented)
Generated by multi-agent consensus review (3 agents) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
🎭 Playwright Test Results❌ Some tests failed
Summary: 629 passed, 4 failed, 9 flaky, 210 skipped Failed Tests🍎 macOS
🪟 Windows
📋 Test Commands (macOS)Copy and paste these commands to run or update snapshots for failed tests:
# Run test
npm run e2e e2e-tests/visual_editing.spec.ts -- -g "discard changes"
# Update snapshots
npm run e2e e2e-tests/visual_editing.spec.ts -- -g "discard changes" --update-snapshots
|
- Extract shared helpers (formatCellValue, isValidTableName, getPaginationInfo, safeJsonParse) to src/lib/supabase_utils.ts - Fix table row key prop to use row.id when available instead of array index - Add SQL injection defense: escape single quotes in table names, validate numeric params - Fix total type handling to properly return null instead of 0 for unknown counts - Add ORDER BY ctid for deterministic pagination ordering - Fix empty table pagination to show "0-0" instead of "1-0" - Reset selectedTable when projectId changes to prevent stale queries - Add LIMIT/OFFSET support to fake SQL test results - Improve Date test assertion to verify exact ISO string output - Add JSON.parse error handling via safeJsonParse helper Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@BugBot run |
🔍 Multi-Agent Code Review3 independent review agents analyzed this PR with different focuses and file orderings. Found 3 new issue(s) via consensus voting. Summary
Issues to Address
🟢 Low Priority Issues (1 item)
See inline comments for details. Generated by multi-agent consensus review (3 agents, consensus threshold: 2/3) |
|
@BugBot run |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| throw new Error("supabase:insert-row: No data provided"); | ||
| } | ||
|
|
||
| const columnNames = columns.map((c) => `"${c}"`).join(", "); |
There was a problem hiding this comment.
🔴 SQL injection via unescaped double quotes in column names
Column names from user-provided data objects are interpolated into SQL queries with double-quote wrapping but without escaping embedded double quotes. In PostgreSQL, double-quoted identifiers require internal double quotes to be escaped by doubling them (""), but this escaping is not performed.
Root Cause and Impact
The insertRow, updateRow, and deleteRow handlers construct SQL queries by taking column names from Object.keys(data) or Object.keys(primaryKey) and wrapping them in double quotes:
const columnNames = columns.map((c) => `"${c}"`).join(", ");If a column name contains a double quote character (e.g., name"--), it produces malformed SQL like:
INSERT INTO "users" ("id", "name"--", "email") VALUES (...);This breaks SQL parsing and could potentially be exploited for SQL injection. While table names are validated with isValidTableName(), column names receive no such validation or escaping.
Impact: A malicious actor with access to the row mutation UI could craft column names to inject arbitrary SQL commands, potentially leading to data exfiltration or database modification.
Prompt for agents
Add column name validation/escaping in the SQL query construction for insertRow, updateRow, and deleteRow handlers. Either:
1. Add a validation function similar to isValidTableName() for column names and reject invalid ones, OR
2. Escape double quotes in column names by replacing " with "" before wrapping in double quotes.
The affected lines are:
- Line 428: `const columnNames = columns.map((c) => `"${c}"`).join(", ");`
- Line 461: `.map((c) => `"${c}" = ${escapeValue(data[c])}`)`
- Line 465: `.map((c) => `"${c}" = ${escapeValue(primaryKey[c])}`)`
- Line 493: `.map((c) => `"${c}" = ${escapeValue(primaryKey[c])}`)`
Recommended fix is to create a helper function like:
const escapeIdentifier = (name: string): string => {
return `"${name.replace(/"/g, '""')}"`;
};
Then use it: `columns.map((c) => escapeIdentifier(c)).join(", ")`
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
15 issues found across 30 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/components/preview_panel/RecordEditorDialog.tsx">
<violation number="1" location="src/components/preview_panel/RecordEditorDialog.tsx:89">
P2: Boolean columns (especially nullable ones) cannot be set to `null` using a simple Checkbox. Once clicked, the value toggles between `true` and `false`, making it impossible to revert to `null`.
Consider using a tri-state control (e.g., a Select with True/False/Null options) or adding a "Clear/Set Null" button next to the checkbox.</violation>
</file>
<file name="src/components/preview_panel/ResultsTable.tsx">
<violation number="1" location="src/components/preview_panel/ResultsTable.tsx:66">
P2: The key generation logic is unstable for rows with `id: 0` because `row.id ? ...` evaluates to false for the number `0`, causing it to fall back to the array index.
Additionally, this assumes the primary key is always named `id`, which may not be true for all tables.
Consider using `row.id != null` (or checking for specific PK columns if available) to ensure stable keys.</violation>
</file>
<file name="src/ipc/handlers/supabase_handlers.ts">
<violation number="1" location="src/ipc/handlers/supabase_handlers.ts:428">
P1: SQL injection vulnerability: Column names from user input are not validated. The `Object.keys(data)` values are used directly in the SQL query without validation or escaping double quotes. A malicious column name containing `"` could escape the identifier quoting. Apply the same validation used for table names (`isValidTableName`) to column names, or escape double quotes by doubling them.</violation>
<violation number="2" location="src/ipc/handlers/supabase_handlers.ts:460">
P1: SQL injection vulnerability: Column names from `dataColumns` and `pkColumns` are not validated before being used in SQL. Add validation for column names before using them in the query.</violation>
<violation number="3" location="src/ipc/handlers/supabase_handlers.ts:464">
P1: SQL injection vulnerability: Column names from `pkColumns` are not validated before being used in the SQL WHERE clause. Add validation for column names.</violation>
</file>
<file name="e2e-tests/database_viewer.spec.ts">
<violation number="1" location="e2e-tests/database_viewer.spec.ts:110">
P2: This setup logic is duplicated from the previous test. Consider extracting it into a `test.beforeEach` block or a helper function (e.g., `setupSupabaseAndConnect(po)`) to improve maintainability and reduce code duplication.</violation>
</file>
<file name="src/components/preview_panel/DatabasePanel.tsx">
<violation number="1" location="src/components/preview_panel/DatabasePanel.tsx:40">
P2: Race condition: If the project changes while a query is executing, the results from the previous project will overwrite the state of the new project.
To fix this, track the current `projectId` using a `useRef` and ensure it matches the `projectId` from the closure before updating state in the `onSuccess`/`onError` callbacks.</violation>
</file>
<file name="src/components/preview_panel/TableDetails.tsx">
<violation number="1" location="src/components/preview_panel/TableDetails.tsx:126">
P1: `RecordEditorDialog` submits empty strings (`""`) for untouched fields. This will cause INSERT operations to fail for non-text columns that have default values (e.g., auto-incrementing `id` columns) because the database expects the column to be omitted to apply the default, but receives an invalid empty string instead.
Filter out empty strings for columns with default values before sending the mutation.</violation>
<violation number="2" location="src/components/preview_panel/TableDetails.tsx:149">
P2: The `AlertDialogAction` automatically closes the dialog immediately when clicked, preventing the loading state (`Loader2`) from being visible and closing the dialog before the operation completes.
Accept the event object and call `e.preventDefault()` to keep the dialog open while the async delete operation is pending. The dialog is already being closed manually via `setDeleteDialogOpen(false)` on success.</violation>
</file>
<file name="src/hooks/useStorageBuckets.ts">
<violation number="1" location="src/hooks/useStorageBuckets.ts:45">
P2: Pagination without `placeholderData: keepPreviousData` causes the UI to flicker or show a loading state when changing pages, which is a poor user experience.
Consistent with other paginated hooks in the repository (e.g., `useSearchApps`), use `keepPreviousData` to maintain the current view while fetching the next page.
**Note:** This fix requires importing `keepPreviousData` from `@tanstack/react-query`.</violation>
</file>
<file name="src/components/preview_panel/SqlEditor.tsx">
<violation number="1" location="src/components/preview_panel/SqlEditor.tsx:45">
P1: The keyboard shortcut handler captures the initial `handleExecute` closure, which uses the initial `value` (defaultValue) and `isExecuting` state.
Because the command is registered only once on mount, subsequent updates to the query or executing state are ignored by the shortcut. Pressing Ctrl/Cmd+Enter will always execute the default query.
**Fix Suggestion**:
Use a `useRef` to hold the latest `handleExecute` function (or the state variables) and access `ref.current` inside the command callback.</violation>
</file>
<file name="src/components/preview_panel/sections/StorageSection.tsx">
<violation number="1" location="src/components/preview_panel/sections/StorageSection.tsx:151">
P2: The "No Files" state is misleading when `offset > 0`. If items are deleted externally, a user on a non-zero offset might see `objects.length === 0` and be told "This bucket is empty", while the bucket actually contains files on previous pages. Checking `total === 0` ensures the bucket is truly empty.</violation>
</file>
<file name="src/components/preview_panel/sections/UsersSection.tsx">
<violation number="1" location="src/components/preview_panel/sections/UsersSection.tsx:29">
P1: When switching between projects (changing `projectId`), the `page` state is not reset. If a user is on page 2 of Project A and switches to Project B (which has fewer users/pages), they may land on an empty page.
In this state, `users` will be empty, triggering the "No Users Yet" empty state, which hides the pagination controls. The user will incorrectly believe Project B has no users and will be unable to navigate back to page 1.
Add a `useEffect` to reset `page` to 1 when `projectId` changes. This requires importing `useEffect` from "react".</violation>
</file>
<file name="src/hooks/useSecrets.ts">
<violation number="1" location="src/hooks/useSecrets.ts:11">
P2: Add `meta: { showErrorToast: true }` to this query. This ensures consistent error reporting (UI toasts) for Supabase operations, matching the pattern in `src/hooks/useSupabase.ts`.</violation>
</file>
<file name="src/components/preview_panel/sections/DatabaseSection.tsx">
<violation number="1" location="src/components/preview_panel/sections/DatabaseSection.tsx:135">
P2: Switching between tabs causes `TabsContent` to unmount its children, leading to state loss.
Specifically:
1. The SQL query in `SqlEditor` is lost because its internal state is destroyed on unmount.
2. Scroll positions in `TableList` and `TableDetails` are lost.
To fix this, use `forceMount` on `TabsContent` to keep the DOM elements alive (hidden) when inactive.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| .map((c) => `"${c}" = ${escapeValue(data[c])}`) | ||
| .join(", "); | ||
|
|
||
| const whereClause = pkColumns |
There was a problem hiding this comment.
P1: SQL injection vulnerability: Column names from pkColumns are not validated before being used in the SQL WHERE clause. Add validation for column names.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/handlers/supabase_handlers.ts, line 464:
<comment>SQL injection vulnerability: Column names from `pkColumns` are not validated before being used in the SQL WHERE clause. Add validation for column names.</comment>
<file context>
@@ -318,6 +337,387 @@ export function registerSupabaseHandlers() {
+ .map((c) => `"${c}" = ${escapeValue(data[c])}`)
+ .join(", ");
+
+ const whereClause = pkColumns
+ .map((c) => `"${c}" = ${escapeValue(primaryKey[c])}`)
+ .join(" AND ");
</file context>
| const whereClause = pkColumns | |
| // Validate column names to prevent SQL injection | |
| for (const c of pkColumns) { | |
| if (!isValidTableName(c)) { | |
| throw new Error(`supabase:delete-row: Invalid column name: ${c}`); | |
| } | |
| } | |
| const whereClause = pkColumns |
| throw new Error("supabase:update-row: No data provided"); | ||
| } | ||
|
|
||
| const setClause = dataColumns |
There was a problem hiding this comment.
P1: SQL injection vulnerability: Column names from dataColumns and pkColumns are not validated before being used in SQL. Add validation for column names before using them in the query.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/handlers/supabase_handlers.ts, line 460:
<comment>SQL injection vulnerability: Column names from `dataColumns` and `pkColumns` are not validated before being used in SQL. Add validation for column names before using them in the query.</comment>
<file context>
@@ -318,6 +337,387 @@ export function registerSupabaseHandlers() {
+ throw new Error("supabase:update-row: No data provided");
+ }
+
+ const setClause = dataColumns
+ .map((c) => `"${c}" = ${escapeValue(data[c])}`)
+ .join(", ");
</file context>
| const setClause = dataColumns | |
| // Validate column names to prevent SQL injection | |
| for (const c of [...dataColumns, ...pkColumns]) { | |
| if (!isValidTableName(c)) { | |
| throw new Error(`supabase:update-row: Invalid column name: ${c}`); | |
| } | |
| } | |
| const setClause = dataColumns |
| throw new Error("supabase:insert-row: No data provided"); | ||
| } | ||
|
|
||
| const columnNames = columns.map((c) => `"${c}"`).join(", "); |
There was a problem hiding this comment.
P1: SQL injection vulnerability: Column names from user input are not validated. The Object.keys(data) values are used directly in the SQL query without validation or escaping double quotes. A malicious column name containing " could escape the identifier quoting. Apply the same validation used for table names (isValidTableName) to column names, or escape double quotes by doubling them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/handlers/supabase_handlers.ts, line 428:
<comment>SQL injection vulnerability: Column names from user input are not validated. The `Object.keys(data)` values are used directly in the SQL query without validation or escaping double quotes. A malicious column name containing `"` could escape the identifier quoting. Apply the same validation used for table names (`isValidTableName`) to column names, or escape double quotes by doubling them.</comment>
<file context>
@@ -318,6 +337,387 @@ export function registerSupabaseHandlers() {
+ throw new Error("supabase:insert-row: No data provided");
+ }
+
+ const columnNames = columns.map((c) => `"${c}"`).join(", ");
+ const values = columns.map((c) => escapeValue(data[c])).join(", ");
+
</file context>
| const columnNames = columns.map((c) => `"${c}"`).join(", "); | |
| // Validate column names to prevent SQL injection | |
| for (const c of columns) { | |
| if (!isValidTableName(c)) { | |
| throw new Error(`supabase:insert-row: Invalid column name: ${c}`); | |
| } | |
| } | |
| const columnNames = columns.map((c) => `"${c}"`).join(", "); |
|
|
||
| const handleSave = async (data: Record<string, unknown>) => { | ||
| try { | ||
| if (editorMode === "insert") { |
There was a problem hiding this comment.
P1: RecordEditorDialog submits empty strings ("") for untouched fields. This will cause INSERT operations to fail for non-text columns that have default values (e.g., auto-incrementing id columns) because the database expects the column to be omitted to apply the default, but receives an invalid empty string instead.
Filter out empty strings for columns with default values before sending the mutation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/preview_panel/TableDetails.tsx, line 126:
<comment>`RecordEditorDialog` submits empty strings (`""`) for untouched fields. This will cause INSERT operations to fail for non-text columns that have default values (e.g., auto-incrementing `id` columns) because the database expects the column to be omitted to apply the default, but receives an invalid empty string instead.
Filter out empty strings for columns with default values before sending the mutation.</comment>
<file context>
@@ -45,6 +75,90 @@ export function TableDetails({
+
+ const handleSave = async (data: Record<string, unknown>) => {
+ try {
+ if (editorMode === "insert") {
+ await insertRow.mutateAsync(data);
+ showSuccess("Row inserted successfully");
</file context>
| editorRef.current = editor; | ||
|
|
||
| // Add keyboard shortcut for executing query (Ctrl/Cmd + Enter) | ||
| editor.addCommand(monaco.KeyMod.CtrlCmd | monaco.KeyCode.Enter, () => { |
There was a problem hiding this comment.
P1: The keyboard shortcut handler captures the initial handleExecute closure, which uses the initial value (defaultValue) and isExecuting state.
Because the command is registered only once on mount, subsequent updates to the query or executing state are ignored by the shortcut. Pressing Ctrl/Cmd+Enter will always execute the default query.
Fix Suggestion:
Use a useRef to hold the latest handleExecute function (or the state variables) and access ref.current inside the command callback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/preview_panel/SqlEditor.tsx, line 45:
<comment>The keyboard shortcut handler captures the initial `handleExecute` closure, which uses the initial `value` (defaultValue) and `isExecuting` state.
Because the command is registered only once on mount, subsequent updates to the query or executing state are ignored by the shortcut. Pressing Ctrl/Cmd+Enter will always execute the default query.
**Fix Suggestion**:
Use a `useRef` to hold the latest `handleExecute` function (or the state variables) and access `ref.current` inside the command callback.</comment>
<file context>
@@ -0,0 +1,119 @@
+ editorRef.current = editor;
+
+ // Add keyboard shortcut for executing query (Ctrl/Cmd + Enter)
+ editor.addCommand(monaco.KeyMod.CtrlCmd | monaco.KeyCode.Enter, () => {
+ handleExecute();
+ });
</file context>
| } | ||
| }; | ||
|
|
||
| const handleDeleteConfirm = async () => { |
There was a problem hiding this comment.
P2: The AlertDialogAction automatically closes the dialog immediately when clicked, preventing the loading state (Loader2) from being visible and closing the dialog before the operation completes.
Accept the event object and call e.preventDefault() to keep the dialog open while the async delete operation is pending. The dialog is already being closed manually via setDeleteDialogOpen(false) on success.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/preview_panel/TableDetails.tsx, line 149:
<comment>The `AlertDialogAction` automatically closes the dialog immediately when clicked, preventing the loading state (`Loader2`) from being visible and closing the dialog before the operation completes.
Accept the event object and call `e.preventDefault()` to keep the dialog open while the async delete operation is pending. The dialog is already being closed manually via `setDeleteDialogOpen(false)` on success.</comment>
<file context>
@@ -45,6 +75,90 @@ export function TableDetails({
+ }
+ };
+
+ const handleDeleteConfirm = async () => {
+ if (!rowToDelete) return;
+ try {
</file context>
| limit: number; | ||
| offset: number; | ||
| }) { | ||
| return useQuery<ListStorageObjectsResult, Error>({ |
There was a problem hiding this comment.
P2: Pagination without placeholderData: keepPreviousData causes the UI to flicker or show a loading state when changing pages, which is a poor user experience.
Consistent with other paginated hooks in the repository (e.g., useSearchApps), use keepPreviousData to maintain the current view while fetching the next page.
Note: This fix requires importing keepPreviousData from @tanstack/react-query.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/useStorageBuckets.ts, line 45:
<comment>Pagination without `placeholderData: keepPreviousData` causes the UI to flicker or show a loading state when changing pages, which is a poor user experience.
Consistent with other paginated hooks in the repository (e.g., `useSearchApps`), use `keepPreviousData` to maintain the current view while fetching the next page.
**Note:** This fix requires importing `keepPreviousData` from `@tanstack/react-query`.</comment>
<file context>
@@ -0,0 +1,65 @@
+ limit: number;
+ offset: number;
+}) {
+ return useQuery<ListStorageObjectsResult, Error>({
+ queryKey: queryKeys.supabase.storageObjects({
+ projectId: projectId ?? "",
</file context>
| <AlertDescription>{objectsError.message}</AlertDescription> | ||
| </Alert> | ||
| </div> | ||
| ) : objects.length === 0 ? ( |
There was a problem hiding this comment.
P2: The "No Files" state is misleading when offset > 0. If items are deleted externally, a user on a non-zero offset might see objects.length === 0 and be told "This bucket is empty", while the bucket actually contains files on previous pages. Checking total === 0 ensures the bucket is truly empty.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/preview_panel/sections/StorageSection.tsx, line 151:
<comment>The "No Files" state is misleading when `offset > 0`. If items are deleted externally, a user on a non-zero offset might see `objects.length === 0` and be told "This bucket is empty", while the bucket actually contains files on previous pages. Checking `total === 0` ensures the bucket is truly empty.</comment>
<file context>
@@ -0,0 +1,270 @@
+ <AlertDescription>{objectsError.message}</AlertDescription>
+ </Alert>
+ </div>
+ ) : objects.length === 0 ? (
+ <div className="flex flex-col items-center justify-center h-full gap-4 text-center p-8">
+ <HardDrive className="w-12 h-12 text-muted-foreground" />
</file context>
| } | ||
|
|
||
| export function useSecrets({ projectId, organizationSlug }: UseSecretsParams) { | ||
| return useQuery({ |
There was a problem hiding this comment.
P2: Add meta: { showErrorToast: true } to this query. This ensures consistent error reporting (UI toasts) for Supabase operations, matching the pattern in src/hooks/useSupabase.ts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/useSecrets.ts, line 11:
<comment>Add `meta: { showErrorToast: true }` to this query. This ensures consistent error reporting (UI toasts) for Supabase operations, matching the pattern in `src/hooks/useSupabase.ts`.</comment>
<file context>
@@ -0,0 +1,84 @@
+}
+
+export function useSecrets({ projectId, organizationSlug }: UseSecretsParams) {
+ return useQuery({
+ queryKey: queryKeys.supabase.secrets({
+ projectId: projectId ?? "",
</file context>
| </TabsContent> | ||
|
|
||
| {/* SQL tab content */} | ||
| <TabsContent value="sql" className="flex-1 m-0 overflow-hidden"> |
There was a problem hiding this comment.
P2: Switching between tabs causes TabsContent to unmount its children, leading to state loss.
Specifically:
- The SQL query in
SqlEditoris lost because its internal state is destroyed on unmount. - Scroll positions in
TableListandTableDetailsare lost.
To fix this, use forceMount on TabsContent to keep the DOM elements alive (hidden) when inactive.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/preview_panel/sections/DatabaseSection.tsx, line 135:
<comment>Switching between tabs causes `TabsContent` to unmount its children, leading to state loss.
Specifically:
1. The SQL query in `SqlEditor` is lost because its internal state is destroyed on unmount.
2. Scroll positions in `TableList` and `TableDetails` are lost.
To fix this, use `forceMount` on `TabsContent` to keep the DOM elements alive (hidden) when inactive.</comment>
<file context>
@@ -0,0 +1,157 @@
+ </TabsContent>
+
+ {/* SQL tab content */}
+ <TabsContent value="sql" className="flex-1 m-0 overflow-hidden">
+ <div className="flex flex-col h-full">
+ {/* SQL Editor - top half */}
</file context>
| return "NULL"; | ||
| } | ||
| if (typeof value === "number") { | ||
| return String(value); |
There was a problem hiding this comment.
🟡 MEDIUM - escapeValue does not handle NaN, Infinity, or -Infinity
String(NaN) produces 'NaN' and String(Infinity) produces 'Infinity', neither of which are valid SQL literals. These values can reach this function via user input (e.g., parseInt("abc") returns NaN from RecordEditorDialog).
| return String(value); | |
| if (typeof value === "number") { | |
| if (!Number.isFinite(value)) { | |
| throw new Error(`Invalid numeric value: ${value}`); | |
| } | |
| return String(value); | |
| } |
Consensus: identified by 3/3 independent reviewers
| predicate: (query) => { | ||
| const key = query.queryKey; | ||
| return ( | ||
| Array.isArray(key) && | ||
| key[0] === "supabase" && | ||
| key[1] === "rows" && | ||
| key[2] === projectId && | ||
| key[4] === table | ||
| ); | ||
| }, |
There was a problem hiding this comment.
🟡 MEDIUM - Fragile query invalidation using hardcoded array indices
This predicate checks key[0] === "supabase" && key[1] === "rows" && key[2] === projectId && key[4] === table, which is tightly coupled to the key structure in queryKeys.ts. It skips index 3 (organizationSlug), meaning it invalidates rows across all organizations. If the query key structure changes, this will silently break.
Consider using a prefix match instead:
queryClient.invalidateQueries({
queryKey: ["supabase", "rows", projectId, organizationSlug, table],
});Consensus: identified by 3/3 independent reviewers
| raw_user_meta_data as user_metadata | ||
| FROM auth.users | ||
| ORDER BY created_at DESC | ||
| LIMIT ${perPage} OFFSET ${offset}; |
There was a problem hiding this comment.
🟡 MEDIUM - perPage and offset interpolated into SQL without numeric validation
Unlike queryTableRows handler which clamps numeric params with Math.max/Math.min/Math.floor, listAuthUsers interpolates perPage and offset directly into the SQL string. While the Zod schema provides min/max constraints at the IPC boundary, defense-in-depth would add validation here too, since this function can be called directly.
| LIMIT ${perPage} OFFSET ${offset}; | |
| LIMIT ${Math.max(1, Math.min(100, Math.floor(Number(perPage))))} OFFSET ${Math.max(0, Math.floor(Number(offset)))}; |
Consensus: identified by 3/3 independent reviewers
| const escapedPrefix = prefix.replace(/'/g, "''"); | ||
| whereClause += ` AND name LIKE '${escapedPrefix}%'`; |
There was a problem hiding this comment.
🟡 MEDIUM - LIKE pattern injection via unescaped % and _ in prefix
The prefix parameter is escaped for single quotes but not for LIKE wildcards. A prefix containing % would match all objects in the bucket, and _ matches any single character. This bypasses the intended prefix filtering.
| const escapedPrefix = prefix.replace(/'/g, "''"); | |
| whereClause += ` AND name LIKE '${escapedPrefix}%'`; | |
| const escapedPrefix = prefix | |
| .replace(/'/g, "''") | |
| .replace(/%/g, "\\%") | |
| .replace(/_/g, "\\_"); | |
| whereClause += ` AND name LIKE '${escapedPrefix}%' ESCAPE '\\'`; |
Consensus: identified by 2/3 independent reviewers
🔍 Multi-Agent Code ReviewFound 4 new issue(s) flagged by 3 independent reviewers. Summary
New Issues
Already Commented (skipped)The following consensus issues were already identified by other reviewers:
🟢 Low Priority Issues (3 items - single agent only)
See inline comments for details. Generated by multi-agent consensus review (3 independent Claude reviewers) |
|
i'll go ahead and close this for now, but feel free to open this later when it's ready |
2 participants
Summary by cubic
Adds a Supabase-backed Database Viewer and Manager in the Preview panel. Browse tables, run SQL, edit rows, manage storage/auth/users/secrets, and view project logs.
New Features
Bug Fixes
Written for commit 209204b. Summary will update on new commits.