feat: coalesced blocks-geometry-change on position updates#289
Merged
feat: coalesced blocks-geometry-change on position updates#289
Conversation
- Emit blocks-geometry-change at most once per frame for updatePosition - Keep block-change with shallow TBlock snapshot (asTBlockShallow) for veto/sync - Add onBlocksGeometryChange / blocks-geometry-change; minimap listens to both - Flush pending geometry on drag end; add Playwright coverage Made-with: Cursor
Reviewer's GuideAdds a coalesced File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The batching logic in
BlockListStoreduplicates theArray.from(...values())+clear+emit('blocks-geometry-change', { blocks })sequence in bothflushBatchedBlocksGeometryEmitand the rAF callback; consider extracting this into a small private helper (e.g.emitBatchedBlocksGeometryIfNeeded()) to keep behavior consistent and reduce future maintenance overhead. - Now that
block-changereceives a shallow snapshot viaasTBlockShallow, it might be worth tightening the type (e.g. aReadonly<TBlock>alias) for that event detail to better enforce the “treat nested values as read-only” contract at compile time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The batching logic in `BlockListStore` duplicates the `Array.from(...values())` + `clear` + `emit('blocks-geometry-change', { blocks })` sequence in both `flushBatchedBlocksGeometryEmit` and the rAF callback; consider extracting this into a small private helper (e.g. `emitBatchedBlocksGeometryIfNeeded()`) to keep behavior consistent and reduce future maintenance overhead.
- Now that `block-change` receives a shallow snapshot via `asTBlockShallow`, it might be worth tightening the type (e.g. a `Readonly<TBlock>` alias) for that event detail to better enforce the “treat nested values as read-only” contract at compile time.
## Individual Comments
### Comment 1
<location path="src/store/block/BlocksList.ts" line_range="27" />
<code_context>
});
}
+
+ /**
+ * Cheap snapshot for graph events (e.g. `block-change`): one object spread, no deep clone.
+ * Nested values (`anchors`, `meta`, `settings`, …) are shared with the live store — treat as read-only.
</code_context>
<issue_to_address>
**nitpick (typo):** Typo in JSDoc: "Emited" should be "Emitted".
Please update the JSDoc for the `block-change` event to use the correct spelling, "Emitted".
</issue_to_address>
### Comment 2
<location path="e2e/tests/block/block-geometry-batching.spec.ts" line_range="36-45" />
<code_context>
+ test("single-block drag emits both block-change and fewer blocks-geometry-change events", async ({ page }) => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a targeted test for the drag-end flush behaviour of blocks-geometry-change
The current test only verifies that `blocks-geometry-change` is emitted less often than `block-change`, which is a coarse confirmation of batching. To exercise the drag-end flush specifically, add a test that performs a very short drag with minimal/zero `waitFrames` and asserts that at least one `blocks-geometry-change` event occurs after mouseup, confirming `flushBatchedBlocksGeometryEmit` runs correctly at drag end.
Suggested implementation:
```typescript
const readGeom = await graphPO.collectGraphEventDetails<{ blocks: { id: string }[] }>("blocks-geometry-change");
```
You’ll need to **add a new test** inside the existing `test.describe("block-change + coalesced blocks-geometry-change", () => { ... })` block, after the existing `"single-block drag emits both..."` test. The test should:
1. Initialize the graph as in the existing test (reuse `TWO_BLOCKS` and `DRAG_SETTINGS`):
```ts
const graphPO = new GraphPageObject(page);
await graphPO.initialize({
blocks: TWO_BLOCKS,
connections: [],
settings: DRAG_SETTINGS,
});
```
2. Begin collecting events **before** starting the drag:
```ts
const readBlockChange = await graphPO.collectGraphEventDetails<{ block: { id: string } }>("block-change");
const readGeom = await graphPO.collectGraphEventDetails<{ blocks: { id: string }[] }>("blocks-geometry-change");
```
3. Perform a very short drag on a single block with minimal batching opportunity (no `waitFrames` or `waitFrames: 0` if your helper requires it). For example, if your `GraphPageObject` has a drag helper similar to:
```ts
await graphPO.dragBlockBy("a", { x: 5, y: 5 });
```
or, if you have to be explicit:
```ts
await graphPO.dragBlock("a", { to: { x: 105, y: 105 }, waitFrames: 0 });
```
4. After `mouseup` has occurred (i.e. after the drag helper resolves), stop collecting events and inspect them. You want to assert:
- At least one `block-change` occurred during the drag.
- At least one `blocks-geometry-change` event was emitted **after** the last `block-change` event, proving the drag-end flush.
Assuming `collectGraphEventDetails` gives you an async iterator-style reader with `.readAll()` or a similar method that returns an array of events with timestamps, an example assertion might look like:
```ts
test("short drag flushes blocks-geometry-change on drag end", async ({ page }) => {
const graphPO = new GraphPageObject(page);
await graphPO.initialize({
blocks: TWO_BLOCKS,
connections: [],
settings: DRAG_SETTINGS,
});
const readBlockChange = await graphPO.collectGraphEventDetails<{ block: { id: string } }>("block-change");
const readGeom = await graphPO.collectGraphEventDetails<{ blocks: { id: string }[] }>("blocks-geometry-change");
// Perform a minimal drag on block "a" with no extra frame waits.
await graphPO.dragBlockBy("a", { x: 5, y: 5 /*, waitFrames: 0 if required */ });
const blockChangeEvents = await readBlockChange.readAll();
const geomEvents = await readGeom.readAll();
expect(blockChangeEvents.length).toBeGreaterThan(0);
expect(geomEvents.length).toBeGreaterThan(0);
// If events include timestamps (e.g. event.timestamp), assert that at least one
// geometry-change event is emitted after the last block-change event.
const lastBlockChangeTime = Math.max(...blockChangeEvents.map(e => e.timestamp));
const geomAfterMouseup = geomEvents.filter(e => e.timestamp > lastBlockChangeTime);
expect(geomAfterMouseup.length).toBeGreaterThan(0);
});
```
5. If your event detail objects don’t yet carry timestamps, you can instead:
- Use `performance.now()` or a monotonic counter injected in your test harness, or
- Enforce ordering based on the collection semantics of `collectGraphEventDetails` (e.g. if it preserves sequence, you can assert that the last `blocks-geometry-change` is emitted after the last drag step, and that at least one such event exists even for a single step drag).
In summary, add a `test("short drag flushes blocks-geometry-change on drag end", ...)` to the suite, following the pattern above, and adapt:
- The drag helper (`dragBlockBy` / `dragBlock`), and
- How you read/inspect events (`readAll`, timestamps, etc.)
to match what `GraphPageObject` and `collectGraphEventDetails` actually expose in your codebase.
</issue_to_address>
### Comment 3
<location path="src/store/block/BlocksList.ts" line_range="259" />
<code_context>
+ * Flushes pending `blocks-geometry-change` immediately (cancels a scheduled rAF if any).
+ * Called when a graph drag ends so the last frame is not lost.
+ */
+ public flushBatchedBlocksGeometryEmit(): void {
+ if (this.batchedGeometryRaf !== null) {
+ cancelAnimationFrame(this.batchedGeometryRaf);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared emit and requestAnimationFrame cleanup helpers so the batching code paths all reuse a single implementation.
You can reduce the extra moving parts without changing behavior by centralizing the flush logic and rAF handling. That addresses points (1) and (3) while keeping your current API surface (`flushBatchedBlocksGeometryEmit`, `scheduleBatchedBlocksGeometryFlush`, `cancelBatchedBlocksGeometry`) intact.
### 1. Deduplicate flush logic
Extract the shared “read map → clear → emit” sequence into a single helper:
```ts
private emitBatchedBlocksGeometry(): void {
if (this.batchedGeometryPending.size === 0) {
return;
}
const blocks = Array.from(this.batchedGeometryPending.values());
this.batchedGeometryPending.clear();
this.graph.emit("blocks-geometry-change", { blocks });
}
```
Use it in both the immediate flush and the rAF callback:
```ts
public flushBatchedBlocksGeometryEmit(): void {
this.clearGeometryRaf();
this.emitBatchedBlocksGeometry();
}
private scheduleBatchedBlocksGeometryFlush(): void {
if (this.batchedGeometryRaf !== null) {
return;
}
this.batchedGeometryRaf = requestAnimationFrame(() => {
this.batchedGeometryRaf = null;
this.emitBatchedBlocksGeometry();
});
}
```
### 2. Centralize rAF management
Avoid setting/cancelling/clearing `batchedGeometryRaf` in multiple places by extracting that as well:
```ts
private clearGeometryRaf(): void {
if (this.batchedGeometryRaf !== null) {
cancelAnimationFrame(this.batchedGeometryRaf);
this.batchedGeometryRaf = null;
}
}
```
Then `cancelBatchedBlocksGeometry` becomes simpler and consistent:
```ts
private cancelBatchedBlocksGeometry(): void {
this.clearGeometryRaf();
this.batchedGeometryPending.clear();
}
```
Net effect:
- Core batching behavior is defined once in `emitBatchedBlocksGeometry`.
- The invariant “`batchedGeometryRaf !== null` means a flush is scheduled” is enforced in one helper.
- The public API methods stay the same, but the control flow is easier to follow and less error‑prone.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+36
to
+45
| test("single-block drag emits both block-change and fewer blocks-geometry-change events", async ({ page }) => { | ||
| const graphPO = new GraphPageObject(page); | ||
| await graphPO.initialize({ | ||
| blocks: TWO_BLOCKS, | ||
| connections: [], | ||
| settings: DRAG_SETTINGS, | ||
| }); | ||
|
|
||
| const readBlockChange = await graphPO.collectGraphEventDetails<{ block: { id: string } }>("block-change"); | ||
| const readGeom = await graphPO.collectGraphEventDetails<{ blocks: { id: string }[] }>("blocks-geometry-change"); |
There was a problem hiding this comment.
suggestion (testing): Consider adding a targeted test for the drag-end flush behaviour of blocks-geometry-change
The current test only verifies that blocks-geometry-change is emitted less often than block-change, which is a coarse confirmation of batching. To exercise the drag-end flush specifically, add a test that performs a very short drag with minimal/zero waitFrames and asserts that at least one blocks-geometry-change event occurs after mouseup, confirming flushBatchedBlocksGeometryEmit runs correctly at drag end.
Suggested implementation:
const readGeom = await graphPO.collectGraphEventDetails<{ blocks: { id: string }[] }>("blocks-geometry-change");You’ll need to add a new test inside the existing test.describe("block-change + coalesced blocks-geometry-change", () => { ... }) block, after the existing "single-block drag emits both..." test. The test should:
-
Initialize the graph as in the existing test (reuse
TWO_BLOCKSandDRAG_SETTINGS):const graphPO = new GraphPageObject(page); await graphPO.initialize({ blocks: TWO_BLOCKS, connections: [], settings: DRAG_SETTINGS, });
-
Begin collecting events before starting the drag:
const readBlockChange = await graphPO.collectGraphEventDetails<{ block: { id: string } }>("block-change"); const readGeom = await graphPO.collectGraphEventDetails<{ blocks: { id: string }[] }>("blocks-geometry-change");
-
Perform a very short drag on a single block with minimal batching opportunity (no
waitFramesorwaitFrames: 0if your helper requires it). For example, if yourGraphPageObjecthas a drag helper similar to:await graphPO.dragBlockBy("a", { x: 5, y: 5 });
or, if you have to be explicit:
await graphPO.dragBlock("a", { to: { x: 105, y: 105 }, waitFrames: 0 });
-
After
mouseuphas occurred (i.e. after the drag helper resolves), stop collecting events and inspect them. You want to assert:- At least one
block-changeoccurred during the drag. - At least one
blocks-geometry-changeevent was emitted after the lastblock-changeevent, proving the drag-end flush.
Assuming
collectGraphEventDetailsgives you an async iterator-style reader with.readAll()or a similar method that returns an array of events with timestamps, an example assertion might look like:test("short drag flushes blocks-geometry-change on drag end", async ({ page }) => { const graphPO = new GraphPageObject(page); await graphPO.initialize({ blocks: TWO_BLOCKS, connections: [], settings: DRAG_SETTINGS, }); const readBlockChange = await graphPO.collectGraphEventDetails<{ block: { id: string } }>("block-change"); const readGeom = await graphPO.collectGraphEventDetails<{ blocks: { id: string }[] }>("blocks-geometry-change"); // Perform a minimal drag on block "a" with no extra frame waits. await graphPO.dragBlockBy("a", { x: 5, y: 5 /*, waitFrames: 0 if required */ }); const blockChangeEvents = await readBlockChange.readAll(); const geomEvents = await readGeom.readAll(); expect(blockChangeEvents.length).toBeGreaterThan(0); expect(geomEvents.length).toBeGreaterThan(0); // If events include timestamps (e.g. event.timestamp), assert that at least one // geometry-change event is emitted after the last block-change event. const lastBlockChangeTime = Math.max(...blockChangeEvents.map(e => e.timestamp)); const geomAfterMouseup = geomEvents.filter(e => e.timestamp > lastBlockChangeTime); expect(geomAfterMouseup.length).toBeGreaterThan(0); });
- At least one
-
If your event detail objects don’t yet carry timestamps, you can instead:
- Use
performance.now()or a monotonic counter injected in your test harness, or - Enforce ordering based on the collection semantics of
collectGraphEventDetails(e.g. if it preserves sequence, you can assert that the lastblocks-geometry-changeis emitted after the last drag step, and that at least one such event exists even for a single step drag).
- Use
In summary, add a test("short drag flushes blocks-geometry-change on drag end", ...) to the suite, following the pattern above, and adapt:
- The drag helper (
dragBlockBy/dragBlock), and - How you read/inspect events (
readAll, timestamps, etc.)
to match whatGraphPageObjectandcollectGraphEventDetailsactually expose in your codebase.
Contributor
|
Preview is ready. |
…t block only Made-with: Cursor
… block snapshot, e2e order Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
blocks-geometry-changeto at most once per animation frame whenever block position updates viaBlockListStore.updatePosition.block-changewith a shallow block snapshot (asTBlockShallow) sopreventDefaultcan skip the store update and the corresponding geometry batch entry.onBlocksGeometryChangemapped toblocks-geometry-change.blocks-geometry-changein addition toblock-change.DragService).e2e/tests/block/block-geometry-batching.spec.ts.Notes
blocks-geometry-changepayload:{ id, x, y, width, height }per block only.TBlockGeometrySnapshottype from the package root.Made with Cursor
Summary by Sourcery
Introduce coalesced per-frame block geometry change events and wire them through the store, plugins, React API, and drag lifecycle.
New Features:
Enhancements:
Tests: