-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Tree: Fix incorrect error notification when deleting last child (closes #20977) #20985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/17.0
Are you sure you want to change the base?
Tree: Fix incorrect error notification when deleting last child (closes #20977) #20985
Conversation
…dren 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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes an issue where users were incorrectly shown a "Menu loading failed" error notification when deleting the last child of a tree node, despite this being an expected empty state. The fix differentiates between actual errors and legitimate empty states by introducing contextual error notifications, and adds safeguards to prevent potential infinite recursion in the tree loading logic.
- Added
ResetReasontype to distinguish between errors, empty states, and fallback scenarios - Extracted
#loadChildrenWithOffsetPagination()as a terminal fallback that prevents infinite recursion - Updated all call sites to provide appropriate context when calling
#resetChildren()
src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-children.manager.ts
Outdated
Show resolved
Hide resolved
src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-children.manager.ts
Outdated
Show resolved
Hide resolved
- 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
Summary
Fixes #20977 - Addresses issues with tree loading after deleting document types, including inappropriate error notifications and potential JavaScript errors.
Changes
Primary Fix: Contextual Error Notifications
The main user-facing issue was an inappropriate "Menu loading failed" error notification appearing when deleting the last child of a tree node, where an empty tree is the expected outcome.
ResetReasontype to differentiate between actual errors and expected empty states ('error'|'empty'|'fallback')#resetChildren()in tree-item-children.manager.ts to accept areasonparameter and only show error notifications when the reason is'error''error'(shows notification)'empty'or'fallback'(no notification)This ensures users only see error messages for actual failures, not for legitimate empty states.
Additional Safeguard: Undefined Items Protection
While investigating, we identified a potential issue in the tree-data.request-manager.ts where
#getTargetResultHasValidParents()could receive undefined data and call.every()on it, which would cause:Added a guard to check if data is undefined before calling
.every(), returning false to trigger proper error handling. This is a defensive safeguard that prevents potential crashes if the backend API returns unexpected data structures.Additional Safeguard: Recursion Prevention
While investigating this issue, we discovered a potential infinite recursion vulnerability in the tree loading logic. The
#resetChildren()method could callloadChildren(), which under certain failure conditions would recursively call#resetChildren()again, creating an infinite loop.To prevent this scenario:
#loadChildrenWithOffsetPagination()as a terminal fallback method that uses only offset pagination and structurally cannot recurse#resetChildren()to call this fallback instead ofloadChildren(), making infinite recursion impossible by design#loadChildrenRetries,#loadPrevItemsRetries,#loadNextItemsRetries) to ensure clean stateThe fix creates a one-way flow: target-based loading can fall back to
#resetChildren(), which calls offset-only loading that never recurses back.Testing