[NAE-2227] MenuItem default headers not working #304
Conversation
- change name of case header field constant - add console log to know there is some error - add resolving of number of headers
Kovy95
requested review from
Retoocs,
machacjozef,
martinkranec,
renczesstefan and
tuplle
WalkthroughUpdates string key in GroupNavigationConstants, adds error logging in default headers factory, injects optional default headers into DefaultTabbedCaseViewComponent, introduces headersCount computed from injected headers or a default value, and binds maxHeaderColumns in the template to headersCount. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App Module
participant DI as Angular DI
participant Comp as DefaultTabbedCaseViewComponent
participant Tpl as Template (nc-header)
App->>DI: Provide NAE_DEFAULT_HEADERS (optional)
DI-->>Comp: Inject NAE_DEFAULT_HEADERS? (may be undefined)
Comp->>Comp: constructor(): headersCount = (_defaultHeaders?.length) ?? 5
Comp-->>Tpl: headersCount
Tpl->>Tpl: maxHeaderColumns = headersCount
sequenceDiagram
autonumber
participant Caller as Consumer
participant Factory as navigationItemDefaultHeadersFactory
Caller->>Factory: Resolve default headers
Factory->>Factory: try { ... } catch (e)
alt Error
Factory->>Factory: console.log("Error during resolving default headers", e)
Factory-->>Caller: undefined
else Success
Factory-->>Caller: headers array
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
projects/netgrif-components-core/src/lib/navigation/model/group-navigation-constants.ts(1 hunks)projects/netgrif-components-core/src/lib/utility/navigation-item-default-headers-factory.ts(1 hunks)projects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/tabbed/default-tabbed-case-view/default-tabbed-case-view.component.html(1 hunks)projects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/tabbed/default-tabbed-case-view/default-tabbed-case-view.component.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
projects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/tabbed/default-tabbed-case-view/default-tabbed-case-view.component.ts (1)
projects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/model/injected-tabbed-case-view-data-with-navigation-item-task-data.ts (1)
InjectedTabbedCaseViewDataWithNavigationItemTaskData(14-34)
⏰ 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 (2)
projects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/tabbed/default-tabbed-case-view/default-tabbed-case-view.component.html (1)
19-19: Verify the default value discrepancy.The template now binds
maxHeaderColumnstoheadersCount, which defaults to 5 in the TS component (line 115). However, the original hardcoded value was 6. This discrepancy may cause a visual regression if 6 was intentional.Please confirm:
- Is the default value of 5 correct, or should it be 6 to match the previous behavior?
- Does the comment "5 meta headers" (line 115 in TS) accurately reflect the intended number of columns?
If 6 was correct, apply this diff in the TS component:
- this.headersCount = 5; // 5 meta headers + this.headersCount = 6; // 6 meta headers (matches previous default)projects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/tabbed/default-tabbed-case-view/default-tabbed-case-view.component.ts (1)
94-95: LGTM! Optional injection pattern is correctly implemented.The use of
@Optional()withNAE_DEFAULT_HEADERSinjection is appropriate, allowing the component to function with or without default headers. The fallback logic properly handles both cases:
- When headers are provided: uses
_defaultHeaders.length- When not provided: defaults to 5
The provider configuration (lines 68-72) correctly uses the factory function with navigation item task data.
Note: See the comment on line 19 regarding the default value change from 6 to 5.
Also applies to: 112-116
| * Boolean field, that is true to use default headers configuration for case view | ||
| * */ | ||
| ITEM_FIELD_ID_USE_CASE_DEFAULT_HEADERS = 'use_default_headers', | ||
| ITEM_FIELD_ID_USE_CASE_DEFAULT_HEADERS = 'use_case_default_headers', |
There was a problem hiding this comment.
🧩 Analysis chain
Breaking change: verify all usages and backend field names.
The constant value changed from 'use_default_headers' to 'use_case_default_headers'. This is a breaking change that will affect:
- Backend field IDs in the data model
- Any existing configuration or navigation items using the old field name
- Cached or stored references to this field
Additionally, there's an inconsistency: ITEM_FIELD_ID_USE_CASE_DEFAULT_HEADERS now uses 'use_case_default_headers' (line 126), but ITEM_FIELD_ID_USE_TASK_DEFAULT_HEADERS still uses 'use_default_headers' (line 136). If this inconsistency is intentional, consider adding a comment explaining why; otherwise, align both constants.
Run the following script to check for any remaining references to the old field name:
🏁 Script executed:
#!/bin/bash
# Description: Search for references to the old field ID 'use_default_headers' in case-related contexts
# Search for the old constant value in case-related code
rg -n "use_default_headers" --type=ts -C3 | grep -i caseLength of output: 0
Run the following to check for any remaining references to the old and new field IDs across the repository:
If neither command yields hits, manually verify any external configurations or backend mappings still expecting use_default_headers.
🏁 Script executed:
#!/bin/bash
rg -n "use_default_headers" -C2
rg -n "use_case_default_headers" -C2Length of output: 8212
Fix and align default‐headers constants
- Change
ITEM_FIELD_ID_USE_TASK_DEFAULT_HEADERS(line 136) from'use_default_headers'to'use_task_default_headers'. - Verify that updating
ITEM_FIELD_ID_USE_CASE_DEFAULT_HEADERSto'use_case_default_headers'doesn’t break existing backend/data‐model mappings or client configurations.
| return undefined; | ||
| } | ||
| } catch (e) { | ||
| console.log('Error during resolving default headers', e) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use LoggerService instead of console.log.
Direct console logging bypasses the application's logging infrastructure and may not be captured in production monitoring systems.
Consider injecting and using LoggerService:
- console.log('Error during resolving default headers', e)
+ // Inject LoggerService as a parameter if not already available
+ loggerService.error('Error during resolving default headers', e);Alternatively, if LoggerService cannot be injected at the factory level, consider propagating the error instead of swallowing it:
- } catch (e) {
- console.log('Error during resolving default headers', e)
- return undefined;
- }
+ } catch (e) {
+ throw new Error(`Error during resolving default headers: ${e}`);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/utility/navigation-item-default-headers-factory.ts
around line 27, replace the direct console.log usage with the application's
LoggerService: inject LoggerService into the factory (or accept it as a
constructor/parameter) and call logger.error('Error during resolving default
headers', e) instead of console.log; if injecting LoggerService is not possible
at this factory level, do not swallow the error—rethrow or return a rejected
Promise so the caller can handle/log it via the application's logging
infrastructure.
| headersMode: string[]; | ||
| allowTableMode: boolean; | ||
| defaultHeadersMode: HeaderMode; | ||
| headersCount: number |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider initializing headersCount at declaration.
While headersCount is always initialized in the constructor (lines 112-116), declaring it with an initial value improves code clarity and prevents potential undefined issues if the constructor logic changes.
Apply this diff:
- headersCount: number
+ headersCount: number = 5;Then in the constructor, you can simplify:
- if (this._defaultHeaders) {
- this.headersCount = this._defaultHeaders.length;
- } else {
- this.headersCount = 5; // 5 meta headers
- }
+ if (this._defaultHeaders) {
+ this.headersCount = this._defaultHeaders.length;
+ }🤖 Prompt for AI Agents
In
projects/netgrif-components/src/lib/navigation/group-navigation-component-resolver/default-components/tabbed/default-tabbed-case-view/default-tabbed-case-view.component.ts
around line 88, initialize headersCount at declaration (e.g., set to 0) to avoid
potential undefined values; then remove the explicit assignment to headersCount
in the constructor (lines ~112-116) so the constructor logic is simplified and
relies on the declared default.
machacjozef
changed the base branch from
release/7.0.0-rev9
to
release/7.0.0-rev8
|
Description
Fixes NAE-2227
Dependencies
none
Third party dependencies
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
manually
Test Configuration
<Please describe configuration for tests to run if applicable, like program parameters, host OS, VM configuration etc. You can use >
Checklist:
Summary by CodeRabbit
New Features
Chores