Document & Media Workspace: Limit route generation (closes #22910)

[nielslyngsoe]

Only generate routes when the dependent data for the routes change.

Done by creating a createObservablePart of the variantOptions, in this way routes are only generated when the 'data of the variants that is relevant for the routes' change.

As well cleaned up an observation of isForbidden as it was only used once.

Fixes #22910

Replaces PR: #22958

No Unit Tests:

This PR has no unit tests, testing this scenario would require quite a bit of mocking, which is hard to maintain and could case tests that parse despite they in real life would not.

As Claude put it:
the test is probably not worth it, and it's likely to cause friction later

Test Notes:

See issue for aspects to test.

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
33.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[AndyButland]

This looks to be OK to me @nielslyngsoe. I've tested it using a custom ISegmentService to generate 150 segments:

using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Services.OperationStatus;

namespace Umbraco.Cms.Web.UI.Custom.Segments;

public class MySegmentComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.Services.AddUnique<ISegmentService, MySegmentService>();
        builder.Services.Configure<SegmentSettings>(settings => settings.Enabled = true);
    }
}

public class MySegmentService : ISegmentService
{
    // High segment count to reproduce the N² split-view route explosion from issue #22910.
    // Variant options ≈ cultures × (1 + SegmentCount); the workspace editor generates that count
    // squared in routes. 150 segments on a single culture (~22,800 routes) makes the per-keystroke
    // freeze clearly visible.
    private const int SegmentCount = 150;

    private readonly Segment[] _segments =
        Enumerable.Range(1, SegmentCount)
            .Select(i => new Segment { Alias = $"segment-{i:D3}", Name = $"Segment {i:D3}" })
            .ToArray();

    public Task<Attempt<PagedModel<Segment>?, SegmentOperationStatus>> GetPagedSegmentsAsync(int skip = 0, int take = 100)
        => Task.FromResult
        (
            Attempt.SucceedWithStatus<PagedModel<Segment>?, SegmentOperationStatus>
            (
                SegmentOperationStatus.Success,
                new PagedModel<Segment> { Total = _segments.Length, Items = _segments.Skip(skip).Take(take) }
            )
        );
}

With that in place, when I've opened up a segment variant document there's a very clear lag of several seconds with the code on v17/dev on every key press when you are typing in the name of the document. That's gone with my proposed PR, and also with yours. I also don't see any lag with switching between documents and variants, and the split view displays as expected.

So I'm fine going with this. It's simpler. I wonder if using a dynamic route (as in #22958) is arguably the more correct approach, since it avoids generating static routes for every variant pairing (≈22,800 here, from 151 × 151 variant options), but with your fix in place that generation no longer causes a practical problem.

As far as I can tell, the problematic code is the de-duplication check in umb-router-slot's routes setter at src/Umbraco.Web.UI.Client/src/packages/core/router/route/router-slot.element.ts:30-39. It uses filter + findIndex to detect path-set changes — O(n²).

From the original reporter's scenario, that was ~35k routes, so ~1.25 billion comparisons per keystroke, observed as a ~7-second freeze.

You've effectively fixed this by no longer regenerating the routes on every keystroke (the variant options no longer re-emit when only the document name changes), so the setter is no longer called with a fresh same-length route set — which is the only case that hits the expensive branch. You might consider also improving the algorithm itself, in case it gets called with a large route set again in future:

 	public set routes(value: UmbRoute[] | undefined) {
 		value ??= [];
 		const oldValue = this.#router.routes;
-		if (
-			value.length !== oldValue?.length ||
-			value.filter((route) => oldValue?.findIndex((r) => r.path === route.path) === -1).length > 0
-		) {
+		if (!oldValue || value.length !== oldValue.length) {
+			this.#router.routes = value;
+			return;
+		}
+
+		const oldPaths = new Set(oldValue.map((route) => route.path));
+		if (value.some((route) => !oldPaths.has(route.path))) {
 			this.#router.routes = value;
 		}
 	}

This takes it from O(n²) to O(n), so the comparison cost grows linearly with the route count rather than quadratically. The functionality is unchanged. It's the same "any new path, gated by a length check", just using a Set lookup instead of a per-element findIndex scan.

This is shared router infrastructure rather than part of your change, so it could be a follow-up, but flagging here mainly because this issue is what surfaced it.

[claude]

Claude finished @nielslyngsoe's task in 3m 56s —— View job

---

PR Review

Target: origin/v17/dev · Based on commit: 2bb5e9a8

Fixes a keystroke-lag regression caused by variantOptions emitting on every document name change, which triggered a full O(n²) route regeneration on each keypress. The fix uses createObservablePart to narrow the observable to only routing-relevant fields (culture, segment, unique), and lazily evaluates the forbidden state rather than reactively observing it.

• Other changes: forbidden.isOn is no longer observed reactively; the ** wildcard route now calls forbidden.getIsOn() synchronously at route-activation time. Routes are no longer regenerated when forbidden state changes mid-session.

---

Suggestions

• document-workspace-editor.element.ts:109–115: The early-return guard uses !this.#appCulture as a required condition, but #appCulture is only set once (when previousCulture === undefined) and never cleared. If somehow the language context fires after variants but produces undefined, routes silently stay as just the not-found route. This is unlikely in practice but the state dependency between two independent observers is worth keeping in mind if the language context behavior changes.

• router-slot.element.ts:30–39 (not changed in this PR, but surfaced here): The de-duplication check in the routes setter is O(n²) — filter + findIndex over the same array. This PR correctly prevents the setter from being called repeatedly, but the underlying algorithm will still hurt if a large route set is ever passed again. @AndyButland's suggested O(n) fix using a Set lookup is worth landing as a follow-up:

  -if (
  -    value.length !== oldValue?.length ||
  -    value.filter((route) => oldValue?.findIndex((r) => r.path === route.path) === -1).length > 0
  -) {
  +if (!oldValue || value.length !== oldValue.length) {
  +    this.#router.routes = value;
  +    return;
  +}
  +const oldPaths = new Set(oldValue.map((route) => route.path));
  +if (value.some((route) => !oldPaths.has(route.path))) {
       this.#router.routes = value;
   }

---

Approved with Suggestions for improvement

Good to go, but please carefully consider the importance of the suggestions.

The core mechanism — createObservablePart projecting to {culture, segment, unique} — is the correct fix for the root cause: defaultMemoization does a JSON.stringify comparison on the projected subset, so name changes no longer trigger emissions. The forbidden-state refactor from reactive (isOn observable + stored field) to lazy-imperative (getIsOn() at route-load time) is also cleaner and correct since the ** route component is created dynamically on each navigation.

Labels applied: area/frontend, category/performance, category/ux.

[Copilot]

Pull request overview

This PR reduces backoffice workspace router churn by ensuring document/media workspace route generation only runs when the route-relevant parts of variant data change (culture/segment/unique), avoiding expensive re-generation on unrelated variant option updates (e.g., per-keystroke updates elsewhere).

Changes:

• Derive a “route-stable” variants observable via createObservablePart(...) so route generation only reacts to changes in culture, segment, or unique.
• Remove explicit forbidden-state observation and instead decide Forbidden vs NotFound lazily via workspaceContext.forbidden.getIsOn() when resolving the catch-all route.
• Avoid regenerating document workspace routes on subsequent app-culture changes (routes don’t depend on the culture value itself; URL syncing still happens).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/Umbraco.Web.UI.Client/src/packages/media/media/workspace/media-workspace-editor.element.ts	Limits route regeneration by observing only route-relevant variant fields; simplifies forbidden/notfound routing.
src/Umbraco.Web.UI.Client/src/packages/documents/documents/workspace/document-workspace-editor.element.ts	Same route-regeneration limiting for documents and avoids unnecessary route rebuilds on app language changes; simplifies forbidden/notfound routing.