feat(header): add new TEDI-Ready component #506#602
Open
ly-tempel-bitweb wants to merge 7 commits intorcfrom
Open
feat(header): add new TEDI-Ready component #506#602ly-tempel-bitweb wants to merge 7 commits intorcfrom
ly-tempel-bitweb wants to merge 7 commits intorcfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete responsive Header system (Header + Logo/Center/Actions) with multiple new subcomponents (Language, Login, Logout, Profile, Role, Search, MobileButton), breakpoint visibility utilities (ShowAt/HideAt), popover border/context support, label-provider locale state, numerous SCSS modules, Storybook stories, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Header as HeaderComponent
participant Breakpoints as BreakpointUtils
participant LabelProv as LabelProvider
participant Popover as Popover
participant Dialog as Dialog
User->>Header: mount / interact (click profile/language/search/role)
Header->>Breakpoints: query breakpoint (useBreakpoint/isBreakpointBelow)
alt Language selection (popover)
User->>Header: open language selector
Header->>Popover: open popover
Popover->>LabelProv: call setLocale(newLocale)
LabelProv->>Header: locale updated -> rerender labels
else Profile on desktop (popover)
User->>Header: click profile
Header->>Popover: open popover -> render content
else Profile on mobile (dialog)
User->>Header: click profile
Header->>Dialog: showModal() -> dialog open
User->>Dialog: press Escape/click overlay
Dialog->>Header: close -> Header restores focus
end
sequenceDiagram
actor User
participant HeaderRole
participant Breakpoints as BreakpointUtils
participant Reps as RepresentativesList
participant Search as SearchInput
participant Callback as onRepresentativeChange
User->>HeaderRole: mount with representatives
HeaderRole->>Breakpoints: determine mode (popover vs accordion)
alt Popover mode (desktop)
User->>HeaderRole: click trigger
HeaderRole->>Reps: show list in popover
else Accordion mode (tablet/mobile)
User->>HeaderRole: click accordion toggle
HeaderRole->>Search: render search input
User->>Search: type query
Search->>Reps: filter results
end
User->>Reps: select representative
Reps->>Callback: invoke onRepresentativeChange(selected)
Callback->>HeaderRole: update selected -> UI updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ly-tempel-bitweb
temporarily deployed
to
github-pages
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ly-tempel-bitweb
temporarily deployed
to
github-pages
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/tedi/components/overlays/popover/popover-content.tsx (1)
38-68:⚠️ Potential issue | 🟡 MinorGuard the width modifier when
widthis"none"and adddisplayNameto the exported component.The
'none'width value does not have a corresponding CSS module class—the SCSS file only generates modifiers for'small','medium', and'large'. Whenwidth="none", the code tries to usestyles['tedi-popover--none'], which is undefined. While the classnames library handles undefined gracefully, this is semantically imprecise. Additionally, exported components must have adisplayNameproperty set per the TEDI guidelines.Proposed fixes
const { withBorder } = useContext(PopoverContext); const titleId = useId(); const hasDescription = Boolean(children); const descriptionId = useId(); + const widthClassName = width === 'none' ? undefined : styles[`tedi-popover--${width}`]; return ( <OverlayContent classNames={{ content: cn( styles['tedi-popover'], - { [styles[`tedi-popover--${width}`]]: width, [styles['tedi-popover--border']]: withBorder }, + widthClassName && { [widthClassName]: true }, + { [styles['tedi-popover--border']]: withBorder }, className ), arrow: cn(styles['tedi-popover__arrow'], { [styles['tedi-popover__arrow--border']]: withBorder }),Add after line 93:
}; +PopoverContent.displayName = 'PopoverContent';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/overlays/popover/popover-content.tsx` around lines 38 - 68, The PopoverContent component currently tries to use styles['tedi-popover--none'] when width === 'none' even though no CSS class exists and also lacks a displayName; update the classNames call in PopoverContent to only append the width modifier when width !== 'none' (e.g., conditionally apply styles[`tedi-popover--${width}`] only for 'small'|'medium'|'large') so no undefined style key is referenced, and after the component definition set PopoverContent.displayName = 'PopoverContent'.src/tedi/components/overlays/popover/popover.tsx (1)
13-46:⚠️ Potential issue | 🟠 MajorKeep
arrowPaddingowned byPopover.
PopoverPropsstill acceptsarrowPadding, and{...rest}comes after the computed value, so callers can accidentally override the padding required bywithBorder.🐛 Proposed fix
-export interface PopoverProps extends Omit<OverlayProps, 'arrowDimensions' | 'openWith' | 'offset'> { +export interface PopoverProps extends Omit<OverlayProps, 'arrowDimensions' | 'openWith' | 'offset' | 'arrowPadding'> { @@ <Overlay + {...rest} arrowDimensions={{ width: ARROW_WIDTH, height: ARROW_HEIGHT, }} arrowPadding={withBorder ? ARROW_PADDING_BORDERED : ARROW_PADDING_DEFAULT} openWith={openWith} role="dialog" - {...rest} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/overlays/popover/popover.tsx` around lines 13 - 46, Popover currently allows callers to override arrowPadding because PopoverProps didn't omit 'arrowPadding' and the JSX spreads {...rest} after the computed arrowPadding; fix by adding 'arrowPadding' to the Omit in the PopoverProps declaration (Omit<OverlayProps, 'arrowDimensions' | 'openWith' | 'offset' | 'arrowPadding'>) and move the {...rest} spread in the Popover component so it is applied before the explicit arrowPadding prop (ensure Popover sets arrowPadding based on withBorder after spreading rest), keeping Popover, PopoverProps, arrowPadding, withBorder and {...rest} as the reference points.src/tedi/providers/label-provider/label-provider.tsx (1)
89-121:⚠️ Potential issue | 🟠 Major
currentLocaledoes not sync when thelocaleprop changes.
currentLocaleis initialized fromlocaleonly on mount. If a consumer passes a newlocaleprop (e.g., controlled from app state), the provider will ignore it and keep the previous value, which will silently desync labels, dayjs, and MUIadapterLocale. Consider syncing prop → state via effect, or deriving the effective locale assetLocale-overrides-prop.♻️ Suggested sync
const [currentLocale, setCurrentLocale] = React.useState<TediLanguage>(locale); + + React.useEffect(() => { + setCurrentLocale(locale); + }, [locale]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/providers/label-provider/label-provider.tsx` around lines 89 - 121, The component initializes currentLocale from the locale prop but never updates it when locale changes; add a React.useEffect that watches the locale prop and calls setCurrentLocale(locale) to keep currentLocale in sync (so mergedLabels, dayjs locale effect, and any MUI adapterLocale stay consistent). Update any memo/deps if needed to include locale/currentLocale symmetrically (mergedLabels depends on currentLocale already) and ensure this effect runs before the dayjs effect by keeping both effects dependent on currentLocale.
🟡 Minor comments (23)
src/tedi/components/buttons/button-content/button-content.tsx-38-38 (1)
38-38:⚠️ Potential issue | 🟡 MinorClarify the
colorprop note.“PS” and “link type links” read awkwardly in API docs; consider using a direct note.
Proposed wording
- * Color schema for button. PS 'text' works only with link type links. + * Color schema for button. Note: 'text' works only with link-type links.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/buttons/button-content/button-content.tsx` at line 38, The JSDoc for the color prop in the ButtonContent component uses awkward phrasing ("PS" and "link type links"); update the comment for the color prop (referencing the color prop on ButtonContent) to a clear, direct sentence such as: "Color scheme for the button. The 'text' value applies only when type is 'link'." Replace the existing line "* Color schema for button. PS 'text' works only with link type links." with that clearer note in the JSDoc or prop comment near ButtonContent's prop/type definition.src/tedi/components/layout/header/components/header-search/header-search.module.scss-20-31 (1)
20-31:⚠️ Potential issue | 🟡 MinorMake the modal body fill the remaining height, not the full modal height.
&-body { height: 100%; }is added below a heading, so the combined heading + body height exceeds100dvh. Use a flex column modal and let the body flex into the remaining space.Proposed fix
.tedi-header-search__modal { position: fixed; top: 0; left: 0; z-index: var(--z-index-header); + display: flex; + flex-direction: column; width: 100%; @@ &-body { - height: 100%; + flex: 1 1 auto; + min-height: 0; + overflow: auto; padding: var(--modal-body-padding); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-search/header-search.module.scss` around lines 20 - 31, The modal body currently uses height: 100% which, combined with &-heading, can exceed the viewport; change the modal container to a column flex layout and make &-body flex into the remaining space (e.g., use flex-direction: column on the modal wrapper and replace height: 100% in &-body with a flex-grow/flex shorthand and appropriate overflow handling) so the heading size is respected and the body fills only the remaining space; update styles around the &-heading and &-body selectors to implement this behavior.src/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.module.scss-26-46 (1)
26-46:⚠️ Potential issue | 🟡 MinorPrevent disabled buttons from entering hover/active visual states.
The
:hover/:activeselectors are more specific than&--disabled, so disabled buttons can still display hover/active colors. Gate interactive states and reset the disabled cursor.Proposed fix
- &:hover { + &:not(:disabled):not([aria-disabled='true']):not(.tedi-header-mobile-button--disabled):hover { color: var(--header-mobile-button-text-hover); } - &:active { + &:not(:disabled):not([aria-disabled='true']):not(.tedi-header-mobile-button--disabled):active { color: var(--header-mobile-button-text-active); } @@ &--disabled { color: var(--header-mobile-button-text-disabled); + cursor: default; background: var(--header-mobile-button-background-disabled); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.module.scss` around lines 26 - 46, Hover/active styles are currently applied regardless of the disabled modifier because those selectors are more specific than &--disabled; update the interactive selectors so they only apply when the disabled modifier is not present (e.g., change &:hover and &:active to a not-disabled variant such as &:not(.<your-component-class>--disabled):hover / &:not(.<your-component-class>--disabled):active or the SCSS equivalent using &--disabled) and add cursor: not-allowed (or cursor: default) and optionally reset hover/active visual properties inside &--disabled so disabled buttons do not change color/appearance on pointer interactions; target the &--disabled selector (and focus-visible if needed) to enforce the disabled cursor and reset styles.src/tedi/components/layout/header/components/header-role/header-role.module.scss-98-108 (1)
98-108:⚠️ Potential issue | 🟡 MinorFix the accordion modifier selector typo.
&---has-representativescompiles totedi-header-role__accordion---has-representatives; the BEM modifier should be--has-representatives.🎨 Proposed fix
- &---has-representatives { + &--has-representatives { gap: var(--layout-grid-gutters-16); justify-content: space-between; }As per coding guidelines, “SCSS files must follow BEM naming convention, enforced by Stylelint”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-role/header-role.module.scss` around lines 98 - 108, The BEM modifier for the accordion is mistyped as &---has-representatives (producing tedi-header-role__accordion---has-representatives); change it to the correct modifier &--has-representatives inside the &__accordion block so it compiles to tedi-header-role__accordion--has-representatives and conforms to the Stylelint BEM rule.src/tedi/components/layout/hide-at.tsx-12-17 (1)
12-17:⚠️ Potential issue | 🟡 MinorAvoid hiding everything while the breakpoint is unknown.
When
currentisnull,isBreakpointBelow(current, bp)returnsfalse, so!isBreakpointBelow(...)becomestrueandHideAt mdhides content before a breakpoint is resolved. Treat unknown breakpoint as “no match” instead.🐛 Proposed fix
const current = useBreakpoint(); const shouldHide = Object.entries(breakpoints).some(([bp, value]) => { if (!value) return false; + if (!current) return false; return !isBreakpointBelow(current, bp as Breakpoint); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/hide-at.tsx` around lines 12 - 17, The hide logic in the HideAt component sets shouldHide by calling isBreakpointBelow with current which can be null, causing everything to be hidden; update the predicate used to compute shouldHide (the Object.entries callback that references breakpoints, value, current and isBreakpointBelow) to treat an unknown breakpoint (current === null) as “no match” by returning false early when current is null (so the callback only returns !isBreakpointBelow(...) when current is non-null and value is truthy), ensuring shouldHide stays false until the breakpoint is resolved.src/tedi/components/layout/hide-at.spec.tsx-71-82 (1)
71-82:⚠️ Potential issue | 🟡 MinorFix the inverted breakpoint expectation.
With real breakpoint ordering,
current = "xxl"and<HideAt sm md>should hide becausexxlis at-or-above bothsmandmd. This test currently proves an impossible mocked state.💚 Proposed fix
- it('renders children when all specified breakpoints are below current', () => { - (useBreakpoint as jest.Mock).mockReturnValue('xxl'); + it('renders children when current breakpoint is below all specified breakpoints', () => { + (useBreakpoint as jest.Mock).mockReturnValue('xs'); (isBreakpointBelow as jest.Mock).mockReturnValue(true); render( <HideAt sm md>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/hide-at.spec.tsx` around lines 71 - 82, The test currently asserts visibility but sets up a real-world state where current breakpoint is 'xxl' and both sm/md are below it (mocks: useBreakpoint -> 'xxl', isBreakpointBelow -> true), so change the assertion to expect the content to be hidden: use queryByText('Visible content') and assert it is null or use expect(...).not.toBeInTheDocument(); update the test description (the it(...) string) to reflect that it should hide children when all specified breakpoints are below the current breakpoint; keep the mocks for useBreakpoint and isBreakpointBelow as-is and only change the query/assertion and test name for the HideAt spec.src/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.spec.tsx-15-80 (1)
15-80:⚠️ Potential issue | 🟡 MinorPrefer semantic RTL queries over DOM selectors.
Most of these assertions use
container.querySelector(...); please switch the user-facing cases toscreen.getByRole(...)/queryByRole(...)and derive class assertions from those elements where possible.💚 Example refactor
- it('renders as a Link when href is provided and not disabled', () => { - const { container } = render(<HeaderMobileButton icon="menu" label="Menu" href="/page" />); + it('renders as a Link when href is provided and not disabled', () => { + render(<HeaderMobileButton icon="menu" label="Menu" href="/page" />); - const link = container.querySelector('a'); - expect(link).toBeInTheDocument(); + const link = screen.getByRole('link', { name: /menu/i }); expect(link).toHaveAttribute('href', '/page'); - expect(container.querySelector('button')).not.toBeInTheDocument(); + expect(screen.queryByRole('button', { name: /menu/i })).not.toBeInTheDocument(); });As per coding guidelines, “Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries (getByTestId)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.spec.tsx` around lines 15 - 80, The tests in header-mobile-button.spec.tsx use container.querySelector(...) non-semantic selectors; update each assertion to use RTL semantic queries (e.g., replace container.querySelector('button') / container.querySelector('a') with screen.getByRole('button', { name: /Menu/i }) or screen.getByRole('link', { name: /Menu/i }) and use queryByRole when absence is expected), obtain the element from screen (or within if needed) and derive class assertions from that element's classList (e.g., check element.className or classList contains "header-mobile-button--selected"/"--disabled"), replace click tests to fireEvent.click on the element returned by getByRole, and for the icon test prefer an accessible query (e.g., getByLabelText or getByRole for the icon if HeaderMobileButton supplies an accessible name) instead of querying data attributes; update references in tests to use these semantic queries for HeaderMobileButton renderings.src/tedi/components/layout/header/components/header-login/header-login.spec.tsx-47-71 (1)
47-71:⚠️ Potential issue | 🟡 MinorUse role queries and avoid optional no-op clicks.
link?.click()does nothing when the selector misses, so the test failure points at the callback rather than the missing element. Query the rendered control semantically and click the required element.💚 Proposed test cleanup
-import { render } from '@testing-library/react'; +import { fireEvent, render, screen } from '@testing-library/react'; @@ it('renders with custom label', () => { @@ - const { container } = render(<HeaderLogin />); + render(<HeaderLogin />); - expect(container.textContent).toContain('Sign in'); + expect(screen.getByRole('link', { name: /sign in/i })).toBeInTheDocument(); }); it('renders with href', () => { - const { container } = render(<HeaderLogin href="/login" />); + render(<HeaderLogin href="/login" />); - const link = container.querySelector('a[href="/login"]'); - expect(link).toBeInTheDocument(); + expect(screen.getByRole('link')).toHaveAttribute('href', '/login'); }); it('handles onClick', () => { const onClick = jest.fn(); - const { container } = render(<HeaderLogin onClick={onClick} />); + render(<HeaderLogin href="/login" onClick={onClick} />); - const link = container.querySelector('a'); - link?.click(); + fireEvent.click(screen.getByRole('link')); expect(onClick).toHaveBeenCalled(); });As per coding guidelines, “Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries (getByTestId)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-login/header-login.spec.tsx` around lines 47 - 71, The test "handles onClick" uses an optional DOM call link?.click() which masks missing elements; update the test for HeaderLogin to use a semantic query (e.g., getByRole('link') or getByRole('button') depending on the component role) which will throw if the element is absent, then invoke an explicit click via fireEvent.click or userEvent.click and assert onClick was called; replace the optional chaining call with a guaranteed element from the rendered result so failures point to missing element instead of the callback.src/tedi/components/layout/header/header.module.scss-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorRemove the
var()fallback value.
box-shadow: 0 1px 5px 0 var(--tedi-alpha-20, rgb(0 0 0 / 20%));uses a fallback, which is disallowed. As per coding guidelines: "Do not use fallback values in CSSvar()functions — writevar(--token-name)instead ofvar(--token-name, fallback)".🐛 Proposed fix
- box-shadow: 0 1px 5px 0 var(--tedi-alpha-20, rgb(0 0 0 / 20%)); + box-shadow: 0 1px 5px 0 var(--tedi-alpha-20);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/header.module.scss` at line 4, Replace the box-shadow value that uses a fallback with a plain CSS variable reference: locate the declaration using box-shadow and the var(--tedi-alpha-20, rgb(0 0 0 / 20%)) fallback and change it to use var(--tedi-alpha-20) only, ensuring no fallback values remain (target symbol: --tedi-alpha-20 and the box-shadow property in header.module.scss).src/tedi/components/layout/header/components/header-login/header-login.tsx-26-68 (1)
26-68:⚠️ Potential issue | 🟡 MinorMissing
displayNameon exported component.
HeaderLoginis attached asHeader.Logininheader.tsxbut nodisplayNameis set on the component. As per coding guidelines: "All exported components must have adisplayNameproperty set" and "always setdisplayNameon exported components".🐛 Proposed fix
export const HeaderLogin = (props: HeaderLoginProps) => { ... }; + +HeaderLogin.displayName = 'Header.Login'; export default HeaderLogin;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-login/header-login.tsx` around lines 26 - 68, The exported component HeaderLogin lacks a displayName; set HeaderLogin.displayName = 'Header.Login' (or the desired display name used when attached as Header.Login) immediately after the HeaderLogin declaration/export so the component has an explicit displayName while keeping the default export unchanged.src/tedi/providers/label-provider/labels-map.ts-194-200 (1)
194-200:⚠️ Potential issue | 🟡 Minor
header.role-selection.search.organizationLabelreuses representative translations.The description says "Label for Organization Search", but
enandruare identical toheader.role-selection.search.label("Search representative" / "Найти представителя"). Onlyetdiffers. This looks like a copy/paste omission — please confirm the intended translations (likely "Search organization" / "Найти организацию").🐛 Suggested translations
'header.role-selection.search.organizationLabel': { description: 'Label for Organization Search in Role selection', components: ['HeaderRole'], et: 'Otsi asutust', - en: 'Search representative', - ru: 'Найти представителя', + en: 'Search organization', + ru: 'Найти организацию', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/providers/label-provider/labels-map.ts` around lines 194 - 200, The entry for key header.role-selection.search.organizationLabel mistakenly reuses representative translations; update the en and ru values for that key to the correct "Search organization" and "Найти организацию" (or confirm the intended phrasing) so the description "Label for Organization Search in Role selection" matches its translations; locate the object for header.role-selection.search.organizationLabel in labels-map.ts and replace the current en/ru strings accordingly.src/tedi/providers/label-provider/labels-map.ts-201-228 (1)
201-228:⚠️ Potential issue | 🟡 Minor
componentsarrays contain a single comma-joined string instead of multiple entries.
components: ['Header, HeaderLogin']is a one-element array with a literal comma. This should be two separate strings so that downstream tooling (docs generation, filtering by component) sees each component entry correctly. Same issue on lines 210, 217, 224.🐛 Proposed fix
- components: ['Header, HeaderLogin'], + components: ['Header', 'HeaderLogin'], ... - components: ['Header, HeaderLogin'], + components: ['Header', 'HeaderLogin'], ... - components: ['Header, HeaderLogout'], + components: ['Header', 'HeaderLogout'], ... - components: ['Header, HeaderLogout'], + components: ['Header', 'HeaderLogout'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/providers/label-provider/labels-map.ts` around lines 201 - 228, The components arrays for the labels 'header.login', 'header.login-small', 'header.logout', and 'header.logout-small' currently contain a single comma-joined string (e.g., "Header, HeaderLogin"); split these into separate array entries so each component is its own string (e.g., ['Header', 'HeaderLogin']) in the labels-map to ensure downstream tooling treats them as distinct components; update the components property for each of those four entries accordingly.src/tedi/components/layout/header/components/header-role/header-role.spec.tsx-215-225 (1)
215-225:⚠️ Potential issue | 🟡 MinorAssertion does not verify "closing" behavior.
The test is named "closes role selection via setIsRoleSelectionOpen when selecting in desktop popover without onRoleSelectionToggle" but only asserts that
Jane Smithis in the document — which is true regardless of whether the popover closed (the trigger label changes to Jane Smith after selection). Add an assertion that the popover content is gone, e.g. the previously-visible other representative (Bob Wilson) is no longer in the document, to actually validate the close behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-role/header-role.spec.tsx` around lines 215 - 225, The test for HeaderRole ("closes role selection via setIsRoleSelectionOpen when selecting in desktop popover without onRoleSelectionToggle") currently only checks that "Jane Smith" appears; update it to also assert the popover closed by verifying the other representative (e.g., "Bob Wilson") is no longer in the DOM after clicking janeButton — use a negative assertion via queryByText or expect(...).not.toBeInTheDocument() after the selection to confirm the popover content was removed.src/tedi/components/layout/header/components/header-logout/header-logout.tsx-50-58 (1)
50-58:⚠️ Potential issue | 🟡 Minor
Linkwithouthrefwhenhrefis undefined.The
isSmall === falsebranch always renders<Link href={href} ...>, even whenhrefis undefined (caller provided onlyonClick). Depending onLink's behavior this may render an<a>withouthref(not keyboard-focusable / not a real link). Consider rendering aButtonfallback for the no-hrefcase, mirroring the logic inHeaderMobileButton.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-logout/header-logout.tsx` around lines 50 - 58, The non-mobile branch always renders <Link href={href} ...> even when href is undefined, which can produce an anchor without href; change the rendering in header-logout.tsx so that when href is falsy you render the same Button fallback used by HeaderMobileButton (preserving onClick, Icon, Text, resolvedLabel and className styles['tedi-header-logout']), otherwise render Link with href; update the conditional around Link to check href and use Button when href is not provided to ensure correct semantics and keyboard behavior.src/tedi/components/layout/header/components/header-language/header-language.tsx-64-74 (1)
64-74:⚠️ Potential issue | 🟡 MinorDisplayed language can desync from the actual
locale.
languageis seeded once frominitialLabeland thereafter updated only insidechangeLanguage. If the app's locale changes from somewhere else (another language switcher, route-level locale change, etc.), the trigger will keep showing the stale label.Consider deriving the displayed label from
localeon each render instead of keeping it in local state — e.g.const language = availableLanguages.find(l => l.locale === locale)?.label ?? currentLanguage ?? availableLanguages[0]?.label ?? '';.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-language/header-language.tsx` around lines 64 - 74, The component seeds local state using initialLabel and then only updates it in changeLanguage, causing the displayed language (language / setLanguage / initialLabel) to desync when locale changes elsewhere; remove the derived local state and instead compute the label on each render from availableLanguages, locale and currentLanguage (e.g. replace the useState(initialLabel) and initialLabel IIFE with a derived const computed from availableLanguages.find(l => l.locale === locale)?.label ?? currentLanguage ?? availableLanguages[0]?.label ?? ''), and update any usages that referenced language to use this derived const while leaving changeLanguage to only perform the locale switch.src/tedi/components/layout/header/components/header-search/header-search.spec.tsx-94-95 (1)
94-95:⚠️ Potential issue | 🟡 MinorNon-semantic close-button lookup indicates a real a11y gap.
The test has to fall back to
dialog.querySelector('[class*="button-close"]')because the modal close<Button>inheader-search.tsxhas no accessible name (only a decorative<Icon name="close">withoutlabel). Once anaria-labelis added to the close button (see comment onheader-search.tsx), switch this to a semantic query likegetByRole('button', { name: /close/i }).As per coding guidelines: "Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-search/header-search.spec.tsx` around lines 94 - 95, The test is using a non-semantic query selector '[class*="button-close"]' because the modal close <Button> in header-search.tsx lacks an accessible name; add an aria-label (or accessible label prop) to the close button in header-search.tsx (e.g., aria-label="Close" on the Button/Icon in the HeaderSearch component) and then update the spec in header-search.spec.tsx to replace dialog.querySelector('[class*="button-close"]') with a semantic query like getByRole('button', { name: /close/i }) (or getByLabelText) to assert and click the close control.src/tedi/components/layout/header/components/header-search/header-search.tsx-42-55 (1)
42-55:⚠️ Potential issue | 🟡 MinorReset
modalOpenwhen dialog is conditionally unmounted, or add viewport/variant state to effect dependencies.When the breakpoint changes from mobile to desktop (unmounting the dialog),
modalOpenremainstrue. If the user then resizes back to mobile, the dialog remounts whilemodalOpenis stilltrue, but the effect doesn't re-run (dependency array is[modalOpen]), so the newly-mounted dialog is never opened viashowModal(). The modal state would be inconsistent with the DOM.Add the conditional mounting state to the dependency array (
isMobileView && mobileVariant === 'modal'), or resetmodalOpentofalsewhen the condition changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-search/header-search.tsx` around lines 42 - 55, The effect controlling dialogRef (dialogRef.current) uses only modalOpen in its dependency array, so when the dialog is unmounted by a breakpoint change (isMobileView/mobileVariant), modalOpen can remain true and a remounted dialog won't auto-open; update the effect to also depend on the mounting condition (isMobileView and mobileVariant === 'modal') or explicitly reset modalOpen to false when that condition becomes false—modify the useEffect that references dialogRef, modalOpen to include isMobileView and mobileVariant (or add a separate useEffect that sets setModalOpen(false) when !(isMobileView && mobileVariant === 'modal')) so dialog state and DOM stay consistent.src/tedi/components/layout/header/components/header-profile/header-profile.tsx-53-62 (1)
53-62:⚠️ Potential issue | 🟡 Minor
selected={modalOpen}is stale whenshowDropdownis set to a mobile breakpoint.With the default
showDropdown='lg'this is fine, but the prop is configurable per-breakpoint (HeaderProfileBreakpointProps). If a consumer setsshowDropdown='xs', at mobile viewportuseDropdownbecomestrueand the Popover owns open state viadropdownOpen, while the renderedHeaderMobileButtonstill bindsselectedtomodalOpen— which will never flip. Either gate the mobile button on!useDropdown, or deriveselectedfrom whichever open state is active (useDropdown ? dropdownOpen : modalOpen).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-profile/header-profile.tsx` around lines 53 - 62, The HeaderMobileButton's selected prop is tied to modalOpen even when useDropdown (computed via isBreakpointBelow and showDropdown / HeaderProfileBreakpointProps) hands control to the Popover; this leaves selected stale for breakpoint-configured dropdowns — update the button so selected reflects the active open state (use dropdownOpen when useDropdown is true, otherwise modalOpen) or only render HeaderMobileButton when !useDropdown; reference useDropdown, isBreakpointBelow, showDropdown, HeaderMobileButton, modalOpen, dropdownOpen, and HeaderProfileBreakpointProps to locate and change the logic.src/tedi/components/layout/header/components/header-role/header-role.tsx-108-156 (1)
108-156:⚠️ Potential issue | 🟡 MinorTablet branch renders the representatives list even when there is only one.
When
!hasMultipleRepresentatives, no toggle button is rendered (line 139 guard), but<HeaderRoleRepresentatives {...representativesProps} keepOpenOnSelect />is still mounted unconditionally at line 155. Depending onHeaderRoleRepresentativesbehaviour (which renders a search input + list for the single rep), users on tablet will see an interactive selector they can never collapse for a role that has no alternatives. Consider gating the list render onhasMultipleRepresentativesto match the desktop branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-role/header-role.tsx` around lines 108 - 156, The tablet branch always mounts HeaderRoleRepresentatives (with keepOpenOnSelect) even when there is only one representative, causing an un-collapsible selector; change the JSX so HeaderRoleRepresentatives is only rendered when hasMultipleRepresentatives is true (matching the desktop branch) and preserve passing representativesProps and keepOpenOnSelect when rendered; ensure the conditional uses the same guard as the toggle button (hasMultipleRepresentatives) and does not depend on isRoleSelectionOpen.src/tedi/components/layout/header/header.stories.tsx-45-68 (1)
45-68:⚠️ Potential issue | 🟡 MinorGlobally monkey-patching
localStorage.setItemhas cross-story side effects.Replacing
localStorage.setItemat the instance level for the duration of a story silently drops allsetItemcalls whose key matchesSTORAGE_KEY, across every component rendered in the Storybook iframe (toolbars, nested stories, addons that share the iframe). If an error is thrown inside the story before unmount, the cleanup never runs and the patched setter persists for the rest of the session.Consider a narrower approach: read the stored theme once, sync
globals.theme, and on cleanup just write the original value back — without interceptingsetItem. If the goal is to prevent theThemeProvidereffect from overwriting the user's saved theme, a scopedThemeProviderwrapper (or resetting the value on unmount) would be safer than a process-wide monkey-patch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/header.stories.tsx` around lines 45 - 68, The current useEffect monkey-patches localStorage.setItem (originalSetItem/localStorage.setItem) to drop writes for STORAGE_KEY which causes global side-effects and can persist if unmounted abnormally; instead remove the monkey-patch: read originalThemeRef.current via localStorage.getItem(STORAGE_KEY), call updateGlobals({ theme: storedTheme }) if needed, and on cleanup restore the original stored value by calling originalSetItem(STORAGE_KEY, originalThemeRef.current) (or set via local ThemeProvider wrapper) without overriding localStorage.setItem so only useEffect, originalThemeRef, STORAGE_KEY, updateGlobals and ThemeProvider are touched.src/tedi/components/layout/header/components/header-profile/header-profile.tsx-118-120 (1)
118-120:⚠️ Potential issue | 🟡 MinorMissing
displayNameonHeaderProfile.The component is attached as
Header.Profileinheader.tsxbut never setsdisplayName, unlikeHeaderLogo/HeaderCenter/HeaderActions.📝 Proposed fix
+HeaderProfile.displayName = 'Header.Profile'; + export default HeaderProfile;As per coding guidelines: "All exported components must have a
displayNameproperty set".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-profile/header-profile.tsx` around lines 118 - 120, HeaderProfile is exported and used as Header.Profile but lacks a displayName; add a displayName property on the HeaderProfile component (e.g., HeaderProfile.displayName = "Header.Profile") before the export to match other components like HeaderLogo, HeaderCenter, and HeaderActions so tooling and devtools show the correct name.src/tedi/components/layout/header/components/header-role/header-role.tsx-199-201 (1)
199-201:⚠️ Potential issue | 🟡 MinorMissing
displayNameon exported component.Per coding guidelines, all exported components must set
displayName(and sub-parts attached to a compound likeHeader.Rolebenefit from a namespaced name for DevTools/storybook subcomponents). Add it before the default export:📝 Proposed fix
+HeaderRole.displayName = 'Header.Role'; + export default HeaderRole;As per coding guidelines: "All exported components must have a
displayNameproperty set".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-role/header-role.tsx` around lines 199 - 201, Set the displayName on the exported component to follow guidelines: add a line that assigns HeaderRole.displayName = 'Header.Role' (or another namespaced name you prefer) immediately before the default export of the HeaderRole component so DevTools/storybook shows the proper name.src/tedi/components/layout/header/header.tsx-34-50 (1)
34-50:⚠️ Potential issue | 🟡 MinorUse
useBreakpointPropswithdefaultServerBreakpointto prevent SSR/CSR hydration mismatch forbottomrendering.The
Headercomponent callsuseBreakpoint()without adefaultServerBreakpointparameter. During SSR,useBreakpointdefaults to'xs', makingisMobiletrue and renderingbottom. On the client, if the viewport ismdor wider,breakpointupdates to a larger value,isMobilebecomes false, andbottomdisappears—causing a hydration mismatch and potential console warning.Pass
defaultServerBreakpointtouseBreakpointProps(matching your desktop assumption, e.g.,'md'or'lg') and usegetCurrentBreakpointPropsto deriveisMobile, or hidebottomatmd+via CSS instead of conditional rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/header.tsx` around lines 34 - 50, The Header component currently calls useBreakpoint() directly which causes SSR/CSR hydration mismatches for the conditional bottom rendering; update Header to use useBreakpointProps with a defaultServerBreakpoint (e.g., 'md' or 'lg') and derive isMobile via getCurrentBreakpointProps so the server and client agree on the initial breakpoint; specifically, replace the useBreakpoint() usage in the Header component and compute isMobile via getCurrentBreakpointProps({ breakpointProps }) (or hide bottom at md+ via CSS) to ensure bottom is not conditionally rendered differently between server and client.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52509c50-a603-489a-8404-cb6ac6e20460
⛔ Files ignored due to path filters (1)
public/header-logo-white.svgis excluded by!**/*.svg
📒 Files selected for processing (44)
src/community/components/layout/header/header.stories.tsxsrc/tedi/components/buttons/button-content/button-content.module.scsssrc/tedi/components/buttons/button-content/button-content.tsxsrc/tedi/components/layout/header/components/header-language/header-language.module.scsssrc/tedi/components/layout/header/components/header-language/header-language.spec.tsxsrc/tedi/components/layout/header/components/header-language/header-language.tsxsrc/tedi/components/layout/header/components/header-login/header-login.module.scsssrc/tedi/components/layout/header/components/header-login/header-login.spec.tsxsrc/tedi/components/layout/header/components/header-login/header-login.tsxsrc/tedi/components/layout/header/components/header-logout/header-logout.module.scsssrc/tedi/components/layout/header/components/header-logout/header-logout.spec.tsxsrc/tedi/components/layout/header/components/header-logout/header-logout.tsxsrc/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.module.scsssrc/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.spec.tsxsrc/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.tsxsrc/tedi/components/layout/header/components/header-profile/header-profile.module.scsssrc/tedi/components/layout/header/components/header-profile/header-profile.spec.tsxsrc/tedi/components/layout/header/components/header-profile/header-profile.tsxsrc/tedi/components/layout/header/components/header-role/header-role-representatives.tsxsrc/tedi/components/layout/header/components/header-role/header-role.module.scsssrc/tedi/components/layout/header/components/header-role/header-role.spec.tsxsrc/tedi/components/layout/header/components/header-role/header-role.tsxsrc/tedi/components/layout/header/components/header-search/header-search.module.scsssrc/tedi/components/layout/header/components/header-search/header-search.spec.tsxsrc/tedi/components/layout/header/components/header-search/header-search.tsxsrc/tedi/components/layout/header/header.module.scsssrc/tedi/components/layout/header/header.spec.tsxsrc/tedi/components/layout/header/header.stories.tsxsrc/tedi/components/layout/header/header.tsxsrc/tedi/components/layout/header/index.tssrc/tedi/components/layout/hide-at.spec.tsxsrc/tedi/components/layout/hide-at.tsxsrc/tedi/components/layout/show-at.spec.tsxsrc/tedi/components/layout/show-at.tsxsrc/tedi/components/layout/sidenav/components/sidenav-toggle/sidenav-toggle.module.scsssrc/tedi/components/overlays/overlay/overlay.tsxsrc/tedi/components/overlays/popover/index.tssrc/tedi/components/overlays/popover/popover-content.tsxsrc/tedi/components/overlays/popover/popover-context.tsxsrc/tedi/components/overlays/popover/popover.module.scsssrc/tedi/components/overlays/popover/popover.stories.tsxsrc/tedi/components/overlays/popover/popover.tsxsrc/tedi/providers/label-provider/label-provider.tsxsrc/tedi/providers/label-provider/labels-map.ts
ly-tempel-bitweb
temporarily deployed
to
github-pages
GitHub Actions
Inactive
21 tasks
21 tasks
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
src/tedi/components/layout/header/components/header-logout/header-logout.spec.tsx (1)
47-62: Use role-based queries instead of container selectors.These assertions can be made semantically and avoid coupling the tests to DOM structure.
Proposed fix
it('renders with custom label', () => { (useBreakpointProps as jest.Mock).mockReturnValue({ getCurrentBreakpointProps: jest.fn((props: Record<string, unknown>) => ({ ...props, label: 'Sign out' })), }); - const { container } = render(<HeaderLogout />); + render(<HeaderLogout />); - expect(container.textContent).toContain('Sign out'); + expect(screen.getByRole('button', { name: /sign out/i })).toBeInTheDocument(); }); it('renders with href', () => { - const { container } = render(<HeaderLogout href="/logout" />); + render(<HeaderLogout href="/logout" />); - const link = container.querySelector('a[href="/logout"]'); - expect(link).toBeInTheDocument(); + expect(screen.getByRole('link', { name: /header.logout/i })).toHaveAttribute('href', '/logout'); });As per coding guidelines, “Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries (getByTestId)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-logout/header-logout.spec.tsx` around lines 47 - 62, Replace non-semantic container queries in header-logout.spec.tsx with role-based queries: for the custom label test (mocking useBreakpointProps) use render(<HeaderLogout />) followed by getByRole('link', { name: /sign out/i }) or getByText(/sign out/i) to assert the visible label; for the href test render(<HeaderLogout href="/logout" />) then use getByRole('link', { name: /sign out|logout/i }) (or getByRole('link') plus toHaveAttribute('href', '/logout')) to assert the anchor exists and has the correct href; keep the useBreakpointProps mock and HeaderLogout component references intact while swapping container.querySelector and container.textContent checks for these semantic queries.src/tedi/components/layout/header/components/header-role/header-role.tsx (2)
164-169: Guard description rendering in non-tablet view.On line 166
representative.descriptionis rendered inside<Text>without checking for existence, so when there is no description the component still emits an empty small<Text>element (extra DOM + spacing). The tablet branch at lines 131–136 correctly guards withrepresentative.description &&. Align this branch for consistency.♻️ Proposed change
- {showDescription && !isOrganization && ( + {showDescription && !isOrganization && representative.description && ( <Text modifiers="small" color="secondary"> {representative.description} </Text> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-role/header-role.tsx` around lines 164 - 169, The current non-tablet render branch emits an empty <Text> when representative.description is missing; update the conditional for the small Text block (the JSX using showDescription, isOrganization and representative.description) to only render when representative.description is truthy (i.e., change the condition to include representative.description &&) so it matches the tablet branch and avoids producing an empty <Text> element.
52-91: Inconsistent handling of callback props.
onRoleSelectionToggleandonRepresentativeChangeare intentionally not destructured on lines 53–62 and are instead accessed viaprops.onRoleSelectionToggle?.(next)/props.onRepresentativeChange?.(rep)on lines 85 and 90, while every other prop is destructured. Either destructure them for consistency or add a brief comment explaining why they are read offprops(e.g., to always reflect the latest callback without adding them to effect deps). The current mixed style is easy to overlook in review and maintenance.♻️ Proposed change
- const { - label, - showDescription = true, - representatives, - isOrganization, - accordionLabels, - searchLabel, - organizationSearchLabel, - searchId, - } = props; + const { + label, + showDescription = true, + representatives, + isOrganization, + accordionLabels, + searchLabel, + organizationSearchLabel, + searchId, + onRoleSelectionToggle, + onRepresentativeChange, + } = props; @@ - props.onRoleSelectionToggle?.(next); + onRoleSelectionToggle?.(next); @@ - props.onRepresentativeChange?.(rep); + onRepresentativeChange?.(rep);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-role/header-role.tsx` around lines 52 - 91, The prop callbacks onRoleSelectionToggle and onRepresentativeChange are accessed directly from props while all other props are destructured, causing inconsistent style; update the HeaderRole component so you either destructure these callbacks alongside label/showDescription/representatives/etc. and call them as onRoleSelectionToggle(next) and onRepresentativeChange(rep) from handleToggle and handleRepresentativeChange, or if you intentionally need the latest prop reference do not destructure them but add a concise comment above the handlers explaining that onRoleSelectionToggle and onRepresentativeChange are read from props to avoid stale closures (and keep the current props.onRoleSelectionToggle?.(…) usage) so reviewers understand the deliberate choice. Ensure the change references the HeaderRole component and the handleToggle and handleRepresentativeChange functions.src/tedi/components/layout/header/header.tsx (2)
40-40: Drop the unnecessary fragment wrapper.
{toggle && <>{toggle}</>}is equivalent to{toggle}— React already handles rendering aReactNodedirectly, and the fragment adds no value. The falsy check is also redundant here since React ignoresfalse/null/undefinedchildren.♻️ Proposed change
- {toggle && <>{toggle}</>} + {toggle}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/header.tsx` at line 40, The fragment wrapper around the toggle is unnecessary; replace the JSX expression "{toggle && <>{toggle}</>}" with simply "{toggle}" in the Header component (the prop/variable named toggle) to remove the redundant fragment and conditional check so the component renders the ReactNode directly.
6-6: Simplify self-referential import path.The path
'../header/header.module.scss'goes up one directory and back into the same folder. Use the direct relative path for clarity and consistency with the other local imports below.♻️ Proposed change
-import styles from '../header/header.module.scss'; +import styles from './header.module.scss';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/header.tsx` at line 6, The import statement for the SCSS module is using a self-referential path; update the import that assigns to the symbol "styles" in header.tsx to use a direct relative path to the same file (replace '../header/header.module.scss' with './header.module.scss') so it matches the other local imports and avoids the redundant up-then-back traversal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/tedi/components/layout/header/components/header-language/header-language.tsx`:
- Around line 124-131: Add the missing aria-label to each language Button so
screen readers announce the full label; in the availableLanguages.map where the
Button is rendered (inside the component using changeLanguage and
key={lang.label}), pass aria-label={lang['aria-label']} (or the correct property
accessor for the Language object's aria-label) to the Button component alongside
aria-current, underline, and onClick.
In
`@src/tedi/components/layout/header/components/header-logout/header-logout.tsx`:
- Around line 30-37: The component calls useBreakpoint() without the server
fallback, causing isMobileView to default to desktop on SSR; update the
breakpoint usage to honor props.defaultServerBreakpoint by passing that server
breakpoint into useBreakpoint (or otherwise using the same server fallback value
returned by useBreakpointProps) so isBreakpointBelow(breakpoint, 'md') uses the
same server-side breakpoint as getCurrentBreakpointProps; reference
useBreakpointProps, getCurrentBreakpointProps, useBreakpoint, isBreakpointBelow
and props.defaultServerBreakpoint when making the change.
In `@src/tedi/components/layout/header/components/header-role/header-role.tsx`:
- Around line 64-70: The selected representative state is only initialized once
via useState(representatives?.[0] ?? ({} as Representative)), which can become
stale when the representatives prop updates; update the component to sync state
when representatives changes by adding a useEffect that calls
setRepresentative(representatives?.[0] ?? undefined) whenever representatives
changes (or convert representative state to Representative | undefined) and
update places that read representative.name/description to guard for undefined,
or alternatively expose a controlled pair (representative,
onRepresentativeChange) so the parent owns selection; also remove the unsafe
cast ({} as Representative) and instead model and handle the empty-list case in
the UI.
In `@src/tedi/components/layout/header/header.stories.tsx`:
- Around line 279-298: The SideNav examples pass an empty ariaLabel which
creates unnamed navigation landmarks; update the SideNav components (e.g., the
instance inside HideAt) to set a meaningful non-empty ariaLabel like "Main
navigation" (or context-appropriate label) instead of ariaLabel="", and apply
the same change to the other SideNav examples referenced in this story so all
SideNav instances have accessible labels.
- Around line 63-68: The TEDI-Ready story's Storybook parameters currently only
set parameters.status.type; update the TEDI-Ready story in header.stories.tsx to
include parameters.status.url as well (so parameters.status is an object with
both type and url). Locate the TEDI-Ready story's parameters block (the
parameters object with layout and status) and add the required status URL value
per the TEDI-Ready convention so parameters.status contains both type and url.
- Around line 173-197: ProfileExample currently uses a static id "theme-toggle"
for the Toggle, causing duplicate IDs; import React's useId and call useId()
inside the ProfileExample component to generate a unique id (e.g., const id =
useId()), then pass that id to the Toggle's id prop instead of the hardcoded
"theme-toggle" so each rendered ProfileExample instance gets a unique ID while
keeping existing handleToggle and useTheme logic intact.
- Around line 222-224: Add an explicit import for the React types used by
StoryWrapperProps so TypeScript can resolve React.Dispatch,
React.SetStateAction, and React.ReactNode; update the top of the module to
import React (or the specific types) and keep the StoryWrapperProps declaration
as-is (references: StoryWrapperProps, React.Dispatch, React.SetStateAction,
React.ReactNode).
In `@src/tedi/components/layout/hide-at.tsx`:
- Around line 9-21: The exported React component HideAt is missing a displayName
which violates the guideline; add a displayName property to the component after
its declaration (e.g., HideAt.displayName = 'HideAt') so tools and devtools show
the proper name; locate the HideAt function (which uses useBreakpoint and
isBreakpointBelow and accepts HideAtProps) and append the displayName assignment
immediately after the function before export/usage.
---
Nitpick comments:
In
`@src/tedi/components/layout/header/components/header-logout/header-logout.spec.tsx`:
- Around line 47-62: Replace non-semantic container queries in
header-logout.spec.tsx with role-based queries: for the custom label test
(mocking useBreakpointProps) use render(<HeaderLogout />) followed by
getByRole('link', { name: /sign out/i }) or getByText(/sign out/i) to assert the
visible label; for the href test render(<HeaderLogout href="/logout" />) then
use getByRole('link', { name: /sign out|logout/i }) (or getByRole('link') plus
toHaveAttribute('href', '/logout')) to assert the anchor exists and has the
correct href; keep the useBreakpointProps mock and HeaderLogout component
references intact while swapping container.querySelector and
container.textContent checks for these semantic queries.
In `@src/tedi/components/layout/header/components/header-role/header-role.tsx`:
- Around line 164-169: The current non-tablet render branch emits an empty
<Text> when representative.description is missing; update the conditional for
the small Text block (the JSX using showDescription, isOrganization and
representative.description) to only render when representative.description is
truthy (i.e., change the condition to include representative.description &&) so
it matches the tablet branch and avoids producing an empty <Text> element.
- Around line 52-91: The prop callbacks onRoleSelectionToggle and
onRepresentativeChange are accessed directly from props while all other props
are destructured, causing inconsistent style; update the HeaderRole component so
you either destructure these callbacks alongside
label/showDescription/representatives/etc. and call them as
onRoleSelectionToggle(next) and onRepresentativeChange(rep) from handleToggle
and handleRepresentativeChange, or if you intentionally need the latest prop
reference do not destructure them but add a concise comment above the handlers
explaining that onRoleSelectionToggle and onRepresentativeChange are read from
props to avoid stale closures (and keep the current
props.onRoleSelectionToggle?.(…) usage) so reviewers understand the deliberate
choice. Ensure the change references the HeaderRole component and the
handleToggle and handleRepresentativeChange functions.
In `@src/tedi/components/layout/header/header.tsx`:
- Line 40: The fragment wrapper around the toggle is unnecessary; replace the
JSX expression "{toggle && <>{toggle}</>}" with simply "{toggle}" in the Header
component (the prop/variable named toggle) to remove the redundant fragment and
conditional check so the component renders the ReactNode directly.
- Line 6: The import statement for the SCSS module is using a self-referential
path; update the import that assigns to the symbol "styles" in header.tsx to use
a direct relative path to the same file (replace '../header/header.module.scss'
with './header.module.scss') so it matches the other local imports and avoids
the redundant up-then-back traversal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ba6c3e6-3d7f-46a8-a090-49c8270bf674
📒 Files selected for processing (29)
src/tedi/components/buttons/button-content/button-content.tsxsrc/tedi/components/cards/card/card-stories-templates.tsxsrc/tedi/components/layout/header/components/header-language/header-language.tsxsrc/tedi/components/layout/header/components/header-login/header-login.spec.tsxsrc/tedi/components/layout/header/components/header-login/header-login.tsxsrc/tedi/components/layout/header/components/header-logout/header-logout.spec.tsxsrc/tedi/components/layout/header/components/header-logout/header-logout.tsxsrc/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.module.scsssrc/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.spec.tsxsrc/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.tsxsrc/tedi/components/layout/header/components/header-profile/header-profile.spec.tsxsrc/tedi/components/layout/header/components/header-profile/header-profile.tsxsrc/tedi/components/layout/header/components/header-role/header-role-representatives.tsxsrc/tedi/components/layout/header/components/header-role/header-role.module.scsssrc/tedi/components/layout/header/components/header-role/header-role.tsxsrc/tedi/components/layout/header/components/header-search/header-search.module.scsssrc/tedi/components/layout/header/components/header-search/header-search.spec.tsxsrc/tedi/components/layout/header/components/header-search/header-search.tsxsrc/tedi/components/layout/header/header.module.scsssrc/tedi/components/layout/header/header.spec.tsxsrc/tedi/components/layout/header/header.stories.tsxsrc/tedi/components/layout/header/header.tsxsrc/tedi/components/layout/header/index.tssrc/tedi/components/layout/hide-at.spec.tsxsrc/tedi/components/layout/hide-at.tsxsrc/tedi/components/overlays/popover/popover-content.tsxsrc/tedi/components/overlays/popover/popover.tsxsrc/tedi/providers/label-provider/label-provider.tsxsrc/tedi/providers/label-provider/labels-map.ts
✅ Files skipped from review due to trivial changes (10)
- src/tedi/components/cards/card/card-stories-templates.tsx
- src/tedi/components/buttons/button-content/button-content.tsx
- src/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.spec.tsx
- src/tedi/components/layout/header/components/header-search/header-search.module.scss
- src/tedi/components/layout/header/components/header-role/header-role.module.scss
- src/tedi/components/layout/header/header.module.scss
- src/tedi/components/layout/header/header.spec.tsx
- src/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.module.scss
- src/tedi/components/layout/header/index.ts
- src/tedi/components/layout/header/components/header-search/header-search.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- src/tedi/components/layout/header/components/header-login/header-login.spec.tsx
- src/tedi/components/layout/header/components/header-login/header-login.tsx
- src/tedi/providers/label-provider/labels-map.ts
- src/tedi/components/overlays/popover/popover.tsx
- src/tedi/components/layout/header/components/header-mobile-button/header-mobile-button.tsx
- src/tedi/components/layout/header/components/header-role/header-role-representatives.tsx
- src/tedi/components/layout/header/components/header-search/header-search.tsx
- src/tedi/components/layout/header/components/header-profile/header-profile.tsx
ly-tempel-bitweb
temporarily deployed
to
github-pages
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/tedi/components/layout/header/components/header-role/header-role.tsx (1)
71-73:⚠️ Potential issue | 🟠 MajorPreserve the current representative when the list refreshes.
This effect resets the selected representative to the first item whenever the parent passes a new
representativesarray, even if the current selection still exists. That can make a user’s selected role jump back after a data refresh or parent re-render.🐛 Proposed fix
useEffect(() => { - setRepresentative(representatives?.[0]); - }, [representatives]); + setRepresentative((current) => { + if (!representatives.length) return undefined; + if (current && representatives.some((rep) => rep.name === current.name)) return current; + + return representatives[0]; + }); + }, [representatives]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-role/header-role.tsx` around lines 71 - 73, The useEffect in the HeaderRole component unconditionally calls setRepresentative(representatives?.[0]) and resets selection on every representatives change; change the effect (the useEffect that references representatives and calls setRepresentative) to first check whether the current representative state (representative) still exists in the new representatives array (compare by a stable identifier such as id or value), and only call setRepresentative(representatives[0]) if the current representative is missing or undefined; otherwise keep the current representative. Ensure the logic handles empty/undefined representatives gracefully.
🧹 Nitpick comments (6)
src/tedi/components/layout/header/components/header-language/header-language.tsx (1)
62-62: UnnecessaryuseMemowrapper.
useMemo(() => languages ?? [], [languages])provides no meaningful memoization — the only work is a nullish coalesce. A plainconst availableLanguages = languages ?? [];is equivalent and simpler. Note that this also changes identity on every render today (since[] !== []whenlanguagesis undefined), but nothing consumes the reference.Proposed fix
- const availableLanguages = useMemo<Language[]>(() => languages ?? [], [languages]); + const availableLanguages = languages ?? [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-language/header-language.tsx` at line 62, The useMemo wrapper around availableLanguages is unnecessary; replace the memoized expression using useMemo in header-language.tsx with a simple nullish coalescing assignment so availableLanguages is defined as const availableLanguages = languages ?? []; remove the import/use of useMemo for this variable and ensure any references still use availableLanguages (symbol: availableLanguages, originally created via useMemo, and the dependency languages).src/tedi/components/layout/hide-at/hide-at.tsx (1)
12-16: Redundantcurrentnull guard.
isBreakpointBelowalready returnsfalsewhencurrentisnull(seesrc/tedi/helpers/hooks/use-breakpoint.ts:48-51), anduseBreakpoint()defaults to'xs'on SSR and never returnsnullon the client. Theif (!current) return false;line is dead code and is also asymmetric withShowAtwhich omits it — worth removing for consistency.Proposed fix
const shouldHide = Object.entries(breakpoints).some(([bp, value]) => { if (!value) return false; - if (!current) return false; return !isBreakpointBelow(current, bp as Breakpoint); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/hide-at/hide-at.tsx` around lines 12 - 16, Remove the redundant null check for current inside the shouldHide computation in the HideAt component: the guard "if (!current) return false;" is dead code because isBreakpointBelow already handles null and useBreakpoint never returns null; update the arrow passed to Object.entries(breakpoints).some to call isBreakpointBelow(current, bp as Breakpoint) (or simply return !isBreakpointBelow(...)) and ensure behavior matches ShowAt; remove the extra conditional and leave shouldHide logic relying solely on isBreakpointBelow.src/tedi/components/layout/show-at/show-at.stories.tsx (1)
19-30: Prefer CSS Modules / design tokens over inline styles with hardcoded colors.Per repo conventions, components should use CSS Modules with SCSS and design tokens rather than inline styles with hardcoded color literals (
#e8f4fd,#b3d9f2,#fde8e8, …). Consider moving theBox/Descriptionhelpers into a shared.module.scss(or reusing existing tokens) and doing the same inhide-at.stories.tsx. Stories get lighter treatment, so treat this as optional.As per coding guidelines: "Use CSS Modules with SCSS for styling, avoiding inline styles" and "Use design tokens from
@tedi-design-system/coreinstead of hardcoding color values".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/show-at/show-at.stories.tsx` around lines 19 - 30, The Box (and similar Description) story helper uses inline hardcoded colors; replace these with CSS Modules/SCSS and design tokens: create a .module.scss with classes (e.g., .box, .description) that apply padding, border-radius and border/background using tokens from `@tedi-design-system/core`, then update the Box and Description components to use those classes instead of inline style and hardcoded hex values; also apply the same refactor to the hide-at story helper for consistency.src/tedi/components/layout/header/components/header-role/header-role-representatives.tsx (2)
4-4: AwkwardSeparatorimport path.From
src/tedi/components/layout/header/components/header-role/, this traverses up six levels and back intotedi/components/misc/separator/separator. A shorter relative path (e.g.../../../../misc/separator/separator) would make this file easier to refactor/move. Minor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-role/header-role-representatives.tsx` at line 4, The Separator import in header-role-representatives.tsx uses an overly long relative path ('../../../../../../tedi/components/misc/separator/separator'); update the import for the Separator symbol to a shorter, correct relative path from this file (e.g. '../../../../misc/separator/separator' or the equivalent path that reaches tedi/components/misc/separator/separator without traversing up six levels) so the import is easier to maintain and the file can be moved/refactored safely.
95-95: Use booleaninertattribute instead of string spread pattern.React 19+ supports
inertas a native boolean attribute. Replace the spread-with-empty-string pattern with the simpler boolean form:Proposed fix
- {...(!isRoleSelectionOpen && { inert: '' })} + inert={!isRoleSelectionOpen}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-role/header-role-representatives.tsx` at line 95, Replace the spread-with-empty-string pattern used to set the inert attribute with a proper boolean prop: locate the JSX in the HeaderRoleRepresentatives component (where isRoleSelectionOpen is used) and change the expression {...(!isRoleSelectionOpen && { inert: '' })} to use the boolean form so the element receives inert when isRoleSelectionOpen is false (i.e., set inert based on the negation of isRoleSelectionOpen).src/tedi/components/layout/header/header.tsx (1)
52-79: MakeshowLogobreakpoint-aware or remove the responsive promise.
showLogois typed as a plainboolean, so callers cannot pass per-breakpoint visibility overrides through the TEDI responsive prop pattern. Either resolve this prop withuseBreakpointPropsor steer consumers to dedicated visibility components without implying prop-level responsiveness.As per coding guidelines, “Responsive props are supported via
useBreakpointPropshook — components must accept per-breakpoint prop overrides.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/header.tsx` around lines 52 - 79, Update HeaderLogo to support TEDI responsive props by making showLogo accept per-breakpoint values and resolving them via useBreakpointProps: change HeaderLogoProps.showLogo from boolean to the responsive prop type your codebase uses (e.g. ResponsiveProp<boolean>) and inside the HeaderLogo component call useBreakpointProps(props, ['showLogo']) (or the project-equivalent helper) to extract the resolved showLogo value before using it; reference HeaderLogoProps, HeaderLogo, showLogo and useBreakpointProps when making the edits so callers can pass breakpoint-specific visibility overrides.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/tedi/components/layout/header/components/header-language/header-language.tsx`:
- Line 107: The aria-label concatenation currently uses selectLabel (from
getLabel('header.select-lang')) which includes a trailing colon, so
aria-label={`${selectLabel ?? getLabel('header.select-lang')},
${displayedLanguage}`} produces awkward output like "Language:, EST"; fix by
removing trailing punctuation or using a colon-less i18n key: trim trailing
characters from selectLabel (e.g., strip trailing ":" or "：" if present) before
concatenation, or call a new key (e.g., getLabel('header.select-lang-short'))
that has no colon; update the aria-label construction in the header-language
component (references: selectLabel, getLabel, displayedLanguage, aria-label) to
use the cleaned string when building the accessible name.
In
`@src/tedi/components/layout/header/components/header-role/header-role-representatives.tsx`:
- Around line 105-131: The code uses rep.name for the React key and for
selection comparison which is fragile; update the Representative model to
include a stable id and change the map/key and equality check to use rep.id
(i.e., in representatives.map use key={rep.id}) and change isSelected to compare
representative?.id === rep.id; also ensure handleSelect and any other selection
logic (handleSelect, representatives.map callback, and any state that stores the
selected representative) use the id field (or accept the full object but base
equality on id) so duplicate display names no longer cause duplicate keys or
simultaneous selection.
In `@src/tedi/components/layout/header/components/header-role/header-role.tsx`:
- Around line 152-170: Replace the block-level <div> used inside the Button
components with an inline <span> to avoid invalid HTML nesting: locate the
Button instance(s) in header-role.tsx (the Button using id={toggleId},
onClick={() => handleToggle()}, aria-expanded={isRoleSelectionOpen},
aria-controls={panelId}) and replace the inner <div
className={styles['tedi-header-role__accordion--toggle']}> with a <span> while
preserving the same className, children (openLabel/closeLabel) and the Icon
usage (including its className conditional for isRoleSelectionOpen); do the same
for the second Button instance in this file so both Button children are inline
elements.
In `@src/tedi/components/layout/hide-at/hide-at.spec.tsx`:
- Around line 71-82: The test in HideAt spec uses a misleading fixture string;
update the rendered child text inside the <HideAt sm md> render to "Hidden
content" so the assertion expect(screen.queryByText('Hidden
content')).not.toBeInTheDocument() matches intent; locate the test case
referencing useBreakpoint (mocked as 'xxl') and isBreakpointBelow (mocked false)
in hide-at.spec.tsx and change the inner span text from "Visible content" to
"Hidden content" to mirror sibling tests.
In `@src/tedi/components/layout/show-at/show-at.spec.tsx`:
- Around line 58-69: The test claims to exercise "any of multiple breakpoints",
but currently a single isBreakpointBelow mockReturnValueOnce(false) makes the
first breakpoint (sm) succeed and short-circuits, so lg is never evaluated;
update the mocks for isBreakpointBelow in the test to simulate first breakpoint
failing and second succeeding (e.g., (isBreakpointBelow as
jest.Mock).mockReturnValueOnce(true) followed by .mockReturnValueOnce(false)) so
Object.entries(...).some(...) evaluates both entries and verifies the "any"
behavior for ShowAt.
---
Duplicate comments:
In `@src/tedi/components/layout/header/components/header-role/header-role.tsx`:
- Around line 71-73: The useEffect in the HeaderRole component unconditionally
calls setRepresentative(representatives?.[0]) and resets selection on every
representatives change; change the effect (the useEffect that references
representatives and calls setRepresentative) to first check whether the current
representative state (representative) still exists in the new representatives
array (compare by a stable identifier such as id or value), and only call
setRepresentative(representatives[0]) if the current representative is missing
or undefined; otherwise keep the current representative. Ensure the logic
handles empty/undefined representatives gracefully.
---
Nitpick comments:
In
`@src/tedi/components/layout/header/components/header-language/header-language.tsx`:
- Line 62: The useMemo wrapper around availableLanguages is unnecessary; replace
the memoized expression using useMemo in header-language.tsx with a simple
nullish coalescing assignment so availableLanguages is defined as const
availableLanguages = languages ?? []; remove the import/use of useMemo for this
variable and ensure any references still use availableLanguages (symbol:
availableLanguages, originally created via useMemo, and the dependency
languages).
In
`@src/tedi/components/layout/header/components/header-role/header-role-representatives.tsx`:
- Line 4: The Separator import in header-role-representatives.tsx uses an overly
long relative path
('../../../../../../tedi/components/misc/separator/separator'); update the
import for the Separator symbol to a shorter, correct relative path from this
file (e.g. '../../../../misc/separator/separator' or the equivalent path that
reaches tedi/components/misc/separator/separator without traversing up six
levels) so the import is easier to maintain and the file can be moved/refactored
safely.
- Line 95: Replace the spread-with-empty-string pattern used to set the inert
attribute with a proper boolean prop: locate the JSX in the
HeaderRoleRepresentatives component (where isRoleSelectionOpen is used) and
change the expression {...(!isRoleSelectionOpen && { inert: '' })} to use the
boolean form so the element receives inert when isRoleSelectionOpen is false
(i.e., set inert based on the negation of isRoleSelectionOpen).
In `@src/tedi/components/layout/header/header.tsx`:
- Around line 52-79: Update HeaderLogo to support TEDI responsive props by
making showLogo accept per-breakpoint values and resolving them via
useBreakpointProps: change HeaderLogoProps.showLogo from boolean to the
responsive prop type your codebase uses (e.g. ResponsiveProp<boolean>) and
inside the HeaderLogo component call useBreakpointProps(props, ['showLogo']) (or
the project-equivalent helper) to extract the resolved showLogo value before
using it; reference HeaderLogoProps, HeaderLogo, showLogo and useBreakpointProps
when making the edits so callers can pass breakpoint-specific visibility
overrides.
In `@src/tedi/components/layout/hide-at/hide-at.tsx`:
- Around line 12-16: Remove the redundant null check for current inside the
shouldHide computation in the HideAt component: the guard "if (!current) return
false;" is dead code because isBreakpointBelow already handles null and
useBreakpoint never returns null; update the arrow passed to
Object.entries(breakpoints).some to call isBreakpointBelow(current, bp as
Breakpoint) (or simply return !isBreakpointBelow(...)) and ensure behavior
matches ShowAt; remove the extra conditional and leave shouldHide logic relying
solely on isBreakpointBelow.
In `@src/tedi/components/layout/show-at/show-at.stories.tsx`:
- Around line 19-30: The Box (and similar Description) story helper uses inline
hardcoded colors; replace these with CSS Modules/SCSS and design tokens: create
a .module.scss with classes (e.g., .box, .description) that apply padding,
border-radius and border/background using tokens from `@tedi-design-system/core`,
then update the Box and Description components to use those classes instead of
inline style and hardcoded hex values; also apply the same refactor to the
hide-at story helper for consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e361fc9-b1a5-4c3b-b7c3-d5967396570e
📒 Files selected for processing (14)
src/tedi/components/layout/header/components/header-language/header-language.tsxsrc/tedi/components/layout/header/components/header-logout/header-logout.spec.tsxsrc/tedi/components/layout/header/components/header-logout/header-logout.tsxsrc/tedi/components/layout/header/components/header-role/header-role-representatives.tsxsrc/tedi/components/layout/header/components/header-role/header-role.spec.tsxsrc/tedi/components/layout/header/components/header-role/header-role.tsxsrc/tedi/components/layout/header/header.stories.tsxsrc/tedi/components/layout/header/header.tsxsrc/tedi/components/layout/hide-at/hide-at.spec.tsxsrc/tedi/components/layout/hide-at/hide-at.stories.tsxsrc/tedi/components/layout/hide-at/hide-at.tsxsrc/tedi/components/layout/show-at/show-at.spec.tsxsrc/tedi/components/layout/show-at/show-at.stories.tsxsrc/tedi/components/layout/show-at/show-at.tsx
✅ Files skipped from review due to trivial changes (1)
- src/tedi/components/layout/header/components/header-role/header-role.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tedi/components/layout/header/components/header-logout/header-logout.spec.tsx
- src/tedi/components/layout/header/components/header-logout/header-logout.tsx
- src/tedi/components/layout/header/header.stories.tsx
21 tasks
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/tedi/components/layout/header/components/header-language/header-language.tsx (1)
62-72: Dead nullish guards on a required prop.
languages: Language[]is declared as a required (non-optional) array inHeaderLanguageProps, solanguages ?? [],languages?.find, andlanguages?.[0]?.label ?? ''are defensive code that can't fire in TypeScript consumers, and theavailableLanguageslocal is a duplicate oflanguages. Either makelanguagestruly optional in the public type (if empty/undefined is a supported usage, which is worth confirming) or drop the extra checks so the intent reads straight.♻️ Simplification if `languages` stays required
- const availableLanguages = languages ?? []; - const displayedLanguage = useMemo(() => { if (locale) { - const found = languages?.find((l) => l.locale === locale); + const found = languages.find((l) => l.locale === locale); if (found) return found.label; } if (currentLanguage) return currentLanguage; - return languages?.[0]?.label ?? ''; + return languages[0]?.label ?? ''; }, [languages, locale, currentLanguage]);…and replace the
availableLanguages.mapbelow withlanguages.map.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-language/header-language.tsx` around lines 62 - 72, The prop `languages` is declared required in `HeaderLanguageProps` so the nullish guards and the duplicate `availableLanguages` are dead code; either make `languages` optional on `HeaderLanguageProps` if undefined/empty is a supported state, or remove the unnecessary guards and the `availableLanguages` alias and use `languages` directly (update the `displayedLanguage` useMemo to reference `languages.find` and `languages[0].label` and change any `availableLanguages.map` below to `languages.map`) to simplify intent and eliminate redundant checks.src/tedi/components/layout/header/components/header-role/header-role-representatives.tsx (2)
4-9: Inconsistent relative-import depths.Lines 4 and 5 walk up further than needed (6 and 5 levels respectively, including an extra
tedi/components/...re-entry), while lines 7 and 9 correctly use 4 levels to reachsrc/tedi/components/.... Normalizing these avoids surprises when this file is moved.♻️ Suggested normalization
-import Separator from '../../../../../../tedi/components/misc/separator/separator'; -import { Text } from '../../../../../components/base/typography/text/text'; +import Separator from '../../../../misc/separator/separator'; +import { Text } from '../../../../base/typography/text/text';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-role/header-role-representatives.tsx` around lines 4 - 9, The file uses inconsistent deep relative imports (e.g., the Separator and Text imports) which go up extra directory levels; update the import paths for Separator, Text (and any other imports using 6/5-level traversals) to match the normalized 4-level pattern used by Icon/IconProps, Button, and Search so all imports resolve from src/tedi/components/... consistently—locate the import statements for Separator and Text in header-role-representatives.tsx and change their relative paths to the same base level as the Icon, Button, and Search imports.
115-115: Confusing parameter namee.The
Search.onChangecallback here delivers a string value, not an event. Usingemakes it look like an event handler at first read. Consider renaming tovalue(and inlining the setter).- onChange={(e) => setInputValue(e)} + onChange={setInputValue}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/components/header-role/header-role-representatives.tsx` at line 115, The onChange handler for the Search component uses a misleading parameter name `e` even though it receives a string; change the arrow param to `value` and inline the setter so it reads like onChange={(value) => setInputValue(value)} (locate the handler on the Search.onChange prop and the setInputValue state setter in header-role-representatives.tsx). This makes it clear the callback receives a string, not an event.src/tedi/components/layout/header/header.stories.tsx (1)
28-38: Consider dropping theas nevercast.
subcomponentsis cast withas never, which is an escape hatch with the same spirit asany/@ts-ignore. Storybook'sMeta['subcomponents']accepts aRecord<string, ComponentType>-ish shape; typing the object asRecord<string, unknown>orMeta<typeof Header>['subcomponents']is usually enough to satisfy the type without the unsafe cast.- } as never, + } satisfies Meta<typeof Header>['subcomponents'],(Verify
satisfieskeeps the shape you want; if it doesn't resolve cleanly,as Meta<typeof Header>['subcomponents']is still strictly safer thanas never.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/layout/header/header.stories.tsx` around lines 28 - 38, The subcomponents object is unsafely cast with "as never"; replace that escape hatch by typing it properly (e.g., use Meta<typeof Header>['subcomponents'] or Record<string, unknown>, or use a "satisfies" expression) so the object containing HeaderLogo, HeaderCenter, HeaderActions, Header.Language, Header.Login, Header.Logout, Header.Profile, Header.Role, Header.Search is typed correctly instead of forced to never; update the declaration so TypeScript/Storybook accepts the Record<string, ComponentType>-like shape without "as never".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/tedi-react/references/components.md`:
- Around line 285-296: The example and short description for Header.Search are
misleading because Header.Search is a wrapper that expects a child Search
component (and optional mobileVariant) rather than being used as a self-closing
element; update the docs so the short description states "wrapper that accepts a
Search child (and optional mobileVariant)" and replace the example with a usage
that passes a Search child to <Header.Search> (e.g., <Header.Search><Search ...
/></Header.Search>), referencing the Header.Search and Search components and
matching the patterns shown in header.stories.tsx.
In `@src/tedi/components/layout/header/header.stories.tsx`:
- Around line 403-414: The two Header.Search instances render the same DOM id
("search-1") across breakpoint branches which can create duplicate ids; update
the Search id props inside the ShowAt lg and HideAt lg branches so they are
unique (for example use "search-1-desktop" and "search-1-mobile" or otherwise
generate distinct ids) and do the same for the later "search-4" pair; locate the
Search usages inside Header.Search (the Search component instances in the
Header.Center and Header.Actions branches) and change their id values so each
conditional rendering branch uses a unique id.
---
Nitpick comments:
In
`@src/tedi/components/layout/header/components/header-language/header-language.tsx`:
- Around line 62-72: The prop `languages` is declared required in
`HeaderLanguageProps` so the nullish guards and the duplicate
`availableLanguages` are dead code; either make `languages` optional on
`HeaderLanguageProps` if undefined/empty is a supported state, or remove the
unnecessary guards and the `availableLanguages` alias and use `languages`
directly (update the `displayedLanguage` useMemo to reference `languages.find`
and `languages[0].label` and change any `availableLanguages.map` below to
`languages.map`) to simplify intent and eliminate redundant checks.
In
`@src/tedi/components/layout/header/components/header-role/header-role-representatives.tsx`:
- Around line 4-9: The file uses inconsistent deep relative imports (e.g., the
Separator and Text imports) which go up extra directory levels; update the
import paths for Separator, Text (and any other imports using 6/5-level
traversals) to match the normalized 4-level pattern used by Icon/IconProps,
Button, and Search so all imports resolve from src/tedi/components/...
consistently—locate the import statements for Separator and Text in
header-role-representatives.tsx and change their relative paths to the same base
level as the Icon, Button, and Search imports.
- Line 115: The onChange handler for the Search component uses a misleading
parameter name `e` even though it receives a string; change the arrow param to
`value` and inline the setter so it reads like onChange={(value) =>
setInputValue(value)} (locate the handler on the Search.onChange prop and the
setInputValue state setter in header-role-representatives.tsx). This makes it
clear the callback receives a string, not an event.
In `@src/tedi/components/layout/header/header.stories.tsx`:
- Around line 28-38: The subcomponents object is unsafely cast with "as never";
replace that escape hatch by typing it properly (e.g., use Meta<typeof
Header>['subcomponents'] or Record<string, unknown>, or use a "satisfies"
expression) so the object containing HeaderLogo, HeaderCenter, HeaderActions,
Header.Language, Header.Login, Header.Logout, Header.Profile, Header.Role,
Header.Search is typed correctly instead of forced to never; update the
declaration so TypeScript/Storybook accepts the Record<string,
ComponentType>-like shape without "as never".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71767eae-bb00-4fd7-9a05-c1a00e68153c
📒 Files selected for processing (9)
skills/tedi-react/references/components.mdsrc/tedi/components/layout/header/components/header-language/header-language.tsxsrc/tedi/components/layout/header/components/header-role/header-role-representatives.tsxsrc/tedi/components/layout/header/components/header-role/header-role.spec.tsxsrc/tedi/components/layout/header/components/header-role/header-role.tsxsrc/tedi/components/layout/header/header.stories.tsxsrc/tedi/components/layout/hide-at/hide-at.spec.tsxsrc/tedi/components/layout/hide-at/hide-at.tsxsrc/tedi/components/layout/show-at/show-at.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/tedi/components/layout/hide-at/hide-at.spec.tsx
- src/tedi/components/layout/show-at/show-at.spec.tsx
- src/tedi/components/layout/hide-at/hide-at.tsx
- src/tedi/components/layout/header/components/header-role/header-role.spec.tsx
ly-tempel-bitweb
temporarily deployed
to
github-pages
GitHub Actions
Inactive
ly-tempel-bitweb
temporarily deployed
to
github-pages
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/tedi/components/layout/header/components/header-profile/header-profile.tsx`:
- Around line 60-97: HeaderMobileButton does not forward refs, so triggerRef in
header-profile.tsx cannot restore focus on mobile; update HeaderMobileButton to
use React.forwardRef<HTMLButtonElement, Props> and attach the forwarded ref to
the underlying button element, preserve its props signature, and export the
forwarded component (symbol: HeaderMobileButton). Then in header-profile.tsx
pass triggerRef into the HeaderMobileButton JSX, and ensure every modal-close
path (the Escape handler already calls triggerRef.current?.focus(), the
overlay-dismiss handler and handleToggleModal when toggling from open to closed)
explicitly calls triggerRef.current?.focus() after setting modalOpen false so
focus restores on mobile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f5b89964-be69-44b8-bd70-be09f55b2310
📒 Files selected for processing (11)
src/tedi/components/layout/header/components/header-profile/header-profile.module.scsssrc/tedi/components/layout/header/components/header-profile/header-profile.tsxsrc/tedi/components/layout/header/components/header-role/header-role-representatives.tsxsrc/tedi/components/layout/header/components/header-role/header-role.module.scsssrc/tedi/components/layout/header/components/header-role/header-role.spec.tsxsrc/tedi/components/layout/header/components/header-role/header-role.tsxsrc/tedi/components/layout/header/header.stories.tsxsrc/tedi/components/layout/header/header.tsxsrc/tedi/components/layout/hide-at/hide-at.stories.tsxsrc/tedi/components/layout/show-at/show-at.stories.tsxsrc/tedi/providers/label-provider/labels-map.ts
✅ Files skipped from review due to trivial changes (3)
- src/tedi/components/layout/header/components/header-profile/header-profile.module.scss
- src/tedi/components/layout/hide-at/hide-at.stories.tsx
- src/tedi/components/layout/header/components/header-role/header-role.module.scss
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tedi/components/layout/show-at/show-at.stories.tsx
- src/tedi/providers/label-provider/labels-map.ts
- src/tedi/components/layout/header/components/header-role/header-role.spec.tsx
ly-tempel-bitweb
temporarily deployed
to
github-pages
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation