Content: align database row removal with permissions - #2547
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — generation failedThe visual recap could not be generated for this pull request. This is informational only and does not block the PR. Diagnostic: No plan URL: Repair changed too much of targeted file plan.mdx; expected a localized parser fix. Agent output: Repaired |
This comment has been minimized.
This comment has been minimized.
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 6 potential issues 🔴
Review Details
Code Review Summary
PR #2547 replaces destructive database-row deletion with membership removal, adds effective-access checks, preserves underlying pages and cross-database memberships, and introduces locking around source associations and row cleanup. The overall direction is sound: the server action checks database administration plus row-level viewer access, rejects source-backed rows, performs property cleanup and membership deletion transactionally, and the UI has dedicated permission-aware selection and row-action states. The added tests cover the main batch and permission paths.
Key Findings
- 🔴 HIGH — The remove confirmation flow can submit a different selection than the snapshot used for preview, allowing an intervening selection change to remove unintended rows.
- 🔴 HIGH — Source refresh deletes associations before reseeding under the new lock, leaving a window where a source-backed row can be removed and the refresh can then fail.
- 🟡 MEDIUM — UI bulk and row-level affordances do not consistently include the per-page viewer access required by the actions, so users can be offered operations that are guaranteed to be rejected.
- 🟡 MEDIUM — Property writes are not serialized with membership removal, allowing an already-authorized concurrent write to recreate database-local values after removal.
🧪 Browser testing: Could not verify — the dev server is healthy, but browser executors lacked navigation/click/screenshot tooling; all 16 planned cases were blocked by that environment issue.
| await removeItems.mutateAsync({ | ||
| documentId: databaseDocumentId, | ||
| itemIds: selectedSnapshot.map((item) => item.id), | ||
| itemIds: selectedItemIds, |
There was a problem hiding this comment.
🔴 Use the captured selection for removal
selectedItems is captured before the confirmation closes, but the action receives the live selectedItemIds state. If the selection changes before the mutation runs, the previewed rows and removed rows diverge and the action can remove unintended memberships. Pass a snapshot of the IDs (or prevent selection changes until the mutation completes).
Additional Info
Found by 1 of 4 code-review agents; confirmed against the changed implementation.
| await db.transaction(async (tx) => { | ||
| await lockDatabaseMemberships( |
There was a problem hiding this comment.
🔴 Keep source associations visible during refresh
The refresh path deletes existing source-row associations before seedMockSourceRows acquires the membership lock and reinserts them. A concurrent remove can observe that temporary absence, treat the row as local, and delete it; reseeding then fails after the source association has already been removed. Serialize the entire delete/reseed lifecycle with removal, or retain a durable source-backed marker until refresh commits.
Additional Info
Found by 1 of 4 code-review agents; confirmed against the changed implementation.
| canRemoveSelected: args.removesFavoriteMembership | ||
| ? canEditSelected | ||
| : selectionComplete && | ||
| args.canManageDatabase && |
There was a problem hiding this comment.
🟡 Include row access in bulk capability checks
Bulk Remove is enabled from database management access and source state, but the action also requires viewer access to every selected page. Database access is not inherited by assertAccess for each row, so a collaborator can see an enabled bulk control and receive a guaranteed server denial for a selection containing an inaccessible page. Require the same row-level viewer capability in the UI while retaining the server check.
Additional Info
Found by 1 of 4 code-review agents; confirmed against the changed implementation.
| const canRemoveFromDatabase = removesFavoriteMembership | ||
| ? databaseDocument?.canEdit === true | ||
| : databaseData !== undefined && | ||
| databaseDocument?.canManage === true && |
There was a problem hiding this comment.
🟡 Gate single-row removal on page access
The single-row removal gate checks database management and source state but not viewer access to item.document, while remove-database-items explicitly requires that access. This can show “Remove from database” for a listed page that the collaborator cannot access, only to fail when clicked. Include the row's effective viewer capability in this gate.
Additional Info
Found by 1 of 4 code-review agents; confirmed against the changed implementation.
| const canRemoveFromDatabase = removesFavoriteMembership | ||
| ? databaseDocument?.canEdit === true | ||
| : databaseData !== undefined && | ||
| databaseDocument?.canManage === true && |
There was a problem hiding this comment.
🟡 Match non-table row actions to server authorization
RowActionsCell enables removal using only database management access and source state, but the backing action also requires viewer access to the specific page. List, gallery, board, and other non-table views can therefore expose a removal action that is guaranteed to fail for rows without page-level access. Add the row access check to this shared gate.
Additional Info
Found by 1 of 4 code-review agents; confirmed against the changed implementation.
| ), | ||
| ); | ||
| } | ||
| if (removedItemIds.length > 0) { |
There was a problem hiding this comment.
🟡 Serialize property writes with membership removal
The cleanup and membership deletion are atomic with each other, but not with set-document-property. A property update can pass its membership check before this transaction, then write after the membership and existing values are deleted, leaving an orphaned database-local value that resurfaces if the page is re-added. Use the same membership lock and revalidate membership for database-local property writes, or otherwise share a transaction boundary.
Additional Info
Found by 2 of 4 code-review agents; confirmed against the changed implementation.
2 participants
Summary\n\n- gate single-row and bulk database actions by the viewer's effective access, so viewers and commenters can select rows but only see the selection count and Clear\n- replace database row deletion with Remove from database, preserving the Page, descendants, shares, other database memberships, and unrelated values\n- fail closed for incomplete selections and source-backed rows, and serialize source association changes with removal\n- keep the selection toolbar anchored outside the horizontally scrolling table\n\n## Verification\n\n- Content typecheck passed\n- 148 tests passed across action, source, parity, permission, layout, and error-toast coverage\n- 17 additional materialization and batch-action tests passed\n- independent authorization and concurrency review found no actionable issues\n- browser QA passed viewer, owner, single-row, bulk, preservation, and horizontal-scroll scenarios; no console or network failures\n\n## Notes\n\n- includes the approved shape artifact and a Content changelog entry\n- no feature flag; no merge requested