feat: improve grouping perf in edgeless#14442
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces per-element reparenting with batchAddChildren/batchRemoveChildren, adds fractional-index ordering for group/ungroup, instruments operations with measureOperation and store.transact, introduces RAF coalescers and adaptive throttling for pointer/snap/pan, builds group-child and connector indexes, and adds image LOD thumbnail generation and viewport refresh throttling. Changes
Sequence Diagram(s)mermaid UI->>Cmd: createGroupFromSelectedCommand() Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## canary #14442 +/- ##
==========================================
+ Coverage 53.88% 53.93% +0.05%
==========================================
Files 2832 2835 +3
Lines 153319 153853 +534
Branches 22942 23065 +123
==========================================
+ Hits 82615 82984 +369
- Misses 67291 67458 +167
+ Partials 3413 3411 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/layer.ts`:
- Line 104: When unmounted() currently disposes subscriptions it fails to clear
the cached map _groupChildSnapshot, so leftover state causes incorrect deltas on
re-mount; update the unmounted() method to call this._groupChildSnapshot.clear()
after (or alongside) the existing disposable cleanup so the snapshot is reset
before a subsequent mounted() call.
In `@blocksuite/framework/std/src/gfx/viewport-element.ts`:
- Around line 126-161: _refreshViewportByViewportUpdate currently compares the
incoming update only to the previous update and can drop a final small-movement
update when no further events arrive; add a deferred guaranteed refresh:
introduce a timer field (e.g. _deferredViewportRefreshTimer) and on every call
to _refreshViewportByViewportUpdate clear that timer, then if the refresh
conditions are met do the immediate refresh as now and clear the timer,
otherwise schedule the timer via setTimeout to call this._refreshViewport()
after GfxViewportElement.VIEWPORT_REFRESH_MAX_INTERVAL (and update
_lastViewportRefreshTime inside that callback), ensuring the timeout is canceled
by subsequent updates so a final refresh runs when movement stops.
🧹 Nitpick comments (14)
blocksuite/affine/gfx/pointer/src/snap/snap-overlay.ts (2)
687-705: Short-circuit inshouldTryDistributeprevents cooldown tick-down when candidates are over the limit.Due to
&&short-circuiting, whenall.length > DISTRIBUTE_ALIGN_MAX_CANDIDATES,shouldRun()is never called and_remainingFrameswon't decrement. This means the cooldown can persist longer thanDISTRIBUTE_ALIGN_COOLDOWN_FRAMESactual align calls if the candidate count fluctuates above/below the threshold.With
DISTRIBUTE_ALIGN_COOLDOWN_FRAMES = 2, the practical impact is negligible — at most one extra skipped frame. If you want the cooldown to always tick down regardless, swap the evaluation order or always callshouldRun():Optional: always tick cooldown
- const shouldTryDistribute = - this._referenceBounds.all.length <= DISTRIBUTE_ALIGN_MAX_CANDIDATES && - this._distributeCooldown.shouldRun(); + const cooldownReady = this._distributeCooldown.shouldRun(); + const shouldTryDistribute = + this._referenceBounds.all.length <= DISTRIBUTE_ALIGN_MAX_CANDIDATES && + cooldownReady;
40-42: Candidate limit constant is a reasonable heuristic.
DISTRIBUTE_ALIGN_MAX_CANDIDATES = 160bounds the O(n²) distribute-alignment loops. Depending on typical scene sizes, this could be worth making configurable or documenting why 160 was chosen (e.g., empirical profiling). Not blocking.blocksuite/affine/blocks/surface/src/surface-model.ts (1)
61-68: Consider usinginstanceofor a more reliable type guard.The duck-typing check (
'type' in model && model.type === 'connector') will match any object with atypeproperty equal to'connector'. SincegetElementByIdreturns models from a known registry,instanceof ConnectorElementModelwould be safer and more maintainable. If there's a specific reasoninstanceofdoesn't work here (e.g., cross-realm or module duplication), a brief comment would help future readers.blocksuite/affine/blocks/image/src/image-edgeless-block.ts (4)
277-309: Side-effects during render may cause extra re-render cycles.
_resetLodSource(blobUrl)(line 280) and_ensureLodThumbnail(blobUrl)(line 305) perform state mutations and kick off async work insiderenderGfxBlock. While the guards prevent infinite loops, the async thumbnail generation callsthis.requestUpdate()on completion (line 214), which can trigger another render. Additionally,_lastShouldUseLodis written both here (line 307) and in_updateLodFromViewport(line 236), so each path can trigger a redundant update from the other.Consider moving the LOD orchestration logic entirely into reactive callbacks (e.g., the
blobUrl$subscription and viewport subscription already inconnectedCallback) and haverenderGfxBlocksimply read the current_lodThumbnailUrlstate without mutating it.
162-183: Consider JPEG format for smaller LOD thumbnails.
canvas.toBlob(resolve)defaults to PNG. At 256×256, a JPEG thumbnail at moderate quality would be significantly smaller in memory, which matters since the goal is performance improvement for large images.♻️ Suggested change
- return new Promise<Blob | null>(resolve => { - canvas.toBlob(resolve); - }); + return new Promise<Blob | null>(resolve => { + canvas.toBlob(resolve, 'image/jpeg', 0.7); + });
259-267:_lastShouldUseLodinitialized beforeblobUrlis likely resolved.At line 266,
this._shouldUseLod(this.blobUrl)is called synchronously inconnectedCallback. SinceblobUrlcomes fromresourceController.blobUrl$, which is populated asynchronously afterrefreshData()/subscribe(), this will almost always evaluate tofalseat this point. This isn't a bug (the viewport subscription will correct it later), but makes the initialization misleading. Consider removing it or adding a subscription onblobUrl$to properly react when the URL becomes available.
152-160: Thumbnail generation loads the full-resolution image on the main thread.
_createImageElementdecodes the full source image to produce a 256px thumbnail. For large images (≥1 MP / ≥1 MB), this defeats the purpose during the decoding step. Consider usingcreateImageBitmapwithresizeWidth/resizeHeightoptions, which lets the browser decode directly to the target size without materializing the full-resolution bitmap. This is well-supported (Chrome 54+, Firefox 98+, Safari 15+, Edge 79+). You could consolidate_createImageElementand_createThumbnailBlobinto a single method that takes the blobUrl directly, fetches the blob, resizes viacreateImageBitmap, and renders toOffscreenCanvas.blocksuite/framework/std/src/gfx/raf-coalescer.ts (1)
63-66: Nit:flushcallscancelFrameeven whenrafIdis null.Unlike
cancel()which guardsif (rafId !== null),flushunconditionally callscancelFrame(rafId). While browsers handlecancelAnimationFrame(null)/clearTimeout(null)gracefully, adding the same guard would be more consistent.♻️ Suggested change
flush() { - if (rafId !== null) cancelFrame(rafId); + if (rafId !== null) { + cancelFrame(rafId); + rafId = null; + } run(); },blocksuite/framework/std/src/gfx/viewport-element.ts (1)
44-46: Consider documenting the rationale for the magic numbers.
VIEWPORT_REFRESH_PIXEL_THRESHOLD = 18andVIEWPORT_REFRESH_MAX_INTERVAL = 120are tuning constants that affect perceived responsiveness. A brief inline comment explaining why these values were chosen (e.g., "18 px ≈ smallest perceptible block-edge shift" or "120 ms ≈ ~8 fps cap for block visibility") would help future maintainers.blocksuite/framework/std/src/gfx/layer.ts (1)
529-550: O(n × m) refresh for group children — acceptable for user-initiated ops but worth noting.
_refreshElementsInLayerdoes three passes overuniqueElements, each callingremoveFromOrderedArray/insertToOrderedArray/_removeFromLayer/_insertIntoLayer— all of which scan the fullcanvasElements,blocks, orlayersarrays. For a group with k children in a canvas of m total elements this is O(k × m) per pass.This is fine for typical user-initiated group/ungroup operations, but could become a bottleneck if called during bulk operations with hundreds of elements. If this proves hot, a batched rebuild (similar to
_initLayers) would be more efficient.blocksuite/affine/gfx/group/src/command/group-api.ts (1)
183-195: Naming inversion betweenbeforeSiblingIndex/afterSiblingIndexandafterIndex/beforeIndexparameters is confusing.
beforeSiblingIndex(the sibling before the group, i.e., lower index) is passed asafterIndex, andafterSiblingIndex(the sibling after the group, i.e., upper index) is passed asbeforeIndex. The semantics are correct (lower bound / upper bound), but the swapped naming between call site and parameter could easily mislead a future reader.Consider renaming
buildUngroupIndexesparameters tolowerBound/upperBoundfor clarity.blocksuite/framework/std/src/gfx/model/surface/surface-model.ts (2)
591-598: Minor redundancy:_rebuildGroupChildrenIndexre-does work from per-element_syncGroupChildrenIndexcalls.During
_initElementModels, each group model triggers_syncGroupChildrenIndex(lines 326, 441-442). Then_rebuildGroupChildrenIndexat line 445 clears everything and rebuilds from scratch. The rebuild ensures consistency, but the prior per-element calls are wasted work. Consider moving the_syncGroupChildrenIndexcalls inside the rebuild, or removing the early per-element calls.Also applies to: 445-446
636-654:_syncGroupChildrenIndexsilently overwrites parent mappings for contested children.If child
Xis listed in two different groups (a data integrity issue), the second group's_syncGroupChildrenIndexcall will overwrite the parent mapping forX, and the first group's entry won't be corrected. This is acceptable since multi-parent membership shouldn't happen, but a debug warning could help surface data corruption.blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
62-71: Use the existingisGfxGroupCompatibleModelhelper for container checks instead of duck-typingchildIds.The
'childIds' in modelcheck is fragile because it relies on property presence rather than explicit model type marking.isGfxGroupCompatibleModeluses a Symbol marker (gfxGroupCompatibleSymbol) to reliably identify containers, and is already widely used throughout the codebase for this purpose.Proposed improvement
+import { isGfxGroupCompatibleModel } from '../model/base.js'; + const shouldAllowDragMoveCoalescing = ( elements: { model: GfxModel }[] ): boolean => { return elements.every(({ model }) => { const isConnector = 'type' in model && model.type === 'connector'; - const isContainer = 'childIds' in model; + const isContainer = isGfxGroupCompatibleModel(model); return !isConnector && !isContainer; }); };
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/__tests__/gfx/surface.unit.spec.ts`:
- Around line 402-406: The test contains a vacuous assertion:
_parentGroupMap.set(shapeId, 'stale-group-id') stores 'stale-group-id' as a
value, but the assertion uses _parentGroupMap.has('stale-group-id') (which
checks keys), so it never meaningfully verifies cache cleanup; fix by either
removing that assertion or replacing it with a proper check—e.g., after calling
surfaceModel.getGroup(shapeId) assert that model._parentGroupMap.get(shapeId)
!== 'stale-group-id' or assert that the entry for shapeId equals fakeGroup.id
(use symbols _parentGroupMap, shapeId, surfaceModel.getGroup, and fakeGroup.id
to locate the code).
🧹 Nitpick comments (5)
blocksuite/integration-test/src/__tests__/edgeless/surface-model.spec.ts (1)
276-299: Consider exposing a test-only seam instead ofas anycasts on private fields.Accessing
_connectorIdsByEndpointand_connectorEndpointsviaas anycouples this test to internal field names. If those maps are renamed or restructured, this test silently breaks without a compile error. A lightweight alternative: expose a package-internal helper (e.g.,/**@internal*/ _seedEndpointCacheForTest(...)) or use a friend-pattern so TypeScript can still catch renames.That said, the test logic itself is correct and the purge assertions are valuable.
blocksuite/framework/std/src/__tests__/gfx/surface.unit.spec.ts (1)
359-417: Tests access private internals viaas any— acceptable for white-box cache tests but fragile.These tests directly manipulate private maps (
_parentGroupMap,_groupChildIdsMap,_groupLikeModels) and call private methods. This is fine for verifying internal cache invariants, but any internal rename will silently break these tests at runtime rather than at compile time. A brief comment noting the intentional coupling to internals would help future maintainers.blocksuite/affine/gfx/pointer/src/__tests__/pan-tool.unit.spec.ts (1)
14-32: Consider cleaning up the unusedgetCallbackhelper.
mockRafreturnsgetCallback(Line 28) which is never used in any of the three tests. If it's not needed for future tests in this file, removing it reduces noise.blocksuite/affine/gfx/group/src/__tests__/group-api.unit.spec.ts (2)
35-84: Fixture uses identical indices for left/right siblings — intentional but worth a brief comment.Both
leftandrightare created with index'a0'(Lines 37–38), which deliberately triggers the "invalid interval" fallback inbuildUngroupIndexes. This is the core setup for Test 1 but could confuse readers unfamiliar with the fallback logic. A short inline comment (e.g.,// same index to trigger invalid-interval fallback) would clarify intent.
130-151: Test asserts exact call count (4) for internal retry strategies — tightly coupled to implementation.
expect(mockedGenerateNKeysBetween).toHaveBeenCalledTimes(4)(Line 147) couples this test to the exact number of batch fallback strategies insidebuildUngroupIndexes. If a new strategy is added or one is removed, this test breaks without any behavioral change. Consider asserting that the key-by-key fallback was reached (i.e.,generateKeyBetweenwas called with expected args) rather than pinning the batch attempt count.
1 participant
fix #14433
PR Dependency Tree
This tree was auto-generated by Charcoal
Summary by CodeRabbit
New Features
Improvements
Documentation