[NAE-2226] Cannot switch between menu items #302
Conversation
- added additional check to openAvailableView method in double-drawer-navigation.service.ts
WalkthroughAdjusts double-drawer navigation click handling to set or clear the current navigation item based on presence of Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant Svc as DoubleDrawerNavigationService
participant Views as AvailableViews
rect rgb(245,248,255)
note over UI,Svc: onItemClick
UI->>Svc: onItemClick(item)
alt item.resource is defined
Svc->>Svc: _currentNavigationItem = item
else item.resource is undefined
Svc->>Svc: _currentNavigationItem = null
end
end
rect rgb(245,255,245)
note over Svc,Views: openAvailableView
Svc->>Svc: Check combined right+more items for match to _currentNavigationItem
alt match exists
Svc-->>UI: Return early (no further resolution)
else no match
Svc->>Views: Resolve and open view
Views-->>Svc: View instance
Svc-->>UI: Opened/updated view
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts (2)
295-319: Add null-safety check to prevent NPE.The early-return logic at lines 298-302 correctly addresses NAE-2226 by preventing redundant view resolution when clicking an item already in the current menu. However,
_currentNavigationItemcan benull(set at line 256), which will cause a null-pointer exception when accessing.id.Apply this diff to add a null-safety check:
public openAvailableView() { let allItems: Array<NavigationItem> = this.rightItems.concat(this.moreItems); - let alreadyClickedItem: NavigationItem = allItems.find(item => item.id === this._currentNavigationItem.id); - if (!!alreadyClickedItem) { + if (this._currentNavigationItem) { + let alreadyClickedItem: NavigationItem = allItems.find(item => item.id === this._currentNavigationItem.id); + if (!!alreadyClickedItem) { - // when the folder has not changed and a menu item is clicked. - return; - } + // when the folder has not changed and a menu item is clicked. + return; + } + } let autoOpenItems: Array<NavigationItem> = allItems.filter(item => DoubleDrawerUtils.hasItemAutoOpenView(item));
254-286: Reset_currentNavigationItemon all navigation flowsCurrently
_currentNavigationItemis only initialized in the constructor and set inonItemClick; it isn’t cleared when the user navigates via home/back buttons, direct URL changes, or other path updates. Add logic in your navigation handlers (e.g. homeClick, pathService URL subscription, back navigation) to reset_currentNavigationItemto avoid stale references triggering the early‐return inopenAvailableView.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts (1)
projects/netgrif-components-core/src/lib/navigation/model/navigation-configs.ts (1)
NavigationItem(12-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Matrix Test (22)
- GitHub Check: Test with SonarCloud
- GitHub Check: Matrix Test (24)
- GitHub Check: Matrix Test (20)
- GitHub Check: task-list-completed
...core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts
Show resolved
Hide resolved
Refined documentation in the openAvailableView method to improve clarity and readability. Adjusted the descriptions of the rule to make the order and logic more explicit.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts (1)
projects/netgrif-components-core/src/lib/navigation/model/navigation-configs.ts (1)
NavigationItem(12-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Matrix Test (22)
- GitHub Check: Matrix Test (20)
- GitHub Check: Matrix Test (24)
- GitHub Check: Test with SonarCloud
- GitHub Check: task-list-completed
...core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts
Outdated
Show resolved
Hide resolved
|
2 participants
Description
Added additional check, whether the item on the same path is selected through menu item click.
Fixes NAE-2226
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
This was tested manually, and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit