Fix/3646 read only list #3647
Fix/3646 read only list #3647
Conversation
📝 WalkthroughWalkthroughDerives a selection mode from item dataModelBindings in ListComponent and uses it to suppress selection controls and row-click behavior when bindings are missing (readonly). Adds tests verifying no radios/checkboxes render and selection does not persist when dataModelBindings are undefined or empty (desktop and mobile). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/layout/List/ListComponent.tsx (1)
335-336: Nit: class condition already excludes readOnly.
[classes.selectedRowCell]: isRowSelected(row) && !readOnlyis fine. Just make sure the readOnly visual state is handled by.readOnlyRow(after enabling CSS). No change required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/layout/List/ListComponent.module.css(1 hunks)src/layout/List/ListComponent.test.tsx(2 hunks)src/layout/List/ListComponent.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/List/ListComponent.test.tsxsrc/layout/List/ListComponent.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/layout/List/ListComponent.test.tsx
**/*.module.css
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and follow existing patterns in
*.module.cssfiles
Files:
src/layout/List/ListComponent.module.css
🧬 Code graph analysis (2)
src/layout/List/ListComponent.test.tsx (1)
src/__mocks__/getLayoutSetsMock.ts (1)
defaultDataTypeMock(3-3)
src/layout/List/ListComponent.tsx (2)
src/features/language/Lang.tsx (1)
Lang(15-23)src/components/form/RadioButton.tsx (1)
RadioButton(24-104)
⏰ 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: Analyze (javascript)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (5)
src/layout/List/ListComponent.test.tsx (1)
228-325: LGTM: solid read-only coverage (desktop + mobile).Good assertions to ensure no controls render and clicks don’t trigger data updates while content remains visible.
src/layout/List/ListComponent.tsx (4)
52-53: LGTM: adding readOnly prop to item config.Matches the new behavior tested and gates interactions elsewhere.
100-102: LGTM: short-circuit row clicks in readOnly.Prevents accidental state changes when readOnly.
268-275: LGTM: hide controls header cell in readOnly.Keeps column alignment consistent when the control column is omitted.
297-299: Missing style: readOnlyRow has no effect unless CSS is enabled.
className={cn({ [classes.readOnlyRow]: readOnly })}relies on.readOnlyRowwhich is commented out in the CSS module. Enable the CSS (see CSS review) or drop the class reference.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/layout/List/ListComponent.module.css (1)
108-111: Uncomment or remove dead hover rule.The
readOnlyRow:hoverrule is still commented, so hover style remains unchanged. Either enable it or delete to avoid confusion.-/*.readOnlyRow:hover {*/ -/* background-color: initial;*/ -/*}*/ +.readOnlyRow:hover { + background-color: initial; +}src/layout/List/ListComponent.test.tsx (1)
214-216: Fix stale comment (now selecting by accessible name).The code uses an accessible name, but the comment says the label is empty.
- // Select the second row - find by value since label is empty + // Select the second row by accessible namesrc/layout/List/ListComponent.tsx (2)
179-194: Mobile (checkbox): a11y + interaction fixed.Switched to
onChange, added a visually-hidden label, and preserved checked/value. Matches prior guidance.
215-229: Mobile (radio): a11y + interaction fixed.Uses
onChangeand provides an accessible label. Consistent with checkbox path.
🧹 Nitpick comments (3)
src/layout/List/ListComponent.test.tsx (2)
228-257: Read-only assertions cover core cases. Add a header-cell assertion for completeness.Great coverage ensuring no radios/checkboxes and no row selection. Consider also asserting that the controls header cell is omitted in readOnly (desktop).
Example (can be placed in this describe block):
it('should not render the controls header cell in readOnly mode', async () => { await render({ component: { readOnly: true } }); await waitFor(() => expect(screen.getByText('Norway')).toBeInTheDocument()); expect(screen.queryByText(/list_component\.controlsHeader/i)).not.toBeInTheDocument(); });
274-289: Restore spies after mobile tests to avoid leakage.
jest.spyOn(useDeviceWidths, 'useIsMobile').mockReturnValue(true);is not restored. Add a globalafterEach(jest.restoreAllMocks)to prevent cross-test interference.afterEach(() => { jest.restoreAllMocks(); });Also applies to: 291-306
src/layout/List/ListComponent.tsx (1)
297-299: Expose non-interactive state to AT.Optionally add
aria-disabledon rows when readOnly to communicate the non-interactive state.- <Table.Row + <Table.Row key={JSON.stringify(row)} onClick={!readOnly ? () => handleRowClick(row) : undefined} + aria-disabled={readOnly || undefined} className={cn({ [classes.readOnlyRow]: readOnly })} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/layout/List/ListComponent.module.css(1 hunks)src/layout/List/ListComponent.test.tsx(2 hunks)src/layout/List/ListComponent.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/List/ListComponent.test.tsxsrc/layout/List/ListComponent.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/layout/List/ListComponent.test.tsx
**/*.module.css
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and follow existing patterns in
*.module.cssfiles
Files:
src/layout/List/ListComponent.module.css
🧬 Code graph analysis (2)
src/layout/List/ListComponent.test.tsx (1)
src/__mocks__/getLayoutSetsMock.ts (1)
defaultDataTypeMock(3-3)
src/layout/List/ListComponent.tsx (2)
src/features/language/Lang.tsx (1)
Lang(15-23)src/components/form/RadioButton.tsx (1)
RadioButton(24-104)
🔇 Additional comments (8)
src/layout/List/ListComponent.module.css (3)
104-107: Good: non-interactive row cursor for readOnly.
tr.readOnlyRowcorrectly communicates non-interactive state via cursor.
116-131: Mobile layout helpers look correct.
mobileContentandmobileControlalign with TSX usage and improve structure on small screens.
112-114: Remove unused.readOnlyCellrule.Only occurrence is its definition (no references found). Eliminate this dead CSS:
-.readOnlyCell { - background-color: initial; -}src/layout/List/ListComponent.tsx (5)
99-103: Good: short-circuit interactions in readOnly.Early return prevents any mutation paths when
readOnlyis true.
268-274: Hide control header in readOnly.Omits the controls column in readOnly; avoids empty column and improves semantics.
301-329: Desktop controls: keyboard/toggle behavior looks correct.
- Checkbox uses
onChange={() => toggle(row)}and stops propagation viaonClick.- RadioButton uses
onChangeand stops propagation.- Removed unreachable
readOnlyCellclass usage.
334-335: Selection styling gated by readOnly.Applying
selectedRowCellonly when not readOnly avoids misleading emphasis.
52-53: Public API: AddreadOnlyto the generated config and update docs
- No
config.generatedfile found undersrc/layout/List; ensure the component’s generated config includes a typedreadOnlyprop (e.g. inconfig.generated.ts) and regenerate.- Reflect the new
readOnlysetting in Studio documentation and examples.
adamhaeger
added
kind/bug
|
/publish |
1 similar comment
|
/publish |
PR release:
|
PR release:
|
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 (1)
src/layout/List/ListComponent.tsx (1)
126-134: Sort toggle starts at descending; make cycle none → ascending → descending → none.Current logic initializes new columns to descending. Adjust to expected UX.
- const handleSort = (key: string) => { - if (sortColumn === key && sortDirection === 'ascending') { - setSortColumn(undefined); - setSortDirection(undefined); - } else { - setSortColumn(key); - setSortDirection(sortColumn === key && sortDirection === 'descending' ? 'ascending' : 'descending'); - } - }; + const handleSort = (key: string) => { + if (sortColumn !== key) { + setSortColumn(key); + setSortDirection('ascending'); + return; + } + if (sortDirection === 'ascending') { + setSortDirection('descending'); + return; + } + // descending -> none + setSortColumn(undefined); + setSortDirection(undefined); + };
🧹 Nitpick comments (3)
src/layout/List/ListComponent.module.css (1)
104-106: Also reset hover state for read-only rows.Avoid hover highlight in read-only mode to prevent an interactive affordance.
.listTable tr.readOnlyRow { cursor: default; } + +.listTable tr.readOnlyRow:hover { + background-color: initial; +}src/layout/List/ListComponent.test.tsx (1)
214-216: Fix stale comment (now using accessible name).The radio has an accessible label; update the comment.
- // Select the second row - find by value since label is empty + // Select the second row by accessible namesrc/layout/List/ListComponent.tsx (1)
174-209: Remove redundant readOnly checks in the mobile branch.This block only renders when
isMobile && !readOnly; inner!readOnlyguards are dead.- {!readOnly && ( - <Checkbox + <Checkbox className={cn(classes.mobileControl)} {...getCheckboxProps({ value: JSON.stringify(row) })} onChange={() => handleRowClick(row)} value={JSON.stringify(row)} checked={isChecked(row)} label={<span className={utilClasses.visuallyHidden}>{getRowLabel(row)}</span>} - /> - )} + /> @@ - {!readOnly && ( - <Radio + <Radio {...getRadioProps({ value: JSON.stringify(row) })} value={JSON.stringify(row)} className={classes.mobileControl} onChange={() => handleSelectedRadioRow({ selectedValue: row })} label={<span className={utilClasses.visuallyHidden}>{getRowLabel(row)}</span>} - /> - )} + />Also applies to: 229-244
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/layout/List/ListComponent.module.css(1 hunks)src/layout/List/ListComponent.test.tsx(2 hunks)src/layout/List/ListComponent.tsx(9 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/List/ListComponent.test.tsxsrc/layout/List/ListComponent.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/layout/List/ListComponent.test.tsx
**/*.module.css
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and follow existing patterns in
*.module.cssfiles
Files:
src/layout/List/ListComponent.module.css
🧬 Code graph analysis (1)
src/layout/List/ListComponent.tsx (5)
src/features/formData/useDataModelBindings.ts (1)
useDataModelBindings(42-57)src/features/saveToGroup/useSaveToGroup.ts (1)
useSaveObjectToGroup(108-136)src/layout/ComponentStructureWrapper.tsx (1)
ComponentStructureWrapper(20-48)src/features/language/Lang.tsx (1)
Lang(15-23)src/components/form/RadioButton.tsx (1)
RadioButton(24-104)
⏰ 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: Install
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (4)
src/layout/List/ListComponent.module.css (2)
100-102: LGTM: mobile layout helpers and control sizing.
mobileControlandmobileContentstyles align with the TSX structure and improve mobile readability.Also applies to: 112-117, 119-123
108-110: Delete unused.readOnlyCelldefinition insrc/layout/List/ListComponent.module.css(lines 108–110); it isn’t referenced anywhere.src/layout/List/ListComponent.test.tsx (1)
228-292: Restore spies after each test to prevent cross-test leakage.jest.clearAllMocks()doesn’t restore mocked implementations; addjest.restoreAllMocks()in anafterEach.describe('ListComponent', () => { beforeEach(() => { jest.clearAllMocks(); jest.useRealTimers(); }); + afterEach(() => { + jest.restoreAllMocks(); + });src/layout/List/ListComponent.tsx (1)
309-357: Optional: replaceJSON.stringify(row)with a stable unique key
Usingkey={JSON.stringify(row)}can be slow and may break if object property order changes. If eachrowhas a unique identifier (for example anidfield), usekey={row.id}instead; otherwise, verify that your data model guarantees consistent ordering of properties or consider falling back to the item index.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/layout/List/ListComponent.tsx (3)
92-101: Don’t copy reserved keys into form data; compute selection by value keys.Current logic includes
group/checkedin comparisons andsetValues, which can break matching and write invalid data.- const selectedRow = !isMultipleSelection - ? (data?.listItems.find((row) => Object.keys(formData).every((key) => row[key] === formData[key])) ?? '') - : ''; + const valueKeys = getValueBindingKeys(bindings); + const selectedRow = !isMultipleSelection + ? (data?.listItems.find((row) => + valueKeys.every((key) => row[key] === (formData as Record<string, unknown>)[key]), + ) ?? '') + : ''; @@ function handleSelectedRadioRow({ selectedValue }: { selectedValue: Row }) { const next: Row = {}; - for (const binding of Object.keys(bindings)) { - next[binding] = selectedValue[binding]; - } + for (const key of valueKeys) { + next[key] = selectedValue[key]; + } setValues(next); }
104-109: Use value-key equality instead of JSON.stringify for selection.Stringifying rows is brittle and order-dependent.
- function isRowSelected(row: Row): boolean { - if (isMultipleSelection) { - return isChecked(row); - } - return JSON.stringify(selectedRow) === JSON.stringify(row); - } + function isRowSelected(row: Row): boolean { + if (isMultipleSelection) return isChecked(row); + return valueKeys.every((key) => row[key] === (formData as Record<string, unknown>)[key]); + }
68-69: Type error: assigning undefined to aria-sort-typed state/vars.
AriaAttributes['aria-sort']does not includeundefined, yet you assign it. Update types to allowundefined.-const [sortDirection, setSortDirection] = useState<AriaAttributes['aria-sort']>('none'); +const [sortDirection, setSortDirection] = useState<AriaAttributes['aria-sort'] | undefined>('none'); @@ - if (sortColumn === key && sortDirection === 'ascending') { + if (sortColumn === key && sortDirection === 'ascending') { setSortColumn(undefined); setSortDirection(undefined); } else { setSortColumn(key); setSortDirection(sortColumn === key && sortDirection === 'descending' ? 'ascending' : 'descending'); } @@ - let sort: AriaAttributes['aria-sort'] = undefined; + let sort: AriaAttributes['aria-sort'] | undefined = undefined;Also applies to: 126-133, 280-283
♻️ Duplicate comments (3)
src/layout/List/ListComponent.tsx (3)
174-206: Mobile (checkbox): use onChange, not onClick, for keyboard accessibility.Same as prior review; Space/Enter won’t toggle reliably via onClick. Switch to onChange.
- <Checkbox + <Checkbox key={JSON.stringify(row)} className={cn(classes.mobile)} {...getCheckboxProps({ value: JSON.stringify(row) })} - onClick={() => handleRowClick(row)} + onChange={() => handleRowClick(row)} value={JSON.stringify(row)} checked={isChecked(row)} label={renderListItems(row, tableHeaders)} />
224-233: Mobile (radio): use onChange for accessibility; avoid onClick.Matches the checkbox path and DS expectations.
- {data?.listItems.map((row) => ( - <Radio + {data?.listItems.map((row) => ( + <Radio key={JSON.stringify(row)} {...getRadioProps({ value: JSON.stringify(row) })} value={JSON.stringify(row)} className={cn(classes.mobile, { [classes.selectedRow]: isRowSelected(row) })} - onClick={() => handleSelectedRadioRow({ selectedValue: row })} + onChange={() => handleSelectedRadioRow({ selectedValue: row })} label={renderListItems(row, tableHeaders)} /> ))}
37-47: Derive selection mode from value-binding keys only (exclude 'group'/'checked').Otherwise a component with only
group/checkedpresent won’t enter readonly mode as intended.type Row = Record<string, string | number | boolean>; -type SelectionMode = 'readonly' | 'single' | 'multiple'; +type SelectionMode = 'readonly' | 'single' | 'multiple'; + +function getValueBindingKeys(bindings: IDataModelBindingsForList): string[] { + return (Object.keys(bindings) as Array<keyof IDataModelBindingsForList>) + .filter((k) => k !== 'group' && k !== 'checked' && Boolean(bindings[k])); +} -function getSelectionMode(bindings: IDataModelBindingsForList): SelectionMode { - const hasValidBindings = Object.keys(bindings).length > 0 && Object.values(bindings).some((b) => b !== undefined); +function getSelectionMode(bindings: IDataModelBindingsForList): SelectionMode { + const hasValidBindings = getValueBindingKeys(bindings).length > 0; if (!hasValidBindings) { return 'readonly'; } return bindings.group ? 'multiple' : 'single'; }
🧹 Nitpick comments (1)
src/layout/List/ListComponent.tsx (1)
195-197: Nit: JSON.stringify as React key/value can be fragile.Prefer a stable id from the row (e.g.,
row.id) if available to avoid collisions and heavy stringification.Also applies to: 224-229, 299-319
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/layout/List/ListComponent.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/List/ListComponent.tsx
🧬 Code graph analysis (1)
src/layout/List/ListComponent.tsx (4)
src/features/formData/useDataModelBindings.ts (1)
useDataModelBindings(42-57)src/features/saveToGroup/useSaveToGroup.ts (1)
useSaveObjectToGroup(108-136)src/layout/ComponentStructureWrapper.tsx (1)
ComponentStructureWrapper(20-48)src/components/form/RadioButton.tsx (1)
RadioButton(24-104)
⏰ 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: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
🔇 Additional comments (7)
src/layout/List/ListComponent.tsx (7)
80-84: LGTM: Clear flags from selection mode.
86-86: LGTM: Using toggle/isChecked only.
114-124: LGTM: Click handler respects readOnly and routes by mode.
271-277: LGTM: Hide controls header in readOnly.
300-302: LGTM: Row click disabled in readOnly and styled distinctly.
303-331: LGTM: Desktop controls use onChange and stop propagation.
337-338: LGTM: Avoid selected styling in readOnly.
|
Description
Reintroduces readOnly support for list component.
List without datamodelbindings now shows a table without checkboxes or radiobuttons.
Result:
Related Issue(s)
Verification/QA
Created task: Altinn/altinn-studio-docs#2333
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores