Skip to content

Conversation

@iOvergaard
Copy link
Contributor

@iOvergaard iOvergaard commented Nov 28, 2025

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.

  • Added ResetReason type to differentiate between actual errors and expected empty states ('error' | 'empty' | 'fallback')
  • Updated #resetChildren() in tree-item-children.manager.ts to accept a reason parameter and only show error notifications when the reason is 'error'
  • Updated all call sites to provide appropriate context:
    • When retries are exhausted → 'error' (shows notification)
    • When no target is found after deleting items → '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:

TypeError: can't access property "every", e is undefined

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 call loadChildren(), which under certain failure conditions would recursively call #resetChildren() again, creating an infinite loop.

To prevent this scenario:

  • Extracted #loadChildrenWithOffsetPagination() as a terminal fallback method that uses only offset pagination and structurally cannot recurse
  • Updated #resetChildren() to call this fallback instead of loadChildren(), making infinite recursion impossible by design
  • Added explicit reset of all retry counters (#loadChildrenRetries, #loadPrevItemsRetries, #loadNextItemsRetries) to ensure clean state

The fix creates a one-way flow: target-based loading can fall back to #resetChildren(), which calls offset-only loading that never recurses back.

Testing

  • Verified that deleting the last child of a tree node no longer shows an error notification
  • Confirmed that legitimate error scenarios (retry exhaustion) still show appropriate notifications
  • Validated that the recursion safeguards prevent potential infinite loops under failure conditions
  • Added defensive checks to prevent crashes from unexpected API responses

…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.
Copilot AI review requested due to automatic review settings November 28, 2025 08:48
Copilot finished reviewing on behalf of iOvergaard November 28, 2025 08:50
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.
Copy link
Contributor

Copilot AI left a 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 ResetReason type 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()

- 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
@iOvergaard iOvergaard changed the title Fix incorrect error notification when deleting last child in tree Tree: Fix incorrect error notification when deleting last child (closes #20977) Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants