Skip to content

Commit 1694e3b

Browse files
authored
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
1 parent 35f73a8 commit 1694e3b

File tree

2 files changed

+85
-11
lines changed

2 files changed

+85
-11
lines changed

src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-children.manager.ts

Lines changed: 81 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ import {
2323
} from '@umbraco-cms/backoffice/entity-action';
2424
import { UMB_NOTIFICATION_CONTEXT } from '@umbraco-cms/backoffice/notification';
2525

26+
type ResetReason = 'error' | 'empty' | 'fallback';
27+
2628
export class UmbTreeItemChildrenManager<
2729
TreeItemType extends UmbTreeItemModel = UmbTreeItemModel,
2830
TreeRootType extends UmbTreeRootModel = UmbTreeRootModel,
@@ -218,7 +220,7 @@ export class UmbTreeItemChildrenManager<
218220
async #loadChildren(reload = false) {
219221
if (this.#loadChildrenRetries > this.#requestMaxRetries) {
220222
this.#loadChildrenRetries = 0;
221-
this.#resetChildren();
223+
this.#resetChildren('error');
222224
return;
223225
}
224226

@@ -302,7 +304,7 @@ export class UmbTreeItemChildrenManager<
302304
We cancel the base target and load using skip/take pagination instead.
303305
This can happen if deep linked to a non existing item or all retries have failed.
304306
*/
305-
this.#resetChildren();
307+
this.#resetChildren(this.#children.getValue().length === 0 ? 'empty' : 'fallback');
306308
}
307309
}
308310

@@ -329,7 +331,7 @@ export class UmbTreeItemChildrenManager<
329331
if (this.#loadPrevItemsRetries > this.#requestMaxRetries) {
330332
// If we have exceeded the maximum number of retries, we need to reset the base target and load from the top
331333
this.#loadPrevItemsRetries = 0;
332-
this.#resetChildren();
334+
this.#resetChildren('error');
333335
return;
334336
}
335337

@@ -378,7 +380,7 @@ export class UmbTreeItemChildrenManager<
378380
If we can't find a new end target we reload the children from the top.
379381
We cancel the base target and load using skip/take pagination instead.
380382
*/
381-
this.#resetChildren();
383+
this.#resetChildren(this.#children.getValue().length === 0 ? 'empty' : 'fallback');
382384
}
383385
}
384386

@@ -409,7 +411,7 @@ export class UmbTreeItemChildrenManager<
409411
if (this.#loadNextItemsRetries > this.#requestMaxRetries) {
410412
// If we have exceeded the maximum number of retries, we need to reset the base target and load from the top
411413
this.#loadNextItemsRetries = 0;
412-
this.#resetChildren();
414+
this.#resetChildren('error');
413415
return;
414416
}
415417

@@ -467,7 +469,7 @@ export class UmbTreeItemChildrenManager<
467469
If we can't find a new end target we reload the children from the top.
468470
We cancel the base target and load using skip/take pagination instead.
469471
*/
470-
this.#resetChildren();
472+
this.#resetChildren(this.#children.getValue().length === 0 ? 'empty' : 'fallback');
471473
}
472474
}
473475

@@ -520,12 +522,81 @@ export class UmbTreeItemChildrenManager<
520522
this.targetPagination.clear();
521523
}
522524

523-
async #resetChildren() {
525+
/**
526+
* Loads children using offset pagination only.
527+
* This is a "safe" fallback that does NOT:
528+
* - Use target pagination
529+
* - Retry with new targets
530+
* - Call #resetChildren (preventing recursion)
531+
* - Throw errors (fails gracefully)
532+
*/
533+
async #loadChildrenWithOffsetPagination(): Promise<void> {
534+
const repository = this.#treeContext?.getRepository();
535+
if (!repository) {
536+
// Terminal fallback - fail silently rather than throwing
537+
return;
538+
}
539+
540+
this.#isLoading.setValue(true);
541+
542+
const parent = this.getStartNode() || this.getTreeItem();
543+
const foldersOnly = this.getFoldersOnly();
544+
const additionalArgs = this.getAdditionalRequestArgs();
545+
546+
const offsetPaging: UmbOffsetPaginationRequestModel = {
547+
skip: 0, // Always from the start
548+
take: this.offsetPagination.getPageSize(),
549+
};
550+
551+
const { data } = parent?.unique
552+
? await repository.requestTreeItemsOf({
553+
parent: { unique: parent.unique, entityType: parent.entityType },
554+
skip: offsetPaging.skip,
555+
take: offsetPaging.take,
556+
paging: offsetPaging,
557+
foldersOnly,
558+
...additionalArgs,
559+
})
560+
: await repository.requestTreeRootItems({
561+
skip: offsetPaging.skip,
562+
take: offsetPaging.take,
563+
paging: offsetPaging,
564+
foldersOnly,
565+
...additionalArgs,
566+
});
567+
568+
if (data) {
569+
const items = data.items as Array<TreeItemType>;
570+
this.#children.setValue(items);
571+
this.setHasChildren(data.total > 0);
572+
this.offsetPagination.setTotalItems(data.total);
573+
}
574+
// Note: On error, we simply don't update state - UI shows stale data
575+
// This is the terminal fallback, no further recovery
576+
577+
this.#isLoading.setValue(false);
578+
}
579+
580+
async #resetChildren(reason: ResetReason = 'error'): Promise<void> {
581+
// Clear pagination state
524582
this.targetPagination.clear();
525583
this.offsetPagination.clear();
526-
this.loadChildren();
527-
const notificationManager = await this.getContext(UMB_NOTIFICATION_CONTEXT);
528-
notificationManager?.peek('danger', { data: { message: 'Menu loading failed. Showing the first items again.' } });
584+
585+
// Reset retry counters to prevent any lingering retry state
586+
this.#loadChildrenRetries = 0;
587+
this.#loadPrevItemsRetries = 0;
588+
this.#loadNextItemsRetries = 0;
589+
590+
// Load using offset pagination only - this is our terminal fallback
591+
await this.#loadChildrenWithOffsetPagination();
592+
593+
// Only show notification for actual errors
594+
if (reason === 'error') {
595+
const notificationManager = await this.getContext(UMB_NOTIFICATION_CONTEXT);
596+
notificationManager?.peek('danger', {
597+
data: { message: 'Menu loading failed. Showing the first items again.' },
598+
});
599+
}
529600
}
530601

531602
#onPageChange = () => this.loadNextChildren();

src/Umbraco.Web.UI.Client/src/packages/management-api/tree/tree-data.request-manager.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,10 @@ export class UmbManagementApiTreeDataRequestManager<
229229
return args.take !== undefined ? args.take : this.#defaultTakeSize;
230230
}
231231

232-
#getTargetResultHasValidParents(data: Array<TreeItemType>, parentUnique: string | null): boolean {
232+
#getTargetResultHasValidParents(data: Array<TreeItemType> | undefined, parentUnique: string | null): boolean {
233+
if (!data) {
234+
return false;
235+
}
233236
return data.every((item) => {
234237
if (item.parent) {
235238
return item.parent.id === parentUnique;

0 commit comments

Comments
 (0)