[NAE-2233] Fix dashboard menu issue #306
Conversation
- fix the problem with bad resolving of double drawer items when the dashboard item is view and not the folder menu item
WalkthroughAdjusts dashboard navigation to set activePath to a parent path for items without children, exposes a public setter to assign the current navigation item on DoubleDrawerNavigationService, replaces internal null with undefined, and renames/makes the parent-path helper public as extractParentPath. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ADC as AbstractDashboardComponent
participant DDNS as DoubleDrawerNavigationService
User->>ADC: select navigation item
ADC->>ADC: check hasChildren and nodePath
alt hasChildren && nodePath
ADC->>DDNS: set currentNavigationItem = undefined
ADC->>ADC: activePath = nodePath
else nodePath present only
ADC->>DDNS: resolve navigation item by nodePath
DDNS-->>ADC: NavigationItem | undefined
alt item found
ADC->>DDNS: set currentNavigationItem(item)
else no item
Note over ADC: currentNavigationItem unchanged
end
ADC->>ADC: activePath = extractParentPath(nodePath)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts (3)
259-266: Use the setter (after widening) instead of touching the backing fieldReplace direct field mutation with the public setter for consistency and to keep semantics centralized.
- this._currentNavigationItem = undefined; + this.currentNavigationItem = undefined;
270-283: Guard against undefinedpathbefore calling.includes
pathcan be undefined (optional chaining in Line 270). Callingpath.includes(...)will throw.- } else if (!path.includes(this.currentPath)) { + } else if (path && !path.includes(this.currentPath)) { this._pathService.activePath = this.extractParent(this.currentPath);
321-324: Wrong redirect source when falling back to “items with view”You compute
itemsWithViewbut redirect usingautoOpenItems[0]. Should redirect toitemsWithView[0].- this._redirectService.redirect(autoOpenItems[0].routing.path); + this._redirectService.redirect(itemsWithView[0].routing.path);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
projects/netgrif-components-core/src/lib/navigation/dashboard/abstract-dashboard.component.ts(1 hunks)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). (4)
- GitHub Check: Matrix Test (24)
- GitHub Check: Matrix Test (20)
- GitHub Check: Test with SonarCloud
- GitHub Check: Matrix Test (22)
| if (menuItemCase.immediateData.find(f => f.stringId === GroupNavigationConstants.ITEM_FIELD_ID_HAS_CHILDREN)?.value && nodePath) { | ||
| this._doubleDrawerNavigationService.currentNavigationItem = undefined; | ||
| this._pathService.activePath = nodePath; | ||
| } else if (nodePath) { | ||
| const menuItem = this._doubleDrawerNavigationService.resolveItemCaseToNavigationItem(menuItemCase); | ||
| if (menuItem) { | ||
| this._doubleDrawerNavigationService.currentNavigationItem = menuItem; | ||
| } | ||
| this._pathService.activePath = this.extractParent(nodePath); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Compile-time mismatch: assigning undefined to setter typed as NavigationItem
This line relies on DoubleDrawerNavigationService.currentNavigationItem accepting undefined. Update the service to type both the backing field and setter as NavigationItem | undefined (see service comment), or introduce a clearCurrentNavigationItem() method.
No code change needed here after widening.
After adjusting the service types, re-run:
🏁 Script executed:
#!/bin/bash
rg -n -C2 '\bcurrentNavigationItem\s*=' projects/netgrif-components-coreLength of output: 1747
🏁 Script executed:
#!/bin/bash
rg -nP '(get|set)\s+currentNavigationItem' -C3 projects/netgrif-components-core/src/lib/navigationLength of output: 1003
Widen currentNavigationItem setter signature to accept undefined
The currentNavigationItem setter in DoubleDrawerNavigationService is currently typed as item: NavigationItem, so assigning undefined on line 269/274 causes a compile error. Change its signature to set currentNavigationItem(item: NavigationItem | undefined) or provide a clearCurrentNavigationItem() method.
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/navigation/dashboard/abstract-dashboard.component.ts
around lines 268 to 277, the code assigns undefined to
_doubleDrawerNavigationService.currentNavigationItem which conflicts with the
setter typed as NavigationItem; update the DoubleDrawerNavigationService API so
the setter accepts undefined by changing its signature to set
currentNavigationItem(item: NavigationItem | undefined) (and keep existing
behavior), or alternatively add a clearCurrentNavigationItem(): void method and
replace assignments of undefined with calls to that method; after updating the
service, adjust imports/types if needed and rebuild to ensure the compile error
is resolved.
projects/netgrif-components-core/src/lib/navigation/dashboard/abstract-dashboard.component.ts
Outdated
Show resolved
Hide resolved
...core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts
Show resolved
Hide resolved
- resolve coderabbit suggestions
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 (2)
projects/netgrif-components-core/src/lib/navigation/dashboard/abstract-dashboard.component.ts(1 hunks)projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts(9 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
🔇 Additional comments (7)
projects/netgrif-components-core/src/lib/navigation/dashboard/abstract-dashboard.component.ts (1)
268-277: LGTM! Navigation logic correctly distinguishes folders from leaf items.The refactored logic appropriately handles two navigation scenarios:
- Folders with children: Navigate to the folder's path and clear the current item (lines 268-270)
- Leaf items without children: Resolve and set the current item, then navigate to the parent path (lines 271-276)
The implementation includes proper error handling by checking if
menuItemexists before assignment (line 273), and leverages the newly exposedextractParentPathmethod from the navigation service.projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts (6)
79-79: LGTM! Field type widened to accept undefined.This change resolves the type mismatch issue flagged in previous reviews and aligns with the new setter signature.
106-106: LGTM! Initialization aligned with undefined semantics.Replacing
nullwithundefinedis consistent with the updated type signature and TypeScript best practices.
148-150: LGTM! Public setter exposed as requested in past reviews.The setter correctly accepts
NavigationItem | undefinedand provides external components with the ability to manage navigation state.
249-249: LGTM! Method calls updated to use renamed public method.All invocations correctly reference
extractParentPath, maintaining consistency with the method visibility change.Also applies to: 265-265, 282-282
383-383: LGTM! Method calls updated consistently.All references to the path extraction utility correctly use the renamed public method.
Also applies to: 517-519
597-605: LGTM! Method visibility and naming improved.Making
extractParentPathpublic and giving it a more descriptive name addresses the code duplication concern from past reviews. The implementation correctly handles root and top-level paths.
...core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts
Show resolved
Hide resolved
machacjozef
changed the base branch from
release/7.0.0-rev9
to
release/7.0.0-rev8
machacjozef
dismissed stale reviews from Retoocs and themself
The base branch was changed.
|
5 participants
Description
Fixes NAE-2233
Dependencies
none
Third party dependencies
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
manually
Test Configuration
Checklist:
Summary by CodeRabbit