[NAE-2251] NAE - Map field options are not translated #310
Conversation
- add resolving i18n to panels
Kovy95
requested review from
Retoocs,
SamuelPalaj,
machacjozef,
renczesstefan and
tuplle
WalkthroughThe changes introduce translation support for immediate data fields and update search request model flexibility. A translation helper method is added to resolve locale-specific values, the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Pre-merge checks✅ 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/filter/models/case-search-request-body.ts(1 hunks)projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts(4 hunks)projects/netgrif-components-core/src/lib/panel/case-panel/abstract-case-panel.component.spec.ts(1 hunks)projects/netgrif-components/src/lib/panel/case-panel/case-panel.component.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-21T11:17:07.053Z
Learnt from: renczesstefan
Repo: netgrif/components PR: 295
File: projects/netgrif-components-core/src/lib/filter/models/case-search-request-body.ts:0-0
Timestamp: 2025-08-21T11:17:07.053Z
Learning: In the netgrif-components codebase, `stringId` has been deprecated in favor of `id` in search request bodies like CaseSearchRequestBody. When creating search payloads for SimpleFilter.fromCaseQuery() or similar methods, use `id` instead of `stringId`. However, accessing the `.stringId` property on existing objects like Case is still valid since the objects retain this property.
Applied to files:
projects/netgrif-components-core/src/lib/filter/models/case-search-request-body.ts
🧬 Code graph analysis (1)
projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts (1)
projects/netgrif-components-core/src/lib/data-fields/i18n-field/models/i18n-field-value.ts (1)
I18nFieldValue(1-5)
⏰ 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). (5)
- GitHub Check: task-list-completed
- GitHub Check: Matrix Test (24)
- GitHub Check: Matrix Test (22)
- GitHub Check: Test with SonarCloud
- GitHub Check: Matrix Test (20)
🔇 Additional comments (4)
projects/netgrif-components/src/lib/panel/case-panel/case-panel.component.spec.ts (1)
107-107: LGTM! Test data updated for I18nFieldValue support.The enum value structure now correctly matches the I18nFieldValue interface used in the production code, with the translations object properly initialized.
projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts (1)
10-10: LGTM! I18nFieldValue import added.The import correctly brings in the I18nFieldValue interface needed for translation support.
projects/netgrif-components-core/src/lib/panel/case-panel/abstract-case-panel.component.spec.ts (1)
166-166: LGTM! Test data structure updated consistently.The enum value structure correctly matches the I18nFieldValue interface, maintaining consistency with the parallel test file update in the components library.
projects/netgrif-components-core/src/lib/filter/models/case-search-request-body.ts (1)
85-89: I need to verify the actual baseline state of theidentifierfield. Let me search for validation logic and check test files.Based on my verification of the codebase and industry standards, the original review comment contains an incorrect characterization of the changes.
The characterization of making
identifieroptional as a breaking change is incorrect.According to standard API design practices, adding optional parameters to an invocation target can be safe, whereas adding required parameters is a breaking change. You can enhance your code gradually by adding optional fields without breaking existing code or interfaces.
The code review shows:
identifier?: string;(already optional)processId?: string;(new optional field)- Existing usage at line 492 creates objects with only
identifier:{identifier: id}as aPetriNetSearchRequestThis pattern is compatible with both fields being optional. The change is additive and non-breaking for client code.
However, a valid concern remains: If both fields can now be omitted, the backend must validate that at least one is present for meaningful queries. This is not a breaking change to the TypeScript interface, but a requirement for runtime correctness that should be documented and validated at the API boundary (backend endpoint).
Likely an incorrect or invalid review comment.
| return {value: this.getTranslation(immediate.value), icon: undefined, type: immediate.type}; | ||
| case 'multichoice': | ||
| return { | ||
| value: immediate.value.map(it => it.defaultValue).join(', '), | ||
| value: immediate.value.map(it => this.getTranslation(it)).join(', '), | ||
| icon: undefined, | ||
| type: immediate.type | ||
| }; | ||
| case 'enumeration_map': | ||
| return { | ||
| value: immediate.options[immediate.value].defaultValue, | ||
| value: this.getTranslation(immediate.options[immediate.value]), | ||
| icon: undefined, | ||
| type: immediate.type | ||
| }; | ||
| case 'multichoice_map': | ||
| return { | ||
| value: immediate.value.map(it => | ||
| immediate.options[it].defaultValue).join(', '), icon: undefined, type: immediate.type | ||
| this.getTranslation(immediate.options[it])).join(', '), | ||
| icon: undefined, | ||
| type: immediate.type | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify type compatibility for immediate data values.
The code now assumes that values for enumeration, multichoice, enumeration_map, and multichoice_map field types conform to the I18nFieldValue interface. Ensure that:
- The
ImmediateDatainterface definitions for these field types guarantee values areI18nFieldValueobjects - Backend APIs always return data in this format for these field types
- Existing data in the system has been migrated to include the
translationsfield (even if empty)
Run the following script to verify the ImmediateData type definitions:
🏁 Script executed:
#!/bin/bash
# Description: Verify ImmediateData interface and type definitions
# Find ImmediateData interface definition
ast-grep --pattern 'interface ImmediateData {
$$$
}'
# Find type definitions for immediate data values
rg -nP --type=ts 'type.*=.*enumeration|multichoice' -A5 -B5 -g '!node_modules'Length of output: 113361
Looking at the shell script results to verify the type safety concerns in the panel-with-immediate-data.ts code.
🏁 Script executed:
#!/bin/bash
# Search for I18nFieldValue type definition and getTranslation method
echo "=== Searching for I18nFieldValue type ==="
rg -nP --type=ts 'type I18nFieldValue|interface I18nFieldValue' -A5
echo "=== Searching for getTranslation method in panel-with-immediate-data.ts ==="
rg -nP --type=ts 'getTranslation' projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts -B5 -A5
echo "=== Searching for where immediate.options is used ==="
rg -nP --type=ts 'immediate\.options' -B3 -A3Length of output: 5260
🏁 Script executed:
#!/bin/bash
# Deep dive into getTranslation and I18nFieldValue
echo "=== Find I18nFieldValue definition ==="
fd -t f -e ts | xargs grep -l "I18nFieldValue" | head -5
rg -nP "type I18nFieldValue|interface I18nFieldValue" -A10 --type=ts
echo "=== Find getTranslation implementation ==="
ast-grep --pattern 'getTranslation($_) {
$$$
}'
echo "=== Check panel-with-immediate-data.ts full content ==="
cat projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts | head -100Length of output: 6895
Type definitions for ImmediateData are insufficient for enumeration_map and multichoice_map field types.
The ImmediateData interface (line 50) defines options?: { defaultValue?: string }, but the code at lines 52 and 59 treats it as a map of I18nFieldValue objects. Additionally, value is typed as any rather than enforcing the expected I18nFieldValue structure for enumeration and multichoice cases.
The type mismatch should be fixed in the interface definition:
// Current ImmediateData.options definition
options?: { defaultValue?: string };
// Should be (for enumeration_map/multichoice_map types)
options?: Record<string, I18nFieldValue>;Also consider tightening the value field type for these field types using discriminated unions if possible, or at minimum document the expected structure in comments.
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts
around lines 43 to 62, the ImmediateData type is too loose for enumeration_map
and multichoice_map: options is currently declared as { defaultValue?: string }
and value is any, but the code treats options as a map of I18nFieldValue and
values as keys/arrays of those keys; update the ImmediateData.interface so
options?: Record<string, I18nFieldValue> (or equivalent map type) and narrow
value types (use a discriminated-union for field type cases or at minimum
document expected structures and replace any with the appropriate string or
string[] types for enumeration_map and multichoice_map) so the runtime access
immediate.options[immediate.value] and immediate.options[it] types-check
correctly.
| value: (immediate as any).placeholder?.defaultValue !== undefined | ||
| ? this.getTranslation((immediate as any).placeholder) | ||
| : ( | ||
| immediate.name?.defaultValue !== undefined | ||
| ? this.getTranslation(immediate.name) | ||
| : this._translate.instant('dialog.submit') | ||
| ), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying the nested ternary logic.
The button value resolution uses nested ternary operators that are difficult to read and maintain. While functionally correct, this could be refactored for clarity.
Consider applying this diff for improved readability:
case 'button':
+ let buttonValue: string;
+ if ((immediate as any).placeholder?.defaultValue !== undefined) {
+ buttonValue = this.getTranslation((immediate as any).placeholder);
+ } else if (immediate.name?.defaultValue !== undefined) {
+ buttonValue = this.getTranslation(immediate.name);
+ } else {
+ buttonValue = this._translate.instant('dialog.submit');
+ }
return {
- value: (immediate as any).placeholder?.defaultValue !== undefined
- ? this.getTranslation((immediate as any).placeholder)
- : (
- immediate.name?.defaultValue !== undefined
- ? this.getTranslation(immediate.name)
- : this._translate.instant('dialog.submit')
- ),
+ value: buttonValue,
icon: undefined, type: immediate.type
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts
around lines 82 to 88, the nested ternary used to compute the button value is
hard to read; replace it by computing a local variable (e.g. resolvedValue)
immediately above where it's used: check if (immediate as
any).placeholder?.defaultValue !== undefined then set resolvedValue =
this.getTranslation((immediate as any).placeholder); else if
(immediate.name?.defaultValue !== undefined) set resolvedValue =
this.getTranslation(immediate.name); else set resolvedValue =
this._translate.instant('dialog.submit'); then use value: resolvedValue. Keep
the same type casts and behavior but remove the nested ternary for clarity.
| protected getTranslation(i18n: I18nFieldValue): string { | ||
| const locale = this._translate.currentLang; | ||
| return (i18n && locale in i18n.translations) | ||
| ? i18n.translations[locale] | ||
| : i18n?.defaultValue; | ||
| } |
There was a problem hiding this comment.
Fix potential undefined return value.
The method signature declares a return type of string, but when i18n is null or undefined, i18n?.defaultValue returns undefined, not a string. This type mismatch could cause runtime issues.
Apply this diff to ensure a string is always returned:
- protected getTranslation(i18n: I18nFieldValue): string {
+ protected getTranslation(i18n: I18nFieldValue | undefined | null): string {
const locale = this._translate.currentLang;
return (i18n && locale in i18n.translations)
? i18n.translations[locale]
- : i18n?.defaultValue;
+ : (i18n?.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.
| protected getTranslation(i18n: I18nFieldValue): string { | |
| const locale = this._translate.currentLang; | |
| return (i18n && locale in i18n.translations) | |
| ? i18n.translations[locale] | |
| : i18n?.defaultValue; | |
| } | |
| protected getTranslation(i18n: I18nFieldValue | undefined | null): string { | |
| const locale = this._translate.currentLang; | |
| return (i18n && locale in i18n.translations) | |
| ? i18n.translations[locale] | |
| : (i18n?.defaultValue ?? ''); | |
| } |
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts
around lines 113 to 118, the getTranslation method can return undefined when
i18n is null/undefined because i18n?.defaultValue may be undefined while the
signature requires a string; update the method to always return a string by
returning a fallback (e.g., empty string or a defined fallback like '') when
neither the locale translation nor defaultValue exist, ensuring the return value
is coerced to string.
|
Description
Fixes NAE-2251
Dependencies
none
Third party dependencies
No new dependencies were introduced
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
Tests