[NAE-2202] Post test fixes #298
Conversation
- Implemented the `resolveI18n` function and `I18nString` interface to support resolving i18n strings. - Updated multiple components, including `new-case-dialog.component.ts`, `abstract-new-case.component.ts`, and `abstract-header-service.ts`, to use `resolveI18n` for titles. - Adjusted the `ProcessList`, `case-process`, and `workflow-panel` components to handle i18n titles via `resolveI18n`. - Modified `PetriNetReference` and `Net` models to support `I18nString` for titles. - Updated `nae.json` to reflect a new API address (`http://localhost:8800/api/`). This change improves i18n support by enabling dynamic translations and prepares the codebase for better localization support.
WalkthroughEdits adjust workflow title fallback, reset per-emission option uniqueness in case process filtering, refine dashboard internal navigation path resolution, normalize item routing paths, auto-open available views after right-side load, and change active-path handling to trigger openAvailableView when the path doesn't change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Dashboard as AbstractDashboardComponent
participant NavSvc as DoubleDrawerNavigationService
participant Router
User->>Dashboard: Click internal navigation item (Case)
Dashboard->>Dashboard: Lookup menuItemCase by stringId
alt menuItemCase found
Dashboard->>NavSvc: getItemRoutingPath(menuItemCase)
NavSvc-->>Dashboard: "/{normalizedRoute}"
Dashboard->>Dashboard: activePath <- field ITEM_FIELD_ID_NODE_PATH (nodePath overrides if present)
Dashboard->>Router: navigate([itemRoute])
else not found
Dashboard->>Dashboard: log warning and return
end
sequenceDiagram
autonumber
participant NavSvc as DoubleDrawerNavigationService
participant RightPane as Right-side Items Stream
participant Drawer as AbstractNavigationDoubleDrawer
RightPane->>NavSvc: emit(items)
NavSvc->>NavSvc: openAvailableView()
Note over NavSvc: Auto-route to available view if present
RightPane-->>Drawer: activePath updates
alt new path differs
Drawer->>Drawer: update currentPath
else same path
Drawer->>Drawer: openAvailableView()
Note over Drawer: Fallback when no path change
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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). (13)
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: 11
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/admin/role-assignment/services/ProcessList.ts (2)
62-66: Method promises an Observable but returns undefined when loadingThis can cause runtime errors for consumers expecting an Observable.
public loadProcesses(): Observable<Array<ProcessListItem>> { if (this.loading) { - return; + return of(this._processes); }
163-165: Log the resolved title to avoid “[object Object]”When title is an I18nString, the current log prints an object.
- this._log.error('Failed to load roles for Petri net [' + net.stringId + '] ' + net.title, err); + this._log.error('Failed to load roles for Petri net [' + net.stringId + '] ' + resolveI18n(net.title), err);projects/netgrif-components-core/src/lib/process/net.ts (1)
98-104: Setter type is narrower than the field/getterGetter and backing field allow string | I18nString, but setter only accepts string. Broaden setter to keep the API consistent.
- set title(value: string) { + set title(value: string | I18nString) { this._title = value; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
projects/netgrif-components-core/src/lib/admin/role-assignment/services/ProcessList.ts(2 hunks)projects/netgrif-components-core/src/lib/header/abstract-header-service.ts(3 hunks)projects/netgrif-components-core/src/lib/panel/workflow-panel/abstract-workflow-panel.component.ts(3 hunks)projects/netgrif-components-core/src/lib/process/net.ts(3 hunks)projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts(1 hunks)projects/netgrif-components-core/src/lib/resources/interface/petri-net-reference.ts(2 hunks)projects/netgrif-components-core/src/lib/resources/public-api.ts(1 hunks)projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts(2 hunks)projects/netgrif-components-core/src/lib/side-menu/content-components/new-case/abstract-new-case.component.ts(2 hunks)projects/netgrif-components/src/lib/dialog/new-case-dialog/new-case-dialog.component.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
projects/netgrif-components-core/src/lib/header/abstract-header-service.ts (1)
projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts (1)
resolveI18n(1-3)
projects/netgrif-components-core/src/lib/resources/interface/petri-net-reference.ts (1)
projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts (1)
I18nString(5-9)
projects/netgrif-components/src/lib/dialog/new-case-dialog/new-case-dialog.component.ts (1)
projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts (1)
resolveI18n(1-3)
projects/netgrif-components-core/src/lib/admin/role-assignment/services/ProcessList.ts (1)
projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts (1)
resolveI18n(1-3)
projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts (1)
projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts (1)
resolveI18n(1-3)
projects/netgrif-components-core/src/lib/panel/workflow-panel/abstract-workflow-panel.component.ts (1)
projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts (1)
resolveI18n(1-3)
projects/netgrif-components-core/src/lib/process/net.ts (1)
projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts (1)
I18nString(5-9)
projects/netgrif-components-core/src/lib/side-menu/content-components/new-case/abstract-new-case.component.ts (1)
projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts (1)
resolveI18n(1-3)
⏰ 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). (8)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Matrix Test (20)
- GitHub Check: Matrix Test (24)
- GitHub Check: Test with SonarCloud
- GitHub Check: Matrix Test (22)
- GitHub Check: task-list-completed
🔇 Additional comments (6)
projects/netgrif-components-core/src/lib/side-menu/content-components/new-case/abstract-new-case.component.ts (1)
78-82: Using resolveI18n for net option labels — LGTM.This safely normalizes string | I18nString to string for the UI.
projects/netgrif-components-core/src/lib/header/abstract-header-service.ts (2)
20-20: Imported resolveI18n — LGTM.Keeps this service independent of i18n providers while handling union types cleanly.
206-207: Resolve group titles to plain strings — LGTM.Correctly handles string | I18nString for section headers.
Also applies to: 231-232
projects/netgrif-components/src/lib/dialog/new-case-dialog/new-case-dialog.component.ts (2)
85-89: Use of resolveI18n for dialog options — LGTM.Matches the side-menu component and keeps behavior consistent.
10-12: resolveI18n is properly re-exported from the public API
The fileprojects/netgrif-components-core/src/lib/resources/public-api.tscontainsexport * from './interface/i18n-string';which includes the
resolveI18nfunction. No further changes needed.projects/netgrif-components-core/src/lib/admin/role-assignment/services/ProcessList.ts (1)
84-88: Using resolveI18n for display title is correctThis ensures ProcessListItem.title is always a string for templates and sorting.
...ts/netgrif-components-core/src/lib/panel/workflow-panel/abstract-workflow-panel.component.ts
Outdated
Show resolved
Hide resolved
| title: new TextField('', this._translate.instant(this.TRANSLATION_TITLE), | ||
| this.workflow.title, this.dataFieldsBehaviour), | ||
| resolveI18n(this.workflow.title), this.dataFieldsBehaviour), | ||
| version: new TextField('', this._translate.instant(this.TRANSLATION_VERSION), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide a safe fallback when title resolves to empty
If defaultValue is empty, display identifier to avoid blank UI.
- title: new TextField('', this._translate.instant(this.TRANSLATION_TITLE),
- resolveI18n(this.workflow.title), this.dataFieldsBehaviour),
+ title: new TextField('', this._translate.instant(this.TRANSLATION_TITLE),
+ resolveI18n(this.workflow.title) || this.workflow.identifier, this.dataFieldsBehaviour),📝 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.
| title: new TextField('', this._translate.instant(this.TRANSLATION_TITLE), | |
| this.workflow.title, this.dataFieldsBehaviour), | |
| resolveI18n(this.workflow.title), this.dataFieldsBehaviour), | |
| version: new TextField('', this._translate.instant(this.TRANSLATION_VERSION), | |
| title: new TextField('', this._translate.instant(this.TRANSLATION_TITLE), | |
| resolveI18n(this.workflow.title) || this.workflow.identifier, this.dataFieldsBehaviour), | |
| version: new TextField('', this._translate.instant(this.TRANSLATION_VERSION), |
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/panel/workflow-panel/abstract-workflow-panel.component.ts
around lines 137–139, the TextField for title can end up with an empty
defaultValue when resolveI18n(this.workflow.title) returns an empty string;
change the code to compute a safe title before creating the TextField (e.g.
const titleValue = resolveI18n(this.workflow.title) || this.workflow.identifier)
and pass titleValue as the defaultValue so the workflow identifier is shown when
the resolved title is empty.
...ts/netgrif-components-core/src/lib/panel/workflow-panel/abstract-workflow-panel.component.ts
Outdated
Show resolved
Hide resolved
| export function resolveI18n(value: string | I18nString): string { | ||
| return typeof value === 'string' ? value : value.defaultValue; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider locale-aware resolution with graceful fallbacks (non-breaking).
Right now translations are ignored. You can keep current call sites working while enabling per-locale resolution and a fallback locale.
-export function resolveI18n(value: string | I18nString): string {
- return typeof value === 'string' ? value : value.defaultValue;
-}
+export function resolveI18n(
+ value: string | I18nString,
+ locale?: string,
+ fallbackLocale?: string
+): string {
+ if (typeof value === 'string') {
+ return value;
+ }
+ if (locale && value.translations && value.translations[locale]) {
+ return value.translations[locale];
+ }
+ if (fallbackLocale && value.translations && value.translations[fallbackLocale]) {
+ return value.translations[fallbackLocale];
+ }
+ return value.defaultValue;
+}📝 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.
| export function resolveI18n(value: string | I18nString): string { | |
| return typeof value === 'string' ? value : value.defaultValue; | |
| } | |
| export function resolveI18n( | |
| value: string | I18nString, | |
| locale?: string, | |
| fallbackLocale?: string | |
| ): string { | |
| if (typeof value === 'string') { | |
| return value; | |
| } | |
| if (locale && value.translations && value.translations[locale]) { | |
| return value.translations[locale]; | |
| } | |
| if (fallbackLocale && value.translations && value.translations[fallbackLocale]) { | |
| return value.translations[fallbackLocale]; | |
| } | |
| return value.defaultValue; | |
| } |
| * Title | ||
| */ | ||
| title: string; | ||
| title: string | I18nString; | ||
| /** |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document the union type intent.
Clarify in the JSDoc that title may be a raw string or I18nString and that UI code should use resolveI18n.
/**
- * Title
+ * Title
+ * May be a raw string or an I18nString. Use `resolveI18n(title)` when rendering.
*/
- title: string | I18nString;
+ title: string | I18nString;📝 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.
| * Title | |
| */ | |
| title: string; | |
| title: string | I18nString; | |
| /** | |
| /** | |
| * Title | |
| * May be a raw string or an I18nString. Use `resolveI18n(title)` when rendering. | |
| */ | |
| title: string | I18nString; |
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/resources/interface/petri-net-reference.ts
around lines 15 to 18, the JSDoc for the title property should explain the union
type intent: update the comment to state that title can be either a raw string
or an I18nString and instruct UI code to call resolveI18n(title) to obtain the
localized string; keep the existing type declaration (string | I18nString) but
expand the JSDoc to clarify usage and expected behavior.
| * Title | ||
| */ | ||
| title: string; | ||
| title: string | I18nString; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type widening to support i18n — good; verify all call sites resolve to string at render time.
Anywhere PetriNetReference.title is rendered should pass through resolveI18n(...).
Run the script to locate remaining direct .title usages and review whether they need resolveI18n:
🏁 Script executed:
#!/bin/bash
# 1) Find all TS uses of `.title`
rg -nP --type=ts -C2 '\.title\b' \
-g '!**/node_modules/**' -g '!**/dist/**'
# 2) List where resolveI18n is already used (for cross-check)
rg -nP --type=ts 'resolveI18n\s*\(' \
-g '!**/node_modules/**' -g '!**/dist/**'Length of output: 83516
🏁 Script executed:
#!/bin/bash
# Locate direct accesses of PetriNetReference.title
rg -nP --type=ts "petriNet\\.title" -g '!**/node_modules/**' -g '!**/dist/**' -n -C2
# Locate general net.title usage (e.g., in ProcessService)
rg -nP --type=ts "net\\.title" -g '!**/node_modules/**' -g '!**/dist/**' -n -C2Length of output: 7542
Wrap all direct PetriNetReference.title usages in resolveI18n
-
projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts (line 47): change
this.addToMap(petriNet.title, …)→this.addToMap(resolveI18n(petriNet.title), …) -
projects/netgrif-components-core/src/lib/search/models/category/task/task-process.ts (line 43): change
this.addToMap(petriNet.title, …)→this.addToMap(resolveI18n(petriNet.title), …) -
projects/netgrif-components-core/src/lib/view/tree-case-view/tree-component/case-tree.service.ts (line 486): change
options: nets.map(net => ({ text: net.title, … }))→text: resolveI18n(net.title)
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/resources/interface/petri-net-reference.ts
around line 17, PetriNetReference.title is typed as string | I18nString and the
review points out direct usages must be resolved; update the three call sites to
call resolveI18n on the title before passing it: in
projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts
(around line 47) change this.addToMap(petriNet.title, …) to
this.addToMap(resolveI18n(petriNet.title), …); in
projects/netgrif-components-core/src/lib/search/models/category/task/task-process.ts
(around line 43) change this.addToMap(petriNet.title, …) to
this.addToMap(resolveI18n(petriNet.title), …); and in
projects/netgrif-components-core/src/lib/view/tree-case-view/tree-component/case-tree.service.ts
(around line 486) change options: nets.map(net => ({ text: net.title, … })) to
use text: resolveI18n(net.title, …). Ensure you import resolveI18n from its
module if not already imported and adjust any types accordingly.
projects/netgrif-components-core/src/lib/resources/public-api.ts
Outdated
Show resolved
Hide resolved
projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts
Show resolved
Hide resolved
projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
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/process/net.ts (1)
98-104: Getter/setter type mismatch breaks compilation.Setter must match getter’s union type; otherwise TS errors. Update setter to accept string | I18nString.
Apply this diff:
- set title(value: string) { + set title(value: string | I18nString) { this._title = value; }projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts (2)
62-73: Tighten Map access to satisfy strict null checks and reduce lookupsCache the Set from get() to avoid repeated lookups and undefined unions under strict mode.
- protected isUniqueOption(key: string, value: string): boolean { - if (!this._uniqueOptionsMap.has(key)) { - this._uniqueOptionsMap.set(key, new Set<string>([value])); - return true; - } - if (this._uniqueOptionsMap.get(key).has(value)) { - return false; - } else { - this._uniqueOptionsMap.get(key).add(value); - return true; - } - } + protected isUniqueOption(key: string, value: string): boolean { + const existing = this._uniqueOptionsMap.get(key); + if (!existing) { + this._uniqueOptionsMap.set(key, new Set<string>([value])); + return true; + } + if (existing.has(value)) { + return false; + } + existing.add(value); + return true; + }
20-21: Initialize _destroyed to a safe defaultPrevents strictPropertyInitialization complaints and clarifies default lifecycle state.
- private _destroyed: boolean; + private _destroyed: boolean = false;
♻️ Duplicate comments (3)
projects/netgrif-components-core/src/lib/resources/public-api.ts (1)
48-48: Semicolon added as previously requested — resolved.projects/netgrif-components-core/src/lib/panel/workflow-panel/abstract-workflow-panel.component.ts (1)
137-139: Provide a non-empty fallback for title.Mirror header behavior to prevent blank UI when title resolves to empty.
Apply this diff:
- title: new TextField('', this._translate.instant(this.TRANSLATION_TITLE), - resolveI18n(this.workflow.title), this.dataFieldsBehaviour), + title: new TextField('', this._translate.instant(this.TRANSLATION_TITLE), + resolveI18n(this.workflow.title) || this.workflow.identifier, this.dataFieldsBehaviour),projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts (1)
46-46: Resolved: reset uniqueness map per emissionClearing _uniqueOptionsMap alongside _optionsMap fixes the stale-state suppression bug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
projects/netgrif-components-core/src/lib/navigation/dashboard/abstract-dashboard.component.ts(2 hunks)projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts(2 hunks)projects/netgrif-components-core/src/lib/panel/workflow-panel/abstract-workflow-panel.component.ts(4 hunks)projects/netgrif-components-core/src/lib/process/net.ts(3 hunks)projects/netgrif-components-core/src/lib/resources/public-api.ts(1 hunks)projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
projects/netgrif-components-core/src/lib/process/net.ts (1)
projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts (1)
I18nString(5-9)
projects/netgrif-components-core/src/lib/panel/workflow-panel/abstract-workflow-panel.component.ts (1)
projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts (1)
resolveI18n(1-3)
projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts (1)
projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts (1)
resolveI18n(1-3)
⏰ 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). (12)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Matrix Test (22)
- GitHub Check: Matrix Test (24)
- GitHub Check: Matrix Test (20)
- GitHub Check: Test with SonarCloud
- GitHub Check: task-list-completed
🔇 Additional comments (6)
projects/netgrif-components-core/src/lib/process/net.ts (1)
9-9: LGTM on import and quote style.projects/netgrif-components-core/src/lib/panel/workflow-panel/abstract-workflow-panel.component.ts (3)
20-20: LGTM: centralized resolver import.
164-165: LGTM: header title now mirrors fallback.
172-176: LGTM: explicit formatting for creation date meta.projects/netgrif-components-core/src/lib/navigation/dashboard/abstract-dashboard.component.ts (1)
16-16: LGTM: import added for constants.projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts (1)
12-12: Good: central i18n resolution at the source of truthImporting and using resolveI18n here is the right call; avoids leaking I18nString downstream.
projects/netgrif-components-core/src/lib/navigation/dashboard/abstract-dashboard.component.ts
Show resolved
Hide resolved
...core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts
Show resolved
Hide resolved
| this._allowedNetsSub = this._optionalDependencies.allowedNetsService.allowedNets$.subscribe(allowedNets => { | ||
| this._optionsMap.clear(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Guard against duplicate subscriptions
If createOptions can be called multiple times, unsubscribe before resubscribing to avoid leaks/duplicate updates.
- this._allowedNetsSub = this._optionalDependencies.allowedNetsService.allowedNets$.subscribe(allowedNets => {
+ if (this._allowedNetsSub && !this._allowedNetsSub.closed) {
+ this._allowedNetsSub.unsubscribe();
+ }
+ this._allowedNetsSub = this._optionalDependencies.allowedNetsService.allowedNets$.subscribe(allowedNets => {📝 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._allowedNetsSub = this._optionalDependencies.allowedNetsService.allowedNets$.subscribe(allowedNets => { | |
| this._optionsMap.clear(); | |
| // Unsubscribe existing subscription to avoid duplicate handlers/leaks | |
| if (this._allowedNetsSub && !this._allowedNetsSub.closed) { | |
| this._allowedNetsSub.unsubscribe(); | |
| } | |
| this._allowedNetsSub = this._optionalDependencies.allowedNetsService.allowedNets$.subscribe(allowedNets => { | |
| this._optionsMap.clear(); | |
| // …rest of your handling logic… | |
| }); |
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts
around lines 44-45, the subscription to allowedNets$ is created unconditionally
which can cause duplicate subscriptions/leaks if createOptions is called
multiple times; before assigning this._allowedNetsSub, check if an existing
this._allowedNetsSub exists and unsubscribe it (or call
this._allowedNetsSub?.unsubscribe()), then create the new subscription so only
one active subscription remains; alternatively implement a takeUntil/subject
teardown pattern if preferred.
projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts
Outdated
Show resolved
Hide resolved
Changed the authentication type from BasicWithRealm to Basic in the auth provider configuration. This simplifies the authentication setup and ensures compatibility with the current API.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nae.json(1 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). (7)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Test with SonarCloud
- GitHub Check: Matrix Test (22)
- GitHub Check: Matrix Test (24)
- GitHub Check: Matrix Test (20)
- Added a call to `openAvailableView` in `resolveCustomViewsInRightSide` to ensure the appropriate view is opened after resolving custom views. - Left a comment in `canAccessView` method to review the routing path logic for potential issues.
There was a problem hiding this comment.
Actionable comments posted: 2
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)
306-309: Fix wrong redirect target and add null-guard.
- Fallback branch redirects using
autoOpenItems[0]instead ofitemsWithView[0].- Guard
_currentNavigationItembeforehasItemView(...).- if (DoubleDrawerUtils.hasItemView(this._currentNavigationItem)) { + if (this._currentNavigationItem && DoubleDrawerUtils.hasItemView(this._currentNavigationItem)) { // is routed by routerLink on item click return; } @@ - if (itemsWithView.length > 0) { - this._redirectService.redirect(autoOpenItems[0].routing.path); - } + if (itemsWithView.length > 0) { + this._redirectService.redirect(itemsWithView[0].routing.path); + }Also applies to: 301-304
♻️ Duplicate comments (1)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts (1)
547-552: Normalize route join; avoid '//' and '/undefined'.Prevents double slashes and handles missing
taskId. Mirrors a prior review.- const url = this._dynamicRoutingService.route; - const prefix = url.startsWith('/') ? '' : '/'; - return `${prefix}${url}/${taskId}`; + const url = this._dynamicRoutingService.route; + const base = url.startsWith('/') ? url : `/${url}`; + const normalized = base.replace(/\/+$/, ''); + return taskId ? `${normalized}/${taskId}` : normalized;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts(4 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). (6)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Matrix Test (22)
- GitHub Check: Matrix Test (20)
- GitHub Check: Matrix Test (24)
- GitHub Check: Test with SonarCloud
🔇 Additional comments (1)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts (1)
97-97: Formatting-only constructor change — OK.No functional impact.
|
|
||
| this.openAvailableView(); | ||
| }, error => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid double auto-open triggers.
openAvailableView() is called here and also via the subscription in onItemClick. This can cause duplicate navigations.
Minimal change: rely on the post-load call and remove the subscription.
- this._rightLoading$.pipe(
- filter(isRightLoading => isRightLoading === false),
- take(1)
- ).subscribe(() => {
- this.openAvailableView();
- })
+ // Auto-open is handled in loadRightSide() after items finish loading.Run to find other call sites to confirm no remaining duplicates:
Also applies to: 269-275
🏁 Script executed:
#!/bin/bash
rg -n "openAvailableView\\(" -C2Length of output: 3958
🏁 Script executed:
#!/bin/bash
sed -n '250,340p' projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.tsLength of output: 4357
🏁 Script executed:
#!/bin/bash
sed -n '120,160p' projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.tsLength of output: 1550
🏁 Script executed:
#!/bin/bash
sed -n '400,480p' projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.tsLength of output: 3484
🏁 Script executed:
#!/bin/bash
rg -n "protected loadLeftSide" -C3 projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.tsLength of output: 237
🏁 Script executed:
#!/bin/bash
sed -n '1,120p' projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.tsLength of output: 5236
Remove duplicate auto-open in onItemClick
The _rightLoading$.pipe(...).subscribe(() => this.openAvailableView()) block in onItemClick fires in addition to the openAvailableView() call at the end of loadRightSide(), causing double navigations. Delete that subscription and rely on the post-load auto-open.
- this._rightLoading$.pipe(
- filter(isRightLoading => isRightLoading === false),
- take(1)
- ).subscribe(() => {
- this.openAvailableView();
- })
+ // auto-open is handled in loadRightSide() after items finish loading📝 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.openAvailableView(); | |
| }, error => { | |
| // auto-open is handled in loadRightSide() after items finish loading | |
| this.openAvailableView(); | |
| }, error => { |
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts
around lines 442-444, there is a redundant subscription inside onItemClick:
remove the _rightLoading$.pipe(...).subscribe(() => this.openAvailableView())
block so it no longer calls openAvailableView() from this subscription; rely on
the existing openAvailableView() invocation that occurs after loadRightSide()
completes to perform the post-load auto-open, and ensure no other code path
reintroduces an immediate open to avoid double navigation.
...core/src/lib/navigation/navigation-double-drawer/service/double-drawer-navigation.service.ts
Outdated
Show resolved
Hide resolved
- fix the problem with opening view in double drawer when its clicked from dashboard
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts (2)
110-116: Unsubscribe the constructor subscriptions to prevent leaksitemClicked$ and itemLoaded$ subscriptions are created in the constructor and never unsubscribed. These live beyond component lifecycle if the service outlives the component.
Apply this diff to track and clean them up:
- this._navigationService.itemClicked$.subscribe((itemClickEvent: MenuItemClickEvent) => { + this._itemClickedSubscription = this._navigationService.itemClicked$.subscribe((itemClickEvent: MenuItemClickEvent) => { this.itemClicked.emit(itemClickEvent); }) - this._navigationService.itemLoaded$.subscribe((itemLoadedEvent: MenuItemLoadedEvent) => { + this._itemLoadedSubscription = this._navigationService.itemLoaded$.subscribe((itemLoadedEvent: MenuItemLoadedEvent) => { this.itemLoaded.emit(itemLoadedEvent); })Add field declarations near other subscriptions:
protected _itemClickedSubscription: Subscription; protected _itemLoadedSubscription: Subscription;And dispose them in ngOnDestroy:
public ngOnDestroy(): void { this._breakpointSubscription?.unsubscribe(); this.loggedOut.complete(); this.stateChanged.complete(); this.itemClicked.complete(); this.resized.complete(); this.itemLoaded.complete(); this._currentNodeSubscription?.unsubscribe(); this._currentPathSubscription?.unsubscribe(); this._loggedUserSubscription?.unsubscribe(); + this._itemClickedSubscription?.unsubscribe(); + this._itemLoadedSubscription?.unsubscribe(); }Also applies to: 158-168
372-374: Normalize URL comparison in canApplyAutoSelect()Direct string equality is brittle with trailing slashes or query/hash changes. Normalize or use Router APIs to avoid false negatives.
Apply this minimal change:
- protected canApplyAutoSelect(): boolean { - return this.configUrl === this._router.url; - } + protected canApplyAutoSelect(): boolean { + return this._normalizeUrl(this.configUrl) === this._normalizeUrl(this._router.url); + }Add helper to the class:
private _normalizeUrl(url?: string): string { if (!url) { return ''; } // strip hash/query and trailing slashes const base = url.split('#')[0].split('?')[0]; return base.replace(/\/+$/, ''); }Please verify this aligns with the “normalized routing slash handling” introduced elsewhere in the PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts(1 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). (6)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Matrix Test (24)
- GitHub Check: Test with SonarCloud
- GitHub Check: Matrix Test (22)
- GitHub Check: Matrix Test (20)
| if (path !== this.currentPath) { | ||
| this.currentPath = path; | ||
| } else { | ||
| this.openAvailableView(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Scope and de-duplicate auto-open to avoid surprise navigation and loops
Else-branch now calls openAvailableView() whenever activePath$ re-emits the same value. This bypasses the earlier canApplyAutoSelect() guard (used below on rightItems$) and can:
- Trigger auto-navigation on routes where it wasn’t intended.
- Repeatedly call openAvailableView() if the same path re-emits (possible re-entrancy, flicker).
Guard by (a) scoping to canApplyAutoSelect(), (b) ensuring right items are available, and (c) making it one-shot per path.
Apply this diff within the subscription:
- if (path !== this.currentPath) {
- this.currentPath = path;
- } else {
- this.openAvailableView();
- }
+ if (path !== this.currentPath) {
+ this.currentPath = path;
+ this._autoOpenForPath = undefined; // reset one-shot flag
+ } else if (this.canApplyAutoSelect()
+ && ((this.rightItems?.length ?? 0) > 0)
+ && this._autoOpenForPath !== path) {
+ this._autoOpenForPath = path;
+ this.openAvailableView();
+ }Add this helper field to the class (near other protected fields):
private _autoOpenForPath?: string;Verification suggestions:
- Confirm openAvailableView() is idempotent when invoked multiple times for the same path.
- Validate that auto-open only occurs on the configured URL and only once per path.
Do you want me to add a focused unit test that asserts a single openAvailableView() invocation when activePath$ re-emits the same value?
Also applies to: 136-143
🏁 Script executed:
#!/bin/bash
set -e
# Locate openAvailableView definition
rg -n 'openAvailableView' -C3 -g '*.ts'
# Locate canApplyAutoSelect definition and usage
rg -n 'canApplyAutoSelect' -C3 -g '*.ts'
# Inspect subscription logic around currentPath
rg -n -C3 'if \(path !== this\.currentPath\)' projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts
# Inspect use of rightItems
rg -n 'rightItems' -C3 projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.tsLength of output: 8795
🏁 Script executed:
#!/bin/bash
# Show class declaration and field definitions in abstract-navigation-double-drawer.ts
sed -n '1,200p' projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts
# Locate any plain `rightItems` property declarations in this file
grep -R "rightItems\s*[:=]" -n projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.tsLength of output: 8392
Guard and de-duplicate auto-open in AbstractNavigationDoubleDrawerComponent
Add a one-shot flag and apply the guard in the activePath$ subscription to prevent unintended loops and bypassing of canApplyAutoSelect():
-
Near the other protected fields, add:
private _autoOpenForPath?: string;
-
In the
activePath$subscription (lines 129–133), replace:- if (path !== this.currentPath) { - this.currentPath = path; - } else { - this.openAvailableView(); - }
with:
if (path !== this.currentPath) { this.currentPath = path; this._autoOpenForPath = undefined; } else if ( this.canApplyAutoSelect() && ((this.rightItems?.length ?? 0) > 0) && this._autoOpenForPath !== path ) { this._autoOpenForPath = path; this.openAvailableView(); }
-
Apply a similar one-shot guard to the
rightItems$subscription (lines 136–143) if auto-open should only ever fire once per path.
Would you like a focused unit test that asserts openAvailableView() is invoked exactly once when activePath$ re-emits the same value?
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/navigation/navigation-double-drawer/abstract-navigation-double-drawer.ts
around lines 129 to 133, the activePath$ subscription can trigger
openAvailableView repeatedly and bypass canApplyAutoSelect(); add a one-shot
guard field (private _autoOpenForPath?: string) near the other protected fields,
then update the activePath$ handler so that when path !== this.currentPath you
set this.currentPath = path and clear _autoOpenForPath, else only call
openAvailableView if canApplyAutoSelect() is true, there are rightItems, and
_autoOpenForPath !== path, and before calling openAvailableView set
_autoOpenForPath = path; also consider applying the same one-shot guard to the
rightItems$ subscription so auto-open fires at most once per path.
projects/netgrif-components-core/src/lib/resources/interface/i18n-string.ts
Outdated
Show resolved
Hide resolved
- fix the problem with i18nfield from admin
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 (5)
projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts (3)
43-53: Prevent duplicate subscriptions in createOptionsIf
createOptions()can be called multiple times, guard/unsubscribe before resubscribing to avoid leaks and duplicate updates.- this._allowedNetsSub = this._optionalDependencies.allowedNetsService.allowedNets$.subscribe(allowedNets => { + if (this._allowedNetsSub && !this._allowedNetsSub.closed) { + this._allowedNetsSub.unsubscribe(); + } + this._allowedNetsSub = this._optionalDependencies.allowedNetsService.allowedNets$.subscribe(allowedNets => {
46-50: Resolve i18n title before dedupe; current code risks type mismatch and duplicate reintroduction
petriNet.titleis typically an i18n object; using it as a string key can store[object Object]and break uniqueness across locales. Resolve once and use consistently.- allowedNets.forEach(petriNet => { - if (this.isUniqueOption(petriNet.title, petriNet.identifier)) { - this.addToMap(petriNet.title, petriNet.identifier); - } - }); + allowedNets.forEach(petriNet => { + const resolvedTitle = resolveI18n(petriNet.title); + if (this.isUniqueOption(resolvedTitle, petriNet.identifier)) { + this.addToMap(resolvedTitle, petriNet.identifier); + } + });Add missing import (outside this hunk):
import {resolveI18n} from '../../../../resources/interface/i18n-string';
61-72: Tighten map access for strict null checksAvoid repeated
.get()and assert non-null after thehasguard.- if (this._uniqueOptionsMap.get(key).has(value)) { - return false; - } else { - this._uniqueOptionsMap.get(key).add(value); - return true; - } + const set = this._uniqueOptionsMap.get(key)!; + if (set.has(value)) { + return false; + } + set.add(value); + return true;projects/netgrif-components-core/src/lib/panel/workflow-panel/abstract-workflow-panel.component.ts (2)
135-136: Keep panel content consistent with header fallbackResolve i18n and apply the same fallback in the panel field.
- title: new TextField('', this._translate.instant(this.TRANSLATION_TITLE), - this.workflow.title, this.dataFieldsBehaviour), + title: new TextField('', this._translate.instant(this.TRANSLATION_TITLE), + resolveI18n(this.workflow.title) || this.workflow.identifier, this.dataFieldsBehaviour),
58-64: Guard lang-change handler against early firingSubscription is created in the constructor; if it fires before
panelContentis built inngOnInit, it can throw. Guard or move subscription tongOnInit.- this._subscription = _translate.onLangChange.subscribe((event: TranslationChangeEvent) => { - this.panelContent.netIdentifier.title = this._translate.instant(this.TRANSLATION_NET); - this.panelContent.title.title = this._translate.instant(this.TRANSLATION_TITLE); - this.panelContent.version.title = this._translate.instant(this.TRANSLATION_VERSION); - this.panelContent.author.title = this._translate.instant(this.TRANSLATION_AUTHOR); - this.panelContent.uploaded.title = this._translate.instant(this.TRANSLATION_UPLOAD); - }); + this._subscription = _translate.onLangChange.subscribe((event: TranslationChangeEvent) => { + if (!this.panelContent) { return; } + this.panelContent.netIdentifier.title = this._translate.instant(this.TRANSLATION_NET); + this.panelContent.title.title = this._translate.instant(this.TRANSLATION_TITLE); + this.panelContent.version.title = this._translate.instant(this.TRANSLATION_VERSION); + this.panelContent.author.title = this._translate.instant(this.TRANSLATION_AUTHOR); + this.panelContent.uploaded.title = this._translate.instant(this.TRANSLATION_UPLOAD); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
projects/netgrif-components-core/src/lib/panel/workflow-panel/abstract-workflow-panel.component.ts(2 hunks)projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts(1 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 (22)
- GitHub Check: Matrix Test (24)
- GitHub Check: Matrix Test (20)
- GitHub Check: Test with SonarCloud
🔇 Additional comments (1)
projects/netgrif-components-core/src/lib/search/models/category/case/case-process.ts (1)
45-45: Good fix: reset uniqueness map per emissionClearing
_uniqueOptionsMapalongside_optionsMapavoids stale-state suppression.
...ts/netgrif-components-core/src/lib/panel/workflow-panel/abstract-workflow-panel.component.ts
Show resolved
Hide resolved
renczesstefan
changed the base branch from
release/7.0.0-rev7
to
release/7.0.0-rev8
- Added a warning log when no mapped menu item is found for a dashboard item to improve debugging clarity. - Introduced null checks for `nodePath` in `navigate` method to prevent potential runtime errors. - Ensured `activePath` is only updated when a valid `nodePath` is retrieved.
- Fixed the routing path logic in the `canAccessView` method by replacing `item.routingPath` with `item.routing?.path` to handle optional chaining properly and prevent potential runtime errors.
|
4 participants
Description
Implements NAE-2202
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?
Unit tests and manual test.
Test Configuration
Checklist:
Summary by CodeRabbit
Bug Fixes
Chores