feat(input-group): new TEDI-Ready component #23#610
Merged
Conversation
21 tasks
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a reusable Field component (input/textarea) with ref forwarding and controlled/uncontrolled modes, introduces an InputGroup system with Prefix/Suffix/Input subcomponents and context for id/label coordination, and updates related form components, styles, stories, and tests to integrate the new patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant InputGroup
participant Prefix
participant Suffix
participant InputWrapper as Input
participant Field
participant DOM
App->>InputGroup: render(label, helper, children)
InputGroup->>InputGroup: create context (inputId, disabled, register/unregister)
App->>Prefix: render Prefix
Prefix->>InputGroup: registerPrefix()
App->>Suffix: render Suffix
Suffix->>InputGroup: registerSuffix()
App->>InputWrapper: render Input child
InputWrapper->>InputGroup: read context (inputId, disabled)
InputWrapper->>Field: clone child with id/disabled/className
Field->>DOM: render <input>/<textarea>
DOM->>Field: user input
Field->>App: onChange(newValue) and onChangeEvent(event)
Prefix-->>InputGroup: unregisterPrefix() on unmount
Suffix-->>InputGroup: unregisterSuffix() on unmount
sequenceDiagram
participant App
participant Field
participant InternalState
participant DOM
App->>Field: render with value (controlled)
Field->>Field: use external value (no internal state)
DOM->>Field: user types
Field->>App: call onChange(newValue) and onChangeEvent(event)
Note over Field: OR uncontrolled mode
App->>Field: render with defaultValue (uncontrolled)
Field->>InternalState: initialize with defaultValue
DOM->>Field: user types
Field->>InternalState: update state
Field->>App: call onChange(newValue) and onChangeEvent(event)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tedi/components/form/select/select.tsx (1)
248-260:⚠️ Potential issue | 🔴 CriticalBug:
instanceIdandinputIdstill use rawid— breaks label association and stability whenidis not provided.After introducing
resolvedId, several spots still reference the originalidprop:
- Line 248:
instanceId={id}— becomesundefinedwhen the consumer relies on an InputGroup-providedinputIdor the generatedReact.useId(); this defeats the stable-id resolution and can cause SSR/react-select instance-id inconsistencies.- Line 260:
inputId={${id}-input}— whenidis undefined this becomes the literal string"undefined-input".- Line 333:
FormLabelusesid={${resolvedId}-input}to sethtmlFor, but the actual internal input id on react-select is derived from rawid(line 260). These no longer match whenprops.idis undefined, so the label’shtmlForwon’t resolve to the input — an accessibility regression.Use
resolvedIdconsistently:- instanceId={id} + instanceId={resolvedId} ... - inputId={`${id}-input`} + inputId={`${resolvedId}-input`}Also applies to: 331-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/select/select.tsx` around lines 248 - 260, The select component still passes the raw id prop into react-select and the FormLabel, causing "undefined-input" and broken htmlFor linkage; update all places using id for the react-select props and label to use resolvedId instead (replace instanceId={id} with instanceId={resolvedId}, inputId={`${resolvedId}-input`} and ensure FormLabel's id/htmlFor use `${resolvedId}-input`) and verify any other occurrences between the render block (around instanceId/inputId and the FormLabel usage) are switched to resolvedId so the generated React.useId() or InputGroup-provided id is used consistently.
🧹 Nitpick comments (13)
src/tedi/components/form/input-group/components/input/input.tsx (1)
13-13: Prefer a typed props object overUnknownType.Using
UnknownType(effectivelyany) forextraPropsdefeats type-checking on the cloned props. SincechildrenisReact.ReactElement, you can type this asPartial<React.ComponentProps<typeof children.type>> & { wrapperClassName?: string }or a narrow inline type to get real safety.As per coding guidelines: "Use proper TypeScript generics instead of
@ts-ignoreoranytypes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/input-group/components/input/input.tsx` at line 13, The extraProps object currently uses UnknownType which disables type-checking; change its type to a proper typed props shape such as Partial<React.ComponentProps<typeof children.type>> & { wrapperClassName?: string } (or a similarly narrow inline type) so cloned props on children are type-safe; update the declaration of extraProps in the input component (referencing extraProps and children) to use that typed generic instead of UnknownType and adjust any usage sites to satisfy the new type.src/tedi/components/form/select/components/select-control.tsx (1)
8-14: Consider className ordering consistency withSelectMenu.Here
props.classNameis passed last, so consumer classes win over BEM defaults; insrc/tedi/components/form/select/components/select-menu.tsxprops.classNameis passed first, so BEM wins. If that asymmetry is intentional (to let external callers override here), it's fine — otherwise consider aligning the two for predictable precedence across select subcomponents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/select/components/select-control.tsx` around lines 8 - 14, The className argument ordering is inconsistent with SelectMenu: in CustomControlBEM (cn call in select-control.tsx) props.className is passed last which lets consumer classes win, whereas SelectMenu passes props.className first so BEM classes win; make them consistent by reordering the cn arguments in CustomControlBEM to match SelectMenu (pass props.className as the first argument, then styles['tedi-select__control'] and the focused modifier styles['tedi-select__control--focused']), so precedence is predictable across select subcomponents.src/tedi/components/form/field/field.stories.tsx (1)
18-22: Consider adding stories for key visual/state variants.Only a
Defaultstory is provided. Per the contributing best-practices ("Default + one story per significant visual/state variant"), consider adding stories that exerciseinvalid,disabled,readOnly,isTextArea, and controlled vs. uncontrolled usage so the component's surface is discoverable in Storybook.As per coding guidelines: "Storybook: story files should follow required structure (Default + one story per significant visual/state variant)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/field/field.stories.tsx` around lines 18 - 22, Add additional Story exports alongside the existing Default to cover key visual/state variants: create exports named Invalid, Disabled, ReadOnly, TextArea (isTextArea), Controlled, and Uncontrolled; each should be a Story with appropriate args (e.g., Invalid: { id: 'field-invalid', invalid: true }, Disabled: { id: 'field-disabled', disabled: true }, ReadOnly: { id: 'field-readonly', readOnly: true }, TextArea: { id: 'field-textarea', isTextArea: true }, Uncontrolled: { id: 'field-uncontrolled', defaultValue: '...' }, Controlled: implement controlled props like value and onChange in args or a play function to demonstrate control). Ensure each new export uses the same Story type as Default and provides distinct ids and minimal props to exercise the component states.src/tedi/components/form/file-upload/file-upload.tsx (2)
195-204: Minor:label ?? ''is dead fallback.The surrounding condition already guarantees
labelis truthy, solabel={label ?? ''}can be simplified tolabel={label}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/file-upload/file-upload.tsx` around lines 195 - 204, The FormLabel prop uses a redundant fallback — change label={label ?? ''} to label={label} since the surrounding condition (!shouldHideLabel && label) already guarantees label is truthy; update the JSX where FormLabel is rendered (references: FormLabel component, resolvedId, label, readOnly, styles['tedi-file-upload__label'], size) to pass label directly without the nullish coalescing.
138-140: Optional chain on a named import is misleading.
useOptionalInputGroup?.()guards againstuseOptionalInputGroupbeing undefined, but it's a statically imported named export — it can never be undefined at runtime. Drop the optional chain for clarity:- const inputGroup = useOptionalInputGroup?.(); + const inputGroup = useOptionalInputGroup();Same pattern appears in
select.tsxandtextfield.tsx; consider aligning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/file-upload/file-upload.tsx` around lines 138 - 140, The optional chaining on the named import useOptionalInputGroup (used in the expression useOptionalInputGroup?.()) is misleading because a statically imported named export cannot be undefined; remove the redundant optional chain and call useOptionalInputGroup() directly to obtain inputGroup, then compute shouldHideLabel and resolvedId from that result (references: useOptionalInputGroup, inputGroup, shouldHideLabel, resolvedId). Apply the same change consistently in the other components that use this pattern (select.tsx and textfield.tsx) so all three files call useOptionalInputGroup() without the ?. operator.src/tedi/components/form/input-group/components/prefix/prefix.tsx (1)
7-7: Prop type too restrictive —...propshas no type to spread.The inline type only declares
childrenandclassName, yet the component destructures...propsand spreads them onto the<div>. With this typing, consumers cannot pass standard div attributes (id,role,data-*, event handlers) in a type-safe way, even though the runtime allows it.♻️ Suggested fix
-export const Prefix = ({ children, className, ...props }: { children: ReactNode; className?: string }) => { +type PrefixProps = React.HTMLAttributes<HTMLDivElement> & { children: ReactNode }; + +export const Prefix = ({ children, className, ...props }: PrefixProps) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/input-group/components/prefix/prefix.tsx` at line 7, The Prefix component's prop typing is too narrow—update the props type so that ...props can accept standard div attributes and event handlers (e.g., use React.HTMLAttributes<HTMLDivElement> or React.ComponentProps<'div'> combined with the existing children/className types) and apply that type to the ({ children, className, ...props }) parameter in the Prefix function so spreading props onto the <div> is type-safe; keep the function name Prefix and its destructuring but replace the inline prop type with the broader div prop type.src/tedi/components/form/input-group/input-group.module.scss (1)
25-28: Nit: hardcoded spacing values.
gap: 10px,min-width: 40px, andmargin-top: 4pxare hardcoded while the rest of the file uses design tokens. Consider switching to tokens (e.g. a spacing scale variable) for consistency.Also applies to: 144-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/input-group/input-group.module.scss` around lines 25 - 28, Replace hardcoded spacing values in src/tedi/components/form/input-group/input-group.module.scss by using the project design tokens/spacing scale instead of literal pixels: update the gap: 10px, min-width: 40px, and the margin-top: 4px occurrence (and the duplicate at the later occurrence) to use the shared spacing variables (e.g., the project's spacing token or CSS custom property used elsewhere in this file) so the InputGroup styles remain consistent with the design system and the other rules in this module.src/tedi/components/form/field/field.tsx (2)
133-135: Remove unnecessary optional chaining onuseOptionalInputGroup.
useOptionalInputGroupis a statically imported hook and is always defined;useOptionalInputGroup?.()is misleading (it suggests the hook might be missing) and masks real import errors. Additionally, line 135 readsprops.idwhileidhas already been destructured from props on line 97 — prefer the destructured local for consistency.♻️ Proposed cleanup
- const inputGroup = useOptionalInputGroup?.(); + const inputGroup = useOptionalInputGroup(); const generatedId = React.useId(); - const resolvedId = props.id ?? inputGroup?.inputId ?? generatedId; + const resolvedId = id ?? inputGroup?.inputId ?? generatedId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/field/field.tsx` around lines 133 - 135, Remove the unnecessary optional chaining and use the destructured id: call the imported hook directly as useOptionalInputGroup() to get inputGroup, keep generatedId from React.useId(), and compute resolvedId using the already-destructured id variable (resolvedId = id ?? inputGroup?.inputId ?? generatedId). Update references to props.id to use the local id for consistency and clarity, and remove the ?. after useOptionalInputGroup to avoid masking import errors.
120-122: Controlled↔uncontrolled switching is not guarded.
innerValueis only seeded fromexternalValueat mount; if a consumer later flips between controlled (valuedefined) and uncontrolled (valueundefined), the previously trackedinnerValuebecomes stale and the displayed value will jump. React's usual remedy is to either (a) warn in dev when the pattern switches, or (b) key the element to force remount. At minimum, consider documenting thatvaluemust not switch between defined/undefined across renders.As per learnings: Form control components must support both controlled and uncontrolled modes.
src/tedi/components/form/textfield/textfield.tsx (1)
277-280: Redundant optional call onuseOptionalInputGroupand inconsistentidaccess.Same two nits as in
field.tsx:useOptionalInputGroupis a statically imported hook (never undefined), souseOptionalInputGroup?.()is misleading. And line 280 readsprops.idwhileidis already destructured on line 234 — use the local consistently.♻️ Proposed cleanup
- const inputGroup = useOptionalInputGroup?.(); + const inputGroup = useOptionalInputGroup(); const generatedId = React.useId(); const shouldHideLabel = inputGroup?.hasExternalLabel; - const resolvedId = props.id ?? inputGroup?.inputId ?? generatedId; + const resolvedId = id ?? inputGroup?.inputId ?? generatedId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/textfield/textfield.tsx` around lines 277 - 280, Remove the misleading optional call and use the destructured id consistently: call useOptionalInputGroup() (not useOptionalInputGroup?.()) to get inputGroup, keep generatedId and shouldHideLabel as-is, and compute resolvedId using the local id variable (resolvedId = id ?? inputGroup?.inputId ?? generatedId) instead of props.id so the component uses the previously destructured id and avoids the redundant optional chaining on useOptionalInputGroup.src/tedi/components/form/input-group/input-group.tsx (2)
97-107: Consider co-locating the context in its own module to avoid cyclic imports.
useInputGroup/useOptionalInputGroupare exported from the same module that also imports./components/input/input,./components/prefix/prefix,./components/suffix/suffix(which in turn importuseInputGroupfrom this very module). This creates an import cycleinput-group.tsx ↔ components/{input,prefix,suffix}/*.tsxthat works today but is fragile — any future top-level side effect or re-export reordering can surface asundefinedat module init. Splitting the context + hooks intoinput-group-context.tsx(or similar) removes the cycle entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/input-group/input-group.tsx` around lines 97 - 107, The InputGroupContext and its hooks (InputGroupContext, useInputGroup, useOptionalInputGroup) should be extracted into a separate module (e.g., input-group-context.tsx) to break the import cycle between input-group.tsx and components/input|prefix|suffix; move the createContext and both hooks into that new file, export them, then update input-group.tsx and the child components to import the context/hooks from the new module instead of from input-group.tsx so the circular dependency is removed.
132-138: InlineregisterPrefix/unregisterPrefixidentities thrashPrefix/Suffixeffects.
registerPrefix,unregisterPrefix,registerSuffix,unregisterSuffixare inline arrow functions insideuseMemo, so every time one of the memo deps (hasPrefix,hasSuffix,disabled,inputId) changes, their references change too. Given thatPrefixandSuffixlist these in their ownuseEffectdeps (seesrc/tedi/components/form/input-group/components/prefix/prefix.tsxand.../suffix/suffix.tsx), each mount triggers:registerPrefix→hasPrefixflips → ctx re-memoized → newregisterPrefix/unregisterPrefixidentities → child effect cleanup runsunregisterPrefix→ effect re-runsregisterPrefix. React 18's automatic batching currently keeps this from looping, but it's needlessly fragile (and likely broken in any sync state-update path).Extract stable callbacks with
useCallback(or put setters directly as identity-stable deps):♻️ Proposed refactor
+ const registerPrefix = useCallback(() => setHasPrefix(true), []); + const unregisterPrefix = useCallback(() => setHasPrefix(false), []); + const registerSuffix = useCallback(() => setHasSuffix(true), []); + const unregisterSuffix = useCallback(() => setHasSuffix(false), []); + const ctxValue = useMemo( () => ({ hasPrefix, hasSuffix, disabled, hasExternalLabel: !!label, inputId, - registerPrefix: () => setHasPrefix(true), - unregisterPrefix: () => setHasPrefix(false), - registerSuffix: () => setHasSuffix(true), - unregisterSuffix: () => setHasSuffix(false), + registerPrefix, + unregisterPrefix, + registerSuffix, + unregisterSuffix, }), - [inputId, hasPrefix, hasSuffix, disabled] + [inputId, hasPrefix, hasSuffix, disabled, label, registerPrefix, unregisterPrefix, registerSuffix, unregisterSuffix] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/input-group/input-group.tsx` around lines 132 - 138, The memoized context object passed from the InputGroup component uses inline arrow functions for registerPrefix/unregisterPrefix and registerSuffix/unregisterSuffix inside useMemo, causing unstable identities that thrash Prefix/Suffix useEffect hooks; to fix, make these callbacks identity-stable by extracting them with useCallback (e.g., define registerPrefix = useCallback(() => setHasPrefix(true), [setHasPrefix]) and similarly for unregisterPrefix/registerSuffix/unregisterSuffix) and then return those stable functions from the useMemo (or instead pass the setHasPrefix/setHasSuffix setters directly if you prefer), ensuring useMemo's deps include only truly changing values like inputId and disabled so Prefix/Suffix effects no longer run cleanup/re-register on unrelated state changes.src/tedi/components/form/input-group/input-group.spec.tsx (1)
11-78: Prefer semantic queries for label/input linkage where possible.The suite is heavy on
container.querySelector('[data-name="..."]'). Root-element and feedback-wrapper queries are legitimately structural, but the label/id tests (e.g. lines 80–106) could usegetByLabelText('Address')to assert that the label actually binds to the<input>through the browser's accessibility tree — that would catch thehtmlFor/idmismatch class of bugs more robustly thanquerySelector('label')plustoHaveAttribute('for', ...).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/form/input-group/input-group.spec.tsx` around lines 11 - 78, Replace structural label/id assertions that use container.querySelector('label') and attribute checks with semantic queries: use getByLabelText('Address') (or getByRole where appropriate) to locate the associated input element and assert it is in the document and correctly tied to the label; then, if you still need to assert the label's htmlFor matches the input id, get the label via getByText('Address') or container.querySelector('label') and assert label.htmlFor === the input.id. Update tests around InputGroup, InputGroup.Input, and label/id assertions to use getByLabelText/getByRole to verify real accessibility linkage instead of relying solely on querySelector.
🤖 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/form/field/field.tsx`:
- Around line 139-160: The Field component spreads inputProps/textareaProps
after sharedProps which allows consumer props to overwrite internal wiring
(sharedProps keys like onChange -> handleChange, value, onFocus, onBlur,
ref/innerRef), breaking internal state and callbacks; change the spread order so
sharedProps are spread last (ensure sharedProps overrides
inputProps/textareaProps) or explicitly merge handlers (e.g., compose consumer
onChange with handleChange) and prevent consumer from overriding ref by applying
ref={innerRef} after merges; update usage around sharedProps, inputProps,
textareaProps, handleChange, innerRef, value, onChange, onFocus, onBlur
accordingly and add JSDoc to inputProps/textareaProps documenting precedence if
you intend consumer override.
In `@src/tedi/components/form/input-group/components/input/input.tsx`:
- Around line 23-24: The duplicate assignments to Input.displayName overwrite
the namespaced value; remove the incorrect second assignment and keep the
intended namespaced name (e.g., set only Input.displayName = 'InputGroup.Input')
so the component exposes the public name; update the Input component's
displayName by deleting the line that sets Input.displayName = 'Input' and
ensure only Input.displayName = 'InputGroup.Input' remains.
- Around line 13-18: The current coalescing in extraProps uses nullish
coalescing (disabled: disabled ?? children.props.disabled) which lets a parent
explicitly set disabled={false} override a child's disabled={true}; change the
logic in extraProps so disabled is computed with logical OR between parent and
child (e.g., disabled: disabled || children.props.disabled) to ensure either
source can disable the child, keep the other mappings (id, className,
wrapperClassName) as-is so FileUpload and similar children still receive
wrapperClassName.
In `@src/tedi/components/form/input-group/components/prefix/prefix.tsx`:
- Around line 22-23: The file sets Prefix.displayName twice (Prefix.displayName
= 'InputGroup.Prefix'; followed by Prefix.displayName = 'Prefix'), so remove the
redundant assignment and keep the intended value; locate the Prefix component's
displayName assignments in prefix.tsx and delete the first/incorrect line (the
one assigning 'InputGroup.Prefix') so only the chosen displayName remains on the
Prefix symbol.
- Line 16: In the Prefix component (prefix.tsx) the native div spreads
{...props} after setting aria-disabled={disabled}, allowing consumer props to
override the context-driven disabled state; change the element render order so
{...props} is spread before aria-disabled={disabled} (i.e., on the div with
classNames(styles['tedi-input-group__prefix'], className) ensure props are
applied first and then set aria-disabled using the local disabled variable) so
the InputGroup-derived aria-disabled cannot be clobbered by consumer props.
In `@src/tedi/components/form/input-group/components/suffix/suffix.tsx`:
- Around line 7-23: The Suffix component's props should be widened to accept
standard div attributes so forward props work: change the prop type on Suffix to
use React.HTMLAttributes<HTMLDivElement> (including children) instead of the
inline `{ children: ReactNode; className?: string }`; spread `...props` before
setting `aria-disabled={disabled}` so context-driven disabled can't be
overridden; and remove the duplicate displayName assignment on Suffix (keep a
single assignment, e.g. `Suffix.displayName = 'Suffix'`). Use the Suffix
component, useInputGroup hook, and registerSuffix/unregisterSuffix names to
locate the changes.
In `@src/tedi/components/form/input-group/input-group.spec.tsx`:
- Around line 95-106: The test "generates an id automatically when none is
provided" is passing an explicit id, so remove the id="test-group" prop from the
rendered InputGroup to exercise the auto-id path in InputGroup (and keep the
same children using InputGroup.Input). After removing the id prop, assert that a
generated id exists on the input and that the label's for attribute matches that
input id (query the rendered input and compare its id to
label?.getAttribute('for')) to verify React.useId() fallback behavior.
In `@src/tedi/components/form/input-group/input-group.stories.tsx`:
- Around line 392-437: Multiple InputGroup instances reuse id="state-example"
and inner Field components set id={state}, causing duplicate DOM IDs and broken
label wiring because InputGroup.Input clones children with id: children.props.id
?? inputId; fix by removing the explicit id={state} from Field (let InputGroup
supply inputId) or generate unique per-cell ids and pass them consistently to
both InputGroup id and the inner Field (e.g., compute a uniqueInputId per cell
and use <InputGroup id={uniqueInputId}> and <Field id={uniqueInputId}>); apply
the same change to the Error row and any other repeated instances so FormLabel
htmlFor (from InputGroup) always matches the actual input id.
In `@src/tedi/components/form/input-group/input-group.tsx`:
- Around line 125-138: The ctxValue memo omits the label dependency so
hasExternalLabel: !!label can become stale; update the useMemo dependency array
for ctxValue (the memo that builds hasPrefix, hasSuffix, disabled,
hasExternalLabel, inputId, registerPrefix/unregisterPrefix,
registerSuffix/unregisterSuffix) to include label (or the derived boolean like
!!label) along with inputId, hasPrefix, hasSuffix, and disabled so consumers
relying on inputGroup?.hasExternalLabel see updates when label changes.
In `@src/tedi/components/form/search/search.tsx`:
- Line 10: The Search component broke consumers by routing the incoming
className to the input; change the API to restore className as the wrapper class
and add a separate inputClassName for the underlying TextField: update the
SearchProps interface to accept className (for the wrapper) and inputClassName
(passed to TextField), ensure the wrapper <div> in the Search component receives
className while passing inputClassName into the TextField prop (inputClassName
or className-as-input depending on TextField API), and update any internal
usages/tests to use the new inputClassName so existing consumers' layout styles
continue to apply to the wrapper.
In `@src/tedi/components/form/textfield/textfield.tsx`:
- Around line 388-400: renderFeedback currently uses the possibly-undefined prop
id for array helper keys/ids which yields "undefined-helper-*" and can collide;
update renderFeedback so the array branch uses resolvedId (same as the
single-helper branch) when constructing the FeedbackText id and key generation;
specifically modify the helper.map branch in renderFeedback to pass
id={`${resolvedId}-helper-${index}`} (and keep key usage consistent, e.g., the
index or a stable identifier) so FeedbackText receives resolvedId-derived ids
instead of id.
- Around line 432-436: The current handler forwards input?.onFocus but drops
input?.onBlur and masks typing with "as UnknownType"; update the focus/blur
wiring to consistently call both input?.onFocus?.(e) and input?.onBlur?.(e) in
addition to the prop handlers (onFocus?.(e), onBlur?.(e)). Remove the unsafe
cast and narrow the unioned input prop before invoking handlers — e.g., detect
whether the underlying element is an input or textarea (or use a generic
component prop type) and call the handler with the correctly typed FocusEvent
(React.FocusEvent<HTMLInputElement> or React.FocusEvent<HTMLTextAreaElement>) so
both input and prop handlers are invoked with proper types for handlers
referenced in this file (input, onFocus, onBlur).
- Line 438: The aria-describedby value is incorrect when helper is an array and
when id is undefined; update the TextField rendering to use resolvedId (not id)
and compute describedBy: if resolvedId is falsy return undefined; if helper is
an array join the IDs `${resolvedId}-helper-0 ${resolvedId}-helper-1 ...` (or
map indices) into a space-separated string, otherwise use
`${resolvedId}-helper`; set aria-describedby to that computed string where
currently aria-describedby={helper ? `${id}-helper` : undefined} so assistive
tech points to the real helper element IDs.
---
Outside diff comments:
In `@src/tedi/components/form/select/select.tsx`:
- Around line 248-260: The select component still passes the raw id prop into
react-select and the FormLabel, causing "undefined-input" and broken htmlFor
linkage; update all places using id for the react-select props and label to use
resolvedId instead (replace instanceId={id} with instanceId={resolvedId},
inputId={`${resolvedId}-input`} and ensure FormLabel's id/htmlFor use
`${resolvedId}-input`) and verify any other occurrences between the render block
(around instanceId/inputId and the FormLabel usage) are switched to resolvedId
so the generated React.useId() or InputGroup-provided id is used consistently.
---
Nitpick comments:
In `@src/tedi/components/form/field/field.stories.tsx`:
- Around line 18-22: Add additional Story exports alongside the existing Default
to cover key visual/state variants: create exports named Invalid, Disabled,
ReadOnly, TextArea (isTextArea), Controlled, and Uncontrolled; each should be a
Story with appropriate args (e.g., Invalid: { id: 'field-invalid', invalid: true
}, Disabled: { id: 'field-disabled', disabled: true }, ReadOnly: { id:
'field-readonly', readOnly: true }, TextArea: { id: 'field-textarea',
isTextArea: true }, Uncontrolled: { id: 'field-uncontrolled', defaultValue:
'...' }, Controlled: implement controlled props like value and onChange in args
or a play function to demonstrate control). Ensure each new export uses the same
Story type as Default and provides distinct ids and minimal props to exercise
the component states.
In `@src/tedi/components/form/field/field.tsx`:
- Around line 133-135: Remove the unnecessary optional chaining and use the
destructured id: call the imported hook directly as useOptionalInputGroup() to
get inputGroup, keep generatedId from React.useId(), and compute resolvedId
using the already-destructured id variable (resolvedId = id ??
inputGroup?.inputId ?? generatedId). Update references to props.id to use the
local id for consistency and clarity, and remove the ?. after
useOptionalInputGroup to avoid masking import errors.
In `@src/tedi/components/form/file-upload/file-upload.tsx`:
- Around line 195-204: The FormLabel prop uses a redundant fallback — change
label={label ?? ''} to label={label} since the surrounding condition
(!shouldHideLabel && label) already guarantees label is truthy; update the JSX
where FormLabel is rendered (references: FormLabel component, resolvedId, label,
readOnly, styles['tedi-file-upload__label'], size) to pass label directly
without the nullish coalescing.
- Around line 138-140: The optional chaining on the named import
useOptionalInputGroup (used in the expression useOptionalInputGroup?.()) is
misleading because a statically imported named export cannot be undefined;
remove the redundant optional chain and call useOptionalInputGroup() directly to
obtain inputGroup, then compute shouldHideLabel and resolvedId from that result
(references: useOptionalInputGroup, inputGroup, shouldHideLabel, resolvedId).
Apply the same change consistently in the other components that use this pattern
(select.tsx and textfield.tsx) so all three files call useOptionalInputGroup()
without the ?. operator.
In `@src/tedi/components/form/input-group/components/input/input.tsx`:
- Line 13: The extraProps object currently uses UnknownType which disables
type-checking; change its type to a proper typed props shape such as
Partial<React.ComponentProps<typeof children.type>> & { wrapperClassName?:
string } (or a similarly narrow inline type) so cloned props on children are
type-safe; update the declaration of extraProps in the input component
(referencing extraProps and children) to use that typed generic instead of
UnknownType and adjust any usage sites to satisfy the new type.
In `@src/tedi/components/form/input-group/components/prefix/prefix.tsx`:
- Line 7: The Prefix component's prop typing is too narrow—update the props type
so that ...props can accept standard div attributes and event handlers (e.g.,
use React.HTMLAttributes<HTMLDivElement> or React.ComponentProps<'div'> combined
with the existing children/className types) and apply that type to the ({
children, className, ...props }) parameter in the Prefix function so spreading
props onto the <div> is type-safe; keep the function name Prefix and its
destructuring but replace the inline prop type with the broader div prop type.
In `@src/tedi/components/form/input-group/input-group.module.scss`:
- Around line 25-28: Replace hardcoded spacing values in
src/tedi/components/form/input-group/input-group.module.scss by using the
project design tokens/spacing scale instead of literal pixels: update the gap:
10px, min-width: 40px, and the margin-top: 4px occurrence (and the duplicate at
the later occurrence) to use the shared spacing variables (e.g., the project's
spacing token or CSS custom property used elsewhere in this file) so the
InputGroup styles remain consistent with the design system and the other rules
in this module.
In `@src/tedi/components/form/input-group/input-group.spec.tsx`:
- Around line 11-78: Replace structural label/id assertions that use
container.querySelector('label') and attribute checks with semantic queries: use
getByLabelText('Address') (or getByRole where appropriate) to locate the
associated input element and assert it is in the document and correctly tied to
the label; then, if you still need to assert the label's htmlFor matches the
input id, get the label via getByText('Address') or
container.querySelector('label') and assert label.htmlFor === the input.id.
Update tests around InputGroup, InputGroup.Input, and label/id assertions to use
getByLabelText/getByRole to verify real accessibility linkage instead of relying
solely on querySelector.
In `@src/tedi/components/form/input-group/input-group.tsx`:
- Around line 97-107: The InputGroupContext and its hooks (InputGroupContext,
useInputGroup, useOptionalInputGroup) should be extracted into a separate module
(e.g., input-group-context.tsx) to break the import cycle between
input-group.tsx and components/input|prefix|suffix; move the createContext and
both hooks into that new file, export them, then update input-group.tsx and the
child components to import the context/hooks from the new module instead of from
input-group.tsx so the circular dependency is removed.
- Around line 132-138: The memoized context object passed from the InputGroup
component uses inline arrow functions for registerPrefix/unregisterPrefix and
registerSuffix/unregisterSuffix inside useMemo, causing unstable identities that
thrash Prefix/Suffix useEffect hooks; to fix, make these callbacks
identity-stable by extracting them with useCallback (e.g., define registerPrefix
= useCallback(() => setHasPrefix(true), [setHasPrefix]) and similarly for
unregisterPrefix/registerSuffix/unregisterSuffix) and then return those stable
functions from the useMemo (or instead pass the setHasPrefix/setHasSuffix
setters directly if you prefer), ensuring useMemo's deps include only truly
changing values like inputId and disabled so Prefix/Suffix effects no longer run
cleanup/re-register on unrelated state changes.
In `@src/tedi/components/form/select/components/select-control.tsx`:
- Around line 8-14: The className argument ordering is inconsistent with
SelectMenu: in CustomControlBEM (cn call in select-control.tsx) props.className
is passed last which lets consumer classes win, whereas SelectMenu passes
props.className first so BEM classes win; make them consistent by reordering the
cn arguments in CustomControlBEM to match SelectMenu (pass props.className as
the first argument, then styles['tedi-select__control'] and the focused modifier
styles['tedi-select__control--focused']), so precedence is predictable across
select subcomponents.
In `@src/tedi/components/form/textfield/textfield.tsx`:
- Around line 277-280: Remove the misleading optional call and use the
destructured id consistently: call useOptionalInputGroup() (not
useOptionalInputGroup?.()) to get inputGroup, keep generatedId and
shouldHideLabel as-is, and compute resolvedId using the local id variable
(resolvedId = id ?? inputGroup?.inputId ?? generatedId) instead of props.id so
the component uses the previously destructured id and avoids the redundant
optional chaining on useOptionalInputGroup.
🪄 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: a0adac50-2838-45f9-947a-49173bc14001
📒 Files selected for processing (21)
src/tedi/components/form/field/field.module.scsssrc/tedi/components/form/field/field.stories.tsxsrc/tedi/components/form/field/field.tsxsrc/tedi/components/form/file-upload/file-upload.tsxsrc/tedi/components/form/input-group/components/input/input.spec.tsxsrc/tedi/components/form/input-group/components/input/input.tsxsrc/tedi/components/form/input-group/components/prefix/prefix.spec.tsxsrc/tedi/components/form/input-group/components/prefix/prefix.tsxsrc/tedi/components/form/input-group/components/suffix/suffix.spec.tsxsrc/tedi/components/form/input-group/components/suffix/suffix.tsxsrc/tedi/components/form/input-group/index.tssrc/tedi/components/form/input-group/input-group.module.scsssrc/tedi/components/form/input-group/input-group.spec.tsxsrc/tedi/components/form/input-group/input-group.stories.tsxsrc/tedi/components/form/input-group/input-group.tsxsrc/tedi/components/form/search/search.tsxsrc/tedi/components/form/select/components/select-control.tsxsrc/tedi/components/form/select/select.module.scsssrc/tedi/components/form/select/select.tsxsrc/tedi/components/form/textfield/textfield.tsxsrc/tedi/index.ts
Contributor
Author
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Approval is disabled; enable |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/form/input-group/components/input/input.tsx`:
- Around line 13-18: The component currently always adds wrapperClassName to
extraProps when cloning children, causing unknown DOM props on intrinsic
elements; update the cloning logic inside InputGroup.Input (where extraProps is
created and used) to only include wrapperClassName when children is a React
element whose type is not a string (i.e., a composite component) or when
children.props.accepts wrapperClassName exists; specifically, compute
wrapperClassName conditionally (e.g., only set extraProps.wrapperClassName if
typeof children.type !== 'string' or React.isValidElement(children) &&
children.type !== 'string') and then cloneElement with that filtered extraProps
so native inputs never receive wrapperClassName.
In `@src/tedi/components/form/input-group/input-group.stories.tsx`:
- Around line 37-49: Add a canonical Default story export for this module so
Storybook shows the baseline composition; create export const Default: Story
that mirrors the standard InputGroup layout used in StartStatic (use the same
args: { label: 'Address' } and the same render content that composes
<InputGroup> with <InputGroup.Prefix>Street</InputGroup.Prefix>,
<InputGroup.Input> and <Field id="start-static-1" />) — you can either duplicate
the render from StartStatic or refactor the render into a shared function and
reference it from both Default and StartStatic.
In `@src/tedi/components/form/textfield/textfield.tsx`:
- Around line 307-319: handleChange and clearInput always call setInnerValue,
causing internal state to diverge in controlled mode; update them to only call
setInnerValue when the component is uncontrolled (use the existing isControlled
flag or derive isControlled from externalValue/props), while still invoking
onChange and onClear as before; specifically modify the functions handleChange
and clearInput to guard setInnerValue behind if (!isControlled) (keep
onChange?.(newValue) and onChange?.('') and onClear?.() intact) so internal
innerValue is only mutated in uncontrolled mode.
- Around line 334-345: The icon wrapper currently uses Wrapper (computed from
onIconClick) but always applies tabIndex, disabled, and onClick props making a
non-interactive <div> focusable and invalid; update the render logic in the
textfield component so when onIconClick is present you render the interactive
button (type="button", onClick={!disabled ? onIconClick : undefined}, tabIndex
and disabled as now), and when onIconClick is absent render a plain
non-interactive div: remove tabIndex, disabled and onClick, and add
aria-hidden="true" to the non-interactive wrapper; adjust usage around Wrapper,
onIconClick, Icon and styles['tedi-textfield__icon-wrapper'] accordingly.
🪄 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: b63dcd01-ca5e-4faf-9c2c-01af7cc0c2a9
📒 Files selected for processing (10)
src/tedi/components/form/field/field.tsxsrc/tedi/components/form/input-group/components/input/input.tsxsrc/tedi/components/form/input-group/components/prefix/prefix.tsxsrc/tedi/components/form/input-group/components/suffix/suffix.tsxsrc/tedi/components/form/input-group/input-group.module.scsssrc/tedi/components/form/input-group/input-group.spec.tsxsrc/tedi/components/form/input-group/input-group.stories.tsxsrc/tedi/components/form/input-group/input-group.tsxsrc/tedi/components/form/search/search.tsxsrc/tedi/components/form/textfield/textfield.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/tedi/components/form/input-group/components/prefix/prefix.tsx
- src/tedi/components/form/search/search.tsx
- src/tedi/components/form/input-group/components/suffix/suffix.tsx
- src/tedi/components/form/input-group/input-group.module.scss
- src/tedi/components/form/field/field.tsx
- src/tedi/components/form/input-group/input-group.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tedi/components/form/textfield/textfield.tsx (1)
232-264:⚠️ Potential issue | 🟠 MajorRoute the leftover
FormLabelPropstoFormLabel, not the wrapper.Anything left in
restafter destructuring is part of theFormLabelPropssurface, but it's currently spread onto the outer<div>. That drops label features and can leak unknown props into the DOM.file-upload.tsxin this PR already forwards the same...restintoFormLabel.🐛 Proposed fix
- ...rest + ...labelProps } = getCurrentBreakpointProps<TextFieldProps>(props) || {}; ... - <div data-name="textfield" {...rest} className={TextFieldBEM}> + <div data-name="textfield" className={TextFieldBEM}> {!shouldHideLabel && ( - <FormLabel id={resolvedId} label={label} required={required} hideLabel={hideLabel} size={labelSize} /> + <FormLabel + {...labelProps} + id={resolvedId} + label={label} + required={required} + hideLabel={hideLabel} + size={labelSize} + /> )}Also applies to: 408-410
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/textfield/textfield.tsx` around lines 232 - 264, The leftover props captured in rest from getCurrentBreakpointProps<TextFieldProps>(props) are FormLabelProps and should be forwarded to the FormLabel component instead of being spread onto the outer wrapper div; update the TextField component to stop spreading ...rest onto the outer <div> and instead pass ...rest into the FormLabel (the same pattern used in file-upload.tsx), and make the same change for the secondary destructuring site around the input/render logic (previously referenced at the second rest usage), ensuring unknown label props are not leaked into the DOM and label features are preserved.
🤖 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/form/file-upload/file-upload.tsx`:
- Around line 129-137: helperId is only generated when the helper prop exists,
so hook-provided uploadErrorHelper (from useFileUpload) isn't given an id and
aria-describedby stays empty; update the logic that sets helperId to also
consider uploadErrorHelper (or any hook-driven helper) by generating an id like
`${resolvedId}-helper` when helper is falsy but uploadErrorHelper is present,
ensure FeedbackText receives that id, and that the input's aria-describedby uses
helperId so assistive tech links the error to the input (look for helperId,
helper prop, useFileUpload, uploadErrorHelper, FeedbackText, and resolvedId in
the file).
In `@src/tedi/components/form/textfield/textfield.tsx`:
- Around line 273-275: The clear-button is shown and active even when the field
is readOnly; update the boolean logic that computes showClearButton (which
currently uses isClearable and value derived from externalValue ?? innerValue)
to also require !readOnly, and ensure clearInput (and any handlers like
handleClearClick) no-ops when readOnly is true so the value is never mutated;
update every occurrence (the showClearButton computation and clear handler(s)
around the value/clearInput logic referenced in this file) to include readOnly
checks.
---
Outside diff comments:
In `@src/tedi/components/form/textfield/textfield.tsx`:
- Around line 232-264: The leftover props captured in rest from
getCurrentBreakpointProps<TextFieldProps>(props) are FormLabelProps and should
be forwarded to the FormLabel component instead of being spread onto the outer
wrapper div; update the TextField component to stop spreading ...rest onto the
outer <div> and instead pass ...rest into the FormLabel (the same pattern used
in file-upload.tsx), and make the same change for the secondary destructuring
site around the input/render logic (previously referenced at the second rest
usage), ensuring unknown label props are not leaked into the DOM and label
features are preserved.
🪄 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: 9e34756c-6325-46b7-893d-b335cf4c9ace
📒 Files selected for processing (6)
src/tedi/components/form/file-upload/file-upload.tsxsrc/tedi/components/form/input-group/components/input/input.tsxsrc/tedi/components/form/input-group/components/prefix/prefix.tsxsrc/tedi/components/form/input-group/components/suffix/suffix.tsxsrc/tedi/components/form/input-group/input-group.module.scsssrc/tedi/components/form/textfield/textfield.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/tedi/components/form/input-group/components/input/input.tsx
- src/tedi/components/form/input-group/components/suffix/suffix.tsx
- src/tedi/components/form/input-group/components/prefix/prefix.tsx
- src/tedi/components/form/input-group/input-group.module.scss
ly-tempel-bitweb
approved these changes
Apr 29, 2026
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
3 participants
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
Refactor
Tests
Documentation
Style