Skip to content

feat(studio): route element delete through SDK removeElement (§3.1)#1465

Merged
vanceingalls merged 1 commit into
mainfrom
06-15-feat_studio_route_element_delete_through_sdk_removeelement_3.1_
Jun 17, 2026
Merged

feat(studio): route element delete through SDK removeElement (§3.1)#1465
vanceingalls merged 1 commit into
mainfrom
06-15-feat_studio_route_element_delete_through_sdk_removeelement_3.1_

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Routes handleDomEditElementDelete through the SDK (sdkSession.removeElement) when an active session resolves the element's hf-id, falling back to the existing server-side /api/projects/{pid}/file-mutations/remove-element/ path otherwise. Also documents (via // ponytail: comment) that z-index reorder (§3.4) is already SDK-cut-over because it writes inline-style patches through commitPositionPatchToHtmlpersistDomEditOperationsonTrySdkPersist.

Stage 7 §3.1 — Delete → removeElement

Changes

  • sdkCutover.ts: Extract internal persistSdkSerialize helper (serialize → write → recordEdit → reload tail shared across ops); add exported sdkDeletePersist(hfId, originalContent, targetPath, session, deps). sdkCutoverPersist refactored to use persistSdkSerialize.
  • useElementLifecycleOps.ts: Add optional onTrySdkDelete? callback to params; handler tries it after reading originalContent and before the server API call. On success: clear selection, show toast, return. Z-index reorder // ponytail: note added (§3.4 verified cut-over).
  • useDomEditCommits.ts: Thread onTrySdkDelete? through UseDomEditCommitsParamsuseElementLifecycleOps.
  • useDomEditSession.ts: Wire onTrySdkDelete from sdkSession (same pattern as onTrySdkPersist).
  • sdkCutover.test.ts: 6 new tests for sdkDeletePersist (null session, element not found, removeElement called, history recorded, reload called, error fallback).

Open decisions (§7)

  • Undo ownership (D): sdkDeletePersist records into Studio editHistory (persistent), consistent with sdkCutoverPersist. SDK session also has in-memory history. Two stacks coexist — same as style/text edits today. s7.4 force-reload after undo/redo keeps Studio history authoritative; no regression introduced.
  • §3.4 z-index reorder: Writes inline-style patches → already SDK-cut-over as setStyle. No DOM-node reordering; no SDK reorder op exists or is needed. Documented with // ponytail: comment.

Verification

  • bun run build
  • bun run test ✅ (26 sdkCutover tests pass, full suite green)
  • bunx oxlint / bunx oxfmt --check
  • 600-line file gate ✅
  • Lefthook pre-commit hooks ✅

🤖 Generated with Claude Code

vanceingalls commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

This was referenced Jun 15, 2026

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the matter of #1465 — element delete routed through sdkSession.removeElement (§3.1).

Intent

Route handleDomEditElementDelete through sdkSession.removeElement(hfId) via a new sdkDeletePersist helper, falling back to the existing /api/projects/{pid}/file-mutations/remove-element/ route when the SDK can't address the element. A small persistSdkSerialize internal helper is extracted from sdkCutoverPersist to avoid duplicating the serialize → write → recordEdit → reloadPreview tail.

Per-file observations

  • packages/studio/src/utils/sdkCutover.ts:80-99persistSdkSerialize extraction is faithful: the four-step tail is character-for-character identical to the old inline version. Good consolidation, and forward-friendly for #1466/#1469/#1470's additional callers.
  • packages/studio/src/utils/sdkCutover.ts:131-150sdkDeletePersist calls sdkSession.removeElement(hfId) bare — not wrapped in session.batch(...) the way sdkCutoverPersist wraps its dispatch loop. A single op may be atomic by construction, but the asymmetry is a small inconsistency that #1471 closes by wrapping it. Worth one comment of acknowledgement here that the batch-wrap is forthcoming, lest a future reader trying to add a sibling op forget to introduce one.
  • packages/studio/src/utils/sdkCutover.ts:138-149No before/after no-op detection. If the element is found by getElement but removeElement silently does nothing (e.g. for an in-flight stale id), this writes an empty-diff history entry and reloads. #1471 introduces the before === after → return false → server-fallback discipline across all persist fns; this PR carries the hazard for now. Same band-aid-shape as the missing batch(); same forward fix. Flag both as siblings.
  • packages/studio/src/hooks/useElementLifecycleOps.ts:77-86 — the SDK attempt runs before domEditSaveTimestampRef.current = Date.now(). If the SDK path succeeds, the timestamp ref is never updated, which means the self-write suppress window in useSdkSession.ts (SELF_WRITE_SUPPRESS_MS) will not be primed for this delete. A subsequent file-change tick could trigger an unnecessary session reload. Worth verifying — sdkDeletePersist does set deps.domEditSaveTimestampRef.current = Date.now() inside persistSdkSerialize, so the ref is in fact written. False alarm; cite resolved.
  • packages/studio/src/hooks/useElementLifecycleOps.ts:109-114 — the // ponytail: note on z-index reorder is good. Verified the comment survives through #1466#1471 unchanged.
  • Server fallback path: the existing remove-element route strips only the element node — it does not cascade-remove GSAP tweens targeting it the way the SDK path does (per #1471's body, this is the documented limitation). In this PR, the fallback runs only when getElement returns null or the SDK throws — a runtime-generated / non-addressable element scenario where targeting tweens are unlikely but not impossible. Worth deferring documentation to #1471 where the ponytail-note lives.

Stale-base hazard intersection

Touches useDomEditCommits.ts, useDomEditSession.ts, useElementLifecycleOps.ts — three of the seven hotspot files. Main's #1473 (extend SDK shadow to delete/timing/gsap-add) touched all three of these and added GSAP-shadow fields to useElementLifecycleOps.ts. Squash-merging without a rebase will revert those #1473 additions. The intent of this PR is "add the SDK delete-path"; reverting #1473's shadow-delete telemetry is not. Rebase before squash.

CI

Same inherited Preflight format drift; no logic failures attributable to this PR.

Verdict

clear-with-nits at 9f2f8fffc0094420187e2b04f8f6d8ed6d63d27a — the wiring is correct, the helper extraction is faithful, the test additions cover the happy and unhappy paths. The two siblings of band-aid pattern (no batch(), no before===after no-op guard) are explicitly closed in #1471 — fine as a train, but each is a hazard if #1465 lands without #1471. Treat as same-train. Re-verify if HEAD moves before stamp.

Review by Via

@vanceingalls vanceingalls force-pushed the 06-15-fix_studio_wire_ontrysdkpersist_to_sdkcutoverpersist_cutover_was_unwired_ branch from 9bde352 to 0384bdd Compare June 17, 2026 03:03
@vanceingalls vanceingalls force-pushed the 06-15-feat_studio_route_element_delete_through_sdk_removeelement_3.1_ branch from 9f2f8ff to 5a0e106 Compare June 17, 2026 03:03
@vanceingalls vanceingalls force-pushed the 06-15-fix_studio_wire_ontrysdkpersist_to_sdkcutoverpersist_cutover_was_unwired_ branch from 0384bdd to fd21513 Compare June 17, 2026 04:48
@vanceingalls vanceingalls force-pushed the 06-15-feat_studio_route_element_delete_through_sdk_removeelement_3.1_ branch 2 times, most recently from f715dcf to e78bb5b Compare June 17, 2026 05:21
@vanceingalls vanceingalls force-pushed the 06-15-fix_studio_wire_ontrysdkpersist_to_sdkcutoverpersist_cutover_was_unwired_ branch from fd21513 to 3ed8576 Compare June 17, 2026 05:21
@vanceingalls vanceingalls force-pushed the 06-15-feat_studio_route_element_delete_through_sdk_removeelement_3.1_ branch from e78bb5b to 94bcbb3 Compare June 17, 2026 05:53
@vanceingalls vanceingalls force-pushed the 06-15-fix_studio_wire_ontrysdkpersist_to_sdkcutoverpersist_cutover_was_unwired_ branch from 3ed8576 to 0701234 Compare June 17, 2026 05:53
@vanceingalls vanceingalls force-pushed the 06-15-feat_studio_route_element_delete_through_sdk_removeelement_3.1_ branch from 94bcbb3 to 5349304 Compare June 17, 2026 08:22
@vanceingalls vanceingalls force-pushed the 06-15-fix_studio_wire_ontrysdkpersist_to_sdkcutoverpersist_cutover_was_unwired_ branch 2 times, most recently from 7d2cd55 to 39c2cac Compare June 17, 2026 17:32
@vanceingalls vanceingalls force-pushed the 06-15-feat_studio_route_element_delete_through_sdk_removeelement_3.1_ branch from 5349304 to 4d77e7f Compare June 17, 2026 17:32

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — §3.1 Element Delete → removeElement

Solid PR. The pattern mirrors the existing sdkCutoverPersist / onTrySdkPersist approach, the persistSdkSerialize extraction reduces duplication, and the test coverage is thorough. A few observations:

The Good

  • persistSdkSerialize extraction — This is a clean DRY refactor. The serialize → write → recordEdit → reload tail was duplicated; pulling it into a shared helper is the right call. The // ponytail: internal comment documenting export intent is a nice touch.
  • Guard placement in handleDomEditElementDelete — The SDK try-block is placed after originalContent is fetched and patchTarget is validated, so sdkDeletePersist gets the originalContent it needs for history recording. The selection.hfId guard correctly ensures only SDK-tracked elements attempt the fast path.
  • Error boundarysdkDeletePersist catches removeElement failures and returns false, letting the server-side fallback path handle it. The test at line 364 ("returns false and does not write on removeElement error") explicitly verifies no side-effects leak on failure. Clean.
  • Test coverage — 6 focused tests covering null session, missing element, happy path (removeElement + write + history + reload), and error fallback. Each test asserts both the return value AND the side-effect expectations. This is how SDK cutover tests should look.

Nits / Observations

  1. Dead onElementDeleted in useElementLifecycleOps — On the base branch, useDomEditSession already stopped wiring onElementDeleted. This PR removes it from UseDomEditCommitsParams (good cleanup), but it still lives in UseElementLifecycleOpsParams (interface, destructure, ?. call at the server-side success path, and the dep array). Since no caller ever passes it anymore, it's dead code. Consider removing it in this PR or a fast-follow — dead optional callbacks in dep arrays are confusing for future readers.

  2. // ponytail: z-index comment — The §3.4 verification note is helpful context. Just flagging that this is documentation-only (no behavior change), which is correct for this PR's scope.

  3. Telemetry event namingsdkDeletePersist reuses the sdk_cutover_success / sdk_cutover_fallback event names with opCount: 1. This works but makes it harder to distinguish delete vs. edit operations in analytics. A future improvement could be adding an op: "delete" field to the event payload, but not blocking.

Verdict

Clean implementation, good test coverage, correct fallback semantics. The persistSdkSerialize extraction is a net improvement to the file.

LGTM — pinging @magi for the stamp. <@U0B1J4SL8H3>

— Miga

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — §3.1 element delete through SDK removeElement

Reviewed at 4d77e7f9decfff5eaeb63fc02425ac7f2a6abdc2. Part of the 18-PR SDK-cutover stack review batch (Group B: §3.x + §3.5).

Clean op. sdkDeletePersist follows the established sdkCutoverPersist shape, the test suite covers null-session / element-not-found / happy / history-recorded / reload / error-fallback. Z-index reorder ponytail note is the right shape for an audit trail.

Concerns

1. Edit-history before semantics diverge across the persist family.

sdkDeletePersist correctly uses the server-fetched originalContent as the before field in recordEdit (so undo replays the actual disk state). But sdkTimingPersist (#1466) and sdkGsap*Persist (#1469, #1470) pass session.serialize() (pre-dispatch) as their before. That means undo on a delete restores disk state, while undo on a timing/gsap edit restores the SDK's in-memory snapshot. When the SDK doc has any drift from disk pre-edit, those undos don't actually restore disk state. Acknowledged for timing in #1466's §7 open decisions, but the asymmetry across the family is worth a comment near persistSdkSerialize so the divergent semantics is visible.

2. Server-fallback delete doesn't resync the SDK doc here.

The fallback path in useElementLifecycleOps does the server remove-element POST then reloadPreview(), but leaves the in-memory SDK session stale (it still believes the element exists). A later SDK edit on a sibling could then serialize() the pre-delete doc and revert the deletion. #1471 fixes exactly this with forceReloadSdkSession?.() after the server write — so this is closed in the stack. Mentioning it here only because the rest of the stack reads #1465 + #1471 together for the delete path's correctness story.

Nits

  • persistSdkSerialize is now shared between sdkCutoverPersist and sdkDeletePersist — nice extraction. The "ponytail: export only if a third caller appears" note is exactly the right rule for now (and a third caller does appear in #1466, so the export decision will surface naturally).
  • The 6 new tests are well-shaped. The "calls reloadPreview on success" assertion is good belt-and-braces.

Verdict

LGTM. The two concerns are stack-level, not blockers on this PR. The §3.1 cut-over itself is clean.

— Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the matter of #1465 — §3.1: route element delete through SDK removeElement.

Re-read at HEAD 4d77e7f9, layering atop Miga's review (4518604168) and Rames Jusso's review (4518621478). My earlier Via review at 9f2f8fff is stale post-rebase and is hereby superseded. The rebase fixed several of my earlier concerns; one new finding has surfaced on this read that neither parallel reviewer flagged.

The wiring is structurally sound

A walk of the call graph at 4d77e7f9:

  • persistSdkSerialize extraction at sdkCutover.ts:87-104 — the four-step serialise → stamp → write → recordEdit → reload tail, previously inline in sdkCutoverPersist, is now a shared module-local helper. Character-for-character identical to the prior inline version; faithful refactor. Both sdkCutoverPersist (L127) and the new sdkDeletePersist (L149) call it. The // ponytail: internal; export only if a third caller appears comment at L86 is exactly the right discipline.
  • sdkDeletePersist body at sdkCutover.ts:139-158removeElement(hfId)persistSdkSerialize. Test suite at sdkCutover.test.ts:301-368 covers null-session, missing-element, happy path, history-recorded, reloadPreview-fired, error-fallback. Six tests, each asserts both return value and side-effect expectation. Solid.
  • useElementLifecycleOps.ts:80-88 — the SDK try-block sits after originalContent is fetched and patchTarget is validated, so the SDK path gets the correct before content for history. selection.hfId guards SDK-tracked elements only; non-hfId elements correctly fall through to the server route.

The new finding the parallel reviews did not surface — sdkDeletePersist runs while the cutover flag is OFF

sdkCutoverPersist at sdkCutover.ts:115 gates on shouldUseSdkCutover(STUDIO_SDK_CUTOVER_ENABLED, !!sdkSession, selection.hfId, ops). That gate is exactly the dark-launch contract — when STUDIO_SDK_CUTOVER_ENABLED=false (the default at manualEditingAvailability.ts:100-104), inline-style edits route to the server path and the SDK is untouched.

sdkDeletePersist, by contrast, has no such gate. Its only guards are:

export async function sdkDeletePersist(...): Promise<boolean> {
  if (!sdkSession || !sdkSession.getElement(hfId)) return false;
  try {
    sdkSession.removeElement(hfId);
    await persistSdkSerialize(sdkSession, targetPath, originalContent, deps, {
      label: "Delete element",
    });
    ...

useSdkSession is opened unconditionally at App.tsx:154 for every project (no flag check), and useDomEditSession.ts:244-252 constructs onTrySdkDelete whenever sdkSession is non-null. useElementLifecycleOps.ts:80-88 calls onTrySdkDelete whenever it is wired AND selection.hfId exists. There is no STUDIO_SDK_CUTOVER_ENABLED consultation anywhere on the delete path.

Net effect at flag-off (the prod default): inline-style edits route to the server (dark-launch intact), but every element delete on an hfId-tracked element routes through the SDK and serialise/writes via persistSdkSerialize. The architecture flips half-on the moment this PR lands. This is band-aid pattern #2 (contradictory rules in the same component) — two functions in the same module document the same dark-launch contract differently and no test exercises the asymmetry (the test mock at sdkCutover.test.ts:14 pins STUDIO_SDK_CUTOVER_ENABLED: true, so the gate divergence is silent in CI).

The fix is one line — gate sdkDeletePersist on STUDIO_SDK_CUTOVER_ENABLED at the top of the function, mirroring sdkCutoverPersist. Alternatively, gate at the call site in useElementLifecycleOps.ts:80. Either resolution is fine; the seam-honest choice is at the function level so both callers (current and any future ones) share the gate.

Either fix this in #1465 directly, or — if the team's preference is to keep deletes always-on-when-session-exists because the value-fidelity risk is low — make that intent explicit with a code comment AND a PR-body annotation, and update manualEditingAvailability.ts to document the asymmetric coverage of the flag. Silent divergence is the band-aid; explicit divergence is at least auditable.

The two earlier-flagged siblings (closed in #1471)

Confirming my earlier Via observations are still extant at 4d77e7f9 but documented as same-train fixes per the briefing:

  • No session.batch(...) wrap on removeElement(hfId). Single-op atomicity by construction, but the asymmetry with sdkCutoverPersist's batched dispatch loop is the kind of small inconsistency a future sibling-op author will replicate without noticing the seam. Closed in #1471.
  • No before === after no-op detection on the persist family. If the element is found by getElement(hfId) but removeElement silently does nothing (stale id, runtime-only element), this writes an empty-diff history entry and reloads. Closed in #1471's universal before === after → return false → server-fallback discipline.

Acknowledging both as same-train fixes rather than re-flagging as blockers, per the cross-PR rubric.

Per Rames's edit-history before semantics divergence (concern 1)

Co-signed. sdkDeletePersist correctly threads server-fetched originalContent as the before field (so undo replays actual disk state). sdkTimingPersist (#1466) and the GSAP persist family (#1469/#1470) pass session.serialize() (pre-dispatch) as their before. The semantic divergence is real and worth a comment near persistSdkSerialize so future maintainers know which family member they're working with. Not blocking this PR alone — but stack-level it is the kind of drift that becomes a band-aid the moment someone tries to factor the families together.

Per Rames's server-fallback resync concern (concern 2)

Co-signed and confirming closed in #1471 via forceReloadSdkSession?.() after the server remove-element write. The standalone failure mode — server fallback leaves the SDK session believing the element still exists; the next SDK edit on a sibling can serialize() the pre-delete doc and revert the deletion — is real but bounded to the gap between #1465 and #1471 landing.

Miga's nits

  • Dead onElementDeleted in useElementLifecycleOps. Co-signed; non-blocking cleanup.
  • Telemetry event naming. sdk_cutover_success / sdk_cutover_fallback with opCount: 1 works but flattens delete vs. edit in analytics. An op: "delete" discriminator field would help. Forward-looking, non-blocking.

Verdict

request-changes at 4d77e7f9 on the un-gated dark-launch finding. The sdkDeletePersist path running while STUDIO_SDK_CUTOVER_ENABLED=false is exactly the contradictory-rules-in-the-same-component shape Miguel's bar treats as request-changes. The fix is one line; the alternative is an explicit, documented asymmetry — but the silent divergence is the band-aid.

The other concerns (missing batch(), missing before === after guard, edit-history before divergence across the family) are stack-level and closed in #1471 per the briefing; this verdict treats them as same-train fixes.

Re-verify if HEAD moves before stamp.

Part of the Group A batch on the 18-PR SDK-cutover stack. See also #1522 (persist contract — request-changes), #1524 (force-reload race — request-changes), #1462 (clean teardown), #1463 (persist wiring), #1466 (timing path — same un-gated-by-flag finding applies).

Review by Via

miguel-heygen
miguel-heygen previously approved these changes Jun 17, 2026

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved as part of SDK cutover stack. Reviewed by Miga, Rames D Jusso, and Via across R1-R4. LGTM.

jrusso1020
jrusso1020 previously approved these changes Jun 17, 2026

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stack-wide stamp — audited bottom-up at the #1539 stack-tip (Rames D Jusso R4 + Miga + Via verified all 16 R3 + 2 CF2 findings at 6c2d66892). SDK-cutover chain cleared end-to-end.

@vanceingalls vanceingalls force-pushed the 06-15-fix_studio_wire_ontrysdkpersist_to_sdkcutoverpersist_cutover_was_unwired_ branch 2 times, most recently from 68fecf3 to fcb5737 Compare June 17, 2026 23:31
Base automatically changed from 06-15-fix_studio_wire_ontrysdkpersist_to_sdkcutoverpersist_cutover_was_unwired_ to main June 17, 2026 23:32
@vanceingalls vanceingalls dismissed stale reviews from jrusso1020 and miguel-heygen June 17, 2026 23:32

The base branch was changed.

Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
@vanceingalls vanceingalls force-pushed the 06-15-feat_studio_route_element_delete_through_sdk_removeelement_3.1_ branch from 4d77e7f to ab23c4e Compare June 17, 2026 23:37
@vanceingalls vanceingalls merged commit bce571c into main Jun 17, 2026
33 of 34 checks passed
@vanceingalls vanceingalls deleted the 06-15-feat_studio_route_element_delete_through_sdk_removeelement_3.1_ branch June 17, 2026 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants