[NAE-2454] View Configuration 2.0#340
Open
Retoocs wants to merge 18 commits into
Open
Conversation
- add params to create-case-request-body.ts
- introduce reload menu frontend action
- introduce edit mode in double drawer menu - leave some todos - fix some memory leaks
- introduce task view dialog - rename existing task dialog to single task dialog - rework task dialog content filtering
- fix edit btn position
- rework loading of folders
- fix dialog overflow
- improve memory usage
- fix user and double drawer race condition
- implement handling of dynamic default sorting in menu item views
- fix memory leak in case-view-service.ts - add docs
WalkthroughAdds navigation menu edit mode, shared meta-header factories, dynamic default sort wiring for case and task views, and a searchBody-based task dialog flow with a new single-task dialog component. ChangesNavigation Menu Edit Mode and Folder Detection
Header Meta-Field Factory and Dynamic Default Sort
Task and Single-Task Dialog Rework
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
- fix tests
Contributor
Author
|
@coderabbitai review this PR |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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/abstract-navigation-double-drawer.ts (1)
158-168: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUnguarded
rightItemsSub.unsubscribe()will throw ifcanApplyAutoSelect()is false.
rightItemsSubis only assigned inside theif (this.canApplyAutoSelect())branch inngOnInit(Line 143). Whenever that condition is false (the common case for any drawer instance not matching the configured auto-select URL),rightItemsSubstaysundefined.ngOnDestroythen unconditionally callsthis.rightItemsSub.unsubscribe(), throwing aTypeErroron every such component's destruction._breakpointSubscriptionalready uses optional chaining (Line 159) for the same reason — the new fields should follow suit.🐛 Proposed fix
public ngOnDestroy(): void { this._breakpointSubscription?.unsubscribe(); this.loggedOut.complete(); this.stateChanged.complete(); this.itemClicked.complete(); this.resized.complete(); this.itemLoaded.complete(); - this.itemClickedSub.unsubscribe(); - this.itemLoadedSub.unsubscribe(); - this.rightItemsSub.unsubscribe(); + this.itemClickedSub?.unsubscribe(); + this.itemLoadedSub?.unsubscribe(); + this.rightItemsSub?.unsubscribe(); }Also applies to: 142-149, 95-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts` around lines 158 - 168, The ngOnDestroy cleanup in abstract-navigation-double-drawer currently unconditionally calls rightItemsSub.unsubscribe(), but rightItemsSub is only initialized in the canApplyAutoSelect() branch of ngOnInit and may be undefined. Update the teardown logic in ngOnDestroy to guard rightItemsSub the same way _breakpointSubscription is guarded, and keep the cleanup consistent with the other subscriptions like itemClickedSub and itemLoadedSub so destruction is safe when auto-select is disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts`:
- Around line 373-375: The case title in the navigation double drawer flow is
still hardcoded in English, bypassing the new translation support. Update the
menu-item creation logic in abstract-navigation-double-drawer.ts where the
object is built with netId, title, and params so that the title comes from the
same i18n/localization source used for the surrounding feature instead of the
literal “New menu item”. Keep the change consistent with the translated keys
already introduced for createMenuItemBtn and editMenuItem, and use the existing
menu-item creation path to ensure non-English users see a localized case title.
- Around line 369-401: The createMenuItem() method in
AbstractNavigationDoubleDrawer is using the deprecated multi-argument
subscribe(next, error) form. Update the observable subscription on the
getNet('menu_item') pipeline to use a single observer object with next and error
handlers instead of separate callback arguments, while preserving the existing
success path, _log.error reporting, and snack bar behavior.
In
`@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts`:
- Line 150: The newly touched RxJS subscriptions in
double-drawer-navigation.service.ts still use the deprecated multi-argument
subscribe(next, error) overload; update each affected subscription in the
double-drawer navigation flow to use the partial-observer form instead. Focus on
the subscribe calls in the service methods around
getNodeByPath/resolveParentPath and the other three matching sites in this file,
and keep the existing pipe(take(1)) behavior unchanged while moving next and
error handlers into a single observer object.
In
`@projects/netgrif-components-core/src/lib/view/case-view/service/case-view-service.ts`:
- Around line 124-131: The dynamic-sort subscribe/update/emit logic is
duplicated in this case-view paging path and also in nextPagePagination and
TaskViewService, so extract it into a shared helper on the service (for example,
applyDynamicSortThen(cb)) and call that from the existing branch. Keep the
behavior the same: when _dynamicDefaultSort$ exists and
_lastHeaderSearchState.fieldIdentifier is empty, subscribe to the sort change,
update _lastHeaderSearchState, and then dispatch the PageLoadRequestContext;
otherwise dispatch immediately.
In `@projects/netgrif-components/src/lib/dialog/dialog.theme.scss`:
- Around line 24-30: The dialog theme rule is using mat-mdc-dialog-container as
a type selector, so it never matches the Angular Material container class.
Update the selector in dialog.theme.scss to target the actual class with a
leading dot, matching the existing .mat-mdc-dialog-container usage elsewhere in
the file, so the overflow-y:auto styling applies to the dialog container as
intended.
In
`@projects/netgrif-components/src/lib/dialog/single-task-view-dialog/single-task-view-dialog.component.scss`:
- Around line 1-77: Remove the unused copy-pasted styles from
single-task-view-dialog.component.scss and keep only the selectors actually
referenced by SingleTaskViewDialogComponent’s template, namely
task-view-container, virtual-scroll-padding, and task-panel-padding-mini. Delete
the leftover dialog-specific classes such as task-tab-background, search-panel,
content-margin, search-width, full-height, public-button, public-button-padding,
single-page-task, logoimg, upper-card, upper-card-title, upper-divider, and
footer-custom so the stylesheet only contains relevant rules.
In
`@projects/netgrif-components/src/lib/navigation/navigation-double-drawer/navigation-double-drawer.component.html`:
- Around line 182-191: Both clickable row <div>s in
navigation-double-drawer.component.html are mouse-only interactive elements and
need keyboard support. Update the item row using onItemClick(item) and the
createMenuItem() row to behave like accessible controls by adding appropriate
keyboard activation handling and focus semantics, and keep the existing click
behavior in sync with those handlers. Use the existing templates around
onItemClick, createMenuItem, and the menu-item rows to apply the same accessible
pattern to both segments.
---
Outside diff comments:
In
`@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts`:
- Around line 158-168: The ngOnDestroy cleanup in
abstract-navigation-double-drawer currently unconditionally calls
rightItemsSub.unsubscribe(), but rightItemsSub is only initialized in the
canApplyAutoSelect() branch of ngOnInit and may be undefined. Update the
teardown logic in ngOnDestroy to guard rightItemsSub the same way
_breakpointSubscription is guarded, and keep the cleanup consistent with the
other subscriptions like itemClickedSub and itemLoadedSub so destruction is safe
when auto-select is disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9304774-99b3-4ef8-96de-92eb652ec0bf
📒 Files selected for processing (48)
projects/netgrif-components-core/src/assets/i18n/de.jsonprojects/netgrif-components-core/src/assets/i18n/en.jsonprojects/netgrif-components-core/src/assets/i18n/sk.jsonprojects/netgrif-components-core/src/lib/header/case-header/case-header.service.tsprojects/netgrif-components-core/src/lib/header/header-modes/sort-mode/abstract-sort-mode.component.tsprojects/netgrif-components-core/src/lib/header/models/meta-fields-factory.tsprojects/netgrif-components-core/src/lib/header/models/public-api.tsprojects/netgrif-components-core/src/lib/header/task-header/task-header.service.tsprojects/netgrif-components-core/src/lib/header/workflow-header/workflow-header.service.tsprojects/netgrif-components-core/src/lib/navigation/model/group-navigation-constants.tsprojects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.spec.tsprojects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.tsprojects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.tsprojects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/util/double-drawer-utils.tsprojects/netgrif-components-core/src/lib/resources/interface/create-case-request-body.tsprojects/netgrif-components-core/src/lib/side-menu/content-components/injection-tokens.tsprojects/netgrif-components-core/src/lib/side-menu/content-components/task-view/model/task-view-injection-data.tsprojects/netgrif-components-core/src/lib/view/case-view/models/dynamic-default-sort-token.tsprojects/netgrif-components-core/src/lib/view/case-view/service/case-view-service.tsprojects/netgrif-components-core/src/lib/view/public-api.tsprojects/netgrif-components-core/src/lib/view/task-view/service/task-view.service.tsprojects/netgrif-components/src/lib/dialog/dialog.module.tsprojects/netgrif-components/src/lib/dialog/dialog.theme.scssprojects/netgrif-components/src/lib/dialog/model/dialog-actions.tsprojects/netgrif-components/src/lib/dialog/public-api.tsprojects/netgrif-components/src/lib/dialog/single-task-view-dialog/single-task-view-dialog.component.htmlprojects/netgrif-components/src/lib/dialog/single-task-view-dialog/single-task-view-dialog.component.scssprojects/netgrif-components/src/lib/dialog/single-task-view-dialog/single-task-view-dialog.component.spec.tsprojects/netgrif-components/src/lib/dialog/single-task-view-dialog/single-task-view-dialog.component.tsprojects/netgrif-components/src/lib/dialog/task-view-dialog/task-view-dialog.component.htmlprojects/netgrif-components/src/lib/dialog/task-view-dialog/task-view-dialog.component.spec.tsprojects/netgrif-components/src/lib/dialog/task-view-dialog/task-view-dialog.component.tsprojects/netgrif-components/src/lib/header/header.component.tsprojects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/model/factory-methods.tsprojects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/simple-views/default-simple-case-view/default-simple-case-view.component.spec.tsprojects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/simple-views/default-simple-case-view/default-simple-case-view.component.tsprojects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/simple-views/default-simple-task-view/default-simple-task-view.component.spec.tsprojects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/simple-views/default-simple-task-view/default-simple-task-view.component.tsprojects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/tabbed/default-tab-view/default-tab-view.component.spec.tsprojects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/tabbed/default-tabbed-case-view/default-tabbed-case-view.component.spec.tsprojects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/tabbed/default-tabbed-case-view/default-tabbed-case-view.component.tsprojects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/tabbed/default-tabbed-task-view/default-tabbed-task-view.component.spec.tsprojects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/tabbed/default-tabbed-task-view/default-tabbed-task-view.component.tsprojects/netgrif-components/src/lib/navigation/model/reload-menu.tsprojects/netgrif-components/src/lib/navigation/navigation-double-drawer/navigation-double-drawer.component.htmlprojects/netgrif-components/src/lib/navigation/navigation-double-drawer/navigation-double-drawer.component.scssprojects/netgrif-components/src/lib/navigation/navigation-double-drawer/navigation-double-drawer.component.tsprojects/netgrif-components/src/lib/navigation/navigation.module.ts
Comment on lines
+1
to
+77
| .task-tab-background{ | ||
| height: 100%; | ||
| width: 100%; | ||
| overflow: auto; | ||
| background-color: transparent; | ||
| } | ||
| .search-panel{ | ||
| margin-top: 8px; | ||
| } | ||
| .content-margin{ | ||
| margin: 0 8px; | ||
| } | ||
| .task-panel-padding-mini{ | ||
| padding-bottom: 8px; | ||
| padding-top: 8px; | ||
| } | ||
|
|
||
| .search-width { | ||
| width: 100%; | ||
| } | ||
|
|
||
| .full-height { | ||
| height: 100%; | ||
| } | ||
| .public-button { | ||
| height: 36px; | ||
| width: 200px; | ||
| } | ||
|
|
||
| .public-button-padding { | ||
| padding: 16px; | ||
| flex-direction: column; | ||
| display: flex; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .single-page-task { | ||
| width: 85% !important; | ||
| padding-bottom: 25px; | ||
| } | ||
|
|
||
| .logoimg { | ||
| height: 120px; | ||
| outline: none !important; | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| .upper-card { | ||
| margin-bottom: 10px; | ||
| } | ||
|
|
||
| .upper-card-title { | ||
| display: flex; | ||
| align-items: center; | ||
| flex-direction: column; | ||
| } | ||
|
|
||
| .upper-divider { | ||
| border-top-width: 20px; | ||
| border-top-color: #023b7e; | ||
| border-radius: 4px; | ||
| margin-bottom: 10px; | ||
| } | ||
|
|
||
| .footer-custom { | ||
| border-radius: 4px; | ||
| margin-bottom: 10px; | ||
| } | ||
| .task-view-container { | ||
| height: calc(100% - 64px) !important; | ||
| padding-left: 8px !important; | ||
| padding-right: 8px !important; | ||
| margin: 0; | ||
| } | ||
| .virtual-scroll-padding { | ||
| padding-right: 4px; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Unused/copy-pasted CSS classes.
Only .task-view-container, .virtual-scroll-padding, and .task-panel-padding-mini are referenced in this component's template. The remaining classes (.task-tab-background, .search-panel, .content-margin, .search-width, .full-height, .public-button, .public-button-padding, .single-page-task, .logoimg, .upper-card, .upper-card-title, .upper-divider, .footer-custom) look like leftovers copied from another dialog and are dead weight here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@projects/netgrif-components/src/lib/dialog/single-task-view-dialog/single-task-view-dialog.component.scss`
around lines 1 - 77, Remove the unused copy-pasted styles from
single-task-view-dialog.component.scss and keep only the selectors actually
referenced by SingleTaskViewDialogComponent’s template, namely
task-view-container, virtual-scroll-padding, and task-panel-padding-mini. Delete
the leftover dialog-specific classes such as task-tab-background, search-panel,
content-margin, search-width, full-height, public-button, public-button-padding,
single-page-task, logoimg, upper-card, upper-card-title, upper-divider, and
footer-custom so the stylesheet only contains relevant rules.
Kovy95
reviewed
Jul 6, 2026
| @@ -0,0 +1,42 @@ | |||
| import {HeaderColumn, HeaderColumnType} from "./header-column"; | |||
| import {CaseMetaField} from "../case-header/case-menta-enum"; | |||
Kovy95
reviewed
Jul 6, 2026
|
|
||
| this._currentNodeSubscription = this._uriService.activeNode$.subscribe(node => { | ||
| this._currentNodeSubscription = this._userService.user$.pipe( | ||
| filter(user => !!user && user.id !== ''), |
Contributor
There was a problem hiding this comment.
We should create a method in userService for this, if isn't exists now, because it is used on more places and in some projects the property id is not used
Contributor
Author
There was a problem hiding this comment.
done, used existing isUserEmpty method
Kovy95
reviewed
Jul 6, 2026
| this.resolveUriForChildViews(childViewConfigPath, childView); | ||
| this.resolveHiddenMenuItemFromChildViews(childViewConfigPath, childView); | ||
| }); | ||
| this._userSubscription = this._userService.user$.pipe(filter(user => !!user && user.id !== '')).subscribe(user => { |
Contributor
There was a problem hiding this comment.
shouldn't be here also take(1) instead of userSubscription ? if this method is called more times, only last subscription will be unsubscribed, and can create memory leaks on userChange. If the intension is to change entries on some userChange, then the subscription should be in constructor and some refresh mechanism should be applied
Kovy95
reviewed
Jul 6, 2026
| return; | ||
| } | ||
| const _case = (outcome.outcome as CreateCaseEventOutcome).aCase; | ||
| const taskView = this._injector.get(NAE_TASK_VIEW_COMPONENT); |
Contributor
There was a problem hiding this comment.
we should directly inject this injectionToken into constructor and not use an injector like this
Kovy95
reviewed
Jul 6, 2026
| data: { | ||
| autoCloseOnEvent: true, | ||
| searchBody: { | ||
| transitionId: "initialize", |
Contributor
There was a problem hiding this comment.
do we want show only initialize transition in dialog? or maybe also item_settings, which is next after initialize
Contributor
Author
There was a problem hiding this comment.
I think showing only initialize trans is enough. Once you finish the task, the dialog closes. Then you can do something else on the page or by one click open settings dialog
Kovy95
reviewed
Jul 6, 2026
| import {SideMenuInjectionData} from "../../../models/side-menu-injection-data"; | ||
| import {TaskSearchRequestBody} from "../../../../filter/models/task-search-request-body"; | ||
|
|
||
| export interface TaskViewInjectionData extends SideMenuInjectionData { |
Contributor
There was a problem hiding this comment.
do we still support side menu ? or is this interface used also in dialogs? we should create interface only for dialogs
Contributor
Author
There was a problem hiding this comment.
Currently I use the map from the interface to save attribute autoCloseOnEvent
Contributor
Author
There was a problem hiding this comment.
done, I introduced TaskViewDialogInjectionData, where the autoCloseOnEvent atrribute exists
Kovy95
reviewed
Jul 6, 2026
| if (!!this._dynamicDefaultSort$ && this._lastHeaderSearchState.fieldIdentifier === '') { | ||
| this._dynamicDefaultSort$.subscribe(sortChangeDesc => { | ||
| this._lastHeaderSearchState = sortChangeDesc; | ||
| this._nextPage$.next(new PageLoadRequestContext(this.activeFilter, Object.assign({}, this._pagination, {number: 0}))); |
Contributor
There was a problem hiding this comment.
i think we want to push defalut value to nextPage before the nextPage pipe in column 94, or we? because now the pipe is executed twice, also with initial null value
Kovy95
reviewed
Jul 6, 2026
| this._closeTaskTabOnNoTasks = bool; | ||
| }); | ||
|
|
||
| if (!!this._dynamicDefaultSort$ && this._lastHeaderSearchState.fieldIdentifier === '') { |
Contributor
There was a problem hiding this comment.
same as in caseViewService, do we want to push this after the requestedPage pipe?
Kovy95
reviewed
Jul 6, 2026
|
|
||
| public processTaskEvents(notification: TaskEventNotification): void { | ||
| if (!notification.success) { | ||
| if (!notification.success || !this._injectedData['autoCloseOnEvent']) { |
Contributor
There was a problem hiding this comment.
shouldnt be this added to interface?
Kovy95
reviewed
Jul 6, 2026
| HotkeyModule.forRoot(), | ||
| UtilityModule | ||
| UtilityModule, | ||
| TranslateModule |
Contributor
There was a problem hiding this comment.
translateLibModule is already imported, and it exports TranslateModule, so we should prevent double importing ( as I see also TranslateLibModule is here twice)
Kovy95
reviewed
Jul 6, 2026
| _redirectService: RedirectService, | ||
| _snackBarService: SnackBarService, | ||
| _dialog: MatDialog, | ||
| _injector: Injector, |
Contributor
There was a problem hiding this comment.
new properties in constructor should be at the most possible end of it
Kovy95
requested changes
Jul 6, 2026
Kovy95
reviewed
Jul 7, 2026
| return; | ||
| } | ||
| this._nextPage$.next(requestContext); | ||
| if (!!this._dynamicDefaultSort$ && this._lastHeaderSearchState.fieldIdentifier === '') { |
Contributor
There was a problem hiding this comment.
duplicate code as in constructor, create function for it and pipe(take(1))
- resolve PR comments
coderabbitai
Bot
added
breaking change
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 (7)
projects/netgrif-components/src/lib/dialog/single-task-view-dialog/single-task-view-dialog.component.ts (2)
23-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated filter-factory logic with
taskViewFilterFactory.
singleTaskViewFilterFactoryis nearly identical totaskViewFilterFactoryinprojects/netgrif-components/src/lib/dialog/task-view-dialog/task-view-dialog.component.ts(Lines 22-30) — same validation, same error message, sameSimpleFilter.fromTaskQuerycall. Consider extracting a shared factory helper to avoid drift between the two dialogs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components/src/lib/dialog/single-task-view-dialog/single-task-view-dialog.component.ts` around lines 23 - 32, The singleTaskViewFilterFactory logic is duplicated with taskViewFilterFactory, including the same null check, error message, and SimpleFilter.fromTaskQuery call. Extract the shared validation and filter निर्माण into a common helper used by both singleTaskViewFilterFactory and taskViewFilterFactory, so both dialogs reuse the same implementation and stay in sync.
73-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
processTaskEventsignoresautoCloseOnEvent, unlike the siblingTaskViewDialogComponent.
this._injectedDatais typed asTaskViewDialogInjectionData(which declaresautoCloseOnEvent), but this method unconditionally closes on any successfulFINISH/CANCELevent, never consulting that flag — whereasTaskViewDialogComponent.processTaskEventsrequires_injectedData.autoCloseOnEventto be truthy (task-view-dialog.component.ts, Line 73). If intentional (single-task dialogs should always auto-close), consider dropping the unused field from this component's contract or adding a short comment explaining the divergence to avoid future confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components/src/lib/dialog/single-task-view-dialog/single-task-view-dialog.component.ts` around lines 73 - 85, `processTaskEvents` currently closes the dialog on every successful `FINISH`/`CANCEL` event without checking `this._injectedData.autoCloseOnEvent`, unlike `TaskViewDialogComponent.processTaskEvents`. Update `SingleTaskViewDialogComponent.processTaskEvents` to either honor the `autoCloseOnEvent` flag before calling `_dialogRef.close`, or if single-task dialogs should always close, remove `autoCloseOnEvent` from the injected data contract and document the intentional difference. Use `processTaskEvents`, `_injectedData`, and `_dialogRef.close` to locate the behavior.projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts (3)
346-358: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
customItemsInitialized/hiddenCustomItemsInitializedare only set after the async user emission resolves.The guard at the top of the method (
if (!view || this.customItemsInitialized || ...) return;) is checked synchronously, but the flags themselves are only flipped inside theuser$subscribe callback. IfinitializeCustomViewsOfViewis invoked more than once before the first non-empty user emits (e.g. two view-init calls in quick succession), both calls will pass the guard and each will push duplicate entries into_childCustomViews/_hiddenCustomItems$once the user resolves.🔧 Proposed fix
public initializeCustomViewsOfView(view: View, viewConfigPath: string): void { if (!view || this.customItemsInitialized || this.hiddenCustomItemsInitialized) return; + this.customItemsInitialized = true; + this.hiddenCustomItemsInitialized = true; this._userService.user$.pipe(filter(user => !this._userService.isUserEmpty(user)), take(1)).subscribe(user => { Object.entries(view.children).forEach(([key, childView]) => { const childViewConfigPath: string = viewConfigPath + '/' + key; this.resolveUriForChildViews(childViewConfigPath, childView); this.resolveHiddenMenuItemFromChildViews(childViewConfigPath, childView); }); this.resolveCustomViewsInRightSide(); this.resolveCustomViewsInLeftSide(); - - this.customItemsInitialized = true; - this.hiddenCustomItemsInitialized = true; }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts` around lines 346 - 358, The initialization guard in initializeCustomViewsOfView is vulnerable to duplicate work because customItemsInitialized and hiddenCustomItemsInitialized are only set inside the async user$.subscribe callback. Mark these flags before subscribing, or add a separate “initialization started” guard in the same method, so repeated calls cannot enqueue duplicate child/custom view resolution while waiting for the first non-empty user emission.
379-422: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
loadLeftSideandloadRightSideare near-duplicates.Both methods now share almost identical fetch → derive-child-ids → fetch-cases → sort/emit → error-handling structure, differing mainly in pagination splitting and item mapping. The just-found missed observer-object fix in
loadRightSideis direct evidence that duplicated logic drifts out of sync. Consider extracting the commongetItemCaseByNodePath(...) -> childCases$resolution into a shared helper parameterized by the divergent mapping/splitting behavior.Also applies to: 424-470
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts` around lines 379 - 422, The `loadLeftSide` and `loadRightSide` flows are duplicated and already drifting apart, so extract the shared fetch/derive-child-ids/fetch-cases/error-handling logic into a common helper used by both methods. Keep the method-specific differences in pagination splitting and item mapping in small callbacks or parameters, and update `loadLeftSide`/`loadRightSide` to delegate to that helper so future observer or error-handling fixes only need to be made once.
136-169: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSerialize deferred navigation loading; don’t queue multiple waiters on
_nodeLoading$
currentNodecan be assigned again while a prior parent lookup is still in flight. Each assignment then registers anotherfilter(!isActive).take(1)callback on the sameLoadingEmitter, so oneoff()can trigger multipleloadNavigationItems(node)calls with different captured nodes. Replace this with a single-flight flow or cancel the previous deferred subscription before registering a new one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts` around lines 136 - 169, `set currentNode` in `double-drawer-navigation.service.ts` can register multiple deferred callbacks on `_nodeLoading$`, causing repeated `loadNavigationItems(node)` calls for stale nodes when parent lookup is still pending. Update the `currentNode` flow to use a single-flight/cancellable deferred load: before adding the `filter(!isActive).take(1)` subscription, cancel any previous waiter or reuse one pending subscription, and ensure only the latest `node` is loaded after `_nodeLoading$.off()` in the `set currentNode` logic.projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts (2)
100-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_injector: Injectorappears unused after switching_taskViewto constructor injection.The past review comment asking to inject
NAE_TASK_VIEW_COMPONENTdirectly rather than viaInjector.get(...)was addressed (line 120 now uses@Inject), but the_injector: Injectorparameter (line 119) and itsInjectorimport (line 2) don't appear to be referenced anywhere else in this file. If nothing else needs it, it can be dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts` around lines 100 - 133, The abstractNavigationDoubleDrawer constructor still keeps an unused Injector dependency after switching to direct NAE_TASK_VIEW_COMPONENT injection. Remove the _injector: Injector parameter from AbstractNavigationDoubleDrawer and drop the Injector import if nothing else in this file uses it; keep the existing `@Inject`(NAE_TASK_VIEW_COMPONENT) _taskView injection as the source of the component.
96-98: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
rightItemsSubmay beundefinedwhenngOnDestroyunsubscribes it unconditionally.
rightItemsSubis only assigned insideif (this.canApplyAutoSelect())(lines 144-151). If that condition is false for a given component instance,rightItemsSubstaysundefined, andngOnDestroy(line 169) callingthis.rightItemsSub.unsubscribe()without optional chaining will throw, potentially interrupting Angular's destroy lifecycle.🔧 Proposed fix
- this.itemClickedSub.unsubscribe(); - this.itemLoadedSub.unsubscribe(); - this.rightItemsSub.unsubscribe(); + this.itemClickedSub?.unsubscribe(); + this.itemLoadedSub?.unsubscribe(); + this.rightItemsSub?.unsubscribe();Also applies to: 144-151, 167-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts` around lines 96 - 98, rightItemsSub can remain undefined when canApplyAutoSelect() is false, but ngOnDestroy in AbstractNavigationDoubleDrawer unsubscribes it unconditionally. Update the destruction logic to guard rightItemsSub the same way as the other subscriptions (for example, using the same optional cleanup pattern already used for itemClickedSub and itemLoadedSub), and make sure the assignment in the auto-select block stays the only place it is created.
♻️ Duplicate comments (4)
projects/netgrif-components-core/src/lib/dialog/models/task-view-dialog-injection-data.ts (1)
3-6: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConsider making
autoCloseOnEventoptional with a documented default.
autoCloseOnEventis declared as a requiredboolean, but at least one caller (openTaskDialoginprojects/netgrif-components/src/lib/dialog/model/dialog-actions.ts) constructs this data via anascast without ever setting it. Sinceasbypasses missing-property checks, this compiles but leavesautoCloseOnEventundefinedat runtime, silently changing dialog-close behavior (see comment ondialog-actions.ts). Making the field optional (autoCloseOnEvent?: boolean) and having consumers explicitly?? false/?? truewould surface the intent and prevent this class of bug.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/dialog/models/task-view-dialog-injection-data.ts` around lines 3 - 6, `TaskViewDialogInjectionData` currently պահանջs `autoCloseOnEvent`, but `openTaskDialog` is already constructing it via an `as` cast without setting that field, so make the property optional in the interface and document the default behavior. Update the consumers that read `TaskViewDialogInjectionData` to explicitly apply a fallback with `??` so the intended close behavior is set in one place rather than relying on an undefined value.projects/netgrif-components/src/lib/dialog/task-view-dialog/task-view-dialog.component.ts (1)
72-75: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAuto-close guard is correct here, but depends on a caller that omits the flag.
The
autoCloseOnEventcheck is correctly implemented per theTaskViewDialogInjectionDatacontract, butprojects/netgrif-components/src/lib/dialog/model/dialog-actions.ts(openTaskDialog) never setsautoCloseOnEventwhen opening this dialog, so it will always early-return and never auto-close for that call site. See the comment ondialog-actions.tsfor the root-cause fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components/src/lib/dialog/task-view-dialog/task-view-dialog.component.ts` around lines 72 - 75, The auto-close logic in processTaskEvents is fine, but openTaskDialog in dialog-actions is not populating the autoCloseOnEvent field, so this dialog always exits early. Update openTaskDialog to pass the autoCloseOnEvent value into the TaskViewDialogInjectionData when creating the dialog, matching the contract used by TaskViewDialogComponent and its processTaskEvents guard.projects/netgrif-components/src/lib/dialog/model/dialog-actions.ts (1)
17-19: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing
autoCloseOnEventbreaks task-dialog auto-close.The injected data only sets
searchBody;autoCloseOnEventis omitted and theas TaskViewDialogInjectionDatacast masks this at compile time. At runtime,TaskViewDialogComponent.processTaskEventsguards on!this._injectedData.autoCloseOnEvent(task-view-dialog.component.ts, Line 73), so withautoCloseOnEventundefinedthis dialog will never auto-close onFINISH/CANCELwhen opened through thisopenDialogaction.🐛 Proposed fix
data: { - searchBody: { stringId: frontAction.args[0] } + searchBody: { stringId: frontAction.args[0] }, + autoCloseOnEvent: true } as TaskViewDialogInjectionData,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components/src/lib/dialog/model/dialog-actions.ts` around lines 17 - 19, The `openDialog` action in `dialog-actions.ts` is constructing `TaskViewDialogInjectionData` with only `searchBody`, so `TaskViewDialogComponent.processTaskEvents` never sees `autoCloseOnEvent` and the dialog cannot auto-close on task completion/cancel. Update the injected `data` object to include `autoCloseOnEvent` with the appropriate value for this action, and avoid relying on the `as TaskViewDialogInjectionData` cast to hide the missing field. Use the `openDialog` flow and `TaskViewDialogInjectionData` shape as the guide so the dialog behavior matches `TaskViewDialogComponent.processTaskEvents`.projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts (1)
424-470: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDeprecated two-argument
subscribe(next, error)reintroduced inloadRightSide's inner subscription.Every other subscription in this diff (including the sibling
loadLeftSideinner subscribe at line 401 and the outer subscribes at 385/427) was migrated to the{ next, error }observer form, matching the fix from the earlier SonarCloud-flagged review. This one (line 443) was missed, reintroducing the same deprecated pattern in the same file.♻️ Proposed fix
- childCases$.pipe(take(1)).subscribe(result => { - result = (result as Case[]).sort((a, b) => orderedChildCaseIds.indexOf(a.stringId) - orderedChildCaseIds.indexOf(b.stringId)); - if (result.length > RIGHT_SIDE_INIT_PAGE_SIZE) { - const rawRightItems: Case[] = result.splice(0, RIGHT_SIDE_INIT_PAGE_SIZE); - this._rightItems$.next(rawRightItems.map(folder => this.resolveItemCaseToNavigationItem(folder)).filter(i => !!i)); - this._moreItems$.next(result.map(folder => this.resolveItemCaseToNavigationItem(folder)).filter(i => !!i)); - } else { - this._rightItems$.next(result.map(folder => this.resolveItemCaseToNavigationItem(folder)).filter(i => !!i)); - } - this.resolveCustomViewsInRightSide(); - this._rightLoading$.off(); - this.itemLoaded.emit({menu: 'right', items: this.rightItems}); - }, error => { - this._log.error(error); - this._rightItems$.next([]); - this._moreItems$.next([]); - this.resolveCustomViewsInRightSide(); - this._rightLoading$.off(); - }); + childCases$.pipe(take(1)).subscribe({ + next: result => { + result = (result as Case[]).sort((a, b) => orderedChildCaseIds.indexOf(a.stringId) - orderedChildCaseIds.indexOf(b.stringId)); + if (result.length > RIGHT_SIDE_INIT_PAGE_SIZE) { + const rawRightItems: Case[] = result.splice(0, RIGHT_SIDE_INIT_PAGE_SIZE); + this._rightItems$.next(rawRightItems.map(folder => this.resolveItemCaseToNavigationItem(folder)).filter(i => !!i)); + this._moreItems$.next(result.map(folder => this.resolveItemCaseToNavigationItem(folder)).filter(i => !!i)); + } else { + this._rightItems$.next(result.map(folder => this.resolveItemCaseToNavigationItem(folder)).filter(i => !!i)); + } + this.resolveCustomViewsInRightSide(); + this._rightLoading$.off(); + this.itemLoaded.emit({menu: 'right', items: this.rightItems}); + }, + error: error => { + this._log.error(error); + this._rightItems$.next([]); + this._moreItems$.next([]); + this.resolveCustomViewsInRightSide(); + this._rightLoading$.off(); + } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts` around lines 424 - 470, The inner subscription in loadRightSide still uses the deprecated two-argument subscribe form. Update that childCases$.pipe(take(1)).subscribe call in DoubleDrawerNavigationService to the observer object style with next and error handlers, matching the surrounding subscriptions and the loadLeftSide pattern already used in this class.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@projects/netgrif-components-core/src/lib/view/case-view/service/case-view-service.ts`:
- Around line 373-382: The subscription in requestPageWithDynamicSort can leak
because _dynamicDefaultSort$ is subscribed without teardown when
_lastHeaderSearchState.fieldIdentifier is empty. Update this path in
CaseViewService to use a one-time subscription (for example by applying take(1))
or otherwise capture and dispose the Subscription in ngOnDestroy, and keep the
existing behavior of updating _lastHeaderSearchState before calling
_nextPage$.next(requestContext).
- Around line 92-95: Initialize the _nextPage$ stream before it is used in
CaseViewService, since the constructor currently calls this._nextPage$.pipe(...)
while the field is still undefined. Update the CaseViewService setup so
_nextPage$ is assigned to a valid observable before requestPageWithDynamicSort
or any pipe usage, and verify the constructor path cannot run until the stream
is created.
In
`@projects/netgrif-components-core/src/lib/view/task-view/service/task-view.service.ts`:
- Around line 90-92: Initialize _requestedPage$ before calling
requestPageWithDynamicSort() in TaskViewService, because
requestPageWithDynamicSort() immediately uses this._requestedPage$.next(...) and
the observable is currently still unset when the first request is triggered
during construction. Move or add the _requestedPage$ assignment so it exists
before the initial requestPageWithDynamicSort(new PageLoadRequestContext(...))
call, and verify the constructor sets it up before any subscriptions to
_requestedPage$ are created.
---
Outside diff comments:
In
`@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts`:
- Around line 100-133: The abstractNavigationDoubleDrawer constructor still
keeps an unused Injector dependency after switching to direct
NAE_TASK_VIEW_COMPONENT injection. Remove the _injector: Injector parameter from
AbstractNavigationDoubleDrawer and drop the Injector import if nothing else in
this file uses it; keep the existing `@Inject`(NAE_TASK_VIEW_COMPONENT) _taskView
injection as the source of the component.
- Around line 96-98: rightItemsSub can remain undefined when
canApplyAutoSelect() is false, but ngOnDestroy in AbstractNavigationDoubleDrawer
unsubscribes it unconditionally. Update the destruction logic to guard
rightItemsSub the same way as the other subscriptions (for example, using the
same optional cleanup pattern already used for itemClickedSub and
itemLoadedSub), and make sure the assignment in the auto-select block stays the
only place it is created.
In
`@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts`:
- Around line 346-358: The initialization guard in initializeCustomViewsOfView
is vulnerable to duplicate work because customItemsInitialized and
hiddenCustomItemsInitialized are only set inside the async user$.subscribe
callback. Mark these flags before subscribing, or add a separate “initialization
started” guard in the same method, so repeated calls cannot enqueue duplicate
child/custom view resolution while waiting for the first non-empty user
emission.
- Around line 379-422: The `loadLeftSide` and `loadRightSide` flows are
duplicated and already drifting apart, so extract the shared
fetch/derive-child-ids/fetch-cases/error-handling logic into a common helper
used by both methods. Keep the method-specific differences in pagination
splitting and item mapping in small callbacks or parameters, and update
`loadLeftSide`/`loadRightSide` to delegate to that helper so future observer or
error-handling fixes only need to be made once.
- Around line 136-169: `set currentNode` in
`double-drawer-navigation.service.ts` can register multiple deferred callbacks
on `_nodeLoading$`, causing repeated `loadNavigationItems(node)` calls for stale
nodes when parent lookup is still pending. Update the `currentNode` flow to use
a single-flight/cancellable deferred load: before adding the
`filter(!isActive).take(1)` subscription, cancel any previous waiter or reuse
one pending subscription, and ensure only the latest `node` is loaded after
`_nodeLoading$.off()` in the `set currentNode` logic.
In
`@projects/netgrif-components/src/lib/dialog/single-task-view-dialog/single-task-view-dialog.component.ts`:
- Around line 23-32: The singleTaskViewFilterFactory logic is duplicated with
taskViewFilterFactory, including the same null check, error message, and
SimpleFilter.fromTaskQuery call. Extract the shared validation and filter
निर्माण into a common helper used by both singleTaskViewFilterFactory and
taskViewFilterFactory, so both dialogs reuse the same implementation and stay in
sync.
- Around line 73-85: `processTaskEvents` currently closes the dialog on every
successful `FINISH`/`CANCEL` event without checking
`this._injectedData.autoCloseOnEvent`, unlike
`TaskViewDialogComponent.processTaskEvents`. Update
`SingleTaskViewDialogComponent.processTaskEvents` to either honor the
`autoCloseOnEvent` flag before calling `_dialogRef.close`, or if single-task
dialogs should always close, remove `autoCloseOnEvent` from the injected data
contract and document the intentional difference. Use `processTaskEvents`,
`_injectedData`, and `_dialogRef.close` to locate the behavior.
---
Duplicate comments:
In
`@projects/netgrif-components-core/src/lib/dialog/models/task-view-dialog-injection-data.ts`:
- Around line 3-6: `TaskViewDialogInjectionData` currently պահանջs
`autoCloseOnEvent`, but `openTaskDialog` is already constructing it via an `as`
cast without setting that field, so make the property optional in the interface
and document the default behavior. Update the consumers that read
`TaskViewDialogInjectionData` to explicitly apply a fallback with `??` so the
intended close behavior is set in one place rather than relying on an undefined
value.
In
`@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts`:
- Around line 424-470: The inner subscription in loadRightSide still uses the
deprecated two-argument subscribe form. Update that
childCases$.pipe(take(1)).subscribe call in DoubleDrawerNavigationService to the
observer object style with next and error handlers, matching the surrounding
subscriptions and the loadLeftSide pattern already used in this class.
In `@projects/netgrif-components/src/lib/dialog/model/dialog-actions.ts`:
- Around line 17-19: The `openDialog` action in `dialog-actions.ts` is
constructing `TaskViewDialogInjectionData` with only `searchBody`, so
`TaskViewDialogComponent.processTaskEvents` never sees `autoCloseOnEvent` and
the dialog cannot auto-close on task completion/cancel. Update the injected
`data` object to include `autoCloseOnEvent` with the appropriate value for this
action, and avoid relying on the `as TaskViewDialogInjectionData` cast to hide
the missing field. Use the `openDialog` flow and `TaskViewDialogInjectionData`
shape as the guide so the dialog behavior matches
`TaskViewDialogComponent.processTaskEvents`.
In
`@projects/netgrif-components/src/lib/dialog/task-view-dialog/task-view-dialog.component.ts`:
- Around line 72-75: The auto-close logic in processTaskEvents is fine, but
openTaskDialog in dialog-actions is not populating the autoCloseOnEvent field,
so this dialog always exits early. Update openTaskDialog to pass the
autoCloseOnEvent value into the TaskViewDialogInjectionData when creating the
dialog, matching the contract used by TaskViewDialogComponent and its
processTaskEvents guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5fd62461-b08d-44b3-ab75-fe6efc10a135
📒 Files selected for processing (23)
projects/netgrif-components-core/src/lib/dialog/models/task-view-dialog-injection-data.tsprojects/netgrif-components-core/src/lib/dialog/public-api.tsprojects/netgrif-components-core/src/lib/header/case-header/case-header.service.spec.tsprojects/netgrif-components-core/src/lib/header/case-header/case-meta-enum.tsprojects/netgrif-components-core/src/lib/header/header-modes/edit-mode/abstract-edit-mode.component.spec.tsprojects/netgrif-components-core/src/lib/header/models/meta-fields-factory.tsprojects/netgrif-components-core/src/lib/header/public-api.tsprojects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.tsprojects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.tsprojects/netgrif-components-core/src/lib/panel/case-panel/abstract-case-panel.component.spec.tsprojects/netgrif-components-core/src/lib/panel/case-panel/abstract-case-panel.component.tsprojects/netgrif-components-core/src/lib/search/header-search-service/header-search.service.tsprojects/netgrif-components-core/src/lib/side-menu/content-components/public-api.tsprojects/netgrif-components-core/src/lib/side-menu/content-components/task-view/model/task-view-injection-data.tsprojects/netgrif-components-core/src/lib/view/case-view/service/case-view-service.tsprojects/netgrif-components-core/src/lib/view/task-view/service/task-view.service.tsprojects/netgrif-components/src/lib/dialog/dialog.module.tsprojects/netgrif-components/src/lib/dialog/dialog.theme.scssprojects/netgrif-components/src/lib/dialog/model/dialog-actions.tsprojects/netgrif-components/src/lib/dialog/single-task-view-dialog/single-task-view-dialog.component.tsprojects/netgrif-components/src/lib/dialog/task-view-dialog/task-view-dialog.component.tsprojects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/model/factory-methods.tsprojects/netgrif-components/src/lib/navigation/navigation-double-drawer/navigation-double-drawer.component.ts
💤 Files with no reviewable changes (2)
- projects/netgrif-components-core/src/lib/side-menu/content-components/public-api.ts
- projects/netgrif-components-core/src/lib/side-menu/content-components/task-view/model/task-view-injection-data.ts
- resolve PR comments
- fix observable initialization - fix import
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/view/case-view/service/case-view-service.ts (1)
373-390: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winInitialize
_nextPage$before the dynamic-sort request. On the process-backed dynamic-sort path,requestPageWithDynamicSort()can deferrequestNextPage()until after the constructor already reachesthis._nextPage$.pipe(...), so_nextPage$is still undefined and the service crashes on first load. Create_nextPage$eagerly, then letrequestNextPage()only push into it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/view/case-view/service/case-view-service.ts` around lines 373 - 390, Initialize `_nextPage$` before any dynamic-sort handling in `requestPageWithDynamicSort`/`requestNextPage`, because the first process-backed load can reach the `_nextPage$.pipe(...)` subscription before `requestNextPage()` runs and `_nextPage$` is still undefined. Make `_nextPage$` eagerly available when the service starts, then change `requestNextPage(page: PageLoadRequestContext)` to only emit into the existing subject instead of conditionally creating it, and keep `requestPageWithDynamicSort` focused on choosing between the dynamic-sort path and the normal `requestNextPage` call.projects/netgrif-components-core/src/lib/view/task-view/service/task-view.service.ts (1)
369-386: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winInitialize
_requestedPage$before subscribing to dynamic sortWhen the injected default sort comes from
processService.getNet(...), it can emit after the constructor reachesthis._requestedPage$.pipe(...), leaving_requestedPage$undefined and crashing initialization. Create_requestedPage$eagerly, then letrequestNextPage()only.next().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/view/task-view/service/task-view.service.ts` around lines 369 - 386, Initialize _requestedPage$ eagerly in task-view.service.ts before requestPageWithDynamicSort can subscribe to _dynamicDefaultSort$, because the current lazy creation in requestNextPage can leave the stream undefined when processService.getNet(...) emits late and the constructor already wires _requestedPage$.pipe(...). Update requestNextPage() to only emit with .next(), and ensure the constructor or initialization path creates the BehaviorSubject up front so requestPageWithDynamicSort and requestNextPage always operate on an existing subject.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@projects/netgrif-components-core/src/lib/view/task-view/service/task-view.service.ts`:
- Line 371: The subscription created in task-view.service’s
nextPage/nextPagePagination flow on _dynamicDefaultSort$ is never torn down, so
repeated calls can leak subscriptions. Update the _dynamicDefaultSort$.subscribe
usage to complete after the first emission with pipe(take(1)), or store the
Subscription returned by subscribe and clean it up in ngOnDestroy alongside the
existing service teardown logic.
---
Outside diff comments:
In
`@projects/netgrif-components-core/src/lib/view/case-view/service/case-view-service.ts`:
- Around line 373-390: Initialize `_nextPage$` before any dynamic-sort handling
in `requestPageWithDynamicSort`/`requestNextPage`, because the first
process-backed load can reach the `_nextPage$.pipe(...)` subscription before
`requestNextPage()` runs and `_nextPage$` is still undefined. Make `_nextPage$`
eagerly available when the service starts, then change `requestNextPage(page:
PageLoadRequestContext)` to only emit into the existing subject instead of
conditionally creating it, and keep `requestPageWithDynamicSort` focused on
choosing between the dynamic-sort path and the normal `requestNextPage` call.
In
`@projects/netgrif-components-core/src/lib/view/task-view/service/task-view.service.ts`:
- Around line 369-386: Initialize _requestedPage$ eagerly in
task-view.service.ts before requestPageWithDynamicSort can subscribe to
_dynamicDefaultSort$, because the current lazy creation in requestNextPage can
leave the stream undefined when processService.getNet(...) emits late and the
constructor already wires _requestedPage$.pipe(...). Update requestNextPage() to
only emit with .next(), and ensure the constructor or initialization path
creates the BehaviorSubject up front so requestPageWithDynamicSort and
requestNextPage always operate on an existing subject.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8277a00a-8d67-4c6c-8fc7-ad794b9680a6
📒 Files selected for processing (4)
projects/netgrif-components-core/src/lib/dialog/models/task-view-dialog-injection-data.tsprojects/netgrif-components-core/src/lib/view/case-view/service/case-view-service.tsprojects/netgrif-components-core/src/lib/view/task-view/service/task-view.service.tsprojects/netgrif-components/src/lib/dialog/dialog.module.ts
💤 Files with no reviewable changes (1)
- projects/netgrif-components/src/lib/dialog/dialog.module.ts
Contributor
Author
|
@coderabbitai is there anything why you request changes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 340 --repo netgrif/components --json reviews,reviewDecision,comments -q '.reviewDecision'
echo "---REVIEWS---"
gh pr view 340 --repo netgrif/components --json reviews -q '.reviews[] | {author: .author.login, state: .state, body: .body}'
echo "---REVIEW COMMENTS---"
gh api repos/netgrif/components/pulls/340/comments --jq '.[] | {user: .user.login, path: .path, line: .line, body: .body}'Length of output: 104321
However, a few items still look outstanding without a confirmed fix or reply: From Kovy95's review (likely the main driver of the "Changes requested" state, since it hasn't been dismissed):
From my own review, still open (lower priority):
None of these are unrecoverable blockers, and several are trivial/nitpick-level, but they explain why the review status still shows "changes requested." Let me know if you'd like me to help draft fixes for any of these, or if some should be deferred to a follow-up PR. [resolve] |
- remove unused injector
|
There was a problem hiding this comment.
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/abstract-navigation-double-drawer.ts (3)
356-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated dialog-open boilerplate between
editMenuItemandcreateMenuItem.Both methods build a near-identical
this._dialog.open(this._taskView, {panelClass: "dialog-task-responsive", data: {...} as TaskViewDialogInjectionData})call, differing only inautoCloseOnEventandsearchBody. Extracting a small private helper would keep the panel class/typing in one place and reduce drift risk as this dialog flow evolves (e.g. addingafterClosed()handling later).♻️ Proposed helper extraction
+ private _openMenuItemTaskDialog(searchBody: TaskViewDialogInjectionData['searchBody'], autoCloseOnEvent: boolean) { + this._dialog.open(this._taskView, { + panelClass: "dialog-task-responsive", + data: {autoCloseOnEvent, searchBody} as TaskViewDialogInjectionData, + }); + } + public editMenuItem(menuItemCase: Case) { - this._dialog.open(this._taskView, { - panelClass: "dialog-task-responsive", - data: { - autoCloseOnEvent: false, - searchBody: { - transitionId: ["item_settings", "move_item", "duplicate_item", "children_order"], - case: { id: menuItemCase.stringId } - } - } as TaskViewDialogInjectionData, - }); + this._openMenuItemTaskDialog({ + transitionId: ["item_settings", "move_item", "duplicate_item", "children_order"], + case: { id: menuItemCase.stringId } + }, false); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts` around lines 356 - 402, Both `editMenuItem` and the success path in `createMenuItem` duplicate the same `_dialog.open(this._taskView, ...)` boilerplate, which should be extracted into a private helper to keep the `panelClass` and `TaskViewDialogInjectionData` wiring in one place. Add a small helper on `AbstractNavigationDoubleDrawer` that accepts the differing `autoCloseOnEvent` and `searchBody` inputs, then have `editMenuItem` and `createMenuItem` call it instead of constructing the dialog config inline.
369-402: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
createMenuItem()untilcurrentNodeis available.UriService.activeNodestarts asundefineduntil the root request resolves, but the edit-mode action is exposed without a root/current-node guard, so clicking it early can throw onthis.currentNode.uriPathbefore the existing error handling runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts` around lines 369 - 402, Guard createMenuItem() in AbstractNavigationDoubleDrawer until currentNode is set, since currentNode.uriPath is used before the root node resolves and can throw early. Add a defensive check or disable the action until currentNode is available, then only call _caseResourceService.createCase after confirming currentNode exists and use its uriPath safely; keep the existing error handling in createMenuItem() for service failures.
144-144: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
rightItemsSubinngOnDestroy().
rightItemsSubis only assigned whencanApplyAutoSelect()passes, so thisunsubscribe()can still throw when that path never runs. Use?.unsubscribe()or initialize it up front.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts` at line 144, The cleanup in ngOnDestroy for AbstractNavigationDoubleDrawer should safely handle rightItemsSub being unset when canApplyAutoSelect() never ran. Update the ngOnDestroy logic to guard the rightItemsSub unsubscribe call with optional chaining or initialize rightItemsSub upfront so the teardown path cannot throw when the subscription was never created.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts`:
- Around line 356-402: Both `editMenuItem` and the success path in
`createMenuItem` duplicate the same `_dialog.open(this._taskView, ...)`
boilerplate, which should be extracted into a private helper to keep the
`panelClass` and `TaskViewDialogInjectionData` wiring in one place. Add a small
helper on `AbstractNavigationDoubleDrawer` that accepts the differing
`autoCloseOnEvent` and `searchBody` inputs, then have `editMenuItem` and
`createMenuItem` call it instead of constructing the dialog config inline.
- Around line 369-402: Guard createMenuItem() in AbstractNavigationDoubleDrawer
until currentNode is set, since currentNode.uriPath is used before the root node
resolves and can throw early. Add a defensive check or disable the action until
currentNode is available, then only call _caseResourceService.createCase after
confirming currentNode exists and use its uriPath safely; keep the existing
error handling in createMenuItem() for service failures.
- Line 144: The cleanup in ngOnDestroy for AbstractNavigationDoubleDrawer should
safely handle rightItemsSub being unset when canApplyAutoSelect() never ran.
Update the ngOnDestroy logic to guard the rightItemsSub unsubscribe call with
optional chaining or initialize rightItemsSub upfront so the teardown path
cannot throw when the subscription was never created.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3ef5451-383a-4cd1-90ef-2d186ca31d0b
📒 Files selected for processing (4)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.spec.tsprojects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.tsprojects/netgrif-components/src/lib/navigation/navigation-double-drawer/navigation-double-drawer.component.spec.tsprojects/netgrif-components/src/lib/navigation/navigation-double-drawer/navigation-double-drawer.component.ts
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
2 participants
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.
Description
New features
Improvements
Fixes
Implements NAE-2454
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?
Manually and by unit tests
Test Configuration
Checklist:
Summary by CodeRabbit