Skip to content

Batch size measurements#673

Open
lukasz-jazwa wants to merge 9 commits intobatch-processor-race-conditions-fixfrom
batch-size-measurements
Open

Batch size measurements#673
lukasz-jazwa wants to merge 9 commits intobatch-processor-race-conditions-fixfrom
batch-size-measurements

Conversation

@lukasz-jazwa
Copy link
Copy Markdown
Collaborator

No description provided.

@lukasz-jazwa lukasz-jazwa force-pushed the batch-processor-race-conditions-fix branch from 11a33d0 to 13245a4 Compare April 27, 2026 08:54
@@ -152,9 +152,8 @@ export class FlowResizeBatchProcessorService {
*/
private processNodeBatch(entries: ProcessedEntry[]): void {
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.

The post-init path now emits commandHandler.emit('updateNodes', { nodes: nodeSizeUpdates }) for all size updates - both nodes without an initial size and nodes whose size changed.

Before this PR, the size-changed branch went through flowCore.updater.applyNodeSize(nodeId, size), which is implemented in InternalUpdater as:

applyNodeSize(nodeId, size): void {
  const node = this.flowCore.getNodeById(nodeId);
  if (!node || this.isResizing() || this.hasSameSize(node, size)) {
    return;
  }
  this.flowCore.commandHandler.emit('resizeNode', { id: nodeId, size });
}

InternalUpdater.applyNodeSize is unchanged in this PR - but the new call site bypasses it entirely for post-init size-changed nodes. As a result:

  • The isResizing() early-return is no longer reached on this path. While the user is mid-drag on a resize handle, actionStateManager.isResizing() returns true and the resize action is the source of truth (with snapping, constraints, transforms). Previously, ResizeObserver-driven updates were suppressed during this window. Now they fire updateNodes and may compete with the resize action - risk of size flicker, fighting snapping, or overwriting transformed values with raw DOM measurements.
  • The isResizing flag is still computed in processNodeBatch, but it now only gates the port-measurement follow-up - not the size update itself.

Either restore the guard (skip pushing to nodeSizeUpdates when isResizing is true), or document explicitly why ResizeObserver-driven updates are now expected to fire mid-resize.

}

flowCore.updater.applyNodeSize(metadata.nodeId, size);
nodeSizeUpdates.push({ id: metadata.nodeId, size });
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.

Size-changed nodes post-init are now routed through 'updateNodes' instead of 'resizeNode', silently dropping resize-domain logic

To be clear about what changed: commandHandler.emit('updateNodes', ...) was already present in the post-init branch before this PR. The structural shape of if (flowCore.isInitialized) { emit('updateNodes', ...) } else { ...applyNodeSize... } is identical before and after. The change is what gets pushed into the emitted array.

Before this PR, the loop split entries into two paths:

  • Branch A - no current size (!currentSize): pushed into nodesNeedingInitialSize, emitted as a single updateNodes post-loop.
  • Branch B - size changed (isSizeChanged(currentSize, size)): called flowCore.updater.applyNodeSize(id, size) per-node inside the loop, which routes through InternalUpdater.applyNodeSize → emits 'resizeNode' with the full domain pipeline.

After this PR, both branches are merged into a single nodeSizeUpdates array, and the post-loop updateNodes emit now carries entries from Branch B as well. So Branch A is unchanged, but Branch B's command path silently switches from 'resizeNode' to 'updateNodes'.

The two handlers are not equivalent:

  • resizeNode (resize-node.ts) handles missing-initial-size via handleMissingInitialSize, applies handleGroupNodeResize (children bounds containment, applyMinimumSizeConstraints), and handleSingleNodeResize (minimum size, snapping).
  • updateNodes (add-update-delete.ts) is a bare passthrough to applyUpdate({ nodesToUpdate: nodes }, 'updateNodes') - no constraints, no group containment, no snapping.

Concrete scenarios that regress post-init (only Branch B - size-changed nodes):

  • A selected group node whose child reflows: ResizeObserver fires for the group → updateNodes is emitted → children bounds containment and minimum-size constraints are not applied.
  • An unselected group node whose child reflows: this is the most surprising case. resizeNode early-returns at resize-node.ts:120 for isGroup(node) && !node.selected, so previously the group's stored size was not touched during a child reflow - it was treated as user-set source of truth. Post-PR, updateNodes overwrites the stored size with the raw DOM measurement on every reflow. This silently mutates an invariant that previously held.
  • A non-group node with dynamic content (e.g. content-driven CSS sizing) shrinks below getMinNodeSize: updateNodes accepts the raw size; previously resizeNode clamped it via applyMinimumSizeConstraints.
  • Snapping configured for resize: previously applied via handleSingleNodeResizeapplySnappingIfNeeded; now skipped on this path.

Branch A (no-initial-size) is unaffected: even pre-PR it bypassed resizeNode's constraint pipeline, because resizeNode's handleMissingInitialSize (resize-node.ts:195-207) itself just emits a bare updateNode without constraints. So the pre-PR updateNodes emit was effectively equivalent to what resizeNode would have done for that case. The regression is exclusively about Branch B - size-changed nodes that previously preserved the full resize-domain semantics and now do not.

Options:

  • Keep the per-node applyNodeSize call for size-changed nodes (preserves 'resizeNode' and its logic), and only batch the no-initial-size path (status quo before the PR).
  • Or introduce a new bulk command like 'resizeNodesBulk' that runs the same group/min-size/snapping pipeline as 'resizeNode' but for an array of nodes, and emit that instead.
  • Or explicitly document that ResizeObserver-driven post-init updates intentionally skip resize-domain logic, and accept the regression.

Tests to add in flow-resize-processor.service.spec.ts and/or resize-node.test.ts:

  • Selected group node with children whose total bounds exceed a shrunk parent size → trigger ResizeObserver post-init → assert children-bounds containment is still applied.
  • Unselected group node whose child reflows → trigger ResizeObserver post-init → assert the group's stored size is not silently overwritten with the raw DOM measurement (preserving the prior invariant), or explicitly document that this invariant is dropped.
  • Non-group node with getMinNodeSize config → trigger ResizeObserver with smaller size post-init → assert size is clamped to minimum.
  • Snapping enabled + ResizeObserver-driven update post-init → assert snapping is applied (or document drop).

Suggested fix (combined with Comment 4):

Re-introduce the pre-PR partition between Branch A and Branch B inside processNodeBatch:

  • Branch A - no current size → batch via commandHandler.emit('updateNodes', ...) (preserves the original batching intent for palette drops / virtualization).
  • Branch B - size changed → per-node flowCore.updater.applyNodeSize(id, size), which already routes through 'resizeNode' with the full pipeline (group containment, min-size, snapping) and respects the isResizing() guard.

This restores pre-PR semantics for size-changed nodes while keeping the bulk benefit for the case the batching was actually introduced for.

@lukasz-jazwa lukasz-jazwa force-pushed the batch-size-measurements branch from f63467c to fe224a5 Compare April 27, 2026 10:08
@lukasz-jazwa lukasz-jazwa force-pushed the batch-size-measurements branch from fe224a5 to 2d1c288 Compare April 27, 2026 13:07
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.

2 participants