fix(user-state): upsert sys_user_preference by (user_id, key) (#2116)

The ObjectStack-backed UserDataAdapter (ui.recent / ui.favorites) tripped
the UNIQUE(user_id, key) constraint on sys_user_preference: every other
navigation logged "Insert operation failed" server-side.

A fresh `recent` adapter is created whenever dataSource/user changes
(UserStateBridge), so cachedRowId starts null. Rapid navigations fire
overlapping debounced save() flushes that both findExisting()->null->
create() — the first insert wins, the second hits the constraint. And
when the row already existed but wasn't found, create() threw with no
recovery.

- Extract an upsert() helper; on create() failure (UNIQUE violation),
  re-find and update in place — a real (user_id, key) upsert.
- Serialize overlapping save() calls via a saveChain so the second save
  sees the cachedRowId the first set and updates instead of inserting.
- save() still swallows errors (provider falls back to localStorage).

Tests: + insert-failure recovery, + concurrent-save serialization (13 pass).

Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Author: os-zhuang

packages/data-objectstack/src/userState.test.ts

@@ -221,6 +221,59 @@ describe('createObjectStackUserStateAdapter', () => {
       }));
     });
 
+    it('recovers from a UNIQUE(user_id, key) insert failure by updating in place', async () => {
+      // First findExisting() (no cached id) misses; create() loses the race and
+      // throws; the recovery findExisting() now sees the row → update.
+      const ds = mockDataSource({
+        find: vi.fn()
+          .mockResolvedValueOnce({ data: [] })
+          .mockResolvedValueOnce({ data: [{ id: 'row-7', user_id: 'u', key: 'k', value: [] }] }) as any,
+        create: vi.fn().mockRejectedValue(new Error('UNIQUE constraint failed')) as any,
+      });
+
+      const adapter = createObjectStackUserStateAdapter({
+        dataSource: ds,
+        userId: 'u',
+        key: 'k',
+      });
+
+      await adapter.save([{ id: 'q' } as any]);
+
+      expect(ds.create).toHaveBeenCalledTimes(1);
+      expect(ds.update).toHaveBeenCalledWith('sys_user_preference', 'row-7', {
+        value: [{ id: 'q' }],
+        updated_at: expect.any(String),
+      });
+    });
+
+    it('serializes concurrent saves so only one insert happens', async () => {
+      // Both saves start before either resolves. Without serialization both
+      // would findExisting()→[]→create() and the second would trip the
+      // UNIQUE constraint. With the save chain, the second waits, sees the
+      // cached row id from the first, and updates.
+      const ds = mockDataSource({
+        find: vi.fn().mockResolvedValue({ data: [] }) as any,
+        create: vi.fn().mockResolvedValue({ id: 'row-1' }) as any,
+      });
+
+      const adapter = createObjectStackUserStateAdapter({
+        dataSource: ds,
+        userId: 'u',
+        key: 'ui.recent',
+      });
+
+      await Promise.all([
+        adapter.save([{ id: 'a' } as any]),
+        adapter.save([{ id: 'a' }, { id: 'b' }] as any),
+      ]);
+
+      expect(ds.create).toHaveBeenCalledTimes(1);
+      expect(ds.update).toHaveBeenCalledWith('sys_user_preference', 'row-1', {
+        value: [{ id: 'a' }, { id: 'b' }],
+        updated_at: expect.any(String),
+      });
+    });
+
     it('swallows errors when save() throws', async () => {
       const onError = vi.fn();
       const ds = mockDataSource({

packages/data-objectstack/src/userState.ts

@@ -104,6 +104,14 @@ export function createObjectStackUserStateAdapter<T = unknown>(
   // without re-querying. Reset on every successful load.
   let cachedRowId: string | number | null = null;
 
+  // Serializes overlapping save() calls. A fresh adapter is created whenever
+  // the data source / user changes (see UserStateBridge), so its cachedRowId
+  // starts null; rapid navigations then fire debounced flushes that can race —
+  // two saves each findExisting()→null→create() and the second trips the
+  // UNIQUE(user_id, key) constraint. Chaining writes means the second save
+  // sees the cachedRowId the first one set and updates instead of inserting.
+  let saveChain: Promise<void> = Promise.resolve();
+
   const findExisting = async (): Promise<UserPreferenceRecord | null> => {
     // NOTE: `QueryParams` uses OData-style `$`-prefixed keys. Using the bare
     // names (`filter`, `limit`) silently drops the predicate at the
@@ -146,6 +154,55 @@ export function createObjectStackUserStateAdapter<T = unknown>(
     return [];
   };
 
+  // Upsert by (user_id, key): update in place when the row is known/found,
+  // otherwise insert — and if the insert loses a UNIQUE(user_id, key) race
+  // (a concurrent writer created the row, or the backend dropped our find
+  // predicate), recover by re-finding and updating rather than surfacing the
+  // failed insert.
+  const upsert = async (items: T[]): Promise<void> => {
+    const now = new Date().toISOString();
+
+    // Fast path: we already know the row id from a previous load/save.
+    if (cachedRowId !== null) {
+      try {
+        await dataSource.update(resource, cachedRowId, { value: items, updated_at: now });
+        return;
+      } catch (updateError) {
+        // Row may have been deleted server-side — fall through to find/insert.
+        cachedRowId = null;
+        onError('save', updateError);
+      }
+    }
+
+    const existing = await findExisting();
+    if (existing && existing.id !== undefined && existing.id !== null) {
+      cachedRowId = existing.id;
+      await dataSource.update(resource, existing.id, { value: items, updated_at: now });
+      return;
+    }
+
+    try {
+      const created = await dataSource.create(resource, {
+        user_id: userId,
+        key,
+        value: items,
+        updated_at: now,
+      });
+      const newId = (created as UserPreferenceRecord | undefined)?.id;
+      if (newId !== undefined && newId !== null) cachedRowId = newId;
+    } catch (createError) {
+      // The row already exists (UNIQUE(user_id, key) violation) — re-find and
+      // update in place. This turns the insert into a real upsert.
+      const recovered = await findExisting();
+      if (recovered && recovered.id !== undefined && recovered.id !== null) {
+        cachedRowId = recovered.id;
+        await dataSource.update(resource, recovered.id, { value: items, updated_at: now });
+        return;
+      }
+      throw createError;
+    }
+  };
+
   return {
     async load(): Promise<T[]> {
       try {
@@ -163,45 +220,15 @@ export function createObjectStackUserStateAdapter<T = unknown>(
     },
 
     async save(items: T[]): Promise<void> {
-      try {
-        const now = new Date().toISOString();
-        // Fast path: we already know the row id from a previous load/save.
-        if (cachedRowId !== null) {
-          try {
-            await dataSource.update(resource, cachedRowId, {
-              value: items,
-              updated_at: now,
-            });
-            return;
-          } catch (updateError) {
-            // Row may have been deleted server-side — fall through to insert.
-            cachedRowId = null;
-            onError('save', updateError);
-          }
-        }
-
-        const existing = await findExisting();
-        if (existing && existing.id !== undefined && existing.id !== null) {
-          cachedRowId = existing.id;
-          await dataSource.update(resource, existing.id, {
-            value: items,
-            updated_at: now,
-          });
-          return;
-        }
-
-        const created = await dataSource.create(resource, {
-          user_id: userId,
-          key,
-          value: items,
-          updated_at: now,
-        });
-        const newId = (created as UserPreferenceRecord | undefined)?.id;
-        if (newId !== undefined && newId !== null) cachedRowId = newId;
-      } catch (error) {
+      // Chain onto any in-flight save so concurrent flushes upsert serially
+      // rather than racing into two inserts (see `saveChain` above). Each link
+      // catches its own errors so one failure never poisons the chain.
+      const run = saveChain.then(() => upsert(items)).catch(error => {
         onError('save', error);
         // Swallow — provider falls back to localStorage as source of truth.
-      }
+      });
+      saveChain = run;
+      return run;
     },
   };
 }