Make Content database row writes reliable - #2779
Conversation
|
Here's a visual recap of what changed: Open the full interactive recap |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🔴
Review Details
Code Review Summary
PR #2779 consolidates Content database row creation, exact-row updates, and natural-key upserts behind a strict, transactional contract with schema/CAS checks and durable idempotency receipts. The overall direction is sound: validation is centralized, receipts are persisted atomically, and the action error contract is shared across Core and Content. This is high risk because it changes authorization and database mutation semantics.
Key Findings
- 🔴 Row-level authorization is stale across the lock window: update and natural-key upsert paths check the target row before waiting, but do not recheck it immediately before writing inside the transaction.
- 🔴 Receipt replay can expose revoked row data: replay returns stored row snapshots before authorizing the receipt’s document, including create and exact update retries.
- 🔴 Untitled row creation regresses: the table sends its default empty title, while the new action rejects empty strings.
- 🟡 Cross-client refresh is missing: the shared mutation path no longer emits the refresh signal used by other open clients.
- 🟡 Read-back can report a committed write as failure: verification occurs after locks are released and can race with a legitimate subsequent edit.
The migration’s timestamp syntax was reviewed against the repository adapter, which rewrites the SQLite form for PostgreSQL, so no migration finding is included.
🧪 Browser testing: Will run after this review (PR touches UI code)
| return built; | ||
| }), | ||
| ); | ||
| return verifyCommittedResult(result); |
There was a problem hiding this comment.
🟡 Publish the database refresh signal after mutation
The consolidated mutation path returns after commit/readback without emitting the refresh signal that the replaced add/upsert actions published. The initiating UI invalidates its own queries, but other open clients relying on useDbSync() can remain stale after a successful create, update, or upsert. Publish the signal only after an applied commit.
Additional Info
Reported by 1 of 4 reviewers; compared against the replaced action implementations and the shared Content sync contract.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Builder reviewed your changes and has a few items to flag 🟡
Review Details
Incremental Code Review Summary
The latest commit closes three previously reported gaps: the preview-menu E2E fixture now sends the mutation envelope, property deletion re-reads the natural-key pointer while holding the database mutation lock, and the natural-key migration DDL is retry-safe. The broader row mutation implementation continues to provide strict validation, CAS revisions, exact-row authorization, durable idempotency receipts, and shared action error transport.
Four parallel reviewers found no genuinely new actionable defects in the latest changes. One reviewer repeated the older PostgreSQL timestamp concern; it was excluded because it is an existing/stale finding already covered by the migration review and the shared migration runner’s dialect rewrite. The refresh-signal issue remains open and was intentionally not reposted.
The three fixed review threads were resolved before this submission:
- Preview-menu mutation envelope
- Delete-property natural-key lock re-read
- Retry-safe natural-key migration
Risk remains high because this PR changes database mutation and concurrency contracts.
🧪 Browser testing: Will run after this review (PR touches UI code)

Problem
Content's AI and table UI couldn't reliably create one database row, update only the fields a caller named, or find and update a row by a stable business identifier. Unknown or malformed values could be silently skipped or coerced, retrying could create duplicates, and a writer could overwrite changes made after its last read.
That made ordinary workflows such as recording feedback exactly once much harder to trust than they should be.
What changes
get-content-databasenow tells callers exactly which workspace authority, space, database, and backing page they are targeting. It also returns the current schema revision and the stable ID, type, writable status, and accepted shape of every property.Using that contract, Content can now:
One shared mutation path validates the exact table, schema, permissions, and current row, applies one atomic write, and reads the result back inside that transaction. Its receipt includes stable row IDs and link, the target tuple, schema and row revisions, affected fields, payload digest, idempotency result, and verified values.
Two safeguards make concurrency and retries predictable:
Unknown, computed, system, source-managed, Blocks, relation, and malformed values are rejected instead of disappearing or becoming plausible empty data. The existing permissive import/source normalizer remains unchanged. Blocks mutation, relationships, form submission, source sync, whole-database migration, Slack routing, and live table configuration remain outside this PR.
Implementation notes
add-database-item, exact-IDupdate-database-item, andupsert-database-item-by-keyare thin action wrappers over the shared transaction path.Reconciliation with stable Blocks identity
PR #2778 landed first, so this branch was merged with current
mainbefore its final review. The overlap was narrow and both contracts remain intact:Verification
Exact artifact:
main33e390c4374b517cfbc215122efe0a5f713d4dd9→ heade47b413a2d9671db6fe9b8acce780cbf06dd205f.Local prospective-merge-tree checks pass:
git diff --check, all 53 repository guards, and the Content product-impact checker tests.Exact-head GitHub checks pass for the branch-relevant lanes, including Content DB, Content parity, typecheck, build, lint/format, security, core integration, and PostgreSQL row locking/concurrency. The PostgreSQL 17 job initializes the framework and Content schemas and runs the 13 row-mutation tests, including strict values, sparse compare-and-swap updates, authorization and replay privacy, natural-key claims, stale conflicts, and concurrent idempotent retries.
One bounded exact-head review focused only on the conflict resolution and row/identity interaction found no actionable defects. It confirmed the combined deletion transaction and the v81-v84 migration ordering preserve both frozen contracts.
Authenticated local-interface acceptance used task-owned disposable accounts and a disposable SQLite database. It demonstrated contract discovery, an untitled UI row, strict create, exact-ID sparse update with omitted-field preservation, natural-key create and update, idempotent replay with the same stable row, schema and row-revision no-write conflicts, receipt/read-back truth, automatic visibility in a second open client, and denial to unshared identities.
Cleanup read-back returned
not_found; direct SQL found zero fixture documents, database rows, or memberships; the server port closed; and the fixture path was removed. No new fixture was created for the base reconciliation.Acceptance boundaries
Review focus