feat: StudioCodeListEditor - display Numberfield or Checkbox based on value type #14398
Conversation
github-actions
bot
added
solution/studio/designer
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14398 +/- ##
==========================================
+ Coverage 95.73% 95.75% +0.01%
==========================================
Files 1908 1912 +4
Lines 24878 24924 +46
Branches 2846 2851 +5
==========================================
+ Hits 23818 23865 +47
Misses 799 799
+ Partials 261 260 -1 ☔ View full report in Codecov by Sentry. |
📝 WalkthroughWalkthroughThe pull request introduces significant enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.test.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/frontend/libs/studio-components".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-storybook" was referenced from the config file in "frontend/libs/studio-components/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...ents/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx
Fixed
Show fixed
Hide fixed
...ents/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx
Fixed
Show fixed
Hide fixed
ErlingHauan
changed the title
…com/Altinn/altinn-studio into add-numberfield-to-code-list-editor
ErlingHauan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.ts (1)
38-41: Consider adding explicit array bounds check.The function assumes the array has elements when accessing the last item. While the current implementation is protected by the check in
getEmptyItem, it would be more robust to add an explicit check here.function getTypeOfLastValue(codeList: CodeList) { + if (codeList.length === 0) return 'string'; const lastCodeListItem = codeList[codeList.length - 1]; return typeof lastCodeListItem.value as 'string' | 'number' | 'boolean'; }frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx (3)
128-129: Consider explicit undefined handling for numberfield.The comment about empty numberfield producing undefined is implementation-specific. Consider making this behavior more explicit:
-const shouldRenderNumberfield = typeof value === 'number' || value === undefined; // Empty numberfield produces undefined +const shouldRenderNumberfield = typeof value === 'number' || value === undefined; +const isEmptyNumberfield = value === undefined;
166-189: Consider adding number validation constraints.The NumberfieldCell could benefit from input validation to ensure data integrity:
const NumberfieldCell = forwardRef<HTMLInputElement, InputCellProps>( ({ label, value, onChange, onFocus, autoComplete }, ref) => { const handleNumberChange = useCallback( (numberValue: number): void => { + // Add validation if needed + if (isNaN(numberValue)) return; onChange(numberValue); }, [onChange], ); return ( <StudioInputTable.Cell.Numberfield ref={ref} aria-label={label} autoComplete={autoComplete} className={classes.textfieldCell} + min={0} // Add if applicable + max={100} // Add if applicable onChange={handleNumberChange} onFocus={onFocus} value={value as number} /> ); }, );
191-215: Optimize checkbox value handling.The CheckboxCell duplicates the string value. Consider simplifying:
const CheckboxCell = forwardRef<HTMLInputElement, InputCellProps>( ({ label, value, onChange, onFocus }, ref) => { const handleBooleanChange = useCallback( (event: React.ChangeEvent<HTMLInputElement>): void => { onChange(event.target.checked); }, [onChange], ); + const stringValue = String(value); return ( <StudioInputTable.Cell.Checkbox ref={ref} aria-label={label} onChange={handleBooleanChange} onFocus={onFocus} checked={value as boolean} - value={String(value)} + value={stringValue} > - {String(value)} + {stringValue} </StudioInputTable.Cell.Checkbox> ); }, );frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditor.test.tsx (1)
484-546: Consider adding edge case tests.The type handling tests are comprehensive but could benefit from additional edge cases:
- Test handling of invalid number inputs
- Test handling of empty/null values
- Test handling of type conversion edge cases (e.g., "true"/"false" strings vs booleans)
Example test case:
it('Handles invalid number input gracefully', async () => { const user = userEvent.setup(); renderCodeListEditor({ codeList: codeListWithNumberValues }); const valueInput = screen.getByRole('textbox', { name: texts.itemValue(1) }); await user.type(valueInput, 'not-a-number'); await user.tab(); expect(onBlurAny).not.toHaveBeenCalled(); });frontend/libs/studio-components/src/components/StudioCodelistEditor/test-data/codeListWithNumberValues.ts (1)
4-23: Enhance test data coverage with diverse scenarios.While the current test items are well-structured, consider adding more diverse test cases to cover:
- Edge cases (e.g., very large numbers, decimals, negative values)
- More meaningful labels/descriptions that reflect the specific test scenarios
- Comments explaining what each test case validates
Example enhancement:
const item1: CodeListItem = { - description: 'Test 1 description', - helpText: 'Test 1 help text', - label: 'Test 1', - value: 1, + description: 'Basic positive integer', + helpText: 'Tests handling of simple positive numbers', + label: 'Positive Integer', + value: 42, }; const item2: CodeListItem = { - description: 'Test 2 description', - helpText: 'Test 2 help text', - label: 'Test 2', - value: 2, + description: 'Decimal number', + helpText: 'Tests handling of decimal numbers', + label: 'Decimal', + value: 3.14, }; const item3: CodeListItem = { - description: 'Test 3 description', - helpText: 'Test 3 help text', - label: 'Test 3', - value: 3, + description: 'Negative number', + helpText: 'Tests handling of negative numbers', + label: 'Negative', + value: -1, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditor.test.tsx(2 hunks)frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx(5 hunks)frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/utils.ts(2 hunks)frontend/libs/studio-components/src/components/StudioCodelistEditor/test-data/codeListWithBooleanValues.ts(1 hunks)frontend/libs/studio-components/src/components/StudioCodelistEditor/test-data/codeListWithNumberValues.ts(1 hunks)frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.test.ts(2 hunks)frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Testing
- GitHub Check: Build environment and run e2e test
🔇 Additional comments (8)
frontend/libs/studio-components/src/components/StudioCodelistEditor/test-data/codeListWithBooleanValues.ts (1)
4-16: LGTM! Well-structured test data.The test data provides good coverage with both true and false boolean values, and includes all necessary fields (description, helpText, label, value) for comprehensive testing.
frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/utils.ts (1)
12-14: LGTM! Good type safety improvement.The change from
stringtoCodeListItemValuetype enhances type safety while maintaining immutability. This aligns well with the PR's objective to support multiple value types.frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.ts (1)
5-18: LGTM! Well-structured empty item templates.The empty item templates are well-defined with appropriate default values for each type.
frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.test.ts (1)
30-60: LGTM! Excellent test coverage.The test cases thoroughly cover all scenarios:
- Empty code list handling
- Type-specific empty item addition for string, number, and boolean
- Mixed type sequences
frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx (2)
Line range hint
52-56: LGTM! Type-safe value handling implemented.The function now correctly handles multiple value types (string, number, boolean) in a type-safe manner.
257-257: LGTM! Consistent use of TypedInputCell.The TextResourceIdCell correctly uses the new TypedInputCell component, maintaining consistency across the codebase.
frontend/libs/studio-components/src/components/StudioCodelistEditor/test-data/codeListWithNumberValues.ts (2)
1-2: LGTM! Clean type imports.The imports follow TypeScript best practices by using the 'type' keyword for better tree-shaking.
25-25: Verify CodeList type compatibility with number values.The export is properly typed, but let's verify that the
CodeListtype definition supports number values for thevalueproperty.✅ Verification successful
Number values are fully supported by CodeList type
The type system explicitly supports number values through
CodeListItemValue = string | boolean | numberunion type. The implementation is type-safe.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check CodeList and CodeListItem type definitions for number value support # Search for CodeList type definition echo "Searching for CodeList type definition..." ast-grep --pattern 'type CodeList = $_' # Search for CodeListItem type definition echo "Searching for CodeListItem type definition..." ast-grep --pattern 'type CodeListItem = { $$$ value: $_ $$$ }'Length of output: 3187
Script:
#!/bin/bash # Search for CodeListItem definition with a simpler pattern echo "Searching for CodeListItem definition..." rg -A 10 "type CodeListItem" || rg -A 10 "interface CodeListItem" # Also check the file where we found CodeList type echo "Checking CodeList.ts file content..." cat frontend/libs/studio-components/src/components/StudioCodelistEditor/types/CodeList.tsLength of output: 1240
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/libs/studio-pure-functions/src/ArrayUtils/ArrayUtils.test.ts (1)
293-303: Add more test cases for better coverage.The test suite for
extractUniqueTypesneeds additional test cases to ensure robust behavior.Apply this diff to add more test cases:
describe('extractUniqueTypes', () => { it('returns array with one occurence of every type from input array', () => { const array = ['hello', 'world', 1, 2, true, false, undefined]; expect(ArrayUtils.extractUniqueTypes(array)).toEqual([ 'string', 'number', 'boolean', 'undefined', ]); }); + + it('returns empty array for empty input', () => { + expect(ArrayUtils.extractUniqueTypes([])).toEqual([]); + }); + + it('returns single type for array with duplicate types', () => { + expect(ArrayUtils.extractUniqueTypes([1, 2, 3])).toEqual(['number']); + }); + + it('handles null values', () => { + expect(ArrayUtils.extractUniqueTypes([null])).toEqual(['object']); + }); });
🧹 Nitpick comments (1)
frontend/libs/studio-pure-functions/src/ArrayUtils/ArrayUtils.test.ts (1)
305-321: Add edge cases for better test coverage.The test suite for
hasSingleTypewould benefit from additional edge cases.Apply this diff to add more test cases:
describe('hasSingleType', () => { it('returns true for an array with all strings', () => { expect(ArrayUtils.hasSingleType(['a', 'b', 'c'])).toBe(true); }); it('returns true for an array with all numbers', () => { expect(ArrayUtils.hasSingleType([1, 2, 3])).toBe(true); }); it('returns false for an array with mixed types', () => { expect(ArrayUtils.hasSingleType([1, 'a', true])).toBe(false); }); it('returns false for an empty array', () => { expect(ArrayUtils.hasSingleType([])).toBe(false); }); + + it('returns true for an array with all booleans', () => { + expect(ArrayUtils.hasSingleType([true, false, true])).toBe(true); + }); + + it('returns true for an array with all nulls', () => { + expect(ArrayUtils.hasSingleType([null, null])).toBe(true); + }); + + it('returns true for an array with all undefined values', () => { + expect(ArrayUtils.hasSingleType([undefined, undefined])).toBe(true); + }); + + it('returns true for a single-element array', () => { + expect(ArrayUtils.hasSingleType([1])).toBe(true); + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/libs/studio-pure-functions/src/ArrayUtils/ArrayUtils.test.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (2)
frontend/libs/studio-pure-functions/src/ArrayUtils/ArrayUtils.test.ts (2)
60-60: LGTM! Improved test suite organization.The rename from
ArrayUtils.intersectiontointersectionimproves consistency with other test suite names.
241-241: LGTM! Fixed typo in test suite name.The rename from
rplaceItemsByValuetoreplaceItemsByValuefixes the typo in the test suite name.
...ents/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx (1)
162-185: Remove unnecessary string conversion.The
value={String(value)}prop appears unnecessary since the checkbox's value isn't used in the component's functionality.- value={String(value)}frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.ts (1)
6-19: Add JSDoc comments to document the purpose of these constants.The constants are well-defined with appropriate default values for each type. Consider adding JSDoc comments to explain their purpose and usage.
+/** + * Default empty item for string values in a code list. + */ export const emptyStringItem: CodeListItem = { value: '', label: '', }; +/** + * Default empty item for number values in a code list. + */ export const emptyNumberItem: CodeListItem = { value: 0, label: '', }; +/** + * Default empty item for boolean values in a code list. + */ export const emptyBooleanItem: CodeListItem = { value: false, label: '', };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/language/src/nb.json(1 hunks)frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx(5 hunks)frontend/libs/studio-components/src/components/StudioCodelistEditor/test-data/texts.ts(1 hunks)frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.ts(1 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/hooks/useCodeListEditorTexts.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/hooks/useCodeListEditorTexts.ts
- frontend/libs/studio-components/src/components/StudioCodelistEditor/test-data/texts.ts
- frontend/language/src/nb.json
🧰 Additional context used
🧠 Learnings (1)
frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx (2)
Learnt from: ErlingHauan
PR: Altinn/altinn-studio#14398
File: frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx:128-136
Timestamp: 2025-02-10T11:28:12.444Z
Learning: The StudioCodeListEditor component's handling of undefined values (when StudioDecimalField is cleared) is verified through the test case that checks if a numberfield is rendered with inputMode='decimal'.
Learnt from: ErlingHauan
PR: Altinn/altinn-studio#14398
File: frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx:0-0
Timestamp: 2025-02-10T11:46:29.320Z
Learning: The Numberfield component handles number validation internally and only calls onChange with valid number or undefined values, making additional validation in parent components unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (9)
frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx (5)
51-57: LGTM! Type-safe value handling implemented.The updated
handleValueChangefunction now correctly accepts the union typeCodeListItemValue, enabling type-safe handling of string, number, and boolean values.
108-137: Well-structured type-safe input handling!The
TypedInputCellimplementation:
- Uses generic types for type-safety
- Handles all value types cleanly with a switch case
- Effectively uses the rest pattern for prop forwarding
- Properly handles the undefined case from StudioDecimalField (verified by tests)
139-161: Clean NumberfieldCell implementation!The component:
- Correctly implements forwardRef pattern
- Uses rest pattern for clean prop forwarding
- Relies on Numberfield's built-in validation (per previous discussion)
186-208: LGTM! Clean TextfieldCell implementation.The component follows React best practices with forwardRef and clean prop forwarding.
219-226: LGTM! Consistent use of TypedInputCell.The component correctly uses TypedInputCell with a safe empty string fallback for currentId.
frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.ts (4)
1-4: LGTM!The imports are well-organized and include all necessary types and utilities.
21-24: LGTM!The function is well-named and correctly composes the helper functions.
39-45: LGTM!The function includes proper error handling and uses ArrayUtils.last as suggested in the past review comments.
26-37: Consider adding type validation for the switch cases.The switch statement handles the basic types but might need to handle additional TypeScript types that could appear in the
typeofresult.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for usage of CodeListItem.value to verify all possible types ast-grep --pattern 'interface CodeListItem { $$$ value: $_ $$$ }'Length of output: 73
Subject: Manual Verification Needed on Type Handling in
createNewEmptyItemOur initial search for the
CodeListIteminterface did not return any results, so we couldn’t confirm whether itsvalueproperty might receive types other than those handled in the switch statement. Given this, it’s not clear if additionaltypeofoutcomes (for example,"string","bigint","symbol", etc.) are possible.Please manually verify:
- The definition of and expected types for the
CodeListItem.valueproperty.- The possible outcomes from
getTypeOfLastValue.This manual check will ensure whether explicit type validation is necessary.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.test.ts (2)
31-44: LGTM! Consider adding documentation about type handling.The tests are well-structured and cover the main scenarios. Consider adding a comment explaining the type inference strategy for empty lists and mixed-type scenarios.
71-76: Add test cases for non-empty codelists.While testing the empty codelist case is important, consider adding test cases for:
- Last value is a string
- Last value is a number
- Last value is a boolean
Would you like me to help draft these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.test.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
🔇 Additional comments (2)
frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.test.ts (2)
2-11: LGTM! Clear and descriptive imports.The renamed function and new imports clearly indicate their purpose and align well with the feature's objectives.
55-62: Consider documenting boolean codelist limitations.Based on past discussions, boolean codelists typically only contain
trueandfalsevalues. While the current implementation allows flexibility for single-value boolean lists (as seen in the example here), it might be worth adding a comment explaining this design decision.Would you like me to help draft documentation that explains the rationale behind allowing flexible boolean codelists?
Description
addEmptyCodeListItemto return'',0orfalse, depending on the previous item's type.Not in scope for this PR:
Related Issue(s)
Verification
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests
Refactor