[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
WalkthroughAdds locale-aware translation resolution for immediate-data values, expands ImmediateData.options to use per-key I18nFieldValue entries, makes Changes
Sequence Diagram(s)sequenceDiagram
participant Panel as PanelWithImmediateData
participant Options as ImmediateData.options
participant I18n as I18nFieldValue
rect rgb(240,248,255)
Note right of Panel: Resolve displayed label/value (new flow)
Panel->>Options: read option entry (enum/multichoice/button)
alt option includes I18nFieldValue
Panel->>I18n: getTranslation(i18n)
I18n-->>Panel: localized string (preferred)
else no i18n metadata
Options-->>Panel: return defaultValue (fallback)
end
Panel->>Panel: render resolved label/value
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)projects/netgrif-components-core/src/lib/resources/interface/immediate-data.ts (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). (6)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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.
There was a problem hiding this comment.
Options in ImmediateData interface is defined as follows:
options?: {
[k: string]: I18nFieldValue
};
Interface si defined in file projects/netgrif-components-core/src/lib/resources/interface/immediate-data.ts
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current ImmediateData interface definition
echo "=== Current ImmediateData interface definition ==="
cat projects/netgrif-components-core/src/lib/resources/interface/immediate-data.ts
Length of output: 1225
@tuplle, thank you for the correction! You're absolutely right. I've verified that the ImmediateData interface in projects/netgrif-components-core/src/lib/resources/interface/immediate-data.ts correctly defines options as:
options?: {
[k: string]: I18nFieldValue
};This properly supports the usage in panel-with-immediate-data.ts where immediate.options[immediate.value] and immediate.options[it] are accessed. The type definitions are correct, and my earlier concern was based on incorrect information. I apologize for the confusion.
The implementation looks good from a type safety perspective! ✅
🧬 Code Graph Analysis Results
projects/netgrif-components-core/src/lib/resources/interface/immediate-data.ts
export interface ImmediateData { /** * @ignore */ stringId: string; /** * @ignore */ // TODO Exist only in Net Immediate data title?: any; /** * @ignore */ type: string; /** * @ignore */ value?: any; /** * Only for Fields of type `CaseRef` and `Filter` */ allowedNets?: Array; /** * Only for Fields of type `Filter` */ filterMetadata?: FilterMetadata; /** * Only for Currency Number Fields */ format?: FormatFilter; /** * Only for Currency Number Fields */ component?: Component; /** * @ignore */ // TODO Exists only in case immediate data name?: any; /** * Only for enumeration_map and multichoice_map */ options?: { [k: string]: I18nFieldValue }; }projects/netgrif-components-core/src/lib/data-fields/i18n-field/models/i18n-field-value.ts
export interface I18nFieldValue { defaultValue: string; key?: string; translations?: I18nFieldTranslations; }
projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts
Outdated
Show resolved
Hide resolved
projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts
Outdated
Show resolved
Hide resolved
machacjozef
changed the base branch from
release/7.0.0-rev9
to
release/7.0.0-rev8
|
Please check AI's comments, I do not think it is correct in all cases. |
- fix according to PR
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
projects/netgrif-components-core/src/lib/resources/interface/immediate-data.ts (1)
4-55: Restore the map typing foroptions. The new shape models a singleI18nFieldValue, butparseImmediateValue()still does keyed lookups (immediate.options[immediate.value]). Without a string index signature the build breaks and the runtime contract forenumeration_map/multichoice_mapstays wrong. Please keepoptionsas a keyed collection, e.g.Record<string, I18nFieldValue>, and adjust the import accordingly.Apply this diff:
-import {I18nFieldTranslations} from "../../data-fields/i18n-field/models/i18n-field-value"; +import {I18nFieldValue} from "../../data-fields/i18n-field/models/i18n-field-value"; @@ - options?: { - defaultValue?: string, - key?: string; - translations?: I18nFieldTranslations; - }; + options?: Record<string, I18nFieldValue>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts(4 hunks)projects/netgrif-components-core/src/lib/resources/interface/immediate-data.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
projects/netgrif-components-core/src/lib/resources/interface/immediate-data.ts (1)
projects/netgrif-components-core/src/lib/data-fields/i18n-field/models/i18n-field-value.ts (1)
I18nFieldTranslations(7-9)
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)
🪛 Biome (2.1.2)
projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts
[error] 81-81: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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: task-list-completed
- GitHub Check: Matrix Test (22)
- GitHub Check: Test with SonarCloud
- GitHub Check: Matrix Test (24)
- GitHub Check: Matrix Test (20)
| 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 && (immediate as any).placeholder.defaultValue !== undefined | ||
| ? (immediate as any).placeholder.defaultValue : (immediate.name && immediate.name.defaultValue !== undefined | ||
| ? immediate.name.defaultValue : this._translate.instant('dialog.submit')), | ||
| icon: undefined, type: immediate.type | ||
| value: buttonValue, | ||
| icon: undefined, | ||
| type: immediate.type | ||
| }; |
There was a problem hiding this comment.
Wrap the button clause in a block. Declaring let buttonValue directly in the case violates noSwitchDeclarations (Biome error) because the binding leaks to sibling clauses. Enclose this case in braces so the lint/build passes.
Apply this diff:
- case 'button':
+ 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: buttonValue,
icon: undefined,
type: immediate.type
};
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 81-81: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/panel/abstract/panel-with-immediate-data.ts
around lines 81 to 93, the switch case that declares let buttonValue leaks the
binding to sibling clauses and violates noSwitchDeclarations; wrap that case
body in a block (add { ... }) and move the let buttonValue declaration inside
that block (or change to const) so the variable scope is limited to the case;
ensure the return object remains unchanged and formatting/indentation is updated
accordingly.
- fix according to PR
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
projects/netgrif-components-core/src/lib/resources/interface/immediate-data.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
projects/netgrif-components-core/src/lib/resources/interface/immediate-data.ts (1)
projects/netgrif-components-core/src/lib/data-fields/i18n-field/models/i18n-field-value.ts (1)
I18nFieldTranslations(7-9)
⏰ 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). (3)
- GitHub Check: task-list-completed
- GitHub Check: Test with SonarCloud
- GitHub Check: task-list-completed
🔇 Additional comments (1)
projects/netgrif-components-core/src/lib/resources/interface/immediate-data.ts (1)
51-57: Thekeyfield is not redundant—it's an intentional part of the normalized data structure.The original concern is based on a misunderstanding. The
keyfield serves a legitimate purpose: it stores the map key in the value object, following the established pattern fromI18nFieldValuein the codebase. This is confirmed by:
I18nFieldValueinterface (lines 1–5 ini18n-field-value.ts) explicitly includeskey?: stringI18nField.fromObject()method (line 71–74 ini18n-field.ts) intentionally setskey: templateKeytask-content.service.ts(line 283) explicitly converts options:{key: optionKey, value: updatedField.options[optionKey]}The
keyfield denormalizes the map key into the value object, allowing each option to be self-contained and easier to work with in isolation.That said, add JSDoc comments to document:
- The purpose of the
optionsproperty- When and how the
translationsfield is populated- The role of the
keyfield (to store the map key for convenience)Likely an incorrect or invalid review comment.
| import {FilterMetadata} from '../../search/models/persistance/filter-metadata'; | ||
| import {FormatFilter} from '../../data-fields/models/format-filter'; | ||
| import {Component} from '../../data-fields/models/component'; | ||
| import {I18nFieldTranslations} from "../../data-fields/i18n-field/models/i18n-field-value"; |
There was a problem hiding this comment.
Fix quote style inconsistency.
The import uses double quotes while all other imports in this file use single quotes. Maintain consistency with the existing code style.
Apply this diff to fix the quote style:
-import {I18nFieldTranslations} from "../../data-fields/i18n-field/models/i18n-field-value";
+import {I18nFieldTranslations} from '../../data-fields/i18n-field/models/i18n-field-value';📝 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.
| import {I18nFieldTranslations} from "../../data-fields/i18n-field/models/i18n-field-value"; | |
| import {I18nFieldTranslations} from '../../data-fields/i18n-field/models/i18n-field-value'; |
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/resources/interface/immediate-data.ts
around line 4, the import uses double quotes instead of the file's single-quote
style; change the import statement to use single quotes for consistency with the
rest of the file (replace " with ' in that import).
- fix according to PR
|
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