feat: support deleting slides design systems by liamdebeasi · Pull Request #2457 · BuilderIO/agent-native

liamdebeasi · 2026-07-28T14:50:03Z

Adds support for deleting slides design systems.

github-actions · 2026-07-28T14:50:18Z

Here's a visual recap of what changed:

liamdebeasi · 2026-07-28T15:41:14Z

@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why

- Allow admin-role (not just owner) to delete a design system, matching the canManage gating already used by the UI's delete button - Clear designSystemId from decks' JSON data payload on delete, not just the SQL column, so list-decks doesn't fall back to a dangling reference - Replace per-row resolveAccess() calls in list-design-systems with a single batched shares query to avoid N+1 queries

builder-io-integration · 2026-07-28T15:55:46Z

CI just kicked off from the new push; not yet resolved, which is expected.

I reviewed all three review-bot findings on PR #2457, agreed with all of them (verified each against actual code), fixed them, and replied on each comment thread:

Admin delete mismatch (list-design-systems.ts/delete-design-system.ts) — the UI showed Delete to admin-role users but the action required owner, always failing. Fixed by requiring admin (which includes owner), matching the precedent in the content template.
Stale designSystemId in deck JSON (delete-design-system.ts) — confirmed list-decks falls back to data.designSystemId when the column is null, so deleted design systems could keep showing as linked. Fixed by clearing the key from each linked deck's JSON payload inside the same transaction.
N+1 resolveAccess in list (list-design-systems.ts) — replaced per-row resolveAccess() calls with one batched shares query, mirroring the pattern in templates/content/actions/list-documents.ts.

Typecheck passes and relevant existing tests pass. Changes are committed and pushed (52406c3c6) to ld/ds-delete, updating PR #2457; CI just started running.

⠀
🟢 Fixed all three review findings on PR #2457, replied on each comment thread, and pushed to the branch.

builder-io-integration

Builder reviewed your changes and found 4 potential issues 🔴

Review Details

Code Review Summary

This incremental review covers the follow-up commit that replaced per-row access resolution with a batched share lookup and updated deletion to remove designSystemId from both the deck column and JSON payload. The three prior review comments were verified fixed and resolved: admin UI/action permissions now agree, deck payloads are scrubbed, and the list no longer performs an access query per row. The remaining implementation still has concurrency and permission-model consistency risks around shared deck JSON and resource linking. Risk level: High because the action mutates shared resources and authorization-sensitive relationships.

Key Findings

🔴 HIGH — Deletion rewrites full deck JSON without the established per-deck lock, so a concurrent edit can be lost.
🟡 MEDIUM — A deck can be linked to the system after the deletion transaction snapshots linked decks, leaving a dangling deleted-system ID.
🟡 MEDIUM — Batched role lookup uses case-sensitive email equality despite the sharing model being case-insensitive.
🟡 MEDIUM — The design-systems skill still documents owner-only deletion while the action now permits admins.

🧪 Browser testing: Attempted full verification, but all flows were blocked because browser automation tools were unavailable; the dev server was running.

builder-io-integration · 2026-07-28T16:13:03Z

+        .where(eq(schema.decks.designSystemId, id));
+
+      const now = new Date().toISOString();
+      for (const deck of linkedDecks) {


🔴 Preserve concurrent deck edits during deletion

This read-modify-write replaces each linked deck's entire JSON payload without acquiring the withDeckLock used by the other deck writers. If a deck edit commits after the SELECT and before this UPDATE, this stale payload can overwrite the user's newer slides or metadata while removing the design-system ID. Reload and mutate each deck inside the established per-deck lock, or use an atomic update that cannot replace unrelated payload fields.

Additional Info

Confirmed against patch-deck.ts, save-deck.ts, update-slide.ts, and apply-design-system.ts write paths.

Agreed, this was a real bug. The read-modify-write on each linked deck's data JSON ran without the per-deck lock every other deck writer (patch-deck, save-deck, add-slide, update-slide) uses, so a concurrent slide edit landing between the SELECT and UPDATE could get silently overwritten by our stale snapshot. Fixed by wrapping each deck's unlink in withDeckLock(deckId, ...) from patch-deck.ts, and moving the read of the deck's current data inside the lock callback (instead of using an earlier snapshot) so we always mutate the freshest row. Pushed in 7548d61.

builder-io-integration · 2026-07-28T16:13:03Z

+    const db = getDb();
+
+    await db.transaction(async (tx) => {
+      const linkedDecks = await tx


🟡 Prevent links from racing with system deletion

The transaction snapshots linked decks once, but create-deck/apply-design-system can still link another deck after this SELECT and before the design-system row is deleted. With no foreign key or shared resource lock, that interleaving leaves a deck pointing at a deleted design system. Serialize linking and deletion around the design-system resource or recheck links before deleting.

Additional Info

Confirmed by tracing apply-design-system.ts, which writes designSystemId without the deletion transaction or a design-system lock.

Agreed this is a real gap, though fully closing it would need a general per-design-system lock that doesn't exist anywhere else in this codebase (only per-deck locks do, for the editor's write path) — adding one felt disproportionate for this edge case. I shrank the window instead: the design system row and its shares are now deleted first, before any deck unlinking happens, so a concurrent apply-design-system/create-deck call racing to attach a new link will have its assertAccess("design-system", ...) check fail as soon as the row is gone (resolveAccess returns null for a missing resource). This doesn't eliminate a sub-millisecond TOCTOU between another request's access check and its write commit, but it removes the much larger window that existed when decks were unlinked before the design system was deleted. Pushed in 7548d61.

builder-io-integration · 2026-07-28T16:13:04Z

+    // calling resolveAccess() per row, which would re-load each resource and
+    // its shares (N+1) and fan out an unbounded Promise.all as the list grows.
+    const principalClauses: NonNullable<ReturnType<typeof and>>[] = [];
+    if (userEmail) {


🟡 Match the sharing model's case-insensitive email handling

The batched user-share predicate compares principalId to getRequestUserEmail() with exact equality. The sharing access model normalizes user emails case-insensitively, so an authenticated email whose casing differs from the stored principal can still pass accessFilter/assertAccess but receive the default viewer role here, hiding the admin delete control. Normalize the request email or use the same case-insensitive predicate as the shared access helpers, including for the owner comparison.

Additional Info

New finding in the batched role computation; prior N+1 issue is resolved.

Agreed, this was a real regression versus the previous per-row resolveAccess() calls, which do go through the core access model's case-insensitive email matching (lower(column) = lowercased-input, see emailColumnMatches/normalizeEmailForAccess in packages/core/src/sharing/access.ts). My batched query used exact eq() on both the share principalId comparison and the JS-side owner-email comparison, so a session email with different casing than the stored principal/owner value could get a lower role than it should (including, worst case, a true owner being shown as a non-owner). Fixed by normalizing both sides to lowercase (normalizeEmail helper) and comparing the share principalId with lower(column) = ... in SQL, matching the core model. Pushed in 7548d61.

builder-io-integration · 2026-07-28T16:13:04Z


+## Deleting a Design System
+
+`delete-design-system` requires owner access and removes the system, its


🟡 Align skill documentation with admin deletion access

The action and UI now consistently allow admin share roles to delete, but this skill still says delete-design-system requires owner access. Agents following the skill will infer different permissions than the action enforces. Update the skill to document the intended admin-or-owner requirement, or revert the action to owner-only and keep the original documentation.

Additional Info

This is a new documentation/behavior mismatch introduced by the follow-up permission change; the prior UI/action mismatch has been fixed.

Agreed, thanks for catching the drift. Updated the skill to say delete-design-system requires admin access or higher (owner or admin share role), matching the action. Pushed in 7548d61.

liamdebeasi · 2026-07-28T17:46:12Z

@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why

- Unlink each deck under its per-deck write lock (same lock as patch-deck/save-deck/update-slide) and re-read the row inside the lock, so this read-modify-write can't clobber a concurrent slide edit - Delete the design system row and its shares before unlinking decks, so a concurrent apply-design-system/create-deck call fails its access check as soon as the resource is gone, shrinking the race window for a fresh link to attach to a design system being deleted - Normalize email comparisons in list-design-systems's batched role query to match the core access model's case-insensitive email matching - Update the design-systems skill doc to say delete requires admin access or higher, matching the action

builder-io-integration · 2026-07-28T17:54:16Z

All four replies posted successfully.

I reviewed the four new review-bot findings on the latest push (commit 52406c3) and agreed with all of them after verifying each against the actual code:

Missing deck lock (🔴) — the deck unlink read-modify-write bypassed withDeckLock, risking a lost update if a slide edit landed mid-operation. Fixed by acquiring the same per-deck lock used by patch-deck/save-deck/update-slide, reading fresh data inside the lock.
Link race during deletion (🟡) — real but narrow race where a new link could attach between reading linked decks and deleting the system. Rather than building a new locking primitive (disproportionate — none exists for design systems anywhere in the codebase), I reordered to delete the design system row first, so concurrent link attempts fail their access check immediately. Explained the residual limits in my reply.
Case-sensitive email comparison (🟡) — confirmed this was a real regression vs. the original per-row resolveAccess() calls, which use case-insensitive matching in core. Fixed by normalizing emails and matching the core model's lower() comparison.
Stale skill doc (🟡) — updated to say admin-or-owner, matching the action.

Typecheck and existing tests pass. Committed as 7548d61b6 and pushed to ld/ds-delete, updating PR #2457. Replied on all four comment threads explaining what was fixed and why (including being explicit about the residual limits of the race-condition fix rather than overclaiming full closure).

⠀
🟢 Fixed all four new review findings, replied on each thread, and pushed to the branch.

builder-io-integration

Builder reviewed your changes and found 1 potential issue 🟡

Review Details

Code Review Summary

This incremental review evaluated the latest commit, which added per-deck locking, re-read behavior, normalized email matching, and updated the design-system skill. The previously reported issues remain open and were not reposted. The lock now protects concurrent deck JSON edits, and the role/documentation fixes are directionally correct. However, the new ordering moves design-system deletion into a committed transaction before deck unlinking, so the operation no longer has all-or-nothing behavior. Risk level: High because this is destructive shared-data mutation.

Key Finding

🟡 MEDIUM — Design-system/share deletion commits before the per-deck unlink operations. If any deck read, JSON parse, lock acquisition, or update fails, the action returns an error after permanently deleting the system while some decks still retain dangling references. Keep resource deletion and all deck updates atomic, or introduce a recoverable deletion state/job.

🧪 Browser testing: Attempted full verification, but all flows were blocked because browser automation tools were unavailable; the dev server was running.

builder-io-integration · 2026-07-28T18:19:59Z

+    // assertAccess("design-system", ...) check fails for anyone trying to
+    // attach a fresh link, shrinking the window for a deck to end up pointing
+    // at a design system we're about to remove.
+    await db.transaction(async (tx) => {


🟡 Keep system deletion and deck unlinking atomic

This transaction commits deletion of the design-system row and all share grants before the subsequent per-deck Promise.all runs. If any deck lock, read, JSON parse, or update fails, the action returns an error after permanently removing the system while only a prefix—or none—of its decks has been unlinked, leaving dangling references. Acquire the relevant locks and perform the resource/share deletion together with all deck updates in one atomic operation, or use a recoverable deletion state/job.

Additional Info

New regression introduced by the latest ordering change; prior concurrent-write and link-race comments were not reposted.

builder-io-integration

Builder reviewed your changes and found 2 potential issues 🔴

Review Details

Code Review Summary

This incremental review evaluated the current PR head after the latest merge. The previously reported comments remain unresolved and were not reposted. The current deletion path still adds the expected delete UI and batched access metadata, but two distinct authorization/data-integrity issues were identified in the current code. Risk level: High because the action performs destructive writes to shared, ownable deck data.

Key Findings

🔴 HIGH — A design-system admin can cause writes to every linked deck without any deck-level access check, bypassing the ownable-deck permission boundary.
🟡 MEDIUM — Deleting the current default system does not promote another owned system, so subsequent automatic deck creation can silently lose its design-system association.

🧪 Browser testing: Will run after this review (PR touches UI code).

builder-io-integration · 2026-07-28T19:26:03Z

+    await Promise.all(
+      linkedDeckIds.map((deckId) =>
+        withDeckLock(deckId, async () => {
+          const [deck] = await db
+            .select({
+              designSystemId: schema.decks.designSystemId,
+              data: schema.decks.data,
+            })
+            .from(schema.decks)
+            .where(eq(schema.decks.id, deckId));
+          if (!deck || deck.designSystemId !== id) return;
+
+          const data = JSON.parse(deck.data);
+          if ("designSystemId" in data) delete data.designSystemId;
+          await db
+            .update(schema.decks)
+            .set({
+              designSystemId: null,
+              data: JSON.stringify(data),
+              updatedAt: new Date().toISOString(),
+            })
+            .where(eq(schema.decks.id, deckId));


🔴 Enforce deck access when unlinking linked decks

The action requires admin access to the design system but then read-modify-writes every linked deck without checking the caller's deck access. A user who can administer a shared design system can therefore mutate another owner's deck (clearing its relational and JSON design-system IDs) without an editor grant. Apply the appropriate deck access boundary or use a deliberately scoped system-managed write that cannot bypass ownable-data rules.

Additional Info

New finding; existing concurrency and atomicity comments were intentionally not reposted.

builder-io-integration · 2026-07-28T19:26:03Z

+      await tx
+        .delete(schema.designSystems)
+        .where(eq(schema.designSystems.id, id));


🟡 Preserve a default design system after deletion

If the deleted system is the owner's current default and other systems remain, this path does not promote a replacement. create-deck without an explicit designSystemId selects only isDefault = true, so subsequent automatic deck creation can silently omit a design system even though usable systems still exist. Promote a deterministic remaining owned system in the same transaction or define an intentional fallback.

Additional Info

Confirmed against create-deck.ts automatic default selection behavior.

feat: support deleting slides design systems

ffa824d

This comment has been minimized.

Sign in to view

liamdebeasi marked this pull request as ready for review July 28, 2026 14:50

Merge branch 'main' into ld/ds-delete

509be65

liamdebeasi requested review from a team and shomix and removed request for a team July 28, 2026 14:50

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

builder-io-integration Bot reviewed Jul 28, 2026

View reviewed changes

steve8708 approved these changes Jul 28, 2026

View reviewed changes

builder-io-integration Bot reviewed Jul 28, 2026

View reviewed changes

Merge branch 'main' into ld/ds-delete

eea7d8c

builder-io-integration Bot reviewed Jul 28, 2026

View reviewed changes


		## Deleting a Design System

		`delete-design-system` requires owner access and removes the system, its

Uh oh!

Conversation

liamdebeasi commented Jul 28, 2026

Uh oh!

This comment has been minimized.

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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 was marked as outdated.

Uh oh!

liamdebeasi commented Jul 28, 2026

Uh oh!

builder-io-integration Bot commented Jul 28, 2026

Uh oh!

builder-io-integration Bot left a comment

Choose a reason for hiding this comment

Code Review Summary

Key Findings

Uh oh!

builder-io-integration Bot Jul 28, 2026

Choose a reason for hiding this comment

🔴 Preserve concurrent deck edits during deletion

Uh oh!

builder-io-integration Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

builder-io-integration Bot Jul 28, 2026

Choose a reason for hiding this comment

🟡 Prevent links from racing with system deletion

Uh oh!

builder-io-integration Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

builder-io-integration Bot Jul 28, 2026

Choose a reason for hiding this comment

🟡 Match the sharing model's case-insensitive email handling

Uh oh!

builder-io-integration Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

builder-io-integration Bot Jul 28, 2026

Choose a reason for hiding this comment

🟡 Align skill documentation with admin deletion access

Uh oh!

builder-io-integration Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

liamdebeasi commented Jul 28, 2026

Uh oh!

builder-io-integration Bot commented Jul 28, 2026

Uh oh!

builder-io-integration Bot left a comment

Choose a reason for hiding this comment

Code Review Summary

Key Finding

Uh oh!

builder-io-integration Bot Jul 28, 2026

Choose a reason for hiding this comment

github-actions Bot commented Jul 28, 2026 •

edited

Loading