fix(server): [YW-267] atomic auto-draft dedup — close TOCTOU race

[youhaowei]

Summary

• Wraps the createInsight check-and-insert in a single ctx.db.transaction so two concurrent calls for the same baseTableId always converge on one unmodified draft — no TOCTOU window.
• Removes the client-side read-then-write dedup gate (if existingDraft navigate + return) from createInsightFromTable; the client now reads existing insights only to compute a gap-free numeric suffix for the modified-insight name, then always delegates to the server.
• Adds skipDedup: true option for createInsightFromInsight callers so derived insights are never silently rerouted to an existing unmodified draft for the same baseTableId.

Tracked internally as YW-267.

Test plan

• turbo typecheck — 44/44 tasks pass
• turbo lint — clean
• prettier --check — clean
• apps/server app-artifacts tests — 9/9 pass (4 new dedup contract tests + 1 skipDedup test)
• packages/app useCreateInsight tests — 33/33 pass (TOCTOU simulation, derived-insight skipDedup contract)
• Key concurrency test: Promise.all fires two createInsight calls for the same baseTableId without awaiting the first; both resolve to the same id and exactly one row in the DB

🤖 Generated with Claude Code

Greptile Summary

This PR closes the TOCTOU race in auto-draft deduplication by moving the check-and-insert into a single server-side ctx.db.transaction, and replaces the now-redundant client-side early-return gate with a pure UX read (suffix computation only). The isUnmodifiedDraft predicate is extracted to @dashframe/types as a single source of truth shared by both the renderer hook and the server gate.

• Atomic server dedup: createInsight wraps the look-up + insert in one transaction; concurrent auto-draft calls for the same baseTableId converge on one row via reuseUnmodifiedDraft: true (opt-in, default is always-insert).
• Client dedup gate removed: createInsightFromTable now reads existing insights only to compute gap-free numeric suffixes for the modified-insight path; it no longer short-circuits before reaching the server.
• Shared predicate: isUnmodifiedDraft / InsightDraftShape moved to @dashframe/types, eliminating the previously duplicated client/server logic.

Confidence Score: 5/5

Safe to merge. The transaction wrapping is correct for the single-connection PGLite target and the limitations for multi-connection stores are explicitly documented with a revisit trigger.

The change is well-scoped: the server transaction closes the specific TOCTOU window it targets, the client-side gate removal is backed by the server taking over responsibility, the shared isUnmodifiedDraft predicate eliminates the previously noted drift risk, and the opt-in semantics (reuseUnmodifiedDraft defaults to always-insert) mean callers that omit the flag can never accidentally trigger the dedup path. Test coverage is thorough across sequential, concurrent, and edge-case scenarios.

No files require special attention.

Important Files Changed

Filename	Overview
apps/server/src/functions/app-artifacts.ts	Core fix: wraps check-and-insert in a db transaction with opt-in reuseUnmodifiedDraft; includes inline comments for multi-connection and full-scan caveats.
packages/types/src/insights.ts	Adds shared InsightDraftShape interface and isUnmodifiedDraft function; adds reuseUnmodifiedDraft opt-in to InsightMutations.create options.
packages/app/src/hooks/useCreateInsight.ts	Removes client-side early-return dedup gate; now reads insights only to compute suffix names; delegates all dedup to the server via reuseUnmodifiedDraft.
apps/server/src/functions/app-artifacts.test.ts	Adds 6 new dedup-contract tests (sequential reuse, TOCTOU simulation, modified-draft bypass, pre-populated bypass, no-reuse-flag default, explicit reuse=false) using real PGLite.
packages/app/src/hooks/useCreateInsight.test.tsx	Updates existing dedup test assertions and adds a TOCTOU concurrent-call convergence test.
packages/types/src/index.ts	Exports new InsightDraftShape type and isUnmodifiedDraft function from the types package barrel.
packages/app-data/src/insights.ts	Threads reuseUnmodifiedDraft through the useInsightMutations create hook to match the updated InsightMutations interface.

_{Reviews (3): Last reviewed commit: "docs(insight): record single-connection ..." | Re-trigger Greptile}

[linear-code]

YW-267

[coderabbitai]

📝 Walkthrough

Walkthrough

Deduplication of unmodified draft insights is moved from a client-side early-return in useCreateInsight to a server-side atomic db.transaction in createInsight. A new isUnmodifiedDraft predicate classifies definitions with no user modifications, and a new skipDedup flag forces a fresh row for derived insights. The client hook now uses getAllInsights only to compute UX naming suffixes.

Changes

Atomic auto-draft dedup for createInsight

Layer / File(s)	Summary
skipDedup type contract and data-layer wiring packages/types/src/insights.ts, packages/app-data/src/insights.ts	Adds skipDedup?: boolean to the InsightMutations.create options type and forwards it through useInsightMutations to api.createInsight.
Server-side isUnmodifiedDraft predicate and atomic transaction dedup apps/server/src/functions/app-artifacts.ts	Introduces isUnmodifiedDraft to classify insight definitions, then wraps createInsight in a db.transaction that returns an existing unmodified draft for the same baseTableId or inserts a new row; skipDedup bypasses the reuse path.
Client hook refactored to delegate dedup to server packages/app/src/hooks/useCreateInsight.ts	Removes client-side early-return reuse from createInsightFromTable; getAllInsights() is now used only to compute UX naming suffixes from modified insights. createInsightFromInsight now passes skipDedup: true to always create fresh rows for derived insights.
Server integration and client unit tests apps/server/src/functions/app-artifacts.test.ts, packages/app/src/hooks/useCreateInsight.test.tsx	Adds a full server dedup test suite (sequential reuse, TOCTOU concurrent convergence, non-reuse for modified/pre-populated insights, skipDedup override). Updates client hook tests to assert server-delegated dedup and adds a concurrency scenario asserting two concurrent createInsightFromTable calls converge on one id.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is incomplete and lacks required sections specified in the template, including the 'Changes' bullet points, 'Screenshots' section, and 'Verification' details.	Add a 'Changes' section with bullet points summarizing the key changes, confirm whether there are UI changes with screenshots or state 'No UI change', and provide explicit 'Verification' steps explaining how the changes were tested at runtime.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing a TOCTOU race condition in auto-draft deduplication by making it atomic.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/server/src/functions/app-artifacts.ts`:
- Around line 805-827: The dedup logic in the createInsight function performs a
full table scan by fetching all insight rows with `.all()` and filtering in
JavaScript by baseTableId nested in the unindexed JSONB definition column. While
this is correct and acceptable at current scale (10-100 insights), add a clear
code comment near the tx.from(insights).all() call explaining this known
scalability limitation, noting that if the insights table grows to 1000+ rows,
consider optimizing by either adding baseTableId as a top-level indexed column
or using Postgres JSONB indexing. This documents the concern for future
maintainers to revisit when scale becomes a bottleneck.

🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d9c55a3d-8589-4980-b543-fc3bc4f57dee

📥 Commits

Reviewing files that changed from the base of the PR and between 422f5d7 and 1fb228a.

📒 Files selected for processing (6)

• apps/server/src/functions/app-artifacts.test.ts
• apps/server/src/functions/app-artifacts.ts
• packages/app-data/src/insights.ts
• packages/app/src/hooks/useCreateInsight.test.tsx
• packages/app/src/hooks/useCreateInsight.ts
• packages/types/src/insights.ts