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:
📝 WalkthroughWalkthroughAdds HTML Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
![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/affine/blocks/paragraph/src/adapters/html.ts`:
- Around line 190-201: The leave handler for case 'p' must skip calling
walkerContext.closeNode() when enter emitted multiple self-contained blocks; in
the enter branch where you compute delta via deltaConverter.astToDelta(o.node),
call getParagraphDeltas(o.node, delta) and if deltas.length > 1, set a
local/global flag on walkerContext (e.g.,
walkerContext.setLocalContext('p:multi', true)) before calling
openParagraphBlocks(...) and walkerContext.skipAllChildren(); then in the
corresponding leave handler for 'p' check that flag
(walkerContext.getLocalContext('p:multi') or clear it) and return early (skip
closeNode()) when it's true so you don't attempt to close a node that was
already closed by openParagraphBlocks.
In `@blocksuite/affine/shared/src/services/font-loader/font-loader-service.ts`:
- Line 119: The forEach callback in the FontLoaderService is using an
expression-bodied arrow which implicitly returns the boolean from
document.fonts.delete(), triggering Biome; change the callback to a block body
or a for...of loop to avoid implicit return—e.g., update the
this.fontFaces.forEach(fontFace => document.fonts.delete(fontFace)) call in the
font cleanup logic to use a block-bodied arrow (call
document.fonts.delete(fontFace) inside braces) or replace it with a for (const
fontFace of this.fontFaces) loop so no value is implicitly returned.
In `@blocksuite/framework/std/src/inline/utils/point-conversion.ts`:
- Around line 79-81: textPointToDomPoint can see stale cache within the same
synchronous task because MutationObserver runs asynchronously; when
textNodeIndexMap.get(text) returns undefined you should avoid silently returning
null for newly-added nodes by synchronously invalidating/rebuilding the inline
root cache. Modify textPointToDomPoint (or its caller) to call
invalidateInlineRootTextCache(rootElement) and rebuild
textNodeIndexMap/prefixLengths when textNodeIndexMap.get(text) is undefined (or
add an explicit invalidateInlineRootTextCache(rootElement) at mutation sites
where DOM changes occur), ensuring the dirty flag and maps are refreshed before
returning.
🧹 Nitpick comments (9)
packages/frontend/core/src/desktop/pages/workspace/detail-page/detail-page.css.ts (1)
50-54: The!importantondisplay: 'table'is fragile — consider documenting why it's needed.Using
display: table+minWidth: 100%as a layout optimization is a known pattern, but the!importantflag and the:first-childselector couple this to a specific DOM structure and competing style. A brief comment explaining what style is being overridden and whytablelayout helps performance here would aid future maintainers.blocksuite/affine/blocks/paragraph/src/adapters/html.ts (2)
40-75:splitDeltaByNewlinecan produce empty delta arrays for consecutive newlines.When
insertis exactly'\n'or contains consecutive'\n'characters, the function produces emptyDeltaInsert[]entries (e.g.,[[], []]). These flow intoopenParagraphBlocksand emit paragraph blocks with empty deltas. If this is intentional (to represent blank lines from<br><br>), it's fine. If not, consider filtering out empty arrays before returning.- return lines; + return lines.filter(line => line.length > 0);
110-130:walkerContextis typed asany.Consider using the actual walker context type from the adapter framework instead of
anyto preserve type safety. This would catch misuse at compile time (e.g., callingopenNodewith wrong arguments).blocksuite/framework/std/src/inline/utils/text.ts (3)
11-56: Good caching design with WeakMap and lazy rebuild.The cache structure is well thought out:
WeakMapfor DOM node keys avoids preventing GC,prefixLengthsavoids recomputation, and the dirty-flag pattern withMutationObserveris a clean invalidation strategy.One observation:
buildInlineRootTextCacheduplicates the text node extraction logic fromgetTextNodesFromElement(lines 90–104). Consider reusinggetTextNodesFromElementinsidebuildInlineRootTextCacheto keep a single source of truth for text node discovery.♻️ Proposed refactor to reuse getTextNodesFromElement
This would require a small internal variant of
getTextNodesFromElementthat also yields each node's parent text-span element, or you could refactorbuildInlineRootTextCacheto callgetTextNodesFromElement(rootElement)and then look up parent info. Example sketch:const buildInlineRootTextCache = ( rootElement: HTMLElement, cache: InlineRootTextCache ) => { - const textSpanElements = Array.from( - rootElement.querySelectorAll('[data-v-text="true"]') - ); - const textNodes: Text[] = []; - const textNodeIndexMap = new WeakMap<Text, number>(); - const prefixLengths: number[] = []; - let prefixLength = 0; - - for (const textSpanElement of textSpanElements) { - const textNode = Array.from(textSpanElement.childNodes).find( - (node): node is Text => node instanceof Text - ); - if (!textNode) continue; - prefixLengths.push(prefixLength); - textNodeIndexMap.set(textNode, textNodes.length); - textNodes.push(textNode); - prefixLength += calculateTextLength(textNode); - } + const textNodes = getTextNodesFromElement(rootElement); + const textNodeIndexMap = new WeakMap<Text, number>(); + const prefixLengths: number[] = []; + let prefixLength = 0; + + for (let i = 0; i < textNodes.length; i++) { + const textNode = textNodes[i]; + prefixLengths.push(prefixLength); + textNodeIndexMap.set(textNode, i); + prefixLength += calculateTextLength(textNode); + }
58-88: MutationObserver is never disconnected — potential resource concern for long-lived roots.The observer is created once per
rootElementand never disconnected. While theWeakMapkeying onrootElementmeans the cache can be GC'd when the element is collected, if the root element has a long lifecycle (e.g., it's reused or kept alive), the observer will persist indefinitely. IfrootElementis ever detached and reattached, or if the inline editor is "disposed" while the element remains referenced, the observer continues to fire and mark dirty.Consider adding an explicit teardown method (e.g.,
clearInlineRootTextCache(rootElement)) that disconnects the observer and deletes the cache entry, to be called during editor disposal.
72-81: MutationObserver doesn't watchattributes— could miss dynamicdata-v-textattribute changes.The observer watches
childListandcharacterDatabut notattributes. Ifdata-v-textattributes are dynamically toggled on existing elements (without adding/removing elements), the cache won't be invalidated. Currently, the codebase only sets this attribute once during component creation, so this is not an immediate concern. If attribute-based dynamic changes are introduced later, consider addingattributeFilter: ['data-v-text']to the observer options.blocksuite/affine/shared/src/services/font-loader/font-loader-service.ts (3)
41-68: Consider extracting theWindowtype augmentation to reduce duplication.The
requestIdleCallback/cancelIdleCallbacktype cast appears in both_scheduleDeferredLoadand_cancelDeferredLoad. A shared type alias or a single cast at the class level would be cleaner.♻️ Suggested refactor
+type WindowWithIdleCallback = Window & { + requestIdleCallback?: ( + callback: () => void, + options?: { timeout?: number } + ) => number; + cancelIdleCallback?: (handle: number) => void; +}; + export class FontLoaderService extends LifeCycleWatcher {Then use
const win = window as WindowWithIdleCallback;in both methods.Also applies to: 70-90
111-112: Nit: extract the eager-font count to a named constant.The magic number
3on line 111 isn't self-documenting. A constant likeEAGER_LOAD_COUNTalongsideDEFERRED_LOAD_DELAY_MSwould clarify intent.
33-35: Thereadygetter behavior should be clarified in documentation rather than flagged as a breaking change.The
readygetter does dynamically reflect the currentfontFacesarray state, so a promise obtained immediately after mount will initially cover only the 3 eager fonts. However, the actual usage pattern in the codebase (edgeless-root-block.tscallsthis.fontLoader.readyduringfirstUpdated()and awaits it immediately) is unaffected by deferred loading.The semantic shift is real but the practical impact is minimal: consumers obtain the
readypromise fresh during initialization rather than caching it early, so they don't encounter the scenario where deferred fonts arrive after a cached promise has already resolved. To prevent future issues, consider:
- Documenting that
readyincludes only currently-loaded fonts- Adding a comment explaining the deferred loading strategy
- Or, provide a separate method that guarantees all configured fonts (eager + deferred)
blocksuite/affine/shared/src/services/font-loader/font-loader-service.ts
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## canary #14429 +/- ##
==========================================
+ Coverage 51.70% 53.52% +1.81%
==========================================
Files 2828 2831 +3
Lines 152747 153338 +591
Branches 22527 22981 +454
==========================================
+ Hits 78985 82070 +3085
+ Misses 70609 68056 -2553
- Partials 3153 3212 +59
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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@blocksuite/affine/blocks/root/src/clipboard/page-clipboard.ts`:
- Around line 164-180: The fallback paste path can call this.std.clipboard.paste
with undefined parent context when document.querySelector('affine-page-root') or
focusFirstParagraph() fails; update the code around firstParagraphId/parentModel
to bail out early if parentModel is not resolved (e.g., if !firstParagraphId ||
!parentModel then return or skip paste) so paste(e, this.std.store,
parentModel?.id, insertIndex) is only invoked when a valid parentModel and
insertIndex exist; adjust the logic that obtains firstParagraphId (via
focusFirstParagraph) and parentModel and ensure paste is only called when
parentModel.id is defined.
🧹 Nitpick comments (11)
blocksuite/framework/std/src/inline/utils/text.ts (2)
26-43: DRY:buildInlineRootTextCacheduplicates the text-node extraction logic fromgetTextNodesFromElement.Both functions query
[data-v-text="true"]and extract childTextnodes using the same pattern. Consider havingbuildInlineRootTextCachecallgetTextNodesFromElement(or a shared helper) to avoid the duplication.♻️ Sketch
const buildInlineRootTextCache = ( rootElement: HTMLElement, cache: InlineRootTextCache ) => { - const textSpanElements = Array.from( - rootElement.querySelectorAll('[data-v-text="true"]') - ); - const textNodes: Text[] = []; const textNodeIndexMap = new WeakMap<Text, number>(); const prefixLengths: number[] = []; let prefixLength = 0; - - for (const textSpanElement of textSpanElements) { - const textNode = Array.from(textSpanElement.childNodes).find( - (node): node is Text => node instanceof Text - ); - if (!textNode) continue; - prefixLengths.push(prefixLength); - textNodeIndexMap.set(textNode, textNodes.length); - textNodes.push(textNode); + const textNodes = getTextNodesFromElement(rootElement); + + for (const [i, textNode] of textNodes.entries()) { + prefixLengths.push(prefixLength); + textNodeIndexMap.set(textNode, i); prefixLength += calculateTextLength(textNode); }Also applies to: 97-111
79-88: MutationObserver is never disconnected.The observer is created once per root element and never
disconnect()-ed. In practice theWeakMap+ browser GC semantics should handle cleanup when the element is detached and unreferenced, but if root elements are swapped or recreated within the editor lifetime you may accumulate orphan observers.Consider exposing a teardown path (e.g.,
disposeInlineRootTextCache(rootElement)) that callsobserver.disconnect()and deletes the cache entry, to be invoked when an inline editor is destroyed.blocksuite/framework/std/src/inline/utils/point-conversion.ts (1)
12-17: Note:getTextNodesFromElementis still imported alongside the new cache utilities.
getTextNodesFromElementis used by other functions in this file (nativePointToTextPoint,getTextPointRoughlyFromElement, etc.) that haven't been migrated to the cache. This is fine — just flagging that those call sites could benefit from the cache in a follow-up if they turn out to be hot paths.blocksuite/affine/shared/src/services/font-loader/font-loader-service.ts (3)
56-62: Duplicatedwindowtype augmentation.The
requestIdleCallback/cancelIdleCallbacktype extension is defined in both_scheduleDeferredLoadand_cancelDeferredLoad. Consider extracting a shared type alias at the module or class level.♻️ Suggested refactor
+type IdleCallbackWindow = Window & { + requestIdleCallback?: ( + callback: () => void, + options?: { timeout?: number } + ) => number; + cancelIdleCallback?: (handle: number) => void; +}; + export class FontLoaderService extends LifeCycleWatcher {Then use
const win = window as IdleCallbackWindow;in both methods.Also applies to: 102-104
84-89: Magic number2000forrequestIdleCallbacktimeout.The other timing values (
DEFERRED_LOAD_DELAY_MS,DEFERRED_LOAD_BATCH_INTERVAL_MS) are named constants, but thistimeout: 2000is inline. Consider extracting it for consistency.
140-143: Hardcoded eager-font count3.The split between eager and deferred fonts is governed by the magic number
3. A named constant (e.g.,EAGER_LOAD_COUNT) would make the intent clearer and the threshold easier to tune.♻️ Suggested refactor
+ private static readonly EAGER_LOAD_COUNT = 3; + // ... override mounted() { const config = this.std.getOptional(FontConfigIdentifier); if (!config || config.length === 0) { return; } - const eagerFonts = config.slice(0, 3); - const deferredFonts = config.slice(3); + const eagerFonts = config.slice(0, FontLoaderService.EAGER_LOAD_COUNT); + const deferredFonts = config.slice(FontLoaderService.EAGER_LOAD_COUNT); this.load(eagerFonts); this._scheduleDeferredLoad(deferredFonts); }blocksuite/affine/blocks/root/src/clipboard/page-clipboard.ts (1)
166-168: Direct DOM query in fallback scenario is appropriate; tag coupling risk is minimal.While
document.querySelector('affine-page-root')does couple to a custom element tag, this is a legitimate fallback pattern used defensively when selection state is stale. The tag is stable, centrally defined, and consistently used across the codebase. No clearer framework-level alternative exists for directly accessing the page root in this context. If you prefer to reduce coupling, consider extracting the tag name to a constant shared witheffects.ts:14, though the current approach is acceptable for this defensive code path.blocksuite/framework/std/src/inline/services/render.ts (2)
62-70: Consider naming the throttle constant for clarity.
0x3f(63) is a bitmask that fires the\rvalidation every 64th event. This is a nice optimization, but the magic number is not self-documenting.♻️ Optional: extract a named constant
+// Validate every 64 Y.Text events (bitmask: 0x3F = 63) +private static readonly CR_VALIDATION_INTERVAL_MASK = 0x3f; + ... - if ( - (this._carriageReturnValidationCounter++ & 0x3f) === 0 && - yText.toString().includes('\r') - ) { + if ( + (this._carriageReturnValidationCounter++ & RenderService.CR_VALIDATION_INTERVAL_MASK) === 0 && + yText.toString().includes('\r') + ) {
163-166: Lit render errors are silently swallowed.The catch block recovers by calling
rerenderWholeEditor(), but the original error is discarded. This could make it difficult to diagnose persistent IME-related rendering issues. Consider logging it before recovery.♻️ Suggested: log before recovery
- } catch { + } catch (error) { // Lit may be crashed by IME input and we need to rerender whole editor for it + console.warn('Lit render failed, attempting full re-render:', error); this.editor.rerenderWholeEditor(); }blocksuite/affine/widgets/drag-handle/src/watchers/edgeless-watcher.ts (1)
71-79: RAF-debounced surface updates with id filtering — good improvement.
_scheduleShowDragHandleFromSurfaceUpdatecoalesces rapid surface/block updates into a single RAF. The subscription at line 374 filters byidto avoid processing updates for unrelated elements. The block update subscription (line 360) similarly checkspayload.id.One subtle point: the
_surfaceElementUpdatedRafIdRAF (line 74) only calls_showDragHandle(), which itself schedules another RAF (_showDragHandleRafId). This means a surface update results in a two-frame delay (one RAF to schedule, another to flush). If single-frame latency matters for visual feedback during element resizing/moving, consider calling_flushShowDragHandledirectly from the surface update RAF.Also applies to: 366-378
blocksuite/affine/blocks/paragraph/src/adapters/html.ts (1)
110-130: TypewalkerContextparameter in three helper functions to improve compile-time safety.
openParagraphBlocks,markMultiParagraphEmitted, andconsumeMultiParagraphEmittedMarkcurrently acceptwalkerContext: any. SinceASTWalkerContextis importable from@blocksuite/storeand this adapter converts toBlockSnapshot, these parameters should be typed asASTWalkerContext<BlockSnapshot>. This would catch misuse at compile time (e.g., typos inopenNode/closeNode/setGlobalContextcalls). Other adapters in the codebase follow this pattern (e.g., image block adapter).
…improve-editor-performance
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@blocksuite/affine/shared/src/services/font-loader/font-loader-service.ts`:
- Around line 42-44: The current ready getter on the FontLoaderService only
awaits the current entries in fontFaces (via fontFace.loaded) and thus resolves
before deferred batches are added; update the implementation so callers can
await "all configured fonts" by either (A) introducing an allFontsReady
Promise/Deferred that is resolved only when all deferred batches finish and have
been pushed into fontFaces (reference: add an allFontsReady property and resolve
it after the deferred batch completion code that populates fontFaces), or (B)
change the ready getter behaviour to track both existing fontFace.loaded
promises and any future batch-completion promise (e.g., combine
fontFaces.map(f=>f.loaded) with a batchCompletion promise) so it does not
resolve prematurely; alternatively if the original semantics are intended, add a
clear doc comment on the ready getter explaining it only reflects fonts loaded
so far. Ensure you modify the code paths that push deferred fonts into fontFaces
to resolve the allFontsReady/deferred signal when done.
In `@blocksuite/affine/widgets/drag-handle/src/watchers/pointer-event-watcher.ts`:
- Around line 175-178: When closestBlock is null you only clear
_lastPointerHitBlockId and widget.anchorBlockId.value but forget to clear the
cached DOM element; update the branch in pointer-event-watcher that handles the
null closestBlock to also set this._lastPointerHitBlockElement = null so both
caches (_lastPointerHitBlockId and _lastPointerHitBlockElement) are reset
together (matching the other reset paths that clear both fields).
🧹 Nitpick comments (3)
blocksuite/affine/shared/src/services/font-loader/font-loader-service.ts (2)
50-76: Hardcoded critical-font list may drift from actual usage.
_isCriticalCanvasFonthardcodes specificFontFamily+FontWeightcombinations (Poppins, Inter, Kalam). If font usage changes in the editor (e.g., a new default family or weight), this will silently fall through to theconfig.slice(0, 3)fallback, which may not pick the right fonts.Consider co-locating the critical-font definitions with the font config or documenting the coupling so future changes don't accidentally regress initial-render quality.
85-91: DuplicaterequestIdleCallback/cancelIdleCallbacktype augmentation.The same
Window &cast appears in both_scheduleDeferredLoad(line 85) and_cancelDeferredLoad(line 131). Extract a shared type alias to keep it DRY and consistent if the signature ever changes.♻️ Suggested extraction
+type WindowWithIdleCallback = Window & { + requestIdleCallback?: ( + callback: () => void, + options?: { timeout?: number } + ) => number; + cancelIdleCallback?: (handle: number) => void; +}; + private readonly _scheduleDeferredLoad = (fonts: FontConfig[]) => { ... - const win = window as Window & { - requestIdleCallback?: (...) => number; - cancelIdleCallback?: (...) => void; - }; + const win = window as WindowWithIdleCallback; ... }; private readonly _cancelDeferredLoad = () => { ... - const win = window as Window & { - cancelIdleCallback?: (handle: number) => void; - }; + const win = window as WindowWithIdleCallback; ... };Also applies to: 131-133
blocksuite/framework/std/src/inline/utils/point-conversion.ts (1)
92-111: Good retry strategy addressing the MutationObserver timing gap.The two-attempt loop with
invalidateInlineRootTextCacheon miss is a clean way to handle the async observer issue. One minor concern:On line 97,
return nullinside the loop short-circuits both the retry (attempt 1) and the linear fallback (lines 115–131). If the cache is stale-empty (e.g. built before nodes were added, observer hasn't fired yet), we bail out without ever invalidating or falling through to the DOM-based scan — even though line 73 already confirmedrootElement.contains(text).Replacing the early return with
continueon the first attempt would let the invalidation + rebuild path run before giving up:Suggested change
for (let attempt = 0; attempt < 2; attempt++) { const { textNodes, textNodeIndexMap, prefixLengths, lineIndexMap } = getInlineRootTextCache(rootElement); - if (textNodes.length === 0) return null; + if (textNodes.length === 0) { + if (attempt === 0) { + invalidateInlineRootTextCache(rootElement); + continue; + } + break; // fall through to linear fallback + } const goalIndex = textNodeIndexMap.get(text);This is an unlikely edge case given the containment guard, so feel free to defer.
blocksuite/affine/shared/src/services/font-loader/font-loader-service.ts
Show resolved
Hide resolved
blocksuite/affine/widgets/drag-handle/src/watchers/pointer-event-watcher.ts
Show resolved
Hide resolved
1 participant
PR Dependency Tree
This tree was auto-generated by Charcoal
Summary by CodeRabbit
New Features
into separate paragraphs while preserving inline formatting.
Bug Fixes
Style
Tests
-based paragraph splitting; re-enabled an e2e drag-page test.
Chores