fix: replace Combobox component for selecting system with Suggestion - #1602
fix: replace Combobox component for selecting system with Suggestion#1602mgunnerud wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughIntroduces DigDir design-system Suggestion component for selecting a registered system, updating types from RegisteredSystem to SuggestionItem across related components. Adds a new CSS class for subtle description text. Adds a new runtime dependency. Updates localization files with an “empty_option” key. Adjusts data mapping to use value/label. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant UI as CreateSystemUserPage
participant Sel as SelectRegisteredSystem (Suggestion)
participant API as Backend API
U->>Sel: Type to search systems
Sel-->>Sel: Filter options (client-side substring)
Sel->>U: Show suggestions / "empty_option" if none
U->>Sel: Choose SuggestionItem (value, label)
Sel->>UI: setSelectedSystem(SuggestionItem)
UI->>API: useGetRegisteredSystemRightsQuery(selectedSystem.value)
API-->>UI: Rights for system
U->>UI: Submit new system user
UI->>API: POST { systemId: value, integrationTitle: label, rights... }
API-->>UI: Created response / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
…inn-components instead)
![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 (4)
src/features/amUI/systemUser/CreateSystemUserPage/SelectRegisteredSystem.tsx (3)
77-88: Wire Label to input for a11y.Associate the Label with the input via htmlFor/id.
- <Field> - <Label>{t('systemuser_creationpage.pull_down_menu_label')}</Label> + <Field> + <Label htmlFor='registered-system-input'>{t('systemuser_creationpage.pull_down_menu_label')}</Label> <Suggestion multiple={false} selected={selectedSystem ?? EMPTY_SUGGESTION_ITEM} onSelectedChange={onSelectSystem} filter={({ text, input }) => { - return text.toLowerCase().includes(input.value.toLowerCase()); + return text.toLocaleLowerCase().includes(input.value.trim().toLocaleLowerCase()); }} > - <Suggestion.Input placeholder={t('systemuser_creationpage.choose')} /> + <Suggestion.Input id='registered-system-input' placeholder={t('systemuser_creationpage.choose')} />
28-28: Avoid sentinel item if library accepts undefined selected.If allowed, pass selected={selectedSystem} and drop EMPTY_SUGGESTION_ITEM to reduce edge‑cases.
-const EMPTY_SUGGESTION_ITEM: SuggestionItem = { value: '', label: '' }; ... - selected={selectedSystem ?? EMPTY_SUGGESTION_ITEM} + selected={selectedSystem}
74-108: Error UX: consider early return on load error.Showing an empty Suggestion alongside an error can confuse users; gate the field behind the error.
- <div className={classes.inputContainer}> - {isLoadingRegisteredSystems ? ( + <div className={classes.inputContainer}> + {isLoadingRegisteredSystems ? ( <DsSpinner aria-label={t('systemuser_creationpage.loading_systems')} /> - ) : ( + ) : isLoadRegisteredSystemsError ? null : ( <Field> <Label htmlFor='registered-system-input'>{t('systemuser_creationpage.pull_down_menu_label')}</Label> ... </Field> )} {isLoadRegisteredSystemsError && ( <DsAlert data-color='danger'> {t('systemuser_creationpage.load_vendors_error')} </DsAlert> )}src/features/amUI/systemUser/CreateSystemUserPage/RightsIncluded.tsx (1)
52-58: Handle rejected promise from unwrap to avoid console noise.Add a no‑op catch so rejections are fully handled by RTKQ error state.
- postNewSystemUser(postObjekt) - .unwrap() - .then((newSystemUser: { id: string }) => { + postNewSystemUser(postObjekt) + .unwrap() + .then((newSystemUser: { id: string }) => { navigate(`/${SystemUserPath.SystemUser}/${SystemUserPath.Overview}`, { state: { createdId: newSystemUser.id }, }); - }); + }) + .catch(() => { /* handled via createSystemUserError */ });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
package.json(1 hunks)src/features/amUI/systemUser/CreateSystemUserPage/CreateSystemUser.module.css(1 hunks)src/features/amUI/systemUser/CreateSystemUserPage/CreateSystemUserPage.tsx(1 hunks)src/features/amUI/systemUser/CreateSystemUserPage/RightsIncluded.tsx(3 hunks)src/features/amUI/systemUser/CreateSystemUserPage/SelectRegisteredSystem.tsx(3 hunks)src/localizations/en.json(2 hunks)src/localizations/no_nb.json(2 hunks)src/localizations/no_nn.json(2 hunks)
⏰ 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). (2)
- GitHub Check: Continuous Integration / Test & Build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (11)
src/localizations/en.json (2)
548-549: LGTM: keys will be used by Suggestion.Matches SelectRegisteredSystem usage for loading and empty states.
291-291: Resolved — safe rendering via Trans. The translation contains an tag but AccessPackageInfoAlert uses react‑i18next with a mapped anchor (target='_blank' rel='noopener noreferrer'), so there’s no dangerouslySetInnerHTML and no change needed.src/localizations/no_nn.json (2)
545-546: LGTM: loading/empty keys added.
286-287: Verify safe rendering for 'info_alert_text' (contains HTML)src/localizations/no_nn.json — info_alert_text includes an tag; repo has DOMPurify, html-react-parser and react-i18next but I could not locate where this key is rendered. Confirm the rendering path uses a safe option: either react-i18next with allowed tags or parse+sanitize (html-react-parser + DOMPurify) before injecting. Do not render via unsanitized dangerouslySetInnerHTML.
src/features/amUI/systemUser/CreateSystemUserPage/CreateSystemUser.module.css (1)
20-22: LGTM: subtle text token is appropriate.src/localizations/no_nb.json (2)
547-548: LGTM: loading/empty keys added and wired.
289-289: HTML in translations — ensure safe renderingsrc/localizations/no_nb.json (≈ line 289) contains a raw tag; render via a safe translator (e.g., react-i18next Trans) or sanitize/replace with parameterized placeholders — avoid dangerouslySetInnerHTML.
src/features/amUI/systemUser/CreateSystemUserPage/RightsIncluded.tsx (1)
73-81: LGTM: header now uses label from SuggestionItem.src/features/amUI/systemUser/CreateSystemUserPage/CreateSystemUserPage.tsx (1)
15-16: LGTM: state type migrated to SuggestionItem.Flow control guards prevent RightsIncluded from rendering without a selection.
package.json (1)
25-25: Approve — @digdir/designsystemet-react@^1.5.0 is compatible with React 19peerDependencies: ">=18.3.1 || ^19.0.0"; package.json uses react/react-dom "^19.1.1"; yarn.lock contains @digdir/designsystemet-react@^1.5.0 — no unmet peer deps or lockfile mismatch found.
src/features/amUI/systemUser/CreateSystemUserPage/SelectRegisteredSystem.tsx (1)
21-24: Experimental import — verify EXPERIMENTAL_Suggestion API (v1.5.0)EXPERIMENTAL_Suggestion is used here with props selected, onSelectedChange and filter — confirm those prop names/signatures against @digdir/designsystemet-react v1.5.0 type declarations (index.d.ts) or docs; if the API is unstable, pin a compatible version or add a local wrapper/typing.
Location: src/features/amUI/systemUser/CreateSystemUserPage/SelectRegisteredSystem.tsx (import at lines 21–24).
1 participant
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Localization
Chores