From da74e3e763c4cf286b65bf4a315de19a2dcf093b Mon Sep 17 00:00:00 2001 From: Kai ki <155355644+zbf1009@users.noreply.github.com> Date: Thu, 25 Jun 2026 19:20:55 +0800 Subject: [PATCH] fix(persistence): address PR review feedback (4 low-cost improvements) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From PR #114 external review agent — adopted the real, low-cost findings; remaining items (false positives / design trade-offs) explained in PR replies: - coerceEpoch: !Number.isNaN → Number.isFinite — reject ±Infinity, which previously slipped through and produced Invalid Date via new Date(Infinity) - enforceRetentionCap pass2/pass3: decrement overflow only when idbDelete actually succeeds — a failed best-effort delete no longer under-evicts - cloudListStories: explicit column list instead of select() — avoid pulling the bulky session_jsonb when only metadata is needed - Supabase stories: composite primary key (user_id, id) + onConflict user_id,id — avoid a cross-user Session.id collision rejecting the second user's save (skeleton not yet deployed, so the migration is edited in place) typecheck + build:cf green. --- lib/persistence/cloudStore.ts | 8 ++++++-- lib/persistence/localStore.ts | 8 ++++---- lib/persistence/types.ts | 4 +++- supabase/migrations/20260624135618_stories.sql | 8 ++++++-- 4 files changed, 19 insertions(+), 9 deletions(-) diff --git a/lib/persistence/cloudStore.ts b/lib/persistence/cloudStore.ts index 957917d..8ab8d5d 100644 --- a/lib/persistence/cloudStore.ts +++ b/lib/persistence/cloudStore.ts @@ -122,7 +122,7 @@ export async function cloudSaveStory( deleted_at: null, session_jsonb: blob.session, }, - { onConflict: "id" }, + { onConflict: "user_id,id" }, ) .select() .single(); @@ -163,9 +163,13 @@ export async function cloudListStories(): Promise { if (!userId) return []; try { const supabase = await createClient(); + // Explicit column list (not select()) so the list query doesn't pull the + // bulky session_jsonb — rowToMeta only needs the denormalized metadata. const { data, error } = await supabase .from("stories") - .select() + .select( + "id, world_setting, style_guide, orientation, scene_count, created_at, updated_at", + ) .eq("user_id", userId) .is("deleted_at", null) .order("updated_at", { ascending: false }); diff --git a/lib/persistence/localStore.ts b/lib/persistence/localStore.ts index d26636b..31d4861 100644 --- a/lib/persistence/localStore.ts +++ b/lib/persistence/localStore.ts @@ -82,8 +82,9 @@ async function enforceRetentionCap(): Promise { // Keep records that still owe the cloud a push (pending edits/soft-deletes // or a synced baseline) — hard-deleting them would lose that work silently. if (r.syncState !== "local-only") continue; - await idbDelete(STORIES_STORE, r.id); - overflow--; + // Only count a slot freed when the delete actually succeeded — a failed + // best-effort delete must not decrement overflow (would under-evict). + if (await idbDelete(STORIES_STORE, r.id)) overflow--; } // 3. Last-resort: if step 2 couldn't reach the cap, every remaining over-cap @@ -101,8 +102,7 @@ async function enforceRetentionCap(): Promise { for (const r of live) { if (overflow <= 0) break; if (r.syncState === "local-only") continue; // already evicted in step 2 - await idbDelete(STORIES_STORE, r.id); - overflow--; + if (await idbDelete(STORIES_STORE, r.id)) overflow--; } } } catch { diff --git a/lib/persistence/types.ts b/lib/persistence/types.ts index 38feccb..9ea1f56 100644 --- a/lib/persistence/types.ts +++ b/lib/persistence/types.ts @@ -19,7 +19,9 @@ export const STORY_SCHEMA_VERSION = 1; * crosses a storage/serialization boundary and could arrive as a non-number, * guarding against the historical `t.getTime is not a function` white-screen. */ export function coerceEpoch(value: unknown, fallback: number): number { - if (typeof value === "number" && !Number.isNaN(value)) return value; + // Number.isFinite (not just !isNaN) so ±Infinity also falls through to the + // fallback — new Date(Infinity).getTime() is NaN, not a usable epoch. + if (typeof value === "number" && Number.isFinite(value)) return value; const d = value instanceof Date ? value : new Date(value as string | number); const t = d.getTime(); return Number.isNaN(t) ? fallback : t; diff --git a/supabase/migrations/20260624135618_stories.sql b/supabase/migrations/20260624135618_stories.sql index 14e54ce..7a81457 100644 --- a/supabase/migrations/20260624135618_stories.sql +++ b/supabase/migrations/20260624135618_stories.sql @@ -16,7 +16,7 @@ -- dropped-then-created (Postgres has no `create policy if not exists`). create table if not exists public.stories ( - id text primary key, -- = Session.id ("s_xxx"), shared with the local record + id text not null, -- = Session.id ("s_xxx"), unique only per user user_id uuid not null references auth.users (id) on delete cascade, world_setting text not null default '', style_guide text not null default '', @@ -26,7 +26,11 @@ create table if not exists public.stories ( created_at timestamptz not null default now(), updated_at timestamptz not null default now(), deleted_at timestamptz, -- soft-delete tombstone; null = live - session_jsonb jsonb not null -- slim Session blob (voice + styleReferenceImage stripped) + session_jsonb jsonb not null, -- slim Session blob (voice + styleReferenceImage stripped) + -- Composite PK: a random Session.id ("s_xxx") is unique only within a user, so + -- scope the key by user_id — otherwise a cross-user id collision would reject + -- the second user's save with a PK violation. + primary key (user_id, id) ); -- List query path: a user's stories newest-first.