Conversation
* Wire ZeroProvider and replace event-sourced reads with Zero useQuery * Fix ZeroProvider unauthenticated render * Fix ZeroProvider token initialization * Refactor: workspace writes to Zero mutators (#336) * Replace event-sourcing writes with Zero mutators * Fix audio duration persistence * Make workflow persistence atomic * Refactor: remove event sourcing and user state infrastructure (#339) * Delete event-sourcing infrastructure and remove per-user state persistence * Remove auth secret fallback * Preserve legacy workspace tables * Remove unused workspace table migration * Fix Zero createdAt column type * Migrate Zero integration to 1.x query/mutator architecture * Refactor Zero auth to cookie-based + dedup server mutators * Fix Zero query auth: fail-closed + rename hasWorkspaceReadAccess (#385) * Enforce authentication and workspace read access checks on Zero query endpoint * Fix Zero query workspace context checks Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
…ove dead token route
Feat: Zero Sync — real-time workspace state via zero-cache
Refactor/Zero shell-only mutators
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR migrates the application from event-sourced workspace state with Supabase realtime to Zero Sync-based state synchronization. Changes include removing event-store infrastructure, replacing projection-based state derivation with direct item persistence, adding Zero client/server mutators and queries, and updating UI components to use Zero connection state instead of event logs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client<br/>(Browser)
participant Provider as ZeroProvider<br/>(React)
participant Zero as Zero Client<br/>(Sync Engine)
participant API as /api/zero/*<br/>(Routes)
participant Auth as Auth<br/>(Session)
participant DB as Database<br/>(Drizzle)
rect rgba(200, 150, 255, 0.5)
Note over Client,DB: Initial Setup
Client->>Provider: Mount with session
Provider->>Auth: Get user session
Auth-->>Provider: userId
Provider->>Zero: Create instance (userId)
Zero-->>Provider: Ready
Provider-->>Client: Render with Zero context
end
rect rgba(150, 200, 255, 0.5)
Note over Client,DB: Query Workspace Items
Client->>Zero: useQuery(workspace.items)
Zero->>API: POST /api/zero/query
API->>Auth: Verify session
API->>DB: Check workspace access
DB-->>API: Access verified
API->>DB: SELECT workspace_items
DB-->>API: Item rows
API-->>Zero: Query result
Zero-->>Client: state[] (cached)
end
rect rgba(255, 200, 150, 0.5)
Note over Client,DB: Mutate Item
Client->>Zero: mutate('item.update', args)
Zero->>API: POST /api/zero/mutate
API->>Auth: Verify session
Auth-->>API: userId
API->>DB: Transaction begin
DB->>DB: Update workspace_items
DB->>DB: Upsert workspace_item_content
DB->>DB: Upsert workspace_item_extracted
DB-->>API: Success
API-->>Zero: Mutation result
Zero->>Zero: Apply to local cache
Zero-->>Client: Updated state
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Greptile SummaryThis PR replaces the event-sourcing workspace state system with Zero real-time sync, removing ~2,600 lines of projection, snapshot, and event infrastructure and replacing client reads with
Confidence Score: 3/5Not safe to merge until the query authorization bypass is resolved; all other findings are P2. One P1 security finding: authenticated users can bypass workspace read-access checks on the Zero query endpoint by crafting a request that prevents workspace ID extraction. This is on the primary data-read path for the new architecture. src/app/api/zero/query/route.ts (auth bypass), src/lib/zero/queries.ts (no server-side ownership predicate in ZQL)
|
| Filename | Overview |
|---|---|
| src/app/api/zero/query/route.ts | New Zero query endpoint with workspace access checks; auth can be bypassed if the Zero protocol body doesn't match the expected parsing format |
| src/app/api/zero/mutate/route.ts | New Zero mutate endpoint; session auth checked before dispatching to serverMutators which enforce workspace write access internally |
| src/lib/zero/server-mutators.ts | Server-side Zero mutators wrap shared mutators with workspace write-access checks and extracted-row sync; auth applied consistently across all mutation paths |
| src/lib/zero/mutators.ts | Client-side optimistic mutators with Zod validation; sanitize functions called are currently no-ops |
| src/lib/zero/zero-schema.gen.ts | Zero schema defines workspace_items and workspace_item_content tables with relationships; workspace_item_extracted intentionally excluded |
| src/lib/zero/client.ts | Module-level Zero singleton with user-scoped instance management; correctly recreates instance on user change |
| src/lib/zero/provider.tsx | ZeroProvider wraps base provider, blocks render during session load, and destroys Zero instance on logout |
| src/hooks/workspace/use-workspace-state.ts | Replaced polling hook with Zero useQuery; always passes extracted: null — OCR/transcript data not available client-side |
| src/lib/workspace/workspace-item-write.ts | New direct Drizzle write helpers used by AI/OCR/audio workflows; DbExecutor typed as any, weakening type safety |
| src/lib/workspace/workspace-item-sanitize.ts | Both sanitize functions are identity stubs providing no actual sanitization |
| src/lib/auth.ts | Adds cross-subdomain cookie support for Zero auth; wildcard trusted origin may not function as intended depending on Better Auth version |
| drizzle/0002_zero_publication.sql | Creates zero_pub publication for WAL replication; does not drop removed tables, causing Drizzle schema drift |
| src/workflows/audio-transcribe/steps/persist-result.ts | Migrated from event-sourcing to direct Drizzle updates within a transaction; structuredData fully replaced on each write |
| src/workflows/ocr-dispatch/steps/persist-results.ts | Migrated from event-sourcing to direct Drizzle updates; each OCR result wrapped in its own transaction |
| src/lib/db/schema.ts | Removes workspaceEvents, workspaceSnapshots, workspaceItemUserState, and workspaceItemProjectionState table definitions; tables still exist in DB |
Reviews (1): Last reviewed commit: "Merge pull request #396 from ThinkEx-OSS..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // for any that require args. | ||
| for (const workspaceId of workspaceIds) { | ||
| const hasAccess = await hasWorkspaceReadAccess(workspaceId, userId); | ||
| if (!hasAccess) { | ||
| throw new Error(WORKSPACE_ACCESS_DENIED_ERROR); | ||
| } | ||
| } | ||
|
|
||
| const result = await handleQueryRequest( | ||
| (name, args) => mustGetQuery(queries, name).fn({ args, ctx }), | ||
| schema, | ||
| body as ReadonlyJSONValue, | ||
| ); | ||
|
|
||
| return NextResponse.json(result); | ||
| } catch (error) { | ||
| if ( |
There was a problem hiding this comment.
If the Zero request body doesn't match the expected [tag, [{args: [{workspaceId}...]}]] format — or if Zero's internal protocol ever changes — workspaceIds will be empty, the for loop is skipped, and handleQueryRequest executes the query anyway. Because the ZQL query in queries.ts only filters rows by workspaceId (no user ownership predicate), any authenticated user who knows a workspace UUID can read its items by crafting a request where args[0].workspaceId is absent or in a different position.
At minimum, add a server-side ownership predicate to the ZQL query in queries.ts so that the Zero server itself enforces access, making the pre-parse check a defense-in-depth layer rather than the only gate.
| import type { Item } from "@/lib/workspace-state/types"; | ||
|
|
||
| export function sanitizeWorkspaceItemForPersistence(item: Item): Item { | ||
| return item; | ||
| } | ||
|
|
||
| export function sanitizeWorkspaceItemChanges( | ||
| changes: Partial<Item>, | ||
| ): Partial<Item> { | ||
| return changes; | ||
| } |
There was a problem hiding this comment.
Both sanitizeWorkspaceItemForPersistence and sanitizeWorkspaceItemChanges are identity functions that return their input unchanged. They are called extensively in mutators.ts and server-mutators.ts but provide no protection. If sanitization is intentionally deferred, a comment explaining that would prevent future callers from assuming safety guarantees that don't exist.
| buildWorkspaceItemTableRows, | ||
| rehydrateWorkspaceItem, | ||
| } from "./workspace-item-model"; | ||
| import { sanitizeWorkspaceItemForPersistence } from "./workspace-item-sanitize"; |
There was a problem hiding this comment.
DbExecutor typed as typeof db | any defeats type safety
type DbExecutor = typeof db | any collapses to any, making the entire type annotation a no-op. The same pattern appears in workspace-state-read.ts. Consider using the actual Drizzle transaction type or a narrow union so TypeScript can catch incorrect executor usage at compile time.
| extracted: null, | ||
| }); |
There was a problem hiding this comment.
extracted data is always null — OCR/transcript fields unavailable to clients
workspace_item_extracted is not included in the Zero schema and is not part of the queries.ts query, so extracted is always null. Any component that previously rendered ocrText, transcriptText, contentPreview, or searchText from workspace state will silently receive nothing. If this is intentional (extracted data is search-only), it's worth documenting.
| CREATE PUBLICATION zero_pub FOR TABLE | ||
| workspace_items, | ||
| workspace_item_content; |
There was a problem hiding this comment.
workspaceEvents, workspaceSnapshots, workspaceItemUserState, and workspaceItemProjectionState have been removed from schema.ts but are not dropped in this migration. drizzle-kit will detect drift on the next run. The PR description mentions retaining tables for rollback safety — a comment or follow-up migration stub would make this intent explicit and prevent accidental auto-generated drops.
| }, | ||
| }, | ||
| secret: process.env.BETTER_AUTH_SECRET || process.env.AUTH_SECRET, | ||
| secret: authSecret, |
There was a problem hiding this comment.
trustedOrigins may not be supported by Better Auth
"https://*.thinkex.app" is added to trustedOrigins. Better Auth's origin validation typically does exact-string matching rather than glob/wildcard matching. If wildcards are not supported, this entry is silently ignored and zero.thinkex.app would need to be listed explicitly for cross-subdomain cookie auth to function.
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| @@ -0,0 +1,3 @@ | |||
| CREATE PUBLICATION zero_pub FOR TABLE | |||
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
@src/lib/db/schema.ts deletes workspace_events, workspace_snapshots, workspace_item_user_state, and workspace_item_projection_state, but the only SQL added for migration 0002 is:
-- drizzle/0002_zero_publication.sql
CREATE PUBLICATION zero_pub FOR TABLE
workspace_items,
workspace_item_content;@package.json still defines pnpm db:migrate, and applying 0000/0001/0002 to a fresh database will therefore continue to create all four legacy tables (plus their policies/trigger objects) even though the checked-in schema no longer models them. That leaves migrated databases permanently out of sync with the repo schema. Either add explicit DROP TABLE ... CASCADE / cleanup statements in this migration, or keep the legacy tables in schema.ts until you intentionally retire them.
| @@ -116,47 +108,21 @@ async function handlePOST(request: NextRequest) { | |||
| ? normalizeWorkspaceItems(customInitialItems) | |||
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
initialItems is still treated as an unchecked cast, but this diff now persists each item directly into workspace_items/workspace_item_content inside the request path. In @src/lib/workspace-state/state.ts, normalizeWorkspaceItems() only returns the incoming array as-is; it does not validate that each entry has the required id, type, name, and data fields. That means a bad client payload now reaches insertWorkspaceItem(), hits NOT NULL / PK constraints, and withServerObservability converts the thrown DB error into a generic 500 instead of a 400. This is a regression in API behavior for untrusted request data and can break share/import flows if they ever pass a malformed item. Validate each item before starting the transaction and reject invalid payloads with a client error.
// src/app/api/workspaces/route.ts
const initialItems = customInitialItems
? normalizeWorkspaceItems(customInitialItems)
: getTemplateInitialItems(effectiveTemplate);
await db.transaction(async (tx) => {
| conflictMessage: "Workspace was modified by another user, please try again", | ||
| async function createWorkspaceItem(workspaceId: string, item: Item) { | ||
| await db.transaction(async (tx) => { | ||
| await insertWorkspaceItem(tx, { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new direct-write helpers persist the item exactly as passed in, but they never stamp lastModified. ts // src/lib/ai/workers/workspace-worker.ts await insertWorkspaceItem(tx, { workspaceId, item: sanitizeWorkspaceItemForPersistence(item), sourceVersion: 0, }); Before this migration, the removed projector set lastModified from the event timestamp for creates/updates, and the canonical Zero mutators still do lastModified: now on every create/update. With this change, AI-created items are stored with NULL last_modified, and later AI edits preserve the old timestamp instead of marking the item fresh. That makes the AI worker the only writer that violates the workspace item shell contract. Set lastModified: Date.now() before insertWorkspaceItem() and before upsertWorkspaceItem() in updateWorkspaceItem() so this path matches the rest of the Zero write surface.
|
|
||
| mutation.mutate(event); | ||
| if (workspaceIdRef.current) { | ||
| zeroRef.current.mutate( |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
createItem() now persists explicit names through Zero without the duplicate-name guard that still exists in the batched create path, so single-card flows can create two sibling items of the same type with the same name. The old /api/workspaces/[id]/events POST handler rejected this for ITEM_CREATED, and the new server wrapper in server-mutators.ts just delegates to the shared item.create mutator, so there is no authoritative backstop anymore. This is reachable from changed callers such as the YouTube/website/audio create flows in WorkspaceHeader and YouTubeSearchToolUI, and it breaks the existing name invariant that hasDuplicateName/checkDuplicateName still enforce elsewhere. Reintroduce the duplicate check before mutators.item.create(...) (or, better, enforce it in the server mutator) so single-item creates stay consistent with createItems() and updateItem().
// src/hooks/workspace/use-workspace-operations.ts
const finalName =
name ||
getNextUniqueDefaultName(currentItemsRef.current, validType, folderId);
if (workspaceIdRef.current) {
zeroRef.current.mutate(// src/hooks/workspace/use-workspace-operations.ts
if (
name &&
hasDuplicateName(allItemsSoFar, finalName, validType, folderId)
) {
return null;
}![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found across 72 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/app/api/zero/query/route.ts">
<violation number="1" location="src/app/api/zero/query/route.ts:73">
P0: The request parser is using the wrong Zero query body shape, so `workspaceIds` can remain empty and the ownership gate is skipped before executing the query. Parse the request as an array of query objects (or fail closed) so access checks always run.</violation>
<violation number="2" location="src/app/api/zero/query/route.ts:142">
P2: Log the error before returning the 500 response. Without this, production failures in this route will be invisible — no stack trace, no error message, nothing to diagnose issues with Zero query handling or DB connectivity.</violation>
</file>
<file name="src/contexts/RealtimeContext.tsx">
<violation number="1" location="src/contexts/RealtimeContext.tsx:50">
P2: `error` connection states are being downgraded to `disconnected`, which hides actual sync failures from realtime context consumers.</violation>
</file>
<file name="src/lib/zero/client.ts">
<violation number="1" location="src/lib/zero/client.ts:10">
P1: Avoid defaulting `NEXT_PUBLIC_APP_URL` to localhost for production-facing Zero endpoints; this can route client mutate/query calls to an invalid host and break sync.</violation>
</file>
<file name=".env.example">
<violation number="1" location=".env.example:68">
P2: The default `ZERO_COOKIE_DOMAIN` points to a production domain, which breaks localhost cookie auth when developers copy `.env.example` for local setup.</violation>
</file>
<file name="src/hooks/workspace/use-workspace-state.ts">
<violation number="1" location="src/hooks/workspace/use-workspace-state.ts:63">
P2: This always rehydrates items with `extracted: null`, so OCR/transcript/contentPreview/search-derived fields cannot be populated in workspace state.</violation>
<violation number="2" location="src/hooks/workspace/use-workspace-state.ts:81">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
A functional `refetch` API was replaced with a production no-op stub, which matches the rule’s prohibition on shipping temporary/stub code in real application paths without explicit rationale.</violation>
</file>
<file name="src/workflows/audio-transcribe/steps/persist-result.ts">
<violation number="1" location="src/workflows/audio-transcribe/steps/persist-result.ts:56">
P2: `hasTranscript` is set to `true` unconditionally, even when transcription returns no segments/text.</violation>
</file>
<file name="src/lib/zero/mutators.ts">
<violation number="1" location="src/lib/zero/mutators.ts:241">
P1: Converting `null` to `undefined` prevents clearing the `layout` field on updates. In Zero, `null` means "set to null" while `undefined` means "leave unchanged". When `upsertItem` calls `tx.mutate.workspace_items.update(...)`, a null layout (meaning "clear it") becomes undefined and the old value is silently preserved.
Use `null` directly instead of coalescing to `undefined`, and cast to `JsonValue | null | undefined`.</violation>
<violation number="2" location="src/lib/zero/mutators.ts:250">
P2: Same `null` → `undefined` coalescing pattern as `toMutateShellRow`. For `upsert`, `undefined` means "use column default" while `null` means "set to null". These happen to align today if defaults are null, but the semantics are wrong and will break if these columns gain non-null defaults or this helper is reused with `update`.</violation>
</file>
<file name="src/lib/ai/workers/__tests__/workspace-worker.edit.test.ts">
<violation number="1" location="src/lib/ai/workers/__tests__/workspace-worker.edit.test.ts:1">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Deleted regression coverage for the workspace edit workflow in a behavior-change PR.</violation>
</file>
<file name="src/lib/workspace/__tests__/workspace-items-projector.test.ts">
<violation number="1" location="src/lib/workspace/__tests__/workspace-items-projector.test.ts:1">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Deleted the projector regression tests instead of preserving coverage for workspace-state behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| */ | ||
| const body = (await request.json()) as unknown; | ||
| const queryRequests = | ||
| Array.isArray(body) && body.length > 1 && Array.isArray(body[1]) |
There was a problem hiding this comment.
P0: The request parser is using the wrong Zero query body shape, so workspaceIds can remain empty and the ownership gate is skipped before executing the query. Parse the request as an array of query objects (or fail closed) so access checks always run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/api/zero/query/route.ts, line 73:
<comment>The request parser is using the wrong Zero query body shape, so `workspaceIds` can remain empty and the ownership gate is skipped before executing the query. Parse the request as an array of query objects (or fail closed) so access checks always run.</comment>
<file context>
@@ -0,0 +1,147 @@
+ */
+ const body = (await request.json()) as unknown;
+ const queryRequests =
+ Array.isArray(body) && body.length > 1 && Array.isArray(body[1])
+ ? (body[1] as QueryRequest[])
+ : [];
</file context>
| } | ||
|
|
||
| const appURL = | ||
| process.env.NEXT_PUBLIC_APP_URL ?? "http://localhost:3000"; |
There was a problem hiding this comment.
P1: Avoid defaulting NEXT_PUBLIC_APP_URL to localhost for production-facing Zero endpoints; this can route client mutate/query calls to an invalid host and break sync.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/zero/client.ts, line 10:
<comment>Avoid defaulting `NEXT_PUBLIC_APP_URL` to localhost for production-facing Zero endpoints; this can route client mutate/query calls to an invalid host and break sync.</comment>
<file context>
@@ -0,0 +1,50 @@
+}
+
+const appURL =
+ process.env.NEXT_PUBLIC_APP_URL ?? "http://localhost:3000";
+
+function createZeroInstance(params: { userId: string }) {
</file context>
| ) { | ||
| return { | ||
| ...row, | ||
| layout: (row.layout ?? undefined) as JsonValue | undefined, |
There was a problem hiding this comment.
P1: Converting null to undefined prevents clearing the layout field on updates. In Zero, null means "set to null" while undefined means "leave unchanged". When upsertItem calls tx.mutate.workspace_items.update(...), a null layout (meaning "clear it") becomes undefined and the old value is silently preserved.
Use null directly instead of coalescing to undefined, and cast to JsonValue | null | undefined.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/zero/mutators.ts, line 241:
<comment>Converting `null` to `undefined` prevents clearing the `layout` field on updates. In Zero, `null` means "set to null" while `undefined` means "leave unchanged". When `upsertItem` calls `tx.mutate.workspace_items.update(...)`, a null layout (meaning "clear it") becomes undefined and the old value is silently preserved.
Use `null` directly instead of coalescing to `undefined`, and cast to `JsonValue | null | undefined`.</comment>
<file context>
@@ -0,0 +1,642 @@
+) {
+ return {
+ ...row,
+ layout: (row.layout ?? undefined) as JsonValue | undefined,
+ };
+}
</file context>
| layout: (row.layout ?? undefined) as JsonValue | undefined, | |
| layout: (row.layout === undefined ? undefined : row.layout === null ? null : row.layout) as JsonValue | null | undefined, |
| case "connecting": | ||
| return "connecting"; | ||
| default: | ||
| return "disconnected"; |
There was a problem hiding this comment.
P2: error connection states are being downgraded to disconnected, which hides actual sync failures from realtime context consumers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/contexts/RealtimeContext.tsx, line 50:
<comment>`error` connection states are being downgraded to `disconnected`, which hides actual sync failures from realtime context consumers.</comment>
<file context>
@@ -37,10 +37,21 @@ export function RealtimeProvider({
+ case "connecting":
+ return "connecting";
+ default:
+ return "disconnected";
+ }
+ },
</file context>
| return "disconnected"; | |
| return zeroConnectionState.name === "error" ? "error" : "disconnected"; |
| .update(workspaceItems) | ||
| .set({ | ||
| processingStatus: "complete", | ||
| hasTranscript: true, |
There was a problem hiding this comment.
P2: hasTranscript is set to true unconditionally, even when transcription returns no segments/text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/workflows/audio-transcribe/steps/persist-result.ts, line 56:
<comment>`hasTranscript` is set to `true` unconditionally, even when transcription returns no segments/text.</comment>
<file context>
@@ -1,95 +1,144 @@
+ .update(workspaceItems)
+ .set({
+ processingStatus: "complete",
+ hasTranscript: true,
+ lastModified: Date.now(),
+ })
</file context>
| ) { | ||
| return { | ||
| ...row, | ||
| structuredData: (row.structuredData ?? undefined) as JsonValue | undefined, |
There was a problem hiding this comment.
P2: Same null → undefined coalescing pattern as toMutateShellRow. For upsert, undefined means "use column default" while null means "set to null". These happen to align today if defaults are null, but the semantics are wrong and will break if these columns gain non-null defaults or this helper is reused with update.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/zero/mutators.ts, line 250:
<comment>Same `null` → `undefined` coalescing pattern as `toMutateShellRow`. For `upsert`, `undefined` means "use column default" while `null` means "set to null". These happen to align today if defaults are null, but the semantics are wrong and will break if these columns gain non-null defaults or this helper is reused with `update`.</comment>
<file context>
@@ -0,0 +1,642 @@
+) {
+ return {
+ ...row,
+ structuredData: (row.structuredData ?? undefined) as JsonValue | undefined,
+ assetData: (row.assetData ?? undefined) as JsonValue | undefined,
+ embedData: (row.embedData ?? undefined) as JsonValue | undefined,
</file context>
| @@ -1,434 +0,0 @@ | |||
| import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; | |||
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
Deleted regression coverage for the workspace edit workflow in a behavior-change PR.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/ai/workers/__tests__/workspace-worker.edit.test.ts, line 1:
<comment>Deleted regression coverage for the workspace edit workflow in a behavior-change PR.</comment>
<file context>
@@ -1,434 +0,0 @@
-import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
-
-const mockGetSession = vi.fn();
-const mockHeaders = vi.fn();
-const mockLoadWorkspaceState = vi.fn();
-const mockCreateEvent = vi.fn();
-const mockExecute = vi.fn();
-const mockTransaction = vi.fn();
-const mockBroadcastWorkspaceEventFromServer = vi.fn();
</file context>
| isLoading: Boolean(workspaceId) && status.type === "unknown", | ||
| error, | ||
| version: 0, | ||
| refetch: async () => {}, |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
A functional refetch API was replaced with a production no-op stub, which matches the rule’s prohibition on shipping temporary/stub code in real application paths without explicit rationale.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/workspace/use-workspace-state.ts, line 81:
<comment>A functional `refetch` API was replaced with a production no-op stub, which matches the rule’s prohibition on shipping temporary/stub code in real application paths without explicit rationale.</comment>
<file context>
@@ -1,52 +1,83 @@
+ isLoading: Boolean(workspaceId) && status.type === "unknown",
+ error,
+ version: 0,
+ refetch: async () => {},
};
}
</file context>
| @@ -1,188 +0,0 @@ | |||
| import { describe, expect, it } from "vitest"; | |||
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
Deleted the projector regression tests instead of preserving coverage for workspace-state behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/workspace/__tests__/workspace-items-projector.test.ts, line 1:
<comment>Deleted the projector regression tests instead of preserving coverage for workspace-state behavior.</comment>
<file context>
@@ -1,188 +0,0 @@
-import { describe, expect, it } from "vitest";
-import type { Item } from "@/lib/workspace-state/types";
-import { eventReducer } from "@/lib/workspace/event-reducer";
-import type { WorkspaceEvent } from "@/lib/workspace/events";
-import { deriveWorkspaceProjectionChangeSet } from "@/lib/workspace/workspace-items-projector";
-import { buildWorkspaceItemTableRows } from "@/lib/workspace/workspace-item-model";
-
-function applyEvents(events: WorkspaceEvent[], baseState: Item[] = []) {
- let state = baseState;
</file context>
…utor type safety, update env example
| .values({ | ||
| workspaceId, | ||
| itemId, | ||
| searchText: transcriptText ?? "", |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
Both workflow steps now hand-write workspace_item_extracted rows with only raw transcript/OCR text and never refresh contentPreview, but the rest of the Zero stack treats extracted rows as a canonical projection built by buildWorkspaceItemTableRows: getItemSearchIndex() lowercases and prefixes name, subtitle, and type, and getItemContentPreview() derives a truncated preview from the full item body. The shared server mutator path explicitly calls that canonical projector to keep extracted rows in sync; these workflow writes bypass it. As a result, audio/OCR items written by workflows now have narrower/stale extracted metadata than items written through mutators (for example, audio summaries and item titles disappear from searchText, and previews never update). Recompute extracted rows through the shared projector after updating shell/content instead of hard-coding searchText here.
// src/workflows/audio-transcribe/steps/persist-result.ts
.values({
workspaceId,
itemId,
searchText: transcriptText ?? "",
transcriptText,
| tx: DbExecutor, | ||
| workspaceId: string, | ||
| ): Promise<Item[]> { | ||
| const shellRows = await tx |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new read path reconstructs each item with separate statements for workspace_items, workspace_item_content, and workspace_item_extracted. In PostgreSQL's default READ COMMITTED isolation, each statement gets its own snapshot, so a writer that commits between these selects can make one API response combine the old shell row with the new content/extracted row (or vice versa). The same pattern exists in loadWorkspaceItemRecord, so server-side update/delete helpers can also read a torn item image. That produces impossible Item objects during active OCR/audio updates or normal item edits. Fix by reading the slices in a single joined statement (preferred) or by explicitly using a repeatable-read snapshot for the transaction.
// src/lib/workspace/workspace-state-read.ts
const shellRows = await tx
.select()
.from(workspaceItems)
.where(eq(workspaceItems.workspaceId, workspaceId))
.orderBy(asc(workspaceItems.createdAt), asc(workspaceItems.itemId));
| "tag": "0001_stormy_rawhide_kid", | ||
| "breakpoints": true | ||
| }, | ||
| { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The journal records a new migration:
{
"idx": 2,
"version": "7",
"when": 1775793600000,
"tag": "0002_zero_publication",
"breakpoints": true
}but @drizzle/meta still contains only 0000_snapshot.json and 0001_snapshot.json, and 0001_snapshot.json still models the legacy tables deleted in this PR. That leaves Drizzle's metadata out of sync with the post-PR schema, so the next drizzle-kit generate will diff against stale state and can emit incorrect follow-up migrations. Commit the generated 0002_snapshot.json for the new schema state alongside the journal entry.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/workspace-canvas/QuizContent.tsx (2)
58-108:⚠️ Potential issue | 🟠 MajorSpurious "new questions added" toast on initial Zero hydration and remount.
prevQuestionIdsRef/prevQuestionCountRefare initialized from the first render. With Zero sync, the first render frequently seesquestions = [](pre-hydration or beforeworkspace_item_contentreplicates), and the next render sees the real questions. The effect then computesquestionsAdded > 0 && currentCount > prevCountas true and fires a success toast every time a user opens a quiz card. Similarly, any remount (navigation, modal open/close) will surface the same false toast if the initial data arrives asynchronously.Guard the "newly added" detection so it only fires after the first non-empty render, e.g., track a
hasHydratedref or skip the toast whenprevCount === 0.💡 Suggested guard
+ const hasHydratedRef = useRef(false); useEffect(() => { const prevCount = prevQuestionCountRef.current; const currentCount = questions.length; const prevIds = prevQuestionIdsRef.current; const currentIds = new Set(questions.map((q) => q.id)); const questionsAdded = questions.filter((q) => !prevIds.has(q.id)).length; const hasUnansweredQuestions = answeredQuestions.length < currentCount; - if (questionsAdded > 0 && currentCount > prevCount) { + if ( + hasHydratedRef.current && + questionsAdded > 0 && + currentCount > prevCount + ) { ... } else if (showResults && hasUnansweredQuestions) { setShowResults(false); } prevQuestionCountRef.current = currentCount; prevQuestionIdsRef.current = currentIds; + if (currentCount > 0) hasHydratedRef.current = true; }, [questions, showResults, answeredQuestions.length, currentIndex]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/workspace-canvas/QuizContent.tsx` around lines 58 - 108, The toast fires on first async hydration because prevQuestionCountRef/prevQuestionIdsRef are initialized from the first render (often empty); add a guard to skip the "new questions added" branch when prevQuestionCountRef.current === 0 (or before a first non-empty render) by introducing a hasHydratedRef or checking prevCount === 0 and currentCount > 0 to suppress the toast and only update refs/state (prevQuestionCountRef, prevQuestionIdsRef) without calling toast, setShowResults, setCurrentIndex, setSelectedAnswer or setIsSubmitted; keep the existing logic for subsequent changes when prevCount > 0.
28-40:⚠️ Potential issue | 🟡 MinorQuiz progress is now lost on remount — confirm this is the intended UX.
With session persistence removed,
currentIndex,answeredQuestions, andshowResultslive purely in local component state. Navigating away from the workspace, closing/reopening the modal, or any unmount resets the quiz. If that's the intentional tradeoff per the Zero migration, consider persisting tosessionStoragekeyed byitem.idto avoid regressing the "resume where I left off" flow.Additionally,
onUpdateDatais kept inQuizContentPropsbut is now dead (_onUpdateData). Parent components (CardRenderer,WorkspaceCardContent) still construct and pass this callback needlessly — drop the prop from the interface and clean up call sites to avoid misleading future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/workspace-canvas/QuizContent.tsx` around lines 28 - 40, Quiz progress state (currentIndex, answeredQuestions, showResults) is lost on unmount because it's only kept in component state; persist these to sessionStorage keyed by item.id (read on mount to restore and write on updates) to preserve resume behavior, and remove the now-unused onUpdateData prop from the QuizContentProps interface and its parameter (_onUpdateData) while also cleaning up call sites that still pass onUpdateData (e.g., CardRenderer, WorkspaceCardContent) so the prop is not misleading.src/app/api/workspaces/route.ts (1)
73-99:⚠️ Potential issue | 🟠 MajorRegenerate the slug on uniqueness retries.
The retry loop catches
23505, but each attempt uses the samegenerateSlug(name)value, so slug conflicts will fail five times instead of resolving.🔧 Proposed retry fix
while (attempts < MAX_ATTEMPTS) { try { - const slug = generateSlug(name); + const baseSlug = generateSlug(name); + const slug = attempts === 0 ? baseSlug : `${baseSlug}-${attempts}`; [workspace] = await db🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/workspaces/route.ts` around lines 73 - 99, The insert retries currently reuse the same slug on uniqueness failures; update the loop so a fresh slug is generated on each attempt by calling generateSlug(name) for every iteration (either move "const slug = generateSlug(name)" to the top of the while loop so it runs before each try, or call generateSlug(name) again inside the 23505 catch branch before continuing), ensuring the slug variable is in scope for the db.insert(workspaces).values(...).returning() call and preserving the existing attempts / MAX_ATTEMPTS logic.
♻️ Duplicate comments (2)
src/lib/ai/workers/workspace-worker.ts (1)
185-224:⚠️ Potential issue | 🟠 Major
lastModifiedstill not stamped on AI-worker writes.
buildItemFromCreateParams(lines 173-182) doesn't setlastModified, and neithercreateWorkspaceItem/createWorkspaceItemsnorupdateWorkspaceItemstamps it before callinginsertWorkspaceItem/upsertWorkspaceItem. As a result, AI-created items land withNULL last_modified, and AI edits preserve the stale value — inconsistent with Zero client mutators (src/lib/zero/mutators.ts) which setlastModified: Date.now()on every create/update. This affects "recent activity" UI, sorts by last modified, and optimistic-update reconciliation.🐛 Proposed fix
async function createWorkspaceItem(workspaceId: string, item: Item) { await db.transaction(async (tx) => { await insertWorkspaceItem(tx, { workspaceId, - item: sanitizeWorkspaceItemForPersistence(item), + item: sanitizeWorkspaceItemForPersistence({ + ...item, + lastModified: Date.now(), + }), sourceVersion: 0, }); }); } async function createWorkspaceItems(workspaceId: string, items: Item[]) { await db.transaction(async (tx) => { + const now = Date.now(); for (const item of items) { await insertWorkspaceItem(tx, { workspaceId, - item: sanitizeWorkspaceItemForPersistence(item), + item: sanitizeWorkspaceItemForPersistence({ ...item, lastModified: now }), sourceVersion: 0, }); } }); } async function updateWorkspaceItem( workspaceId: string, itemId: string, updater: (item: Item) => Item, ) { await db.transaction(async (tx) => { const existing = await loadWorkspaceItemRecord(tx, { workspaceId, itemId }); if (!existing) { throw new Error(`Item not found with ID: ${itemId}`); } await upsertWorkspaceItem(tx, { workspaceId, sourceVersion: existing.sourceVersion, - item: sanitizeWorkspaceItemForPersistence(updater(existing.item)), + item: sanitizeWorkspaceItemForPersistence({ + ...updater(existing.item), + lastModified: Date.now(), + }), }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ai/workers/workspace-worker.ts` around lines 185 - 224, AI-worker persistence paths are not stamping lastModified, causing NULL or stale last_modified values; update buildItemFromCreateParams to set lastModified: Date.now() for new items and modify createWorkspaceItem, createWorkspaceItems (before calling insertWorkspaceItem) to ensure sanitizeWorkspaceItemForPersistence receives an item with lastModified set, and change updateWorkspaceItem to set lastModified: Date.now() on the updated object before calling upsertWorkspaceItem (use existing.item as the base, apply updater, then overwrite lastModified), so all insertWorkspaceItem/upsertWorkspaceItem calls persist a current lastModified like Zero mutators do.src/hooks/workspace/use-workspace-operations.ts (1)
389-425:⚠️ Potential issue | 🟠 MajorRestore the duplicate-name guard on single-create paths.
createItems()still skips explicit duplicates, butcreateItem(),createFolder(), andcreateFolderWithItems()can now send duplicate sibling names throughitem.create/folder.createWithItems. Please apply the samehasDuplicateName(...)check before these single-create mutations, or enforce it in the server mutator so all write paths share the invariant.Also applies to: 747-759, 798-810
🧹 Nitpick comments (7)
src/lib/auth.ts (3)
58-75: Wildcard subdomains are supported by Better Auth. No action needed forhttps://*.thinkex.app.Separate note: The
NODE_ENV === "development"branch adding"*"to trusted origins (line 71) is very permissive; audit to ensure this wildcard cannot leak into preview/staging builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth.ts` around lines 58 - 75, The trustedOrigins array currently adds a wildcard ("*") when process.env.NODE_ENV === "development" (the spread that creates ["*"]); change this to only allow the wildcard in true local development environments (avoid letting it into preview/staging/vercel builds) by tightening the condition — for example, require an additional signal such as process.env.VERCEL === undefined or process.env.VERCEL_ENV === "development" (i.e., use process.env.NODE_ENV === "development" && !process.env.VERCEL or check process.env.VERCEL_ENV) so that the wildcard cannot be injected in preview/staging; update the spread expression that currently uses (process.env.NODE_ENV === "development" ? ["*"] : []) accordingly.
107-110: Remove leading dot from cookie domain to align with Better Auth best practices.The
.thinkex.appdomain format deviates from Better Auth's documented guidance, which recommends domains without leading dots (e.g.,thinkex.appinstead of.thinkex.app). While modern browsers handle both formats equivalently per RFC 6265, Better Auth explicitly advises the simpler format for security and compatibility. Change tothinkex.app.The coupling of
ZERO_COOKIE_DOMAINto auth cookie configuration is intentional (documented in.env.exampleunder "Zero Sync"), but consider adding a comment in the code clarifying this design decision to prevent accidental changes by future operators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth.ts` around lines 107 - 110, Update the crossSubDomainCookies.domain default to remove the leading dot (use "thinkex.app" instead of ".thinkex.app") in the crossSubDomainCookies block, keeping the override via process.env.ZERO_COOKIE_DOMAIN intact; also add a short inline comment next to ZERO_COOKIE_DOMAIN/crossSubDomainCookies explaining that this env var is intentionally shared with Zero Sync (refer to .env.example) so future maintainers don’t remove or change the coupling.
25-25: Adding explicit validation forauthSecretis good practice, but Better Auth already prevents silent failures in production.While the code passes
authSecret(which could beundefined) tobetterAuth({ secret }), Better Auth's built-in validation will throw an error in production if neitherBETTER_AUTH_SECRETnorAUTH_SECRETis set, preventing silent fallback to an insecure default. The middleware in this codebase usesgetSessionCookie(), which only checks cookie presence and does not depend on the secret.That said, an explicit validation check remains a good defensive measure to fail immediately and clearly during initialization rather than relying on Better Auth's deferred validation.
Optional: Add explicit validation
const authSecret = process.env.BETTER_AUTH_SECRET || process.env.AUTH_SECRET; +if (!authSecret && process.env.NODE_ENV === "production") { + throw new Error( + "[auth] BETTER_AUTH_SECRET (or AUTH_SECRET) must be set in production", + ); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth.ts` at line 25, The code currently derives authSecret via the authSecret constant and passes it to betterAuth({ secret }) without an explicit check; add a defensive validation right after the authSecret assignment to throw a clear, early error if authSecret is falsy (e.g., throw new Error('Missing auth secret: set BETTER_AUTH_SECRET or AUTH_SECRET')), so initialization fails fast and provides a clear message; keep betterAuth({ secret: authSecret }) and leave getSessionCookie() behavior unchanged.src/lib/workspace/workspace-state-read.ts (1)
96-97: Avoidas anyon JSONB columns.
ocrPages/transcriptSegmentsare typedunknownfrom Drizzle'sjsonb(). Casting toanydiscards type safety and silently unblocks invalid shapes. Prefer the same narrowing style used forstructuredDataabove (as Record<string, unknown> | nullor the actual projection types).🔧 Proposed fix
- ocrPages: (extracted.ocrPages as any) ?? null, - transcriptSegments: (extracted.transcriptSegments as any) ?? null, + ocrPages: + (extracted.ocrPages as Record<string, unknown> | null) ?? null, + transcriptSegments: + (extracted.transcriptSegments as Record<string, unknown> | null) ?? + null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/workspace/workspace-state-read.ts` around lines 96 - 97, The ocrPages and transcriptSegments fields are being cast with "as any", which discards type safety; update the assignments in workspace-state-read.ts to use the same narrowing style as structuredData instead of "as any" — e.g., narrow extracted.ocrPages and extracted.transcriptSegments to a safe JSON shape such as Record<string, unknown> | null or the concrete projection types (e.g., OcrPage[] | null, TranscriptSegment[] | null) so their types remain checked and invalid shapes are not silently accepted; change the two expressions that currently use (extracted.ocrPages as any) and (extracted.transcriptSegments as any) to the appropriate narrowed types.src/lib/zero/mutators.ts (2)
432-449:createsilently overridesargs.item.idwithargs.id.
itemSchemarequiresid: z.string()on the nesteditem, and the mutator then spreads...args.item, id: args.id— making the nested id meaningless (the outerargs.idalways wins). This is confusing for callers and invites bugs where a caller sets onlyargs.item.idand the top-levelargs.idis stale/mismatched.Consider either (a) dropping
idfromitemSchemahere so the outerargs.idis the single source of truth, or (b) validating thatargs.id === args.item.idwhen both are present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/zero/mutators.ts` around lines 432 - 449, The create mutator currently overwrites args.item.id with args.id which makes the nested id meaningless; update the create handler (the defineMutator for zeroMutatorSchemas.item.create, where toItem(...) and insertItem(...) are called) to validate that if args.item.id is present it must equal args.id and otherwise throw a clear validation error (or reject the request), so the top-level args.id remains the single source of truth; reference args.id and args.item.id in this check and keep the subsequent toItem(...) call unchanged so insertItem(...) still receives the canonical id.
313-353: DeaduserIdparameter oninsertItem/upsertItem.Both helpers take
userId: string | nullbut never use it. Since these mutators are intentionally auth-agnostic (auth happens inserver-mutators.ts), dropping the parameter avoids misleading callers into thinking they're providing auth context.♻️ Proposed cleanup
async function insertItem( tx: ZeroTx, params: { workspaceId: string; item: Item; sourceVersion?: number; - userId: string | null; }, ) { @@ async function upsertItem( tx: ZeroTx, params: { workspaceId: string; item: Item; sourceVersion: number; - userId: string | null; }, ) {Call sites in
mutators.item.{create,update,createMany,patchMany,updateMany}andfolder.createWithItemsdrop theuserId:field accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/zero/mutators.ts` around lines 313 - 353, Remove the dead userId parameter from the insertItem and upsertItem function signatures and all call sites; specifically update async function insertItem(tx: ZeroTx, params: { workspaceId: string; item: Item; sourceVersion?: number; }) and async function upsertItem(tx: ZeroTx, params: { workspaceId: string; item: Item; sourceVersion: number; }) and then delete any userId: ... being passed from callers (e.g., mutators.item.create/update/createMany/patchMany/updateMany and folder.createWithItems). Keep the existing logic that builds rows via buildWorkspaceItemTableRows and calls to toMutateShellRow/toMutateContentRow and tx.mutate.workspace_items / workspace_item_content unchanged.src/lib/ai/workers/workspace-worker.ts (1)
309-316: Remove unusedeventandversionfields from the return type.The return type declares
event?: unknownandversion?: number, but the implementation never populates these fields in any code path. The only caller that reads them (quiz-tools.tslines 81-82) receivesundefinedvalues, which it then passes downstream. These dead fields create a misleading API surface. Remove them along with their usages inquiz-tools.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ai/workers/workspace-worker.ts` around lines 309 - 316, The return type of the function in workspace-worker.ts declares unused optional fields event?: unknown and version?: number—remove these fields from the Promise return type and any related type annotations in that function (the signature that currently returns { success, message, itemId?, cardCount?, event?, version? }), then update any call sites (notably the consumer in quiz-tools.ts where the result is destructured or accessed around lines that read event/version) to stop referencing event and version; ensure only success, message, itemId, and cardCount remain in the returned shape and used downstream (adjust any interface/type aliases if present to match the new shape).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 66-68: The Zero Sync env block (NEXT_PUBLIC_ZERO_SERVER and
ZERO_COOKIE_DOMAIN) is out of order and triggers dotenv-linter's UnorderedKey;
move the two lines for NEXT_PUBLIC_ZERO_SERVER and ZERO_COOKIE_DOMAIN so they
appear before the SUPERMEMORY_ENABLED entry (or reorder the surrounding env keys
to match the linter's expected alphabetical/grouping), ensuring the variable
names remain unchanged and the block stays contiguous.
In `@drizzle/0002_zero_publication.sql`:
- Around line 1-3: The CREATE PUBLICATION zero_pub statement is not idempotent
and will fail if zero_pub already exists; update the migration by replacing the
plain CREATE PUBLICATION zero_pub FOR TABLE ... with a DO block that checks
pg_catalog.pg_publication (or pg_publication_rel) for an existing publication
named 'zero_pub' and only executes CREATE PUBLICATION zero_pub FOR TABLE
workspace_items, workspace_item_content when it does not exist; also confirm
that the referenced tables workspace_items and workspace_item_content have
primary keys or unique indexes (Zero requirement) before creating the
publication.
In `@src/app/api/workspaces/route.ts`:
- Around line 111-123: The workspace row is currently inserted before the
item-insert transaction, so failures can leave an empty workspace; move the
workspace insert into the same db.transaction that inserts initialItems.
Specifically, start a single db.transaction (use the same tx passed into
insertWorkspaceItem) and perform the workspace creation and then loop to call
insertWorkspaceItem(tx, {...}) for each initial item inside that transaction,
then remove the compensating db.delete(workspaces)... throw insertError cleanup
since the transaction will roll back on error. Ensure you reference the existing
functions/objects: db.transaction, insertWorkspaceItem, and the
workspaces/workspace creation logic so both operations run atomically.
In `@src/app/dashboard/page.tsx`:
- Around line 86-87: isSaving is incorrectly derived from
zeroConnectionState.name === "connecting", causing false positives on reconnect
and false negatives when local mutations are queued; update the logic to reflect
pending local mutations instead of transport state by reading Zero's
pending-mutation metric (e.g. zero.pendingMutationCount or
zeroConnectionState.pendingMutations) and set isSaving = pendingCount > 0 (or
similar), or if the Zero API doesn't expose a pending count, remove the save
indicator entirely; locate the usage around zeroConnectionState and isSaving in
page.tsx and replace the connection-state-based check with a
pending-mutation-based check or drop the indicator.
In `@src/contexts/RealtimeContext.tsx`:
- Around line 42-54: Update the useMemo that computes connectionStatus so it
maps zeroConnectionState.name values "error" and "needs-auth" to the
RealtimeContextType "error" status instead of returning "disconnected", and
change the memo dependency from [zeroConnectionState] to
[zeroConnectionState.name]; locate the memo block that defines connectionStatus
(the useMemo referencing zeroConnectionState) and adjust the switch to include
cases "error" and "needs-auth" returning "error" while leaving "connected" and
"connecting" as-is.
In `@src/hooks/workspace/use-workspace-operations.ts`:
- Around line 988-990: The hook currently hardcodes isPending: false, isError:
false, error: null which masks real mutator failures; update the
useWorkspaceOperations hook so that any mutator invocation (e.g.,
save/delete/update workspace functions inside the hook) sets isPending=true
while the request runs, then on success sets isPending=false and clears error,
and on rejection captures the thrown ApplicationError (or network error), sets
isPending=false, isError=true, and populates error with the caught error
object/message so callers can observe and react to failed saves/deletes or
authorization errors.
- Around line 589-627: The current deleteItem flow deletes the PDF file before
the authoritative server-side deletion (see deleteItem, currentItemsRef,
pdfData.fileUrl, zeroRef.current.mutate and mutators.item.delete); change the
flow so the DB/server delete is confirmed first and only then remove the file
asset. Concretely: call zeroRef.current.mutate(mutators.item.delete(...)) and
await/confirm success (or handle the outbox/server-side file cleanup) before
invoking the fetch DELETE for pdfData.fileUrl; if the mutator fails, skip file
deletion and surface/log the error.
- Around line 651-740: The update logic in updateAllItems currently only handles
deletes when items.length < previousItemCount and adds when >, missing cases
where a delete and add occur but lengths match; change the flow to always
compute deletedIds (items in currentItemsRef.current not in newIds), addedItems
(items in items not in currentIds, passed through
sanitizeWorkspaceItemForPersistence), and layoutUpdates (as currently computed
with getLayoutForBreakpoint), then if any of deletedIds, addedItems, or
layoutUpdates are non-empty call
zeroRef.current.mutate(mutators.item.updateMany({ workspaceId:
currentWorkspaceId, deletedIds, addedItems: addedItems?.map(...), layoutUpdates,
previousItemCount })) so a single updateMany applies all changes atomically;
keep existing mapping shape for addedItems and reuse
currentItemsRef/currentIds/newIds variables to locate items.
In `@src/hooks/workspace/use-workspace-state.ts`:
- Around line 36-64: The Zero query for items must include the related
workspaceItemExtracted and the hook must pass that data into
rehydrateWorkspaceItem instead of hardcoding extracted: null; update
queries.workspace.items to add .related("workspaceItemExtracted") (alongside
workspaceItemContent) and in use-workspace-state.ts map the returned related
workspaceItemExtracted row to the extracted argument of rehydrateWorkspaceItem
so PDF/image OCR pages and audio transcriptSegments are populated (identify the
mapping near the rehydrateWorkspaceItem call in use-workspace-state.ts and the
query definition in queries.workspace.items).
In `@src/lib/workspace/workspace-item-sanitize.ts`:
- Around line 1-11: The two sanitizer functions
sanitizeWorkspaceItemForPersistence and sanitizeWorkspaceItemChanges are
currently no-ops, which prevents dropping transient/client-only fields and
breaks the "empty-changes short-circuit"; either implement sanitization to
remove derived fields (e.g., lastModified, ephemeral UI state, any fields not
persisted to workspace_items/workspace_item_content) or remove these functions
and their call sites so updates go directly to mutators.item.update. Locate
sanitizeWorkspaceItemForPersistence and sanitizeWorkspaceItemChanges and either
(A) implement a whitelist/blacklist that removes non-persistent keys from Item
and Partial<Item> before returning (ensuring sanitizeWorkspaceItemChanges can
return an empty object when nothing remains), or (B) delete these functions and
refactor call sites (such as use-workspace-operations.ts) to stop invoking them.
In `@src/lib/workspace/workspace-item-write.ts`:
- Around line 151-177: The current upsertWorkspaceItem() only performs an UPDATE
(the tx.update(...).set(...) block) so if the shell row is missing it never
INSERTs and later content/extracted upserts produce orphan rows; change
upsertWorkspaceItem() to perform an upsert: after the UPDATE attempt, check
whether any row was affected and if zero, INSERT a shell row into workspaceItems
with at minimum workspaceId, itemId, createdAt/updatedAt and any required
non-null columns (type/name/folderId as appropriate) or use a single INSERT ...
ON CONFLICT (workspaceId, itemId) DO UPDATE to atomically create-or-update the
shell; ensure loadWorkspaceItemRecord() will then find the shell row.
In `@src/lib/zero/client.ts`:
- Around line 9-24: The createZeroInstance function currently uses a non-null
assertion for process.env.NEXT_PUBLIC_ZERO_SERVER and a localhost fallback for
appURL; change it to validate required env vars up-front: throw a clear Error if
NEXT_PUBLIC_ZERO_SERVER is missing, and if NODE_ENV === "production" (or when
deploying) also require NEXT_PUBLIC_APP_URL instead of falling back to
"http://localhost:3000"; use those validated values for Zero({ cacheURL,
mutateURL, queryURL, ... }) so cacheURL, mutateURL and queryURL are never
undefined and produce an explicit configuration error instead of downstream
cryptic failures.
In `@src/lib/zero/zero-schema.gen.ts`:
- Around line 14-37: The createdAt column in the workspaceItems table schema is
marked optional but the DB column is NOT NULL with a default, so update the
workspaceItems table definition to remove .optional() and use
number().from("created_at") for the createdAt field (modify the createdAt
property in the workspaceItems .columns call).
In `@src/workflows/audio-transcribe/steps/persist-result.ts`:
- Around line 66-103: The code performs a non-atomic read-modify-write: you call
tx.select(...) to read workspaceItemContent.assetData into currentAssetData then
tx.update(workspaceItemContent).set(...) to write back merged assetData, which
can lose concurrent updates; instead remove the SELECT and perform a single
UPDATE that merges JSONB on the DB side (use PostgreSQL jsonb
concatenation/merge via an sql template or query builder to set assetData =
workspace_item_content.asset_data || '{"duration": ...}'::jsonb when
result.duration is present), keep setting structuredData.summary and updatedAt
in the same UPDATE, and only conditionally include the jsonb merge when
result.duration > 0; also fix the incorrect cast of result.segments (don’t cast
segments to Record<string, unknown> — use a truthful type such as unknown or the
actual array type) so the value stored in assetData/columns matches its real
type.
In `@src/workflows/ocr-dispatch/steps/persist-results.ts`:
- Around line 18-100: The loop currently does per-result db.transaction calls
and reads workspaceItemContent.assetData without locking, causing a lost-update
race; change to use a single outer db.transaction for the whole results array
(replace per-result db.transaction(...) with one transaction enclosing the loop)
and inside that transaction either (A) change the SELECT from
workspaceItemContent to acquire a row lock (use the query builder's
.for("update") on the select of workspaceItemContent.assetData) before computing
nextAssetData, or (B) avoid read-modify-write entirely by performing the merge
in SQL (use jsonb_set or the jsonb '-' operator to remove/add the ocrError key
on workspaceItemContent.assetData in the UPDATE statement); reference the
symbols workspaceItemContent, assetData, db.transaction, workspaceItemExtracted,
workspaceItems, and use jsonb_set / '-' if choosing the SQL-merge approach.
---
Outside diff comments:
In `@src/app/api/workspaces/route.ts`:
- Around line 73-99: The insert retries currently reuse the same slug on
uniqueness failures; update the loop so a fresh slug is generated on each
attempt by calling generateSlug(name) for every iteration (either move "const
slug = generateSlug(name)" to the top of the while loop so it runs before each
try, or call generateSlug(name) again inside the 23505 catch branch before
continuing), ensuring the slug variable is in scope for the
db.insert(workspaces).values(...).returning() call and preserving the existing
attempts / MAX_ATTEMPTS logic.
In `@src/components/workspace-canvas/QuizContent.tsx`:
- Around line 58-108: The toast fires on first async hydration because
prevQuestionCountRef/prevQuestionIdsRef are initialized from the first render
(often empty); add a guard to skip the "new questions added" branch when
prevQuestionCountRef.current === 0 (or before a first non-empty render) by
introducing a hasHydratedRef or checking prevCount === 0 and currentCount > 0 to
suppress the toast and only update refs/state (prevQuestionCountRef,
prevQuestionIdsRef) without calling toast, setShowResults, setCurrentIndex,
setSelectedAnswer or setIsSubmitted; keep the existing logic for subsequent
changes when prevCount > 0.
- Around line 28-40: Quiz progress state (currentIndex, answeredQuestions,
showResults) is lost on unmount because it's only kept in component state;
persist these to sessionStorage keyed by item.id (read on mount to restore and
write on updates) to preserve resume behavior, and remove the now-unused
onUpdateData prop from the QuizContentProps interface and its parameter
(_onUpdateData) while also cleaning up call sites that still pass onUpdateData
(e.g., CardRenderer, WorkspaceCardContent) so the prop is not misleading.
---
Duplicate comments:
In `@src/lib/ai/workers/workspace-worker.ts`:
- Around line 185-224: AI-worker persistence paths are not stamping
lastModified, causing NULL or stale last_modified values; update
buildItemFromCreateParams to set lastModified: Date.now() for new items and
modify createWorkspaceItem, createWorkspaceItems (before calling
insertWorkspaceItem) to ensure sanitizeWorkspaceItemForPersistence receives an
item with lastModified set, and change updateWorkspaceItem to set lastModified:
Date.now() on the updated object before calling upsertWorkspaceItem (use
existing.item as the base, apply updater, then overwrite lastModified), so all
insertWorkspaceItem/upsertWorkspaceItem calls persist a current lastModified
like Zero mutators do.
---
Nitpick comments:
In `@src/lib/ai/workers/workspace-worker.ts`:
- Around line 309-316: The return type of the function in workspace-worker.ts
declares unused optional fields event?: unknown and version?: number—remove
these fields from the Promise return type and any related type annotations in
that function (the signature that currently returns { success, message, itemId?,
cardCount?, event?, version? }), then update any call sites (notably the
consumer in quiz-tools.ts where the result is destructured or accessed around
lines that read event/version) to stop referencing event and version; ensure
only success, message, itemId, and cardCount remain in the returned shape and
used downstream (adjust any interface/type aliases if present to match the new
shape).
In `@src/lib/auth.ts`:
- Around line 58-75: The trustedOrigins array currently adds a wildcard ("*")
when process.env.NODE_ENV === "development" (the spread that creates ["*"]);
change this to only allow the wildcard in true local development environments
(avoid letting it into preview/staging/vercel builds) by tightening the
condition — for example, require an additional signal such as process.env.VERCEL
=== undefined or process.env.VERCEL_ENV === "development" (i.e., use
process.env.NODE_ENV === "development" && !process.env.VERCEL or check
process.env.VERCEL_ENV) so that the wildcard cannot be injected in
preview/staging; update the spread expression that currently uses
(process.env.NODE_ENV === "development" ? ["*"] : []) accordingly.
- Around line 107-110: Update the crossSubDomainCookies.domain default to remove
the leading dot (use "thinkex.app" instead of ".thinkex.app") in the
crossSubDomainCookies block, keeping the override via
process.env.ZERO_COOKIE_DOMAIN intact; also add a short inline comment next to
ZERO_COOKIE_DOMAIN/crossSubDomainCookies explaining that this env var is
intentionally shared with Zero Sync (refer to .env.example) so future
maintainers don’t remove or change the coupling.
- Line 25: The code currently derives authSecret via the authSecret constant and
passes it to betterAuth({ secret }) without an explicit check; add a defensive
validation right after the authSecret assignment to throw a clear, early error
if authSecret is falsy (e.g., throw new Error('Missing auth secret: set
BETTER_AUTH_SECRET or AUTH_SECRET')), so initialization fails fast and provides
a clear message; keep betterAuth({ secret: authSecret }) and leave
getSessionCookie() behavior unchanged.
In `@src/lib/workspace/workspace-state-read.ts`:
- Around line 96-97: The ocrPages and transcriptSegments fields are being cast
with "as any", which discards type safety; update the assignments in
workspace-state-read.ts to use the same narrowing style as structuredData
instead of "as any" — e.g., narrow extracted.ocrPages and
extracted.transcriptSegments to a safe JSON shape such as Record<string,
unknown> | null or the concrete projection types (e.g., OcrPage[] | null,
TranscriptSegment[] | null) so their types remain checked and invalid shapes are
not silently accepted; change the two expressions that currently use
(extracted.ocrPages as any) and (extracted.transcriptSegments as any) to the
appropriate narrowed types.
In `@src/lib/zero/mutators.ts`:
- Around line 432-449: The create mutator currently overwrites args.item.id with
args.id which makes the nested id meaningless; update the create handler (the
defineMutator for zeroMutatorSchemas.item.create, where toItem(...) and
insertItem(...) are called) to validate that if args.item.id is present it must
equal args.id and otherwise throw a clear validation error (or reject the
request), so the top-level args.id remains the single source of truth; reference
args.id and args.item.id in this check and keep the subsequent toItem(...) call
unchanged so insertItem(...) still receives the canonical id.
- Around line 313-353: Remove the dead userId parameter from the insertItem and
upsertItem function signatures and all call sites; specifically update async
function insertItem(tx: ZeroTx, params: { workspaceId: string; item: Item;
sourceVersion?: number; }) and async function upsertItem(tx: ZeroTx, params: {
workspaceId: string; item: Item; sourceVersion: number; }) and then delete any
userId: ... being passed from callers (e.g.,
mutators.item.create/update/createMany/patchMany/updateMany and
folder.createWithItems). Keep the existing logic that builds rows via
buildWorkspaceItemTableRows and calls to toMutateShellRow/toMutateContentRow and
tx.mutate.workspace_items / workspace_item_content unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 795a902b-0c75-4ff1-ad3e-84b9b6513cb4
📒 Files selected for processing (72)
.env.exampledrizzle/0002_zero_publication.sqldrizzle/meta/_journal.jsonpackage.jsonsrc/app/api/share/[id]/route.tssrc/app/api/workspaces/[id]/events/route.tssrc/app/api/workspaces/[id]/route.tssrc/app/api/workspaces/[id]/state/route.tssrc/app/api/workspaces/route.tssrc/app/api/workspaces/slug/[slug]/route.tssrc/app/api/zero/mutate/route.tssrc/app/api/zero/query/route.tssrc/app/dashboard/page.tsxsrc/components/workspace-canvas/FlashcardWorkspaceCard.tsxsrc/components/workspace-canvas/QuizContent.tsxsrc/components/workspace-canvas/WorkspaceContent.tsxsrc/contexts/RealtimeContext.tsxsrc/contexts/WorkspaceContext.tsxsrc/hooks/workspace/use-workspace-events.tssrc/hooks/workspace/use-workspace-mutation.tssrc/hooks/workspace/use-workspace-operations.tssrc/hooks/workspace/use-workspace-realtime.tssrc/hooks/workspace/use-workspace-state.tssrc/hooks/workspace/workspace-state-cache.test.tssrc/hooks/workspace/workspace-state-cache.tssrc/lib/ai/tools/tool-utils.tssrc/lib/ai/tools/workspace-tools.tssrc/lib/ai/workers/__tests__/workspace-worker.edit.test.tssrc/lib/ai/workers/common.tssrc/lib/ai/workers/workspace-worker.tssrc/lib/auth.tssrc/lib/db/relations.tssrc/lib/db/schema.tssrc/lib/db/types.tssrc/lib/realtime/server-broadcast.tssrc/lib/stores/workspace-store.tssrc/lib/utils/format-workspace-context.tssrc/lib/workspace-state/item-data-schemas.tssrc/lib/workspace-state/item-helpers.tssrc/lib/workspace-state/state.tssrc/lib/workspace-state/types.tssrc/lib/workspace/__tests__/event-reducer.bulk-items-patched.test.tssrc/lib/workspace/__tests__/event-reducer.lightweight-ocr.test.tssrc/lib/workspace/__tests__/version-helpers.test.tssrc/lib/workspace/__tests__/workspace-event-client-payload.test.tssrc/lib/workspace/__tests__/workspace-event-store.test.tssrc/lib/workspace/__tests__/workspace-item-model.test.tssrc/lib/workspace/__tests__/workspace-items-projector.test.tssrc/lib/workspace/event-reducer.tssrc/lib/workspace/events.tssrc/lib/workspace/mutation-helpers.tssrc/lib/workspace/state-loader.tssrc/lib/workspace/version-helpers.tssrc/lib/workspace/workspace-event-client-payload.tssrc/lib/workspace/workspace-event-store.tssrc/lib/workspace/workspace-item-model-schemas.tssrc/lib/workspace/workspace-item-model-shared.tssrc/lib/workspace/workspace-item-model-types.tssrc/lib/workspace/workspace-item-model.tssrc/lib/workspace/workspace-item-sanitize.tssrc/lib/workspace/workspace-item-write.tssrc/lib/workspace/workspace-items-projector.tssrc/lib/workspace/workspace-state-read.tssrc/lib/zero/client.tssrc/lib/zero/mutators.tssrc/lib/zero/provider.tsxsrc/lib/zero/queries.tssrc/lib/zero/server-mutators.tssrc/lib/zero/zero-schema.gen.tssrc/workflows/audio-transcribe/steps/persist-result.tssrc/workflows/ocr-dispatch/steps/persist-results.tstsconfig.json
💤 Files with no reviewable changes (27)
- src/lib/ai/tools/workspace-tools.ts
- src/lib/workspace-state/state.ts
- src/lib/workspace/tests/event-reducer.bulk-items-patched.test.ts
- src/lib/workspace/tests/version-helpers.test.ts
- src/lib/workspace/tests/workspace-event-client-payload.test.ts
- src/lib/workspace/tests/event-reducer.lightweight-ocr.test.ts
- src/lib/workspace-state/item-helpers.ts
- src/lib/workspace/tests/workspace-items-projector.test.ts
- src/lib/workspace/event-reducer.ts
- src/lib/workspace/workspace-event-client-payload.ts
- src/lib/db/schema.ts
- src/lib/workspace-state/item-data-schemas.ts
- src/hooks/workspace/workspace-state-cache.test.ts
- src/lib/workspace/events.ts
- src/hooks/workspace/use-workspace-events.ts
- src/lib/workspace/state-loader.ts
- src/hooks/workspace/use-workspace-mutation.ts
- src/lib/ai/workers/tests/workspace-worker.edit.test.ts
- src/hooks/workspace/use-workspace-realtime.ts
- src/lib/workspace/version-helpers.ts
- src/lib/workspace/workspace-event-store.ts
- src/lib/realtime/server-broadcast.ts
- src/hooks/workspace/workspace-state-cache.ts
- src/app/api/workspaces/[id]/state/route.ts
- src/app/api/workspaces/[id]/events/route.ts
- src/lib/workspace/workspace-items-projector.ts
- src/lib/workspace/tests/workspace-event-store.test.ts
| # Zero Sync | ||
| NEXT_PUBLIC_ZERO_SERVER=http://localhost:4848 | ||
| ZERO_COOKIE_DOMAIN=localhost |
There was a problem hiding this comment.
Move the Zero env block to satisfy dotenv-linter.
NEXT_PUBLIC_ZERO_SERVER currently triggers UnorderedKey; move the Zero Sync block before SUPERMEMORY_ENABLED or adjust the ordering expected by the linter.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 67-67: [UnorderedKey] The NEXT_PUBLIC_ZERO_SERVER key should go before the SUPERMEMORY_ENABLED key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 66 - 68, The Zero Sync env block
(NEXT_PUBLIC_ZERO_SERVER and ZERO_COOKIE_DOMAIN) is out of order and triggers
dotenv-linter's UnorderedKey; move the two lines for NEXT_PUBLIC_ZERO_SERVER and
ZERO_COOKIE_DOMAIN so they appear before the SUPERMEMORY_ENABLED entry (or
reorder the surrounding env keys to match the linter's expected
alphabetical/grouping), ensuring the variable names remain unchanged and the
block stays contiguous.
| CREATE PUBLICATION zero_pub FOR TABLE | ||
| workspace_items, | ||
| workspace_item_content; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does @rocicorp/zero zero-cache require REPLICA IDENTITY FULL on published Postgres tables?
💡 Result:
No, @rocicorp/zero zero-cache does not require REPLICA IDENTITY FULL on published Postgres tables by default. Official documentation on supported Postgres features, configuration, and replication (sources 1,2,5,9) does not mention any such requirement. zero-cache uses standard logical replication via Postgres publications, which defaults to using the primary key or unique index for row identification (source 2 explicitly requires tables to have a primary key or unique index for sync). REPLICA IDENTITY FULL was added as an explicit support feature in Zero 0.17 (source 9), implying it handles tables needing it (e.g., those without PK/unique index or with non-PK updates), but it is not a general requirement for all tables—only when necessary for correct replication of updates/deletes.
Citations:
- 1: https://zero.rocicorp.dev/docs/zero-cache-config
- 2: https://zero.rocicorp.dev/docs/postgres-support
- 3: https://zero.rocicorp.dev/docs/connecting-to-postgres
- 4: https://zero.rocicorp.dev/docs/release-notes/0.17
Make the migration idempotent to handle replayed runs.
CREATE PUBLICATION zero_pub will fail if the publication already exists (e.g., during migration replay). Postgres lacks CREATE PUBLICATION IF NOT EXISTS, so wrap the creation in a DO block to check for existence first.
Suggested idempotent migration
-CREATE PUBLICATION zero_pub FOR TABLE
- workspace_items,
- workspace_item_content;
+DO $$
+BEGIN
+ IF NOT EXISTS (SELECT 1 FROM pg_publication WHERE pubname = 'zero_pub') THEN
+ CREATE PUBLICATION zero_pub FOR TABLE
+ workspace_items,
+ workspace_item_content;
+ END IF;
+END $$;Note: Verify that both workspace_items and workspace_item_content have primary keys or unique indices, as Zero requires them for logical replication. REPLICA IDENTITY FULL is only needed in edge cases (e.g., tables without primary keys), not as a general requirement.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE PUBLICATION zero_pub FOR TABLE | |
| workspace_items, | |
| workspace_item_content; | |
| DO $$ | |
| BEGIN | |
| IF NOT EXISTS (SELECT 1 FROM pg_publication WHERE pubname = 'zero_pub') THEN | |
| CREATE PUBLICATION zero_pub FOR TABLE | |
| workspace_items, | |
| workspace_item_content; | |
| END IF; | |
| END $$; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@drizzle/0002_zero_publication.sql` around lines 1 - 3, The CREATE PUBLICATION
zero_pub statement is not idempotent and will fail if zero_pub already exists;
update the migration by replacing the plain CREATE PUBLICATION zero_pub FOR
TABLE ... with a DO block that checks pg_catalog.pg_publication (or
pg_publication_rel) for an existing publication named 'zero_pub' and only
executes CREATE PUBLICATION zero_pub FOR TABLE workspace_items,
workspace_item_content when it does not exist; also confirm that the referenced
tables workspace_items and workspace_item_content have primary keys or unique
indexes (Zero requirement) before creating the publication.
| try { | ||
| await db.transaction(async (tx) => { | ||
| for (const item of initialItems) { | ||
| await insertWorkspaceItem(tx, { | ||
| workspaceId: workspace.id, | ||
| item, | ||
| sourceVersion: 0, | ||
| }); | ||
| } | ||
| }); | ||
| } catch (insertError) { | ||
| await db.delete(workspaces).where(eq(workspaces.id, workspace.id)); | ||
| throw insertError; |
There was a problem hiding this comment.
Make workspace creation and initial item inserts atomic.
The workspace row is committed before this item transaction starts, then a compensating delete tries to clean up failures. If the process dies or the delete fails after an item insert error, an empty workspace can remain and Zero may replicate the transient row. Move the workspace insert and item inserts into the same db.transaction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/workspaces/route.ts` around lines 111 - 123, The workspace row is
currently inserted before the item-insert transaction, so failures can leave an
empty workspace; move the workspace insert into the same db.transaction that
inserts initialItems. Specifically, start a single db.transaction (use the same
tx passed into insertWorkspaceItem) and perform the workspace creation and then
loop to call insertWorkspaceItem(tx, {...}) for each initial item inside that
transaction, then remove the compensating db.delete(workspaces)... throw
insertError cleanup since the transaction will roll back on error. Ensure you
reference the existing functions/objects: db.transaction, insertWorkspaceItem,
and the workspaces/workspace creation logic so both operations run atomically.
| const zeroConnectionState = useConnectionState(); | ||
| const isSaving = zeroConnectionState.name === "connecting"; |
There was a problem hiding this comment.
isSaving no longer reflects actual save progress.
Deriving isSaving from zeroConnectionState.name === "connecting" conflates transport state with mutation state:
- Initial page load and every reconnect will briefly flash the "saving" indicator even when nothing is dirty.
- While offline (
disconnected/error), local mutations are queued by Zero but the UI will display "not saving", which is the opposite of the user expectation.
Consider wiring this to pending mutation count from Zero (or simply dropping the save indicator if Zero's local-first model makes it meaningless) rather than to connection state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/dashboard/page.tsx` around lines 86 - 87, isSaving is incorrectly
derived from zeroConnectionState.name === "connecting", causing false positives
on reconnect and false negatives when local mutations are queued; update the
logic to reflect pending local mutations instead of transport state by reading
Zero's pending-mutation metric (e.g. zero.pendingMutationCount or
zeroConnectionState.pendingMutations) and set isSaving = pendingCount > 0 (or
similar), or if the Zero API doesn't expose a pending count, remove the save
indicator entirely; locate the usage around zeroConnectionState and isSaving in
page.tsx and replace the connection-state-based check with a
pending-mutation-based check or drop the indicator.
| const connectionStatus = useMemo<RealtimeContextType["connectionStatus"]>( | ||
| () => { | ||
| switch (zeroConnectionState.name) { | ||
| case "connected": | ||
| return "connected"; | ||
| case "connecting": | ||
| return "connecting"; | ||
| default: | ||
| return "disconnected"; | ||
| } | ||
| }, | ||
| [zeroConnectionState], | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What connection state names can @rocicorp/zero/react useConnectionState return, and what do error and needs-auth mean in the official Rocicorp Zero docs?
💡 Result:
The @rocicorp/zero/react hook useConnectionState returns the current connection state of the Zero instance, with possible state names: 'connecting', 'connected', 'disconnected', 'error', 'needs-auth', and 'closed'. In the official Rocicorp Zero docs: - 'error' occurs if zero-cache crashes or if the mutate or query endpoints return a network or HTTP error (excluding 401/403). Reads are allowed, but writes are rejected. The state includes a 'reason' string. - 'needs-auth' occurs if the mutate or query endpoints return a 401 or 403 status code (indicating unauthorized access). Reads are allowed, but writes are rejected. The state includes a 'reason' object. Re-authenticate the user and call zero.connection.connect (optionally with new auth token) to reconnect.
Citations:
- 1: https://zero.rocicorp.dev/docs/connection
- 2: https://zero.rocicorp.dev/docs/offline
- 3: https://www.mintlify.com/rocicorp/mono/api/client/hooks
🏁 Script executed:
# First, find and inspect the RealtimeContext.tsx file
find . -name "RealtimeContext.tsx" -type fRepository: ThinkEx-OSS/thinkex
Length of output: 97
🏁 Script executed:
# Read the specific file to see the full context including type definitions
cat -n src/contexts/RealtimeContext.tsx | head -80Repository: ThinkEx-OSS/thinkex
Length of output: 2725
Map Zero's error and needs-auth states to the "error" status instead of collapsing them to "disconnected".
Zero's useConnectionState returns error and needs-auth as distinct states that both reject writes. Mapping them to "disconnected" hides actionable failures from the UI: needs-auth requires explicit user re-authentication, while error indicates a transient zero-cache or backend failure. The RealtimeContextType type already defines an "error" status for this purpose, but the current switch statement never returns it.
Also, the dependency array should be [zeroConnectionState.name] instead of [zeroConnectionState] to avoid unnecessary re-memoization when the state object instance changes.
🔧 Proposed fix
const connectionStatus = useMemo<RealtimeContextType["connectionStatus"]>(
() => {
switch (zeroConnectionState.name) {
case "connected":
return "connected";
case "connecting":
return "connecting";
+ case "disconnected":
+ return "disconnected";
+ case "error":
+ case "needs-auth":
+ return "error";
default:
return "disconnected";
}
},
- [zeroConnectionState],
+ [zeroConnectionState.name],
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/contexts/RealtimeContext.tsx` around lines 42 - 54, Update the useMemo
that computes connectionStatus so it maps zeroConnectionState.name values
"error" and "needs-auth" to the RealtimeContextType "error" status instead of
returning "disconnected", and change the memo dependency from
[zeroConnectionState] to [zeroConnectionState.name]; locate the memo block that
defines connectionStatus (the useMemo referencing zeroConnectionState) and
adjust the switch to include cases "error" and "needs-auth" returning "error"
while leaving "connected" and "connecting" as-is.
| await tx | ||
| .update(workspaceItems) | ||
| .set({ | ||
| type: rows.item.type, | ||
| name: rows.item.name, | ||
| subtitle: rows.item.subtitle, | ||
| color: rows.item.color, | ||
| folderId: rows.item.folderId, | ||
| layout: rows.item.layout, | ||
| lastModified: rows.item.lastModified, | ||
| sourceVersion: rows.item.sourceVersion, | ||
| dataSchemaVersion: rows.item.dataSchemaVersion, | ||
| contentHash: rows.item.contentHash, | ||
| processingStatus: rows.item.processingStatus, | ||
| hasOcr: rows.item.hasOcr, | ||
| ocrStatus: rows.item.ocrStatus, | ||
| ocrPageCount: rows.item.ocrPageCount, | ||
| hasTranscript: rows.item.hasTranscript, | ||
| sourceCount: rows.item.sourceCount, | ||
| updatedAt: new Date().toISOString(), | ||
| }) | ||
| .where( | ||
| and( | ||
| eq(workspaceItems.workspaceId, params.workspaceId), | ||
| eq(workspaceItems.itemId, rows.item.itemId), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Make upsertWorkspaceItem() upsert the shell row too.
This updates workspaceItems but never inserts it when the shell row is missing. The following content/extracted upserts can then create orphan rows that workspace reads ignore because loadWorkspaceItemRecord() returns null without a shell.
🐛 Proposed fix
- await tx
- .update(workspaceItems)
- .set({
+ await tx
+ .insert(workspaceItems)
+ .values(rows.item)
+ .onConflictDoUpdate({
+ target: [workspaceItems.workspaceId, workspaceItems.itemId],
+ set: {
type: rows.item.type,
name: rows.item.name,
subtitle: rows.item.subtitle,
color: rows.item.color,
folderId: rows.item.folderId,
layout: rows.item.layout,
lastModified: rows.item.lastModified,
sourceVersion: rows.item.sourceVersion,
dataSchemaVersion: rows.item.dataSchemaVersion,
contentHash: rows.item.contentHash,
processingStatus: rows.item.processingStatus,
hasOcr: rows.item.hasOcr,
ocrStatus: rows.item.ocrStatus,
ocrPageCount: rows.item.ocrPageCount,
hasTranscript: rows.item.hasTranscript,
sourceCount: rows.item.sourceCount,
updatedAt: new Date().toISOString(),
- })
- .where(
- and(
- eq(workspaceItems.workspaceId, params.workspaceId),
- eq(workspaceItems.itemId, rows.item.itemId),
- ),
- );
+ },
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await tx | |
| .update(workspaceItems) | |
| .set({ | |
| type: rows.item.type, | |
| name: rows.item.name, | |
| subtitle: rows.item.subtitle, | |
| color: rows.item.color, | |
| folderId: rows.item.folderId, | |
| layout: rows.item.layout, | |
| lastModified: rows.item.lastModified, | |
| sourceVersion: rows.item.sourceVersion, | |
| dataSchemaVersion: rows.item.dataSchemaVersion, | |
| contentHash: rows.item.contentHash, | |
| processingStatus: rows.item.processingStatus, | |
| hasOcr: rows.item.hasOcr, | |
| ocrStatus: rows.item.ocrStatus, | |
| ocrPageCount: rows.item.ocrPageCount, | |
| hasTranscript: rows.item.hasTranscript, | |
| sourceCount: rows.item.sourceCount, | |
| updatedAt: new Date().toISOString(), | |
| }) | |
| .where( | |
| and( | |
| eq(workspaceItems.workspaceId, params.workspaceId), | |
| eq(workspaceItems.itemId, rows.item.itemId), | |
| ), | |
| ); | |
| await tx | |
| .insert(workspaceItems) | |
| .values(rows.item) | |
| .onConflictDoUpdate({ | |
| target: [workspaceItems.workspaceId, workspaceItems.itemId], | |
| set: { | |
| type: rows.item.type, | |
| name: rows.item.name, | |
| subtitle: rows.item.subtitle, | |
| color: rows.item.color, | |
| folderId: rows.item.folderId, | |
| layout: rows.item.layout, | |
| lastModified: rows.item.lastModified, | |
| sourceVersion: rows.item.sourceVersion, | |
| dataSchemaVersion: rows.item.dataSchemaVersion, | |
| contentHash: rows.item.contentHash, | |
| processingStatus: rows.item.processingStatus, | |
| hasOcr: rows.item.hasOcr, | |
| ocrStatus: rows.item.ocrStatus, | |
| ocrPageCount: rows.item.ocrPageCount, | |
| hasTranscript: rows.item.hasTranscript, | |
| sourceCount: rows.item.sourceCount, | |
| updatedAt: new Date().toISOString(), | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/workspace/workspace-item-write.ts` around lines 151 - 177, The
current upsertWorkspaceItem() only performs an UPDATE (the
tx.update(...).set(...) block) so if the shell row is missing it never INSERTs
and later content/extracted upserts produce orphan rows; change
upsertWorkspaceItem() to perform an upsert: after the UPDATE attempt, check
whether any row was affected and if zero, INSERT a shell row into workspaceItems
with at minimum workspaceId, itemId, createdAt/updatedAt and any required
non-null columns (type/name/folderId as appropriate) or use a single INSERT ...
ON CONFLICT (workspaceId, itemId) DO UPDATE to atomically create-or-update the
shell; ensure loadWorkspaceItemRecord() will then find the shell row.
| const appURL = | ||
| process.env.NEXT_PUBLIC_APP_URL ?? "http://localhost:3000"; | ||
|
|
||
| function createZeroInstance(params: { userId: string }) { | ||
| return new Zero({ | ||
| schema, | ||
| cacheURL: process.env.NEXT_PUBLIC_ZERO_SERVER!, | ||
| mutateURL: `${appURL}/api/zero/mutate`, | ||
| queryURL: `${appURL}/api/zero/query`, | ||
| userID: params.userId, | ||
| context: { | ||
| userId: params.userId, | ||
| }, | ||
| mutators, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fail fast on missing NEXT_PUBLIC_ZERO_SERVER and avoid localhost fallback in production.
- Line 15 uses a non-null assertion on
process.env.NEXT_PUBLIC_ZERO_SERVER. If the env var is absent (e.g., misconfigured Vercel deploy),cacheURLbecomesundefinedand you'll get a cryptic downstream error from@rocicorp/zeroinstead of a clear configuration failure. - The
appURLlocalhost fallback on line 10 will silently produce broken mutate/query URLs in a production build ifNEXT_PUBLIC_APP_URLisn't baked in. Given the PR's deployment notes list these as required, a hard check protects against regressions.
🛡️ Proposed fix
-const appURL =
- process.env.NEXT_PUBLIC_APP_URL ?? "http://localhost:3000";
+const appURL =
+ process.env.NEXT_PUBLIC_APP_URL ??
+ (process.env.NODE_ENV === "production"
+ ? (() => {
+ throw new Error("NEXT_PUBLIC_APP_URL is required in production");
+ })()
+ : "http://localhost:3000");
+
+const cacheURL = process.env.NEXT_PUBLIC_ZERO_SERVER;
+if (!cacheURL) {
+ throw new Error("NEXT_PUBLIC_ZERO_SERVER is required");
+}
function createZeroInstance(params: { userId: string }) {
return new Zero({
schema,
- cacheURL: process.env.NEXT_PUBLIC_ZERO_SERVER!,
+ cacheURL,
mutateURL: `${appURL}/api/zero/mutate`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const appURL = | |
| process.env.NEXT_PUBLIC_APP_URL ?? "http://localhost:3000"; | |
| function createZeroInstance(params: { userId: string }) { | |
| return new Zero({ | |
| schema, | |
| cacheURL: process.env.NEXT_PUBLIC_ZERO_SERVER!, | |
| mutateURL: `${appURL}/api/zero/mutate`, | |
| queryURL: `${appURL}/api/zero/query`, | |
| userID: params.userId, | |
| context: { | |
| userId: params.userId, | |
| }, | |
| mutators, | |
| }); | |
| } | |
| const appURL = | |
| process.env.NEXT_PUBLIC_APP_URL ?? | |
| (process.env.NODE_ENV === "production" | |
| ? (() => { | |
| throw new Error("NEXT_PUBLIC_APP_URL is required in production"); | |
| })() | |
| : "http://localhost:3000"); | |
| const cacheURL = process.env.NEXT_PUBLIC_ZERO_SERVER; | |
| if (!cacheURL) { | |
| throw new Error("NEXT_PUBLIC_ZERO_SERVER is required"); | |
| } | |
| function createZeroInstance(params: { userId: string }) { | |
| return new Zero({ | |
| schema, | |
| cacheURL, | |
| mutateURL: `${appURL}/api/zero/mutate`, | |
| queryURL: `${appURL}/api/zero/query`, | |
| userID: params.userId, | |
| context: { | |
| userId: params.userId, | |
| }, | |
| mutators, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/zero/client.ts` around lines 9 - 24, The createZeroInstance function
currently uses a non-null assertion for process.env.NEXT_PUBLIC_ZERO_SERVER and
a localhost fallback for appURL; change it to validate required env vars
up-front: throw a clear Error if NEXT_PUBLIC_ZERO_SERVER is missing, and if
NODE_ENV === "production" (or when deploying) also require NEXT_PUBLIC_APP_URL
instead of falling back to "http://localhost:3000"; use those validated values
for Zero({ cacheURL, mutateURL, queryURL, ... }) so cacheURL, mutateURL and
queryURL are never undefined and produce an explicit configuration error instead
of downstream cryptic failures.
| const workspaceItems = table("workspace_items") | ||
| .from("workspace_items") | ||
| .columns({ | ||
| workspaceId: string().from("workspace_id"), | ||
| itemId: string().from("item_id"), | ||
| type: string(), | ||
| name: string(), | ||
| subtitle: string(), | ||
| color: string().optional(), | ||
| folderId: string().from("folder_id").optional(), | ||
| layout: json().optional(), | ||
| lastModified: number().from("last_modified").optional(), | ||
| sourceVersion: number().from("source_version"), | ||
| dataSchemaVersion: number().from("data_schema_version"), | ||
| contentHash: string().from("content_hash"), | ||
| processingStatus: string().from("processing_status").optional(), | ||
| hasOcr: boolean().from("has_ocr"), | ||
| ocrStatus: string().from("ocr_status").optional(), | ||
| ocrPageCount: number().from("ocr_page_count"), | ||
| hasTranscript: boolean().from("has_transcript"), | ||
| sourceCount: number().from("source_count"), | ||
| createdAt: number().from("created_at").optional(), | ||
| }) | ||
| .primaryKey("workspaceId", "itemId"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the database column type and find all Zero/client usages of createdAt/updatedAt.
set -euo pipefail
echo "Database timestamp columns:"
fd '\.sql$' drizzle -x rg -n '"created_at"|"updated_at"' {}
echo
echo "Zero schema/client timestamp usages:"
rg -n -C3 'createdAt|updatedAt|created_at|updated_at|number\(\)\.from\("created_at"\)' --type=ts --type=tsxRepository: ThinkEx-OSS/thinkex
Length of output: 2094
🌐 Web query:
Rocicorp Zero schema PostgreSQL timestamp with time zone column type mapping number string created_at
💡 Result:
In Rocicorp Zero, a PostgreSQL timestamp with time zone (timestamptz) column maps to the number type in the Zero schema definition. The resulting JavaScript/TypeScript type is number. This applies to date, timestamp, timestamptz, time, and timetz Postgres types. For a created_at column of type timestamptz, define it in your Zero schema as created_at: number. The value is a numeric timestamp, likely milliseconds since Unix epoch.
Citations:
- 1: https://zero.rocicorp.dev/docs/postgres-support
- 2: https://github.com/rocicorp/zero-docs/blob/main/contents/docs/postgres-support.mdx
🏁 Script executed:
# Find the migration file that defines workspace_items table
fd '\.sql$' drizzle -x grep -l 'workspace_items' {} | head -5Repository: ThinkEx-OSS/thinkex
Length of output: 132
🏁 Script executed:
# Once found, show the exact column definitions for workspace_items
fd '\.sql$' drizzle -x grep -A 30 'CREATE TABLE.*workspace_items' {}Repository: ThinkEx-OSS/thinkex
Length of output: 2377
Remove .optional() from createdAt schema definition.
Line 35 maps createdAt as number().optional(), but the database defines created_at as timestamp with time zone DEFAULT now() NOT NULL. The number() type is correct for this column, but .optional() contradicts the NOT NULL constraint and weakens the generated row typing. Use number().from("created_at") instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/zero/zero-schema.gen.ts` around lines 14 - 37, The createdAt column
in the workspaceItems table schema is marked optional but the DB column is NOT
NULL with a default, so update the workspaceItems table definition to remove
.optional() and use number().from("created_at") for the createdAt field (modify
the createdAt property in the workspaceItems .columns call).
| const [contentRow] = await tx | ||
| .select({ | ||
| assetData: workspaceItemContent.assetData, | ||
| }) | ||
| .from(workspaceItemContent) | ||
| .where( | ||
| and( | ||
| eq(workspaceItemContent.workspaceId, workspaceId), | ||
| eq(workspaceItemContent.itemId, itemId), | ||
| ), | ||
| ) | ||
| .limit(1); | ||
|
|
||
| const currentAssetData = | ||
| (contentRow?.assetData as Record<string, unknown> | null) ?? {}; | ||
|
|
||
| await tx | ||
| .update(workspaceItemContent) | ||
| .set({ | ||
| structuredData: { | ||
| summary: result.summary ?? "", | ||
| }, | ||
| ...(typeof result.duration === "number" && result.duration > 0 | ||
| ? { | ||
| assetData: { | ||
| ...currentAssetData, | ||
| duration: result.duration, | ||
| }, | ||
| } | ||
| : {}), | ||
| updatedAt, | ||
| }) | ||
| .where( | ||
| and( | ||
| eq(workspaceItemContent.workspaceId, workspaceId), | ||
| eq(workspaceItemContent.itemId, itemId), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Read-modify-write on assetData is not atomic — prefer JSONB merge in SQL.
The SELECT at lines 66-77 then UPDATE at lines 82-103 form a non-atomic read-modify-write on workspace_item_content.assetData. Under PostgreSQL's default READ COMMITTED isolation, another transaction committing between the SELECT and UPDATE can have its assetData keys silently clobbered. Wrapping in db.transaction does not lock the row on a plain SELECT.
Audio transcription today may be the sole writer to this row's assetData, but if any other workflow (e.g., an "update duration" worker, user rename that touches metadata, re-transcribe) writes concurrently, keys will be lost. Using JSONB's concat operator does the merge in one statement and is race-free:
🛡️ Proposed fix using `sql` template
- const [contentRow] = await tx
- .select({
- assetData: workspaceItemContent.assetData,
- })
- .from(workspaceItemContent)
- .where(
- and(
- eq(workspaceItemContent.workspaceId, workspaceId),
- eq(workspaceItemContent.itemId, itemId),
- ),
- )
- .limit(1);
-
- const currentAssetData =
- (contentRow?.assetData as Record<string, unknown> | null) ?? {};
-
await tx
.update(workspaceItemContent)
.set({
structuredData: {
summary: result.summary ?? "",
},
...(typeof result.duration === "number" && result.duration > 0
? {
- assetData: {
- ...currentAssetData,
- duration: result.duration,
- },
+ assetData: sql`COALESCE(${workspaceItemContent.assetData}, '{}'::jsonb) || ${JSON.stringify({ duration: result.duration })}::jsonb`,
}
: {}),
updatedAt,
})Also: the result.segments as unknown as Record<string, unknown> cast on lines 30-33 and 43-46 is a type lie (segments is an array). JSONB accepts the array fine at runtime, but consider a truthful cast (e.g. to unknown) or typing the column with a matching interface.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [contentRow] = await tx | |
| .select({ | |
| assetData: workspaceItemContent.assetData, | |
| }) | |
| .from(workspaceItemContent) | |
| .where( | |
| and( | |
| eq(workspaceItemContent.workspaceId, workspaceId), | |
| eq(workspaceItemContent.itemId, itemId), | |
| ), | |
| ) | |
| .limit(1); | |
| const currentAssetData = | |
| (contentRow?.assetData as Record<string, unknown> | null) ?? {}; | |
| await tx | |
| .update(workspaceItemContent) | |
| .set({ | |
| structuredData: { | |
| summary: result.summary ?? "", | |
| }, | |
| ...(typeof result.duration === "number" && result.duration > 0 | |
| ? { | |
| assetData: { | |
| ...currentAssetData, | |
| duration: result.duration, | |
| }, | |
| } | |
| : {}), | |
| updatedAt, | |
| }) | |
| .where( | |
| and( | |
| eq(workspaceItemContent.workspaceId, workspaceId), | |
| eq(workspaceItemContent.itemId, itemId), | |
| ), | |
| ); | |
| await tx | |
| .update(workspaceItemContent) | |
| .set({ | |
| structuredData: { | |
| summary: result.summary ?? "", | |
| }, | |
| ...(typeof result.duration === "number" && result.duration > 0 | |
| ? { | |
| assetData: sql`COALESCE(${workspaceItemContent.assetData}, '{}'::jsonb) || ${JSON.stringify({ duration: result.duration })}::jsonb`, | |
| } | |
| : {}), | |
| updatedAt, | |
| }) | |
| .where( | |
| and( | |
| eq(workspaceItemContent.workspaceId, workspaceId), | |
| eq(workspaceItemContent.itemId, itemId), | |
| ), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/workflows/audio-transcribe/steps/persist-result.ts` around lines 66 -
103, The code performs a non-atomic read-modify-write: you call tx.select(...)
to read workspaceItemContent.assetData into currentAssetData then
tx.update(workspaceItemContent).set(...) to write back merged assetData, which
can lose concurrent updates; instead remove the SELECT and perform a single
UPDATE that merges JSONB on the DB side (use PostgreSQL jsonb
concatenation/merge via an sql template or query builder to set assetData =
workspace_item_content.asset_data || '{"duration": ...}'::jsonb when
result.duration is present), keep setting structuredData.summary and updatedAt
in the same UPDATE, and only conditionally include the jsonb merge when
result.duration > 0; also fix the incorrect cast of result.segments (don’t cast
segments to Record<string, unknown> — use a truthful type such as unknown or the
actual array type) so the value stored in assetData/columns matches its real
type.
| for (const result of results) { | ||
| const ocrPages = result.ok ? result.pages : []; | ||
| const ocrText = getOcrPagesTextContent(ocrPages) || null; | ||
| const updatedAt = new Date().toISOString(); | ||
|
|
||
| await db | ||
| .insert(workspaceItemExtracted) | ||
| .values({ | ||
| workspaceId, | ||
| itemId: result.itemId, | ||
| searchText: ocrText ?? "", | ||
| ocrText, | ||
| ocrPages: ocrPages as unknown as Record<string, unknown>, | ||
| updatedAt: new Date().toISOString(), | ||
| }) | ||
| .onConflictDoUpdate({ | ||
| target: [ | ||
| workspaceItemExtracted.workspaceId, | ||
| workspaceItemExtracted.itemId, | ||
| ], | ||
| set: { | ||
| await db.transaction(async (tx) => { | ||
| await tx | ||
| .insert(workspaceItemExtracted) | ||
| .values({ | ||
| workspaceId, | ||
| itemId: result.itemId, | ||
| searchText: ocrText ?? "", | ||
| ocrText, | ||
| ocrPages: ocrPages as unknown as Record<string, unknown>, | ||
| searchText: ocrText ?? "", | ||
| updatedAt: new Date().toISOString(), | ||
| }, | ||
| }); | ||
| } | ||
| updatedAt, | ||
| }) | ||
| .onConflictDoUpdate({ | ||
| target: [ | ||
| workspaceItemExtracted.workspaceId, | ||
| workspaceItemExtracted.itemId, | ||
| ], | ||
| set: { | ||
| ocrText, | ||
| ocrPages: ocrPages as unknown as Record<string, unknown>, | ||
| searchText: ocrText ?? "", | ||
| updatedAt, | ||
| }, | ||
| }); | ||
|
|
||
| const event = createEvent( | ||
| "BULK_ITEMS_PATCHED", | ||
| { | ||
| updates: results.map((result) => { | ||
| const statusPatch = ( | ||
| result.ok | ||
| ? { | ||
| ocrStatus: "complete" as const, | ||
| ocrError: undefined, | ||
| } | ||
| : { | ||
| ocrStatus: "failed" as const, | ||
| ocrError: result.error, | ||
| } | ||
| ) satisfies Partial<PdfData> | Partial<ImageData>; | ||
| await tx | ||
| .update(workspaceItems) | ||
| .set({ | ||
| ocrStatus: result.ok ? "complete" : "failed", | ||
| hasOcr: result.ok && ocrPages.length > 0, | ||
| ocrPageCount: ocrPages.length, | ||
| lastModified: Date.now(), | ||
| }) | ||
| .where( | ||
| and( | ||
| eq(workspaceItems.workspaceId, workspaceId), | ||
| eq(workspaceItems.itemId, result.itemId), | ||
| ), | ||
| ); | ||
|
|
||
| return { | ||
| id: result.itemId, | ||
| changes: { | ||
| data: statusPatch as Item["data"], | ||
| }, | ||
| }; | ||
| }), | ||
| }, | ||
| userId, | ||
| ); | ||
| const [contentRow] = await tx | ||
| .select({ | ||
| assetData: workspaceItemContent.assetData, | ||
| }) | ||
| .from(workspaceItemContent) | ||
| .where( | ||
| and( | ||
| eq(workspaceItemContent.workspaceId, workspaceId), | ||
| eq(workspaceItemContent.itemId, result.itemId), | ||
| ), | ||
| ) | ||
| .limit(1); | ||
|
|
||
| const currentAssetData = | ||
| (contentRow?.assetData as Record<string, unknown> | null) ?? {}; | ||
| const nextAssetData = result.ok | ||
| ? (() => { | ||
| const { ocrError: _ocrError, ...rest } = currentAssetData; | ||
| return rest; | ||
| })() | ||
| : { | ||
| ...currentAssetData, | ||
| ocrError: result.error, | ||
| }; | ||
|
|
||
| await appendWorkspaceEventOrThrow({ | ||
| workspaceId, | ||
| event, | ||
| conflictMessage: `Version conflict appending event ${event.id} to workspace ${workspaceId}. Workflow will retry automatically.`, | ||
| }); | ||
| await tx | ||
| .update(workspaceItemContent) | ||
| .set({ | ||
| assetData: nextAssetData, | ||
| updatedAt, | ||
| }) | ||
| .where( | ||
| and( | ||
| eq(workspaceItemContent.workspaceId, workspaceId), | ||
| eq(workspaceItemContent.itemId, result.itemId), | ||
| ), | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Lost-update race on assetData and N sequential transactions.
Two related issues in this loop:
-
Read-modify-write race on
assetData. TheSELECTat Lines 62–73 doesn't lock the row (defaultREAD COMMITTED), so a concurrent writer touchingworkspaceItemContent.assetDatafor the same(workspaceId, itemId)between the read and theUPDATEat Lines 87–98 will have its change silently clobbered. This is reachable because AI tools / audio / re-OCR flows also write directly toworkspaceItemContent(per PR objectives). Either add.for("update")on the select, or perform the merge at the SQL level withjsonb_set/ the-operator so the write is self-contained. -
Per-result transaction and non-atomic batch. Wrapping each
resultin its owndb.transaction(...)means a partial failure leaves some items persisted and others not. It also multiplies round-trip cost for large OCR batches. Consider a single outer transaction for the batch (or, if per-item isolation is desired, document that explicitly and make the workflow idempotent on retry).
🔧 Sketch for point 1 (row-level lock)
- const [contentRow] = await tx
- .select({
- assetData: workspaceItemContent.assetData,
- })
- .from(workspaceItemContent)
- .where(
- and(
- eq(workspaceItemContent.workspaceId, workspaceId),
- eq(workspaceItemContent.itemId, result.itemId),
- ),
- )
- .limit(1);
+ const [contentRow] = await tx
+ .select({
+ assetData: workspaceItemContent.assetData,
+ })
+ .from(workspaceItemContent)
+ .where(
+ and(
+ eq(workspaceItemContent.workspaceId, workspaceId),
+ eq(workspaceItemContent.itemId, result.itemId),
+ ),
+ )
+ .limit(1)
+ .for("update");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const result of results) { | |
| const ocrPages = result.ok ? result.pages : []; | |
| const ocrText = getOcrPagesTextContent(ocrPages) || null; | |
| const updatedAt = new Date().toISOString(); | |
| await db | |
| .insert(workspaceItemExtracted) | |
| .values({ | |
| workspaceId, | |
| itemId: result.itemId, | |
| searchText: ocrText ?? "", | |
| ocrText, | |
| ocrPages: ocrPages as unknown as Record<string, unknown>, | |
| updatedAt: new Date().toISOString(), | |
| }) | |
| .onConflictDoUpdate({ | |
| target: [ | |
| workspaceItemExtracted.workspaceId, | |
| workspaceItemExtracted.itemId, | |
| ], | |
| set: { | |
| await db.transaction(async (tx) => { | |
| await tx | |
| .insert(workspaceItemExtracted) | |
| .values({ | |
| workspaceId, | |
| itemId: result.itemId, | |
| searchText: ocrText ?? "", | |
| ocrText, | |
| ocrPages: ocrPages as unknown as Record<string, unknown>, | |
| searchText: ocrText ?? "", | |
| updatedAt: new Date().toISOString(), | |
| }, | |
| }); | |
| } | |
| updatedAt, | |
| }) | |
| .onConflictDoUpdate({ | |
| target: [ | |
| workspaceItemExtracted.workspaceId, | |
| workspaceItemExtracted.itemId, | |
| ], | |
| set: { | |
| ocrText, | |
| ocrPages: ocrPages as unknown as Record<string, unknown>, | |
| searchText: ocrText ?? "", | |
| updatedAt, | |
| }, | |
| }); | |
| const event = createEvent( | |
| "BULK_ITEMS_PATCHED", | |
| { | |
| updates: results.map((result) => { | |
| const statusPatch = ( | |
| result.ok | |
| ? { | |
| ocrStatus: "complete" as const, | |
| ocrError: undefined, | |
| } | |
| : { | |
| ocrStatus: "failed" as const, | |
| ocrError: result.error, | |
| } | |
| ) satisfies Partial<PdfData> | Partial<ImageData>; | |
| await tx | |
| .update(workspaceItems) | |
| .set({ | |
| ocrStatus: result.ok ? "complete" : "failed", | |
| hasOcr: result.ok && ocrPages.length > 0, | |
| ocrPageCount: ocrPages.length, | |
| lastModified: Date.now(), | |
| }) | |
| .where( | |
| and( | |
| eq(workspaceItems.workspaceId, workspaceId), | |
| eq(workspaceItems.itemId, result.itemId), | |
| ), | |
| ); | |
| return { | |
| id: result.itemId, | |
| changes: { | |
| data: statusPatch as Item["data"], | |
| }, | |
| }; | |
| }), | |
| }, | |
| userId, | |
| ); | |
| const [contentRow] = await tx | |
| .select({ | |
| assetData: workspaceItemContent.assetData, | |
| }) | |
| .from(workspaceItemContent) | |
| .where( | |
| and( | |
| eq(workspaceItemContent.workspaceId, workspaceId), | |
| eq(workspaceItemContent.itemId, result.itemId), | |
| ), | |
| ) | |
| .limit(1); | |
| const currentAssetData = | |
| (contentRow?.assetData as Record<string, unknown> | null) ?? {}; | |
| const nextAssetData = result.ok | |
| ? (() => { | |
| const { ocrError: _ocrError, ...rest } = currentAssetData; | |
| return rest; | |
| })() | |
| : { | |
| ...currentAssetData, | |
| ocrError: result.error, | |
| }; | |
| await appendWorkspaceEventOrThrow({ | |
| workspaceId, | |
| event, | |
| conflictMessage: `Version conflict appending event ${event.id} to workspace ${workspaceId}. Workflow will retry automatically.`, | |
| }); | |
| await tx | |
| .update(workspaceItemContent) | |
| .set({ | |
| assetData: nextAssetData, | |
| updatedAt, | |
| }) | |
| .where( | |
| and( | |
| eq(workspaceItemContent.workspaceId, workspaceId), | |
| eq(workspaceItemContent.itemId, result.itemId), | |
| ), | |
| ); | |
| }); | |
| } | |
| for (const result of results) { | |
| const ocrPages = result.ok ? result.pages : []; | |
| const ocrText = getOcrPagesTextContent(ocrPages) || null; | |
| const updatedAt = new Date().toISOString(); | |
| await db.transaction(async (tx) => { | |
| await tx | |
| .insert(workspaceItemExtracted) | |
| .values({ | |
| workspaceId, | |
| itemId: result.itemId, | |
| searchText: ocrText ?? "", | |
| ocrText, | |
| ocrPages: ocrPages as unknown as Record<string, unknown>, | |
| updatedAt, | |
| }) | |
| .onConflictDoUpdate({ | |
| target: [ | |
| workspaceItemExtracted.workspaceId, | |
| workspaceItemExtracted.itemId, | |
| ], | |
| set: { | |
| ocrText, | |
| ocrPages: ocrPages as unknown as Record<string, unknown>, | |
| searchText: ocrText ?? "", | |
| updatedAt, | |
| }, | |
| }); | |
| await tx | |
| .update(workspaceItems) | |
| .set({ | |
| ocrStatus: result.ok ? "complete" : "failed", | |
| hasOcr: result.ok && ocrPages.length > 0, | |
| ocrPageCount: ocrPages.length, | |
| lastModified: Date.now(), | |
| }) | |
| .where( | |
| and( | |
| eq(workspaceItems.workspaceId, workspaceId), | |
| eq(workspaceItems.itemId, result.itemId), | |
| ), | |
| ); | |
| const [contentRow] = await tx | |
| .select({ | |
| assetData: workspaceItemContent.assetData, | |
| }) | |
| .from(workspaceItemContent) | |
| .where( | |
| and( | |
| eq(workspaceItemContent.workspaceId, workspaceId), | |
| eq(workspaceItemContent.itemId, result.itemId), | |
| ), | |
| ) | |
| .limit(1) | |
| .for("update"); | |
| const currentAssetData = | |
| (contentRow?.assetData as Record<string, unknown> | null) ?? {}; | |
| const nextAssetData = result.ok | |
| ? (() => { | |
| const { ocrError: _ocrError, ...rest } = currentAssetData; | |
| return rest; | |
| })() | |
| : { | |
| ...currentAssetData, | |
| ocrError: result.error, | |
| }; | |
| await tx | |
| .update(workspaceItemContent) | |
| .set({ | |
| assetData: nextAssetData, | |
| updatedAt, | |
| }) | |
| .where( | |
| and( | |
| eq(workspaceItemContent.workspaceId, workspaceId), | |
| eq(workspaceItemContent.itemId, result.itemId), | |
| ), | |
| ); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/workflows/ocr-dispatch/steps/persist-results.ts` around lines 18 - 100,
The loop currently does per-result db.transaction calls and reads
workspaceItemContent.assetData without locking, causing a lost-update race;
change to use a single outer db.transaction for the whole results array (replace
per-result db.transaction(...) with one transaction enclosing the loop) and
inside that transaction either (A) change the SELECT from workspaceItemContent
to acquire a row lock (use the query builder's .for("update") on the select of
workspaceItemContent.assetData) before computing nextAssetData, or (B) avoid
read-modify-write entirely by performing the merge in SQL (use jsonb_set or the
jsonb '-' operator to remove/add the ocrError key on
workspaceItemContent.assetData in the UPDATE statement); reference the symbols
workspaceItemContent, assetData, db.transaction, workspaceItemExtracted,
workspaceItems, and use jsonb_set / '-' if choosing the SQL-merge approach.
1 participant
Zero Sync Migration
Migrates workspace state management from event-sourcing to Zero real-time sync.
What changed
useWorkspaceStatehook reads from Zero's local replica via@rocicorp/zero/reactuseQuery— no more API pollingworkspace_items— skipworkspace_item_contententirelyPRs included
Deployment
us-west-2atzero.thinkex.appNEXT_PUBLIC_ZERO_SERVER,ZERO_COOKIE_DOMAINzero_pubonworkspace_items+workspace_item_contentSummary by CodeRabbit
Release Notes
New Features
/api/zero/queryand/api/zero/mutateendpoints for Zero operationsRefactor
Chores
/api/workspaces/[id]/eventsand/api/workspaces/[id]/state)