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
This commit is contained in:
Jacob Overgaard
2025-12-01 20:12:23 +01:00
committed by GitHub
parent 35f73a8a31
commit 1694e3bad2
2 changed files with 85 additions and 11 deletions

View File

@@ -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<void> {
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<TreeItemType>;
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<void> {
// 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();

View File

@@ -229,7 +229,10 @@ export class UmbManagementApiTreeDataRequestManager<
return args.take !== undefined ? args.take : this.#defaultTakeSize;
}
#getTargetResultHasValidParents(data: Array<TreeItemType>, parentUnique: string | null): boolean {
#getTargetResultHasValidParents(data: Array<TreeItemType> | undefined, parentUnique: string | null): boolean {
if (!data) {
return false;
}
return data.every((item) => {
if (item.parent) {
return item.parent.id === parentUnique;