From 81fb51c716b1ceaba16a005efd6c28ed7e2fd459 Mon Sep 17 00:00:00 2001 From: Jacob Overgaard <752371+iOvergaard@users.noreply.github.com> Date: Fri, 28 Nov 2025 09:46:01 +0100 Subject: [PATCH 1/3] Fix infinite recursion and incorrect error notifications in tree children loading This commit addresses two critical issues in the tree item children manager: 1. **Infinite recursion vulnerability**: The #resetChildren() method called loadChildren(), which could recursively call #resetChildren() again if the underlying issue persisted, creating an infinite loop. 2. **Inappropriate error messages**: The "Menu loading failed" notification was shown even in legitimate scenarios, such as when deleting the last child of a node, where an empty tree is the expected outcome. Changes made: - Add ResetReason type ('error' | 'empty' | 'fallback') to differentiate between error states and expected empty states - Extract #loadChildrenWithOffsetPagination() as a terminal fallback method that uses only offset pagination and never calls #resetChildren(), structurally preventing recursion - Update #resetChildren() to: - Accept a reason parameter to determine whether to show error notification - Reset all retry counters (#loadChildrenRetries, #loadPrevItemsRetries, #loadNextItemsRetries) to ensure clean state - Call #loadChildrenWithOffsetPagination() instead of loadChildren() - Only show error notification when reason is 'error' - Update all call sites of #resetChildren() with appropriate reasons: - 'error' when retries are exhausted (actual failures) - 'empty' or 'fallback' when no new target is found (may be expected, e.g., after deleting items) The fix makes infinite recursion structurally impossible by creating a one-way flow: target-based loading can fall back to #resetChildren(), which calls offset-only loading that never recurses back. --- .../tree-item/tree-item-children.manager.ts | 89 ++++++++++++++++--- 1 file changed, 79 insertions(+), 10 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-children.manager.ts b/src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-children.manager.ts index 381dadae9bd7..f4832dac0354 100644 --- a/src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-children.manager.ts +++ b/src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-children.manager.ts @@ -23,6 +23,8 @@ import { } from '@umbraco-cms/backoffice/entity-action'; import { UMB_NOTIFICATION_CONTEXT } from '@umbraco-cms/backoffice/notification'; +type ResetReason = 'error' | 'empty' | 'fallback'; + export class UmbTreeItemChildrenManager< TreeItemType extends UmbTreeItemModel = UmbTreeItemModel, TreeRootType extends UmbTreeRootModel = UmbTreeRootModel, @@ -218,7 +220,7 @@ export class UmbTreeItemChildrenManager< async #loadChildren(reload = false) { if (this.#loadChildrenRetries > this.#requestMaxRetries) { this.#loadChildrenRetries = 0; - this.#resetChildren(); + this.#resetChildren('error'); return; } @@ -302,7 +304,7 @@ export class UmbTreeItemChildrenManager< We cancel the base target and load using skip/take pagination instead. This can happen if deep linked to a non existing item or all retries have failed. */ - this.#resetChildren(); + this.#resetChildren(this.#children.getValue().length === 0 ? 'empty' : 'fallback'); } } @@ -329,7 +331,7 @@ export class UmbTreeItemChildrenManager< if (this.#loadPrevItemsRetries > this.#requestMaxRetries) { // If we have exceeded the maximum number of retries, we need to reset the base target and load from the top this.#loadPrevItemsRetries = 0; - this.#resetChildren(); + this.#resetChildren('error'); return; } @@ -378,7 +380,7 @@ export class UmbTreeItemChildrenManager< If we can't find a new end target we reload the children from the top. We cancel the base target and load using skip/take pagination instead. */ - this.#resetChildren(); + this.#resetChildren(this.#children.getValue().length === 0 ? 'empty' : 'fallback'); } } @@ -409,7 +411,7 @@ export class UmbTreeItemChildrenManager< if (this.#loadNextItemsRetries > this.#requestMaxRetries) { // If we have exceeded the maximum number of retries, we need to reset the base target and load from the top this.#loadNextItemsRetries = 0; - this.#resetChildren(); + this.#resetChildren('error'); return; } @@ -467,7 +469,7 @@ export class UmbTreeItemChildrenManager< If we can't find a new end target we reload the children from the top. We cancel the base target and load using skip/take pagination instead. */ - this.#resetChildren(); + this.#resetChildren(this.#children.getValue().length === 0 ? 'empty' : 'fallback'); } } @@ -520,12 +522,79 @@ export class UmbTreeItemChildrenManager< this.targetPagination.clear(); } - async #resetChildren() { + /** + * Loads children using offset pagination only. + * This is a "safe" fallback that does NOT: + * - Use target pagination + * - Retry with new targets + * - Call #resetChildren (preventing recursion) + */ + async #loadChildrenWithOffsetPagination(): Promise { + const repository = this.#treeContext?.getRepository(); + if (!repository) throw new Error('Could not request children, repository is missing'); + + this.#isLoading.setValue(true); + + const parent = this.getStartNode() || this.getTreeItem(); + const foldersOnly = this.getFoldersOnly(); + const additionalArgs = this.getAdditionalRequestArgs(); + + const offsetPaging: UmbOffsetPaginationRequestModel = { + skip: 0, // Always from the start + take: this.offsetPagination.getPageSize(), + }; + + const { data } = parent?.unique + ? await repository.requestTreeItemsOf({ + parent: { unique: parent.unique, entityType: parent.entityType }, + skip: offsetPaging.skip, + take: offsetPaging.take, + paging: offsetPaging, + foldersOnly, + ...additionalArgs, + }) + : await repository.requestTreeRootItems({ + skip: offsetPaging.skip, + take: offsetPaging.take, + paging: offsetPaging, + foldersOnly, + ...additionalArgs, + }); + + if (data) { + const items = data.items as Array; + this.#children.setValue(items); + this.setHasChildren(data.total > 0); + this.offsetPagination.setTotalItems(data.total); + this.targetPagination.setCurrentItems(items); + this.targetPagination.setTotalItems(data.total); + } + // Note: On error, we simply don't update state - UI shows stale data + // This is the terminal fallback, no further recovery + + this.#isLoading.setValue(false); + } + + async #resetChildren(reason: ResetReason = 'error'): Promise { + // Clear pagination state this.targetPagination.clear(); this.offsetPagination.clear(); - this.loadChildren(); - const notificationManager = await this.getContext(UMB_NOTIFICATION_CONTEXT); - notificationManager?.peek('danger', { data: { message: 'Menu loading failed. Showing the first items again.' } }); + + // Reset retry counters to prevent any lingering retry state + this.#loadChildrenRetries = 0; + this.#loadPrevItemsRetries = 0; + this.#loadNextItemsRetries = 0; + + // Load using offset pagination only - this is our terminal fallback + await this.#loadChildrenWithOffsetPagination(); + + // Only show notification for actual errors + if (reason === 'error') { + const notificationManager = await this.getContext(UMB_NOTIFICATION_CONTEXT); + notificationManager?.peek('danger', { + data: { message: 'Menu loading failed. Showing the first items again.' }, + }); + } } #onPageChange = () => this.loadNextChildren(); From b53bccda69c8dc4506e50fbb46334add77c0c743 Mon Sep 17 00:00:00 2001 From: Jacob Overgaard <752371+iOvergaard@users.noreply.github.com> Date: Fri, 28 Nov 2025 09:51:23 +0100 Subject: [PATCH 2/3] Fix undefined items array causing tree to break after deletion This fixes the root cause of issue #20977 where deleting a document type would cause the tree to "forever load" with a JavaScript error. The error occurred in #getTargetResultHasValidParents() which called .every() on data without checking if it was undefined. When the API returned undefined items (e.g., after deleting the last child), this caused: TypeError: can't access property "every", e is undefined The fix adds a guard to check if data is undefined before calling .every(), returning false in that case to trigger the proper error handling flow. --- .../management-api/tree/tree-data.request-manager.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Web.UI.Client/src/packages/management-api/tree/tree-data.request-manager.ts b/src/Umbraco.Web.UI.Client/src/packages/management-api/tree/tree-data.request-manager.ts index d30f9be8a8f3..c2653a26028c 100644 --- a/src/Umbraco.Web.UI.Client/src/packages/management-api/tree/tree-data.request-manager.ts +++ b/src/Umbraco.Web.UI.Client/src/packages/management-api/tree/tree-data.request-manager.ts @@ -229,7 +229,10 @@ export class UmbManagementApiTreeDataRequestManager< return args.take !== undefined ? args.take : this.#defaultTakeSize; } - #getTargetResultHasValidParents(data: Array, parentUnique: string | null): boolean { + #getTargetResultHasValidParents(data: Array | undefined, parentUnique: string | null): boolean { + if (!data) { + return false; + } return data.every((item) => { if (item.parent) { return item.parent.id === parentUnique; From 331aff84bdcb4a6a5d76069f84b4e831b3fb3ab4 Mon Sep 17 00:00:00 2001 From: Jacob Overgaard <752371+iOvergaard@users.noreply.github.com> Date: Fri, 28 Nov 2025 09:55:16 +0100 Subject: [PATCH 3/3] Address code review feedback on terminal fallback method - Change error throwing to silent return for graceful failure handling - Remove target pagination state updates from offset-only loading method - Update JSDoc to clarify that method does not throw errors --- .../core/tree/tree-item/tree-item-children.manager.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-children.manager.ts b/src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-children.manager.ts index f4832dac0354..7884dc02ceee 100644 --- a/src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-children.manager.ts +++ b/src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-children.manager.ts @@ -528,10 +528,14 @@ export class UmbTreeItemChildrenManager< * - Use target pagination * - Retry with new targets * - Call #resetChildren (preventing recursion) + * - Throw errors (fails gracefully) */ async #loadChildrenWithOffsetPagination(): Promise { const repository = this.#treeContext?.getRepository(); - if (!repository) throw new Error('Could not request children, repository is missing'); + if (!repository) { + // Terminal fallback - fail silently rather than throwing + return; + } this.#isLoading.setValue(true); @@ -566,8 +570,6 @@ export class UmbTreeItemChildrenManager< this.#children.setValue(items); this.setHasChildren(data.total > 0); this.offsetPagination.setTotalItems(data.total); - this.targetPagination.setCurrentItems(items); - this.targetPagination.setTotalItems(data.total); } // Note: On error, we simply don't update state - UI shows stale data // This is the terminal fallback, no further recovery