Tree: Fix incorrect error notification when deleting last child (closes #20977)#20985
Merged
madsrasmussen merged 3 commits intorelease/17.0from Dec 1, 2025
Merged
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.
Contributor
There was a problem hiding this comment.
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
leekelleher
reviewed
Dec 1, 2025
Member
leekelleher
left a comment
There was a problem hiding this comment.
I've tested it out, it functionally works as described.
I do think @madsrasmussen should review the code changes before we approve/merge this.
madsrasmussen
approved these changes
Dec 1, 2025
Contributor
madsrasmussen
left a comment
There was a problem hiding this comment.
Great! changes look good to me
This was referenced Dec 8, 2025
Closed
Bump Umbraco.Cms.Persistence.EFCore from 17.0.0 to 17.0.1
karl-sjogren/umbraco-extend-everything#134
Closed
This was referenced Dec 8, 2025
This was referenced Jan 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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