[NAE-2234] Fix dashboard menu #308
Conversation
- resolve menu items when app is open throught link or refreshed
WalkthroughPathService is introduced as a central data channel for menu resolver results/errors, navigation components expose and consume a path-resolver loading state, and DoubleDrawer navigation API/flow is extended to handle resolver-originated actions and parent-path extraction. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant G as GroupNavigationComponentResolverService
participant P as PathService
participant D as DoubleDrawerNavigationService
participant A as AbstractNavigationDoubleDrawer
Note over U,G: user selects menu item
U->>G: resolve view for menu item
alt resolver produces data
G-->>P: set datafieldsForMenuResolver(data)
G->>D: emit resolved view portal (navigation)
else resolver error
G-->>P: set datafieldsForMenuResolverError(error)
G->>D: forward error
end
Note over P,D: PathService holds resolver results until closed
P-->>D: (optional) subscribers react to data/error
D->>D: set fromResolver = true (flow flag)
D->>A: update currentNavigationItem / currentPath (uses extractParentPath when needed)
A->>A: guard openAvailableView with pathResolverLoading$
A->>Router: perform router.navigate(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (1)
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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts (1)
331-333: Wrong array used for redirectShould redirect to the first item from
itemsWithView, notautoOpenItems.Apply this diff:
- 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 (10)
package.json(1 hunks)projects/netgrif-components-core/package.json(1 hunks)projects/netgrif-components-core/src/lib/navigation/dashboard/abstract-dashboard.component.ts(1 hunks)projects/netgrif-components-core/src/lib/navigation/group-navigation-component-resolver/group-navigation-component-resolver.service.ts(3 hunks)projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts(6 hunks)projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts(11 hunks)projects/netgrif-components-core/src/lib/navigation/service/path.service.ts(3 hunks)projects/netgrif-components/package.json(2 hunks)projects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-group-navigation-component-resolver.service.ts(2 hunks)projects/netgrif-components/src/lib/navigation/navigation-double-drawer/navigation-double-drawer.component.html(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts (1)
projects/netgrif-components-core/src/lib/navigation/utility/navigation-item-task-utility-methods.ts (1)
extractFieldValueFromData(127-133)
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)
projects/netgrif-components-core/src/lib/navigation/service/path.service.ts (2)
projects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-group-navigation-component-resolver.service.ts (1)
Injectable(19-76)projects/netgrif-components-core/src/lib/resources/interface/data-groups.ts (1)
DataGroup(7-28)
⏰ 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). (6)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Matrix Test (24)
- GitHub Check: Matrix Test (22)
- GitHub Check: Matrix Test (20)
- GitHub Check: Test with SonarCloud
🔇 Additional comments (16)
package.json (1)
3-3: Version bump is consistent with monorepo release.The version bump from 7.0.0-rc.13 to 7.0.0-rc.14 aligns with the release cycle. No new dependencies have been introduced, and all peer dependency declarations remain unchanged, which is consistent with PR objectives stating no third-party dependencies are added.
projects/netgrif-components/package.json (2)
3-3: Version bump is consistent with monorepo release.The version bump from 7.0.0-rc.13 to 7.0.0-rc.14 aligns with the release cycle.
32-32: Peer dependency version correctly updated.The @netgrif/components-core peer dependency version is correctly updated from 7.0.0-rc.13 to 7.0.0-rc.14, maintaining version alignment across the monorepo. This ensures consumers of netgrif-components will use the corresponding core library version that includes the related PathService and navigation updates referenced in the PR summary.
projects/netgrif-components-core/package.json (1)
3-3: Version bump is consistent with monorepo release.The version bump from 7.0.0-rc.13 to 7.0.0-rc.14 aligns with the release cycle. No dependency declarations have been modified, which is appropriate since the PR introduces no new third-party dependencies.
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts (8)
107-107: LGTMExplicit initialization to
undefinedaligns with the widened type.
149-151: Setter addition is finePublic setter for
currentNavigationItemis OK; consistent with internal usage.
217-220: Setter for resolver flag is finePublic setter looks good; once initialized to
false, this will be safe.
254-254: Correct parent extractionSwitch to
extractParentPathhere is appropriate.
264-264: Track clicked itemAssigning
_currentNavigationItemon click is correct.
270-271: Use parent extraction on custom view branchGood change; maintains expected navigation behavior.
392-392: LGTMParent path extraction in left-side loading call is correct.
76-76: Initialize_fromResolverto a safe defaultProperty should be initialized to provide clear semantic intent. Initialize to
false.- private _fromResolver: boolean; + private _fromResolver: boolean = false;Likely an incorrect or invalid review comment.
projects/netgrif-components-core/src/lib/navigation/group-navigation-component-resolver/group-navigation-component-resolver.service.ts (1)
24-27: Gate writes using Subject termination (isStopped), not subscription closed.This code depends on
PathService.isMenuResolverClosed. Ensure that accessor returns Subject termination state (isStopped) rather than.closed, so repeat writes are correctly ignored after complete/error. See PathService fix.Also applies to: 49-52
projects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-group-navigation-component-resolver.service.ts (1)
22-24: Constructor wiring LGTM.Injection order matches base class and
super(...)signature. No issues spotted.Please ensure consuming modules don’t provide their own
PathServicetokens that could shadow the root provider.projects/netgrif-components-core/src/lib/navigation/service/path.service.ts (1)
53-55: Encapsulation ofactivePath$viaasObservable()is correct.Good API hygiene.
projects/netgrif-components/src/lib/navigation/navigation-double-drawer/navigation-double-drawer.component.html (1)
157-177: No changes required — LoadingEmitter is already Subscribable.LoadingEmitter extends BehaviorSubject, which is a standard RxJS Observable/Subscribable type. The getter at line 229–231 returns the LoadingEmitter instance directly, and AsyncPipe in Angular accepts any Subscribable. The template usage
(pathResolverLoading$ | async)is valid and requires no modification.Likely an incorrect or invalid review comment.
| const menuItem = this._doubleDrawerNavigationService.resolveItemCaseToNavigationItem(menuItemCase); | ||
| if (menuItem) { | ||
| this._doubleDrawerNavigationService.currentNavigationItem = menuItem; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging when menuItem resolution fails.
The conditional update of currentNavigationItem is good defensive coding. However, when menuItem is null or undefined, it might indicate a configuration issue that would benefit from a warning log to aid debugging.
Consider adding a log statement:
const menuItem = this._doubleDrawerNavigationService.resolveItemCaseToNavigationItem(menuItemCase);
if (menuItem) {
this._doubleDrawerNavigationService.currentNavigationItem = menuItem;
+} else {
+ this._log.warn(`Could not resolve menu item for case ${menuItemCase.stringId}`);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/navigation/dashboard/abstract-dashboard.component.ts
around lines 268 to 271, the code silently skips setting currentNavigationItem
when resolveItemCaseToNavigationItem returns null/undefined; add a defensive
warning log in the else path that logs a clear message plus the menuItemCase
(and any relevant identifying context like the dashboard id or caller) so
maintainers can trace misconfigurations—use the existing logger/service pattern
(e.g., this.logger.warn or this._doubleDrawerNavigationService.logger)
consistent with the project and keep the message concise and actionable.
| if (menuItemCase.immediateData.find(f => f.stringId === GroupNavigationConstants.ITEM_FIELD_ID_HAS_CHILDREN)?.value && nodePath) { | ||
| this._pathService.activePath = nodePath; | ||
| } else if (nodePath) { | ||
| this._pathService.activePath = this._doubleDrawerNavigationService.extractParentPath(nodePath); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract hasChildren check for improved readability.
The inline field lookup in line 272 increases cognitive complexity. Extracting the hasChildren check to a named variable would improve readability and make the conditional logic clearer.
Apply this diff to extract the hasChildren check:
+const hasChildren = menuItemCase.immediateData.find(f => f.stringId === GroupNavigationConstants.ITEM_FIELD_ID_HAS_CHILDREN)?.value;
-if (menuItemCase.immediateData.find(f => f.stringId === GroupNavigationConstants.ITEM_FIELD_ID_HAS_CHILDREN)?.value && nodePath) {
+if (hasChildren && nodePath) {
this._pathService.activePath = nodePath;
} else if (nodePath) {
this._pathService.activePath = this._doubleDrawerNavigationService.extractParentPath(nodePath);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (menuItemCase.immediateData.find(f => f.stringId === GroupNavigationConstants.ITEM_FIELD_ID_HAS_CHILDREN)?.value && nodePath) { | |
| this._pathService.activePath = nodePath; | |
| } else if (nodePath) { | |
| this._pathService.activePath = this._doubleDrawerNavigationService.extractParentPath(nodePath); | |
| } | |
| const hasChildren = menuItemCase.immediateData.find(f => f.stringId === GroupNavigationConstants.ITEM_FIELD_ID_HAS_CHILDREN)?.value; | |
| if (hasChildren && nodePath) { | |
| this._pathService.activePath = nodePath; | |
| } else if (nodePath) { | |
| this._pathService.activePath = this._doubleDrawerNavigationService.extractParentPath(nodePath); | |
| } |
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/navigation/dashboard/abstract-dashboard.component.ts
around lines 272 to 276, the inline lookup for whether the menu item has
children should be extracted to a clearly named boolean (e.g., hasChildren) to
improve readability; create const hasChildren =
!!menuItemCase.immediateData.find(f => f.stringId ===
GroupNavigationConstants.ITEM_FIELD_ID_HAS_CHILDREN)?.value and then use if
(hasChildren && nodePath) { this._pathService.activePath = nodePath; } else if
(nodePath) { this._pathService.activePath =
this._doubleDrawerNavigationService.extractParentPath(nodePath); } to replace
the existing inline check.
| this._userService.user$.pipe(filter(u => !!u && u.id !== ''), take(1)).subscribe(() => { | ||
| this.resolveInitialValueOfPath(); | ||
|
|
||
| this._currentPathSubscription = this._pathService.activePath$.subscribe(path => { | ||
| if (path !== this.currentPath) { | ||
| if (path !== this.currentPath && !this._pathResolverLoading$.isActive) { | ||
| this.currentPath = path; | ||
| } else { | ||
| } else if (!this._pathResolverLoading$.isActive) { | ||
| this.openAvailableView(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid redundant updates by filtering duplicates at the source.
Replace the manual equality check with distinctUntilChanged() on activePath$ to reduce churn and simplify the conditional.
- this._currentPathSubscription = this._pathService.activePath$.subscribe(path => {
+ this._currentPathSubscription = this._pathService.activePath$
+ .pipe(distinctUntilChanged())
+ .subscribe(path => {
if (path !== this.currentPath && !this._pathResolverLoading$.isActive) {
this.currentPath = path;
} else if (!this._pathResolverLoading$.isActive) {
this.openAvailableView();
}
});Also import distinctUntilChanged from rxjs/operators.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts
around lines 130 to 139, replace the manual equality check inside the
activePath$ subscription with distinctUntilChanged() to filter out duplicate
path emissions at the source and simplify the branch logic; import
distinctUntilChanged from 'rxjs/operators' (or 'rxjs' pipeable operators
depending on project conventions), pipe activePath$ through
distinctUntilChanged() before subscribing, then update the subscription body to
assume each emission is different (remove the path !== this.currentPath check)
and keep handling of _pathResolverLoading$ and openAvailableView() as before.
| public get pathResolverLoading$() { | ||
| return this._pathResolverLoading$; | ||
| } |
There was a problem hiding this comment.
Expose pathResolverLoading$ as Observable<boolean> for template safety.
Return an observable to satisfy AsyncPipe and encapsulate the emitter.
Apply this diff:
- public get pathResolverLoading$() {
- return this._pathResolverLoading$;
- }
+ public get pathResolverLoading$(): Observable<boolean> {
+ return this._pathResolverLoading$.asObservable();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public get pathResolverLoading$() { | |
| return this._pathResolverLoading$; | |
| } | |
| public get pathResolverLoading$(): Observable<boolean> { | |
| return this._pathResolverLoading$.asObservable(); | |
| } |
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts
around lines 229 to 231, the getter currently exposes the internal Subject
directly; change the getter to return an Observable<boolean> (e.g. return
this._pathResolverLoading$.asObservable()) so templates can safely use AsyncPipe
and the emitter stays encapsulated.
| const groupNavigationRoute = this._config.getServicesConfiguration()?.groupNavigation?.groupNavigationRoute; | ||
| if (this._router.url.includes(groupNavigationRoute)) { | ||
| this._pathResolverLoading$.on(); |
There was a problem hiding this comment.
Guard against undefined groupNavigationRoute before calling .includes.
this._router.url.includes(groupNavigationRoute) can receive undefined. Add a null-check to avoid type/runtime issues.
- if (this._router.url.includes(groupNavigationRoute)) {
+ if (groupNavigationRoute && this._router.url.includes(groupNavigationRoute)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const groupNavigationRoute = this._config.getServicesConfiguration()?.groupNavigation?.groupNavigationRoute; | |
| if (this._router.url.includes(groupNavigationRoute)) { | |
| this._pathResolverLoading$.on(); | |
| const groupNavigationRoute = this._config.getServicesConfiguration()?.groupNavigation?.groupNavigationRoute; | |
| if (groupNavigationRoute && this._router.url.includes(groupNavigationRoute)) { | |
| this._pathResolverLoading$.on(); |
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts
around lines 389 to 391, guard against groupNavigationRoute being undefined
before calling .includes: fetch groupNavigationRoute from the config, then check
it is a non-empty string (e.g., if (groupNavigationRoute && typeof
groupNavigationRoute === 'string') or use truthy check) before evaluating
this._router.url.includes(groupNavigationRoute); only call
this._pathResolverLoading$.on() when that safe check passes.
| public extractParentPath(path: string): string { | ||
| if (path === '/') { | ||
| return path; | ||
| } | ||
| if (path.lastIndexOf('/') === 0) { | ||
| if (path?.lastIndexOf('/') === 0) { | ||
| return '/'; | ||
| } | ||
| return path.substring(0, path.lastIndexOf('/')); | ||
| return path?.substring(0, path?.lastIndexOf('/')); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Normalize and use PathService.SEPARATOR
Use the shared separator constant and normalize trailing slashes for robustness.
Apply this diff:
- public extractParentPath(path: string): string {
- if (path === '/') {
- return path;
- }
- if (path?.lastIndexOf('/') === 0) {
- return '/';
- }
- return path?.substring(0, path?.lastIndexOf('/'));
- }
+ public extractParentPath(path: string): string {
+ const sep = PathService.SEPARATOR;
+ if (!path || path === sep) return sep;
+ const normalized = path.endsWith(sep) && path !== sep ? path.slice(0, -1) : path;
+ const lastSlash = normalized.lastIndexOf(sep);
+ return lastSlash <= 0 ? sep : normalized.substring(0, lastSlash);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts
around lines 606 to 614, the extractParentPath function should use the shared
PathService.SEPARATOR and normalize trailing slashes: trim any trailing
separators (except when the path is exactly the separator), handle the
root/separator case explicitly, then find the last separator index using
PathService.SEPARATOR and return the substring up to that index (or the
separator for top-level entries). Update the implementation to reference
PathService.SEPARATOR, remove optional chaining where unnecessary, and ensure
inputs like '/some/path/' correctly yield '/some' and '/' remains '/'.
| this._datafieldsForMenuResolver$ = new ReplaySubject<Array<DataGroup>>(); | ||
| } |
There was a problem hiding this comment.
Cap ReplaySubject buffer to 1.
Default ReplaySubject stores unbounded history. Use ReplaySubject<Array<DataGroup>>(1).
- this._datafieldsForMenuResolver$ = new ReplaySubject<Array<DataGroup>>();
+ this._datafieldsForMenuResolver$ = new ReplaySubject<Array<DataGroup>>(1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this._datafieldsForMenuResolver$ = new ReplaySubject<Array<DataGroup>>(); | |
| } | |
| this._datafieldsForMenuResolver$ = new ReplaySubject<Array<DataGroup>>(1); | |
| } |
🤖 Prompt for AI Agents
In projects/netgrif-components-core/src/lib/navigation/service/path.service.ts
around lines 46 to 47, the ReplaySubject is created without a buffer size which
allows it to store an unbounded history; update the constructor to limit the
buffer to 1 by instantiating ReplaySubject<Array<DataGroup>>(1) so only the
latest emission is retained.
| public set datafieldsForMenuResolver(data: Array<DataGroup>) { | ||
| if (!this._datafieldsForMenuResolver$.closed) { | ||
| this._datafieldsForMenuResolver$.next(data); | ||
| this._datafieldsForMenuResolver$.complete(); | ||
| } else { | ||
| this._logger.error('PathService can have datafields for Menu resolver set only once'); | ||
| } | ||
| } | ||
|
|
||
| public set datafieldsForMenuResolverError(error: Error) { | ||
| if (!this._datafieldsForMenuResolver$.closed) { | ||
| this._datafieldsForMenuResolver$.error(error instanceof HttpErrorResponse ? error.error.message : error.message); | ||
| this._datafieldsForMenuResolver$.complete(); | ||
| } else { | ||
| this._logger.error('PathService can\'t handle error for datafields for Menu resolver, because stream is already closed'); | ||
| } | ||
| } | ||
|
|
||
| public get datafieldsForMenuResolver(): Observable<Array<DataGroup>> { | ||
| return this._datafieldsForMenuResolver$.asObservable(); | ||
| } | ||
|
|
||
| get isMenuResolverClosed(): boolean { | ||
| return this._datafieldsForMenuResolver$.closed; | ||
| } | ||
|
|
There was a problem hiding this comment.
Use isStopped instead of closed for lifecycle checks.
.closed reflects unsubscription, not completion/error. Use .isStopped to correctly guard one-shot semantics and to implement isMenuResolverClosed.
- public set datafieldsForMenuResolver(data: Array<DataGroup>) {
- if (!this._datafieldsForMenuResolver$.closed) {
+ public set datafieldsForMenuResolver(data: Array<DataGroup>) {
+ if (!this._datafieldsForMenuResolver$.isStopped) {
this._datafieldsForMenuResolver$.next(data);
this._datafieldsForMenuResolver$.complete();
} else {
this._logger.error('PathService can have datafields for Menu resolver set only once');
}
}
- public set datafieldsForMenuResolverError(error: Error) {
- if (!this._datafieldsForMenuResolver$.closed) {
+ public set datafieldsForMenuResolverError(error: Error) {
+ if (!this._datafieldsForMenuResolver$.isStopped) {
this._datafieldsForMenuResolver$.error(error instanceof HttpErrorResponse ? error.error.message : error.message);
- this._datafieldsForMenuResolver$.complete();
} else {
this._logger.error('PathService can\'t handle error for datafields for Menu resolver, because stream is already closed');
}
}
- get isMenuResolverClosed(): boolean {
- return this._datafieldsForMenuResolver$.closed;
+ get isMenuResolverClosed(): boolean {
+ return this._datafieldsForMenuResolver$.isStopped;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public set datafieldsForMenuResolver(data: Array<DataGroup>) { | |
| if (!this._datafieldsForMenuResolver$.closed) { | |
| this._datafieldsForMenuResolver$.next(data); | |
| this._datafieldsForMenuResolver$.complete(); | |
| } else { | |
| this._logger.error('PathService can have datafields for Menu resolver set only once'); | |
| } | |
| } | |
| public set datafieldsForMenuResolverError(error: Error) { | |
| if (!this._datafieldsForMenuResolver$.closed) { | |
| this._datafieldsForMenuResolver$.error(error instanceof HttpErrorResponse ? error.error.message : error.message); | |
| this._datafieldsForMenuResolver$.complete(); | |
| } else { | |
| this._logger.error('PathService can\'t handle error for datafields for Menu resolver, because stream is already closed'); | |
| } | |
| } | |
| public get datafieldsForMenuResolver(): Observable<Array<DataGroup>> { | |
| return this._datafieldsForMenuResolver$.asObservable(); | |
| } | |
| get isMenuResolverClosed(): boolean { | |
| return this._datafieldsForMenuResolver$.closed; | |
| } | |
| public set datafieldsForMenuResolver(data: Array<DataGroup>) { | |
| if (!this._datafieldsForMenuResolver$.isStopped) { | |
| this._datafieldsForMenuResolver$.next(data); | |
| this._datafieldsForMenuResolver$.complete(); | |
| } else { | |
| this._logger.error('PathService can have datafields for Menu resolver set only once'); | |
| } | |
| } | |
| public set datafieldsForMenuResolverError(error: Error) { | |
| if (!this._datafieldsForMenuResolver$.isStopped) { | |
| this._datafieldsForMenuResolver$.error(error instanceof HttpErrorResponse ? error.error.message : error.message); | |
| } else { | |
| this._logger.error('PathService can\'t handle error for datafields for Menu resolver, because stream is already closed'); | |
| } | |
| } | |
| public get datafieldsForMenuResolver(): Observable<Array<DataGroup>> { | |
| return this._datafieldsForMenuResolver$.asObservable(); | |
| } | |
| get isMenuResolverClosed(): boolean { | |
| return this._datafieldsForMenuResolver$.isStopped; | |
| } |
🤖 Prompt for AI Agents
In projects/netgrif-components-core/src/lib/navigation/service/path.service.ts
around lines 61 to 86, the code currently uses Subject.closed to decide whether
to next/error/complete and to report isMenuResolverClosed; replace all uses of
.closed with .isStopped because .closed indicates unsubscription whereas
.isStopped correctly indicates the Subject has completed or errored, so change
the two guards in the setters and the getter isMenuResolverClosed to check
this._datafieldsForMenuResolver$.isStopped instead of .closed while keeping the
same next/error/complete calls and log messages.
| ngOnDestroy() { | ||
| this._activePath$.complete(); | ||
| if (!this._datafieldsForMenuResolver$.closed) { | ||
| this._datafieldsForMenuResolver$.complete(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Complete subjects using isStopped guard.
Align with the lifecycle checks above.
- ngOnDestroy() {
- this._activePath$.complete();
- if (!this._datafieldsForMenuResolver$.closed) {
- this._datafieldsForMenuResolver$.complete();
- }
- }
+ ngOnDestroy(): void {
+ this._activePath$.complete();
+ if (!this._datafieldsForMenuResolver$.isStopped) {
+ this._datafieldsForMenuResolver$.complete();
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ngOnDestroy() { | |
| this._activePath$.complete(); | |
| if (!this._datafieldsForMenuResolver$.closed) { | |
| this._datafieldsForMenuResolver$.complete(); | |
| } | |
| } | |
| ngOnDestroy(): void { | |
| this._activePath$.complete(); | |
| if (!this._datafieldsForMenuResolver$.isStopped) { | |
| this._datafieldsForMenuResolver$.complete(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In projects/netgrif-components-core/src/lib/navigation/service/path.service.ts
around lines 96 to 101, the subject completion guard uses the Subject.closed
property; change this to use the Subject.isStopped guard to match the lifecycle
checks elsewhere — i.e., check that this._datafieldsForMenuResolver$.isStopped
is false before calling complete() on it (and keep completing this._activePath$
as is).
| <div *ngIf="(rightLoading$ | async) || (pathResolverLoading$ | async)" fxLayout="column" fxLayoutAlign="center center" class="margin-top-default"> | ||
| <mat-spinner color="primary" diameter="40" mode="indeterminate"></mat-spinner> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Loading block condition is correct; consider reducing duplicate subscriptions.
Optional: Use an ng-container with local variables (e.g., *ngIf="(rightLoading$ | async) as rl; else loading") to avoid multiple subscriptions and repeated expressions.
- Updated `GroupNavigationComponentResolverService` test to include `PathService` as a dependency.
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/group-navigation-component-resolver/group-navigation-component-resolver.service.spec.ts(2 hunks)
⏰ 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 (20)
- GitHub Check: Matrix Test (24)
- GitHub Check: Matrix Test (22)
- GitHub Check: Test with SonarCloud
🔇 Additional comments (1)
projects/netgrif-components-core/src/lib/navigation/group-navigation-component-resolver/group-navigation-component-resolver.service.spec.ts (1)
49-53: Constructor changes are correct and properly configured.PathService is properly provided via Angular's root injection mechanism (
providedIn: 'root'), so the test setup requires no additional configuration. The constructor correctly adds PathService as a dependency and passes it to the superclass.
...tion/group-navigation-component-resolver/group-navigation-component-resolver.service.spec.ts
Outdated
Show resolved
Hide resolved
- Updated `GroupNavigationComponentResolverService` test
Kovy95
dismissed
tuplle’s stale review
The merge-base changed after approval.
|
Description
Fixes NAE-2234
Dependencies
none
Third party dependencies
Blocking Pull requests
NAE-2233
#306
How Has Been This Tested?
manually
Test Configuration
<Please describe configuration for tests to run if applicable, like program parameters, host OS, VM configuration etc. You can use >
Checklist:
Summary by CodeRabbit
Bug Fixes
New Features