From 1694e3bad23a95e0672f12b3fa44bd07b2f693c1 Mon Sep 17 00:00:00 2001 From: Jacob Overgaard <752371+iOvergaard@users.noreply.github.com> Date: Mon, 1 Dec 2025 20:12:23 +0100 Subject: [PATCH] Tree: Fix incorrect error notification when deleting last child (closes #20977) (#20985) * 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. * 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. * 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 --- .../tree-item/tree-item-children.manager.ts | 91 +++++++++++++++++-- .../tree/tree-data.request-manager.ts | 5 +- 2 files changed, 85 insertions(+), 11 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 381dadae9b..7884dc02ce 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,81 @@ 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) + * - Throw errors (fails gracefully) + */ + async #loadChildrenWithOffsetPagination(): Promise { + const repository = this.#treeContext?.getRepository(); + if (!repository) { + // Terminal fallback - fail silently rather than throwing + return; + } + + 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); + } + // 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(); 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 d30f9be8a8..c2653a2602 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;