Document URLs: Retrieve only the current culture's URL for the Info workspace links panel (closes #23196) - #23265
Conversation
…o document workspace.
|
Claude finished @AndyButland's task in 7m 1s —— View job PR ReviewTarget: Scopes the
Note Complexity advisory — This PR may benefit from splitting.
This is an observation, not a blocker. The full review follows below. Suggestions
Approved with Suggestions for improvementGood to go. The optimization is well-targeted, the breaking-change mitigations correctly follow the established patterns, and the test coverage addresses all the key cases (invariant, variant, unknown culture). The suggestions above are minor polish items. |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the Management API + backoffice “Info → Links” panel flow by allowing URL resolution to be scoped to the currently displayed culture for variant content, avoiding unnecessary per-culture routing/content-finder execution while keeping collision detection behavior intact.
Changes:
- Added culture-aware overloads for document URL resolution (
IPublishedUrlInfoProvider,IDocumentUrlFactory) and implemented server-side culture scoping inPublishedUrlInfoProvider. - Extended
GET /umbraco/management/api/v1/document/urlswith optional?culture=and updated OpenAPI + generated TS client types accordingly. - Updated backoffice URLs fetching to request only the displayed culture for variant documents (and to re-fetch on culture switch), with unit/integration/client tests covering the new behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Factories/DocumentUrlFactoryTests.cs | Adds unit coverage ensuring the factory passes culture through and that parameterless overloads delegate as “all cultures”. |
| tests/Umbraco.Tests.Integration/Umbraco.Web.BackOffice/UrlAndDomains/DomainAndUrlsTests.cs | Adds integration coverage for culture-scoped URL results and unknown-culture fallback behavior. |
| tests/Umbraco.Tests.Integration/Umbraco.Core/Services/PublishedUrlInfoProviderTests.cs | Adds integration coverage that invariant content ignores requested culture. |
| src/Umbraco.Web.UI.Client/src/packages/documents/documents/url/repository/document-url.server.data-source.ts | Adds optional culture to the client request so the server can scope URL resolution. |
| src/Umbraco.Web.UI.Client/src/packages/documents/documents/url/repository/document-url.server.data-source.test.ts | Verifies culture is included/omitted in the query string as expected. |
| src/Umbraco.Web.UI.Client/src/packages/documents/documents/url/repository/document-url.repository.ts | Introduces a culture-aware requestUrls() path that still populates the URL store. |
| src/Umbraco.Web.UI.Client/src/packages/documents/documents/url/info-app/document-links-workspace-info-app.element.ts | Requests URLs using the displayed culture for variant documents and re-requests on culture switch (debounced). |
| src/Umbraco.Web.UI.Client/src/packages/documents/documents/url/document-urls-data-resolver.ts | Tracks displayed culture reactively and exposes it as requestCulture + getRequestCulture(). |
| src/Umbraco.Web.UI.Client/src/packages/core/backend-api/types.gen.ts | Updates generated client types to include the optional culture query parameter. |
| src/Umbraco.Core/Routing/PublishedUrlInfoProvider.cs | Implements culture-scoped URL resolution for variant content and filters “other URLs” accordingly. |
| src/Umbraco.Core/Routing/IPublishedUrlInfoProvider.cs | Adds default-interface-method overload supporting optional culture scoping (non-breaking). |
| src/Umbraco.Cms.Api.Management/OpenApi.json | Adds the optional culture query parameter to the document URLs endpoint contract. |
| src/Umbraco.Cms.Api.Management/Factories/IDocumentUrlFactory.cs | Adds culture-aware overloads via default interface methods. |
| src/Umbraco.Cms.Api.Management/Factories/DocumentUrlFactory.cs | Wires the optional culture through to IPublishedUrlInfoProvider. |
| src/Umbraco.Cms.Api.Management/Controllers/Document/DocumentUrlController.cs | Adds the optional culture query param endpoint overload and delegates from the obsolete signature. |
…d of resolving all cultures.
…ault implementation removal).
…tching languages.
|
I just tested this PR and it looks good 💪, I followed the manual steps and I also checked the info tabs in split view which also worked with getting both of the selected culture urls |
|
iOvergaard
left a comment
There was a problem hiding this comment.
LGTM.
Pulled it down, merged v17/dev in (clean), and verified: regenerated OpenApi.json and the client types both diff to zero against yours, so the whole chain's in sync. Browser-tested too — Info panel requests only the displayed culture (?culture=en-US, then ?culture=da on switch), and no more "not in cache" flicker.
Mads's points look handled 👍



Description
When editing content in the backoffice, the Info workspace view's "Links" panel calls
GET /umbraco/management/api/v1/document/urls. For each document this resolves the URL for every installed culture and, to detect naming collisions, routes each generated URL back through the full content-finder pipeline (PublishedUrlInfoProvider.GetAllAsync→VerifyCollisionAsync→IPublishedRouter.RouteRequestAsync).On a site with many languages and custom, expensive
IContentFinders, this is costly: a reporter with a 24-language setup and slow finders sees the finder chain run 24 times just to render the links for one document — even though the panel only ever displays the culture currently being viewed (the client already filtered the all-cultures response down to the current culture for variant documents).To mitigate this, I've amended the behaviour to request and resolve only the culture being displayed for variant documents, turning N finder routes into 1 per panel load.
Fixes #23196.
Server
IPublishedUrlInfoProvider.GetAllAsyncgains an optional culture overload (added as a default interface method for binary compatibility).PublishedUrlInfoProviderscopes the lookup to the requested culture when it is a valid installed culture, and ignoring culture for invariant content.IDocumentUrlFactorygains culture-aware overloads (default interface methods).DocumentUrlControllerGET .../document/urlsgains an optional?culture=query parameter.OpenApi.jsonand the generated TypeScript client updated to include the new optional parameter.Client
Performance Considerations
This PR is primarily to support setups that have many languages, on the basis that given the URLs are viewed one by one, it makes sense to not have to wait to retrieve them all.
There is a downside in that now on switching language, there will be a request to get the URL for that language. We are no longer getting them all up front. On balance this feels right - it's likely not a very common editor interaction to repeatedly switch between languages in quick succession, and the request is now small and much faster. I've improved the loading indication so there's no longer a flicker of "This document is published but not in the cache". I did consider a cache - so once a URL is retrieved it's not got again unless it's necessary - but it seemed like a lot of moving parts and a staleness risk for little benefit.
Testing
Automated
Unit tests on server and client, and integration tests on the server, have been added/updated to verify the functionality.
Manual
GET .../document/urls?id=<key>&culture=<current-culture>.