Skip to content

fix(supabase): don't treat an update that matched 0 rows as success - #674

Open
LinusOssmann wants to merge 1 commit into
LegendApp:mainfrom
LinusOssmann:fix/supabase-update-zero-rows
Open

LinusOssmann wants to merge 1 commit into
LegendApp:mainfrom
LinusOssmann:fix/supabase-update-zero-rows

Conversation

@LinusOssmann

Copy link
Copy Markdown

Summary

When a row is created and edited before the create request has returned, syncedCrud sends the update right away, in parallel with the create. That's intentional (see the test "update before create returns does update"). With syncedSupabase the default update does .update(input).eq(id).select(). If the PATCH reaches PostgREST before the INSERT, it matches 0 rows and PostgREST answers 200 []. The plugin returns data[0] (undefined) without an error, crud treats that as success and clears the pending change. The INSERT then lands with the create-time snapshot, and the edit is gone: no error, no retry, getPendingChanges() is empty.

We saw this in our app as "fields filled in right after creating a record are sometimes empty on the server" and confirmed it with a HAR capture: PATCH with the new values → 200 [], then INSERT with the old values.

Repro

Against current main, with a minimal PostgREST mock where the INSERT is slower than the PATCH:

const server = new Map<string, any>();
const client: any = {
    from: () => ({
        select: () => { const q: any = Promise.resolve({ data: [], error: null }); q.gt = () => q; q.eq = () => q; return q; },
        insert: (row: any) => ({ select: async () => {
            await promiseTimeout(30); // slower INSERT
            server.set(row.id, { ...row, created_at: 'c', updated_at: 'u1' });
            return { data: [server.get(row.id)], error: null };
        } }),
        update: (patch: any) => ({ eq: (_f: string, id: string) => ({ select: async () => {
            if (!server.has(id)) return { data: [], error: null }; // 0 rows
            server.set(id, { ...server.get(id), ...patch, updated_at: 'u2' });
            return { data: [server.get(id)], error: null };
        } }) }),
    }),
};
const obs$: any = observable(syncedSupabase({
    supabase: client, collection: 'items' as any, as: 'object',
    fieldCreatedAt: 'created_at', fieldUpdatedAt: 'updated_at',
    retry: { infinite: true, delay: 5, backoff: 'constant' }, onError: () => {},
}) as any);
obs$.get();
await promiseTimeout(0);
obs$.id1.set({ id: 'id1', name: 'new' });
await promiseTimeout(0);
obs$.id1.note.set('edited while insert in flight');
await promiseTimeout(200);

expect(server.get('id1')?.note).toBe('edited while insert in flight');
// main: the server row is { id: 'id1', name: 'new', created_at: 'c', updated_at: 'u1' }, so the edit is lost
// request order: insert start → update 0 rows → insert done

The same happens whenever the row doesn't exist (yet) on the server: a row hidden by RLS, or a row deleted remotely. In all of these cases the local change is silently dropped as "synced".

Proposed fix

Treat an UPDATE that matched no rows as an error in the default update of syncedSupabase. With retry configured, the update is retried and succeeds as soon as the INSERT has landed. Without retry, onError is called (with revert), so the app at least finds out. PR follows.

Open question: a row that is permanently missing (deleted remotely, or hidden by RLS) would now keep retrying when retry.infinite is set. Alternatives would be an option (e.g. updateRequiresRow: true, or upsert on update), or having crud wait for an in-flight create of the same id before sending the update. I tried the latter in crud.ts (if (pendingCreate?.promise) await pendingCreate.promise). It fixes this too, but it breaks the existing tests "update before create returns does update" and "stale queued update retry does not run after create failure", so it looks like a design decision on your side. Happy to go either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant