fix(visual-builder): claim the field lock from the empty-block add - #641
Conversation
The empty-state placeholder never selects the field, so its add button claimed no lock and a peer editor was never told the field was changing. Send FOCUS_FIELD with the element's edit stack before ADD_INSTANCE, skip the send when the stack is empty (the parent reads that as a deselect and would release the lock), and refuse the add outright when a peer holds the field, matching the click listener's peer-lock gate.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
| } catch (error) { | ||
| console.error("Visual Builder: Failed to add instance", error); | ||
| } | ||
| observeParentAndFocusNewInstance({ |
There was a problem hiding this comment.
Catch swallows failure but control falls through to observeParentAndFocusNewInstance. Before this change a rejected ADD_INSTANCE propagated and skipped the observe; now a failed add still starts an observer waiting for an instance that will never appear (and the mutation observer / focus attempt lingers).
Either return from the catch, or move the observe into the try after the await.
| } catch (error) { | |
| console.error("Visual Builder: Failed to add instance", error); | |
| } | |
| observeParentAndFocusNewInstance({ | |
| } catch (error) { | |
| console.error("Visual Builder: Failed to add instance", error); | |
| return; | |
| } | |
| observeParentAndFocusNewInstance({ |
There was a problem hiding this comment.
Good catch, fixed in 07a1c65. The catch returns now, so a failed add no longer leaves an observer waiting on an instance that never arrives.
I went with the return rather than moving the observe inside the try, so the success path still reads top to bottom.
|
|
||
| // The empty-state add never selects the field, so nothing else claims the | ||
| // lock. Fire and forget: the parent does not await the claim either. | ||
| const DOMEditStack = getDOMEditStack(event.currentTarget as Element); |
There was a problem hiding this comment.
event.currentTarget is the add button, so the stack is derived from the button's ancestors. The capture-phase click listener builds the stack from the clicked field element. Are these guaranteed identical here — i.e. is the placeholder always rendered inside the data-cslp field element, not in a portal/overlay layer? If the placeholder ever renders outside the field subtree, the stack silently comes back empty and the add proceeds with no lock (the DOMEditStack.length branch is skipped, not blocked).
Would be more robust to resolve the element from details.fieldMetadata.cslpValue rather than DOM position.
There was a problem hiding this comment.
Fair challenge. Today they are identical: generateEmptyBlocks reads data-cslp off emptyBlockParent and then hydrates the component into that same element, so the button is always inside the field subtree. So this is not currently reachable.
Switched to your suggestion anyway in 07a1c65, since it is also what the other two FOCUS_FIELD senders do. Both mouseClick.ts:451 and handleFormFieldFocus in FieldToolbar.tsx pass the resolved editableElement, not a button, so resolving by cslp makes this consistent with them rather than being the odd one out:
const fieldElement =
document.querySelector(
`[${DATA_CSLP_ATTR_SELECTOR}="${details.fieldMetadata.cslpValue}"]`
) ?? event.currentTarget;Kept currentTarget as the fallback so a missed lookup degrades to the old behaviour instead of an empty stack.
| onClick={(e) => | ||
| sendAddInstanceEvent(e as unknown as MouseEvent) | ||
| } |
There was a problem hiding this comment.
Double cast through unknown is a smell. Preact gives JSX.TargetedMouseEvent<HTMLButtonElement>; typing the handler param as that drops both casts here and the as Element on currentTarget.
async function sendAddInstanceEvent(
event: JSX.TargetedMouseEvent<HTMLButtonElement>
) { ... }then onClick={sendAddInstanceEvent}.
There was a problem hiding this comment.
Done in 07a1c65. Typing the param as JSX.TargetedMouseEvent<HTMLButtonElement> dropped both casts and let the handler be passed straight to onClick. The as Element on currentTarget went with it.
- Return from the ADD_INSTANCE catch so a failed add no longer starts an observer waiting for an instance that will never appear. - Resolve the field element from the cslp rather than the button's DOM position, so a portal render cannot silently yield an empty edit stack and skip the lock claim. Falls back to the button. - Type the handler as JSX.TargetedMouseEvent<HTMLButtonElement>, dropping the double cast through unknown and the cast on currentTarget. - Clear mocks in beforeEach rather than afterEach, remove the appended host node RTL does not clean up, and replace the fixed 10-microtask drain with a waitFor on an observable signal.
What
The empty-state placeholder for a multiple field now claims the auto-draft field
lock before it adds the first instance.
Why
The placeholder (
VB_EmptyBlockParentClass) never selects the field. The canvasclick listener is capture-phase and returns early for empty blocks, so nothing
sent
FOCUS_FIELDand no lock was claimed. Another editor was never told thefield was being changed, and the add went through even when a peer held it.
How
FOCUS_FIELDwith the element's edit stack beforeADD_INSTANCE, matchingthe click listener. Fire and forget, because the parent's
lockFocusedFieldissynchronous and does not await the claim either, so awaiting the message would
only add the parent's content-type and entry fetches to the click path.
deselect and calls
releaseCurrentFieldLock(), which would release the lockinstead of claiming it.
getPeerLockForFieldreports a peer lock, the same no-op aclick on a peer-locked field already gets.
ADD_INSTANCEsend in try/catch, asaddInstanceButtondoes.Testing
sent, and a peer-held field adds nothing. Each was red before the change.
clicking the placeholder add sends
POST /draft/focus(201) and the lock appearsin
_field_lock_infowith a TTL. Repeated on a modular-blocks field and amultiple file field nested in a block. On the modular-blocks field the lock then
narrows to the new instance path and the instance is logged in the change set.
Not in this change
Still open on the ticket, deliberately out of scope here: the instance delete that
truncates a multiple field (reproduces through both the canvas and the draft API,
and looks server-side in the draft apply step), the parent-to-child lock hand-off
that leaves a brief window with no lock held, and reorder.