mirror of
https://github.com/supabase/supabase.git
synced 2026-10-05 09:25:06 +03:00
fix(studio): guard two undefined dereferences crashing the table editor and SQL editor (#49412)
<!-- ccr-slack-attribution --> _Requested by **Ali Waseem** · [Slack thread](https://supabase.slack.com/archives/C063LNYJJKS/p1787336596649619)_ **Before:** editing a cell in the table editor could throw, and the edit was silently lost — the typed value vanished and nothing was saved. Separately, opening the SQL editor could throw before the editor rendered, and the global error boundary replaced the entire page, so there was no editor at all until a reload. **After:** a row change with no matching previous row is a no-op instead of a throw, and the SQL editor shows its normal loading state instead of taking down the page. Two independent undefined guards for two confirmed Sentry crashes, one per commit so either can be dropped on its own. **How:** the first commit moves the existing previousRow guard in `useOnRowsChange` above the `changedColumn` computation that dereferences it, and drops the non-null assertion that hid the problem from TypeScript. The second reads the snippet content in `deriveSnippetIdentity` through optional chaining, so a missing `snippets` map, or an entry without a `snippet`, resolves to still-loading — the same answer the old code gave for an id that is not in the map. Behaviour is unchanged in every case that did not crash. ## I have read the [CONTRIBUTING.md](https://github.com/supabase/supabase/blob/master/CONTRIBUTING.md) file. YES ## What kind of change does this PR introduce? Bug fix. ## What is the current behavior? **[K7M, Cannot read properties of undefined (reading 'idx')](https://supabase.sentry.io/issues/7681899596/)** — 4 events / 1 user — in `apps/studio/components/grid/components/grid/Grid.utils.tsx`. Inside `useOnRowsChange`, the callback passed to `Object.keys(rowData).find(...)` reads the candidate column off `previousRow` through a non-null assertion, three lines above the `if (!previousRow || !changedColumn) return` that was meant to protect it. `rows.find(...)` returns undefined whenever no row matches, and the assertion is why TypeScript never flagged the dereference. The four events came from one user inside about two minutes, so it is deterministic rather than a one-off, and every throw is an edit the user loses. **[K7J, Cannot use 'in' operator to search for a snippet uuid in undefined](https://supabase.sentry.io/issues/7680905437/)** — 1 event / 1 user, full-page crash — in `apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.ts`, line 331. `deriveSnippetIdentity` applies the `in` operator to its `snippets` argument and then reads `snippets[id].snippet.content`. The parameter is declared required and non-optional, but `snippets` arrived undefined at runtime, so `in` threw and the error reached the global error boundary, which unmounted the whole SQL editor page. The `snippets[id].snippet` read on the same line is a second unguarded dereference: an entry without a `snippet` crashes identically. ## What is the new behavior? Both crashes become no-ops. - Grid: return early when `previousRow` is missing, then compute `changedColumn`, with the assertion removed. When a previous row is found, the code takes exactly the path it took before. - SQL editor: `snippets?.[id]?.snippet?.content === undefined` replaces the `in` check. A missing map, a missing entry, and an entry with no `snippet` all read as still loading, which is what the surrounding code already does while a snippet is being fetched. The parameter type is left required, since the only caller (`useSnippetIdentity.ts`) passes the store's `snippets` record, which is typed as always present — the type is not the thing that was wrong. Two test cases are added to the existing `deriveSnippetIdentity` block, which previously only passed fully populated maps: one for an undefined `snippets` map, and one for an entry missing its `snippet`. ## Additional context **Why `snippets` was undefined is unexplained.** The store initialises it to an empty object (`apps/studio/state/sql-editor/sql-editor-state.ts:34`) and its only reassignment writes an object, so there is no code path in studio that sets it to undefined. This commit is a defensive guard against a crash, not a root-cause fix, and nothing here should be read as an explanation. **Verification:** no local checks were possible in the authoring environment — the checkout has no `node_modules` and `pnpm install` cannot complete there, so typecheck, lint and the studio unit tests could not be run. CI on this PR is the only verification. --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
1 parent
1cffe632e3
commit
235488e66b
3 files changed
+22
-5
No files matched your search
@@ -20,11 +20,10 @@ export function useOnRowsChange(rows: SupaRow[]) {
|
||||
(_rows: SupaRow[], data: RowsChangeData<SupaRow, unknown>) => {
|
||||
const rowData = _rows[data.indexes[0]]
|
||||
const previousRow = rows.find((x) => x.idx == rowData.idx)
|
||||
const changedColumn = Object.keys(rowData).find(
|
||||
(name) => rowData[name] !== previousRow![name]
|
||||
)
|
||||
if (!previousRow) return
|
||||
|
||||
if (!previousRow || !changedColumn) return
|
||||
const changedColumn = Object.keys(rowData).find((name) => rowData[name] !== previousRow[name])
|
||||
if (!changedColumn) return
|
||||
|
||||
const enumArrayColumns = snap.originalTable.columns
|
||||
?.filter((column) => {
|
||||
|
||||
@@ -185,6 +185,22 @@ describe('SQLEditor.utils.ts:deriveSnippetIdentity', () => {
|
||||
})
|
||||
expect(result).toEqual({ id: 'existing-id', isLoading: false })
|
||||
})
|
||||
test('is loading when the snippets map itself is missing', () => {
|
||||
const result = deriveSnippetIdentity({
|
||||
urlId: 'existing-id',
|
||||
generatedId: 'generated-id',
|
||||
snippets: undefined as unknown as Record<string, { snippet: { content?: unknown } }>,
|
||||
})
|
||||
expect(result).toEqual({ id: 'existing-id', isLoading: true })
|
||||
})
|
||||
test('is loading when the snippets entry has no snippet on it', () => {
|
||||
const result = deriveSnippetIdentity({
|
||||
urlId: 'existing-id',
|
||||
generatedId: 'generated-id',
|
||||
snippets: { 'existing-id': {} as { snippet: { content?: unknown } } },
|
||||
})
|
||||
expect(result).toEqual({ id: 'existing-id', isLoading: true })
|
||||
})
|
||||
})
|
||||
|
||||
const buildDebugSnippet = (uncheckedSql: string) => ({
|
||||
|
||||
@@ -328,7 +328,9 @@ export function deriveSnippetIdentity({
|
||||
}): { id: string; isLoading: boolean } {
|
||||
const id = !urlId || urlId === 'new' ? generatedId : urlId
|
||||
|
||||
const snippetIsLoading = !(id in snippets && snippets[id].snippet.content !== undefined)
|
||||
// `snippets` is typed as always present, but has been seen arriving `undefined` at runtime, and
|
||||
// an entry can be missing its `snippet`. Either case means the content isn't there yet.
|
||||
const snippetIsLoading = snippets?.[id]?.snippet?.content === undefined
|
||||
const isLoading = urlId === 'new' ? false : snippetIsLoading
|
||||
|
||||
return { id, isLoading }
|
||||
|
||||
Reference in new issue
Block a user