perf: Reduce re-renders and network requests during drag and drop#276
perf: Reduce re-renders and network requests during drag and drop#276rickbutterfield merged 4 commits intov5/devfrom
Conversation
…and drop - Remove 500ms debounce timer, render previews immediately on data change - Add deep comparison (hasChanged) on content/settings properties to skip re-renders when Umbraco reassigns same data with new object references - Add catch-up rendering in observer callbacks — only triggers render if no successful render has occurred yet, solving initial load race condition without causing re-renders on every sort operation - Remove custom sort mode handling (sortModeActive, observeSortMode, renderSortModeFallback) — handled natively by Umbraco since 17.2 - Switch stylesheets from <link> elements to adoptedStyleSheets with CSSStyleSheet objects cached on shared BlockPreviewContext, eliminating all stylesheet re-fetching during re-renders or element recreation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
test connection |
CLAUDE.md - BlockPreview v5OverviewBlockPreview is an Umbraco community package that enables rich HTML backoffice previews for Block Grid, Block List, and Rich Text editors. Technologies:
Target Platform: Umbraco CMS v17+ Package: Umbraco.Community.BlockPreview on NuGet Repository StructureProject Dependencies:
Build CommandsFull Solution: dotnet build Umbraco.Community.BlockPreview.slnMain Package (Release): dotnet build src/Umbraco.Community.BlockPreview/Umbraco.Community.BlockPreview.csproj --configuration Release
dotnet build examples/Umbraco.Community.BlockPreview.TestSite/Umbraco.Community.BlockPreview.TestSite.csprojFrontend Assets: cd src/Umbraco.Community.BlockPreview.UI
# Install dependencies (requires Node.js >=22.12.0)
npm install
# Development mode with hot reload
npm run dev
# Build for production
npm run buildRun Test Site: dotnet run --project examples/Umbraco.Community.BlockPreview.TestSiteTest site credentials:
VersioningUses Nerdbank.GitVersioning for automatic versioning.
Teamwork & CollaborationBranching:
CI/CD:
Contributing: See
Quick ReferenceKey Directories:
Projects:
Documentation: Architecture NotesRazor view caching: Never cache Debugging approach: When investigating rendering bugs, add diagnostic logging first before building fixes. Symptoms like empty strings, disposed object exceptions, and load-dependent failures can all stem from a single shared-state concurrency bug. Log the actual exception types and locations to avoid misdiagnosing the root cause. |
ChangelogAll notable changes to this project will be documented in this file. The format is based on Keep a Changelog, 5.3.2 - 2026-02-12Fixed
5.3.1 - 2026-02-12Fixed
5.3.0 - 2026-02-12Added
Fixed
5.2.1 - 2026-01-23Fixed
5.2.0 - 2026-01-22Added
Fixed
New Contributors: @lakesol (#221), @skttl (#218) 5.1.0 - 2026-01-08Added
Changed
Fixed
New Contributors: @Copilot (#175), @GianniDPC (#214) 5.0.0 - 2025-11-27Added
Changed
New Contributors: @BenWhite27 (#116) v4 (Umbraco 16)4.2.2 - 2026-02-12Fixed
4.2.1 - 2026-02-12Fixed
4.2.0 - 2026-02-12Added
Fixed
Changed
4.1.1 - 2026-01-23Fixed
4.1.0 - 2026-01-22Added
Changed
Fixed
4.0.7 - 2025-11-28Fixed4.0.6 - 2025-11-27Changed
Full Changelog: https://github.com/rickbutterfield/BlockPreview/releases |
|
{ |
|
test123 |
|
{ |
ChangelogAll notable changes to this project will be documented in this file. The format is based on Keep a Changelog, 5.3.2 - 2026-02-12Fixed
5.3.1 - 2026-02-12Fixed
5.3.0 - 2026-02-12Added
Fixed
5.2.1 - 2026-01-23Fixed
5.2.0 - 2026-01-22Added
Fixed
New Contributors: @lakesol (#221), @skttl (#218) 5.1.0 - 2026-01-08Added
Changed
Fixed
New Contributors: @Copilot (#175), @GianniDPC (#214) 5.0.0 - 2025-11-27Added
Changed
New Contributors: @BenWhite27 (#116) v4 (Umbraco 16)4.2.2 - 2026-02-12Fixed
4.2.1 - 2026-02-12Fixed
4.2.0 - 2026-02-12Added
Fixed
Changed
4.1.1 - 2026-01-23Fixed
4.1.0 - 2026-01-22Added
Changed
Fixed
4.0.7 - 2025-11-28Fixed4.0.6 - 2025-11-27Changed
Full Changelog: https://github.com/rickbutterfield/BlockPreview/releases |
|
{ |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code ReviewOverall this is a solid performance PR with well-motivated changes. The refactoring to a base class is a nice improvement, and the adoptedStyleSheets approach is the right call for eliminating redundant fetches. A few issues worth discussing before merging. Potential Bugs1. Accumulating context subscriptions in observeBlockPropertyValue In all three concrete elements, observeBlockValue() sets up an observation callback that calls observeBlockPropertyValue() every time block entry context values change. Each call creates a new consumeContext subscription without cleaning up the previous one. Over the lifetime of a session, subscriptions accumulate. The fix: guard with a flag so the manager context is only subscribed once, or replace rather than stack the subscription. 2. blockIndex in block-grid is always 0 block-grid-preview.custom-view.element.ts line 138: blockGridValue.contentData is assigned contents (not filtered by contentUdi like block-list does), so contentData[0] === contents[0] and indexOf always returns 0. Suggest using: contents.findIndex(x => x.key === this._blockContext.contentUdi) 3. getOrCreateStylesheet is not safe under concurrent requests block-preview.context.ts lines 60-70: two elements requesting the same stylesheet concurrently both miss the cache, both fetch it, and the second silently overwrites the first. A proper guard stores the in-flight Promise to deduplicate concurrent requests. 4. RTE: observeBlockWorkspaceFallback called unconditionally rich-text-preview.custom-view.element.ts line 73 calls observeBlockWorkspaceFallback() synchronously right after starting consumeContext(UMB_DOCUMENT_WORKSPACE_CONTEXT). Since context resolution is async, _workspaceContextResolved is still false at that point, so the fallback always registers. If the document workspace resolves later, both code paths call handleWorkspaceData then observeBlockValue(), potentially creating duplicate entry context subscriptions. Breaking Change: RC dependencyDirectory.Packages.props bumps the minimum to [17.3.0-rc, 18.0.0). The PR notes sort mode was handled natively since 17.2, but the package now requires an unreleased RC. Users on stable 17.1 or 17.2 cannot install this version. Is this intentional pending 17.3 GA? Also: removing packageSourceMapping from NuGet.config and placing the Umbraco prereleases MyGet feed before nuget.org could cause unexpected package resolution in CI. Consider restoring source mapping or placing the prerelease feed after nuget.org. Design NotesCatch-up mechanism assumption The guard on !this._htmlMarkup && !this._isLoading means that after the first successful render, changes to layouts/exposes/propertyAlias from the manager context no longer trigger re-renders. Re-renders are driven solely by content/settings property changes via hasChanged in updated(). This is sound for drag-and-drop, but a brief comment documenting the intentional design would help future maintainers avoid accidentally removing the guard. JSON.stringify deep comparison The hasChanged guard on content/settings is the right technique. For very large block data it is O(n) on every assignment, but unlikely to be a practical bottleneck. Minor
Good work overall. The adoptedStyleSheets caching and deep-comparison guard are exactly the right fixes for the drag-and-drop churn. Items 1 and 2 are the ones most worth addressing before merging to avoid subtle rendering correctness issues. |
- Guard #observeBlockPropertyValue with flag to prevent accumulating context subscriptions on repeated entry context emissions - Fix blockIndex in block-grid always returning 0 by using findIndex with contentUdi instead of indexOf on unfiltered contentData - Deduplicate concurrent stylesheet fetches by caching the in-flight Promise rather than the resolved CSSStyleSheet - Fix RTE duplicate subscriptions by wrapping document workspace context in try/catch so fallback only registers on failure - Set Umbraco dependency to [17.3.0, 18.0.0) (stable, not RC) - Remove accidental testing-1-2-3.config scratch data Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Code review - see details below |
Code ReviewThe overall direction here is solid. Deep comparison to prevent unnecessary re-renders during drag-and-drop, Bug: File:
A cleaner approach: track resolution inside the callback and explicitly call the fallback if the context was never resolved, rather than relying on a try/catch that never fires. Inconsistency: File: BlockGrid was correctly updated to: this._blockContext.blockIndex = contents.findIndex(x => x.key === this._blockContext.contentUdi);BlockList still uses: this._blockContext.blockIndex = contents?.indexOf(this.blockListValue.contentData[0]);
File:
This works in practice but has fragile corners: property key ordering differences will cause false positives/negatives; Failed stylesheet fetches are permanently cached File: The promise.catch(() => this.#stylesheetCache.delete(href));
this.#stylesheetCache.set(href, promise);Silent failure when File: Removing the error/loading state assignment when validation fails means the preview silently shows nothing when data is genuinely missing. During initial load this silence is fine, but for permanent failures the user gets no indication. Consider setting the error only after the workspace context has resolved and validation still fails. Minimum version floor may be too aggressive File: The PR description says sort mode is handled natively since 17.2, yet the floor is raised to 17.3, excluding users on 17.2 without a stated reason. If there is no 17.3-specific API requirement (just an RC used for dev), NuGet.config: Removing source mapping allows packages to be resolved from either Minor: missing newline at EOF
Overall this is a meaningful performance improvement and the |
Code ReviewOverall this is a well-thought-out performance PR. The core ideas — immediate renders instead of 500ms debounce, Bugs1. Negative promise caching in
const promise = fetch(href)
.then(response => response.text())
.then(css => { ... });
this.#stylesheetCache.set(href, promise);If the fetch fails (network error, 404, etc.), the rejected promise is cached. All future calls return the same rejected promise — the stylesheet can never load without a page reload. Fix by removing from the cache on failure: const promise = fetch(href)
.then(r => { if (!r.ok) throw new Error(`Failed to fetch stylesheet: ${href}`); return r.text(); })
.then(css => { const s = new CSSStyleSheet(); s.replaceSync(css); return s; });
promise.catch(() => this.#stylesheetCache.delete(href));
this.#stylesheetCache.set(href, promise);2. No A 404 response won't throw — 3. Behavioral change in try {
this.consumeContext(UMB_DOCUMENT_WORKSPACE_CONTEXT, ...);
} catch {
this.observeBlockWorkspaceFallback();
}
4. In all three subclasses: if (!this._htmlMarkup && !this._isLoading) {
this.renderBlockPreview();
}This doesn't check Performance5. hasChanged: (val: any, old: any) => JSON.stringify(val) !== JSON.stringify(old)This serialises the entire block data on every property assignment. For large block grids this could be noticeable. Also, property key order differences can cause false positives ( Configuration concerns6. The PR adds the Umbraco prerelease MyGet feed and removes the 7. Minimum version bump: 17.1.0 → 17.3.0 The PR description says sort mode is "handled natively by Umbraco since 17.2", but the minimum is bumped to 17.3.0. Is 17.3.0 the true minimum for all the APIs used here, or should this be 17.2.0? Worth documenting the specific API that requires 17.3. Minor
Summary
The |
1 participant
Summary
hasChanged) oncontent/settingsproperties to prevent re-renders when Umbraco reassigns the same data with new object references during drag and droprenderBlockPreview()as a catch-up mechanism when no successful render has occurred yet — fixes initial load race condition without causing re-renders on every sortsortModeActive,observeSortMode,renderSortModeFallback) — this is handled natively by Umbraco since 17.2<link>elements toadoptedStyleSheetswithCSSStyleSheetobjects cached on the sharedBlockPreviewContext, eliminating all stylesheet re-fetching during re-renders or element recreationTest plan
🤖 Generated with Claude Code