refactor: replace old Checkbox component and Added new TextInput, MaskedInput, and Select components#7697
Open
abhishek-bruno wants to merge 2 commits intousebruno:mainfrom
Open
Conversation
…kedInput, and Select components - Deleted the old Checkbox and StyledWrapper components from the components directory. - Introduced a new Checkbox component with enhanced features including indeterminate state and customizable icons. - Added a new StyledWrapper for the Checkbox to manage styles and layout. - Created InputWrapper and StyledWrapper components for consistent form field handling. - Added new TextInput, MaskedInput, and Select components with their respective styles for improved UI consistency.
abhishek-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
WalkthroughThe PR removes the legacy Checkbox under Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
packages/bruno-app/src/ui/InputWrapper/StyledWrapper.js (1)
1-2: ExtractINPUT_SIZESto a separate constants module to break the circular dependency.
StyledWrapper.jsimportsINPUT_SIZESfrom./index, while./indeximportsStyledWrapper. This circular dependency creates an initialization order risk. SinceINPUT_SIZESis a static object with no dependencies, move it toconstants.jsand import it from both files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/ui/InputWrapper/StyledWrapper.js` around lines 1 - 2, The import of INPUT_SIZES from ./index creates a circular dependency with StyledWrapper; extract INPUT_SIZES into a new constants module (e.g., constants.js) exported as a named export and update references: remove INPUT_SIZES from ./index, export it from constants.js, then import { INPUT_SIZES } in StyledWrapper.js and in index.js (or any other consumers) instead of from ./index; ensure the StyledWrapper component (StyledWrapper) uses the imported constant and that index.js re-exports or imports StyledWrapper without reintroducing the constant to avoid the circular init problem.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/ui/ActionIcon/StyledWrapper.js`:
- Around line 46-49: The hover color check in StyledWrapper uses
props.$colorOnHover but ActionIcon passes the numeric token as $n, so the hover
branch never runs; update StyledWrapper to use props.$n (replace all occurrences
of props.$colorOnHover with props.$n) and keep the theme lookup using
props.theme.colors?.text?.[props.$n] || props.$n so the hover color resolves
correctly when ActionIcon supplies $n.
In `@packages/bruno-app/src/ui/Checkbox/index.js`:
- Around line 101-107: The checkbox label/description/error block in the
Checkbox component is not using a semantic <label> or ARIA linkage; update the
render so the visible text is wrapped in a <label htmlFor={inputId}> (or use
aria-labelledby) and ensure the checkbox input has a stable id (generate one
inside the Checkbox component if no id prop is provided, e.g. inputId), and also
wire description/error via aria-describedby using unique IDs (e.g.
`${inputId}-desc`, `${inputId}-err`) so assistive tech can reliably associate
the text with the input.
- Around line 93-94: Add a runtime prop validation in the Checkbox component to
enforce controlled vs uncontrolled usage: inside the Checkbox function (or
component render) check if both props checked and defaultChecked are provided
and throw or console.error and return null (or throw an Error) to prevent
rendering; update the component's prop validation (propTypes or TypeScript
types) if present to document exclusivity and ensure callers only pass one of
checked or defaultChecked, referencing the checked and defaultChecked props and
the Checkbox component name to locate the change.
In `@packages/bruno-app/src/ui/Checkbox/StyledWrapper.js`:
- Line 47: The focus ring currently appends "40" to the color string in
StyledWrapper's box-shadow which only works for hex colors; change it to
normalize the color and apply alpha properly (e.g., use a utility like
polished's rgba or tinycolor2) and replace box-shadow: 0 0 0 2px ${(props) =>
(props.$color || props.theme.primary.solid)}40; with a call that converts
(props.$color || props.theme.primary.solid) to an rgba/hsla with alpha 0.25 (or
equivalent) so rgb()/hsl()/named colors are handled correctly.
- Line 11: The style lookup uses SIZES[props.$size || 'md'] which can throw when
props.$size is an unexpected truthy value; in StyledWrapper guard every access
by resolving a safe size key first (e.g., check
SIZES.hasOwnProperty(props.$size) or SIZES[props.$size] and fall back to 'md')
and then use that resolved key for gap and the other size-based reads; update
the gap line and the other occurrences (lines ~19-20) to use the validated
sizeKey (referencing SIZES and props.$size in your fix).
In `@packages/bruno-app/src/ui/MaskedInput/index.js`:
- Around line 92-95: The visibility toggle button currently is removed from
keyboard navigation via tabIndex={-1} and lacks an accessible name; update the
button in the MaskedInput component (the element using handleToggle, isVisible
and className="masked-input-toggle") by removing tabIndex={-1} so it remains
tabbable and add an aria-label that reflects the current state (e.g., show "Hide
value" when isVisible is true and "Show value" when false) so screen reader
users can understand the action.
In `@packages/bruno-app/src/ui/Select/index.js`:
- Around line 234-244: The clear control in the Select component is currently a
non-focusable span; make it keyboard-accessible by replacing that span with a
semantic button (or, if keeping the span, add tabIndex={0} and keyDown handler
that calls handleClear on Enter/Space) and ensure ARIA label remains ("Clear
selection") and existing className ("select-section select-right-section
select-clear") and IconX usage are preserved; update any event handlers to
preventDefault where needed and ensure handleClear is called for both click and
keyboard activation.
- Around line 291-301: The trigger div is missing programmatic
labeling/descriptions; update the trigger JSX in Select (the const trigger
element) to include aria-labelledby and aria-describedby attributes that
reference the stable ids provided by InputWrapper (e.g., props like labelId,
descriptionId, errorId or the existing wrapper id props). Construct
aria-describedby by joining descriptionId and errorId only if they exist, and
set aria-labelledby to the label id if present; apply the same changes to the
other trigger rendering later (the other trigger instance around lines 345-346)
so assistive tech can read the label/description/error.
---
Nitpick comments:
In `@packages/bruno-app/src/ui/InputWrapper/StyledWrapper.js`:
- Around line 1-2: The import of INPUT_SIZES from ./index creates a circular
dependency with StyledWrapper; extract INPUT_SIZES into a new constants module
(e.g., constants.js) exported as a named export and update references: remove
INPUT_SIZES from ./index, export it from constants.js, then import { INPUT_SIZES
} in StyledWrapper.js and in index.js (or any other consumers) instead of from
./index; ensure the StyledWrapper component (StyledWrapper) uses the imported
constant and that index.js re-exports or imports StyledWrapper without
reintroducing the constant to avoid the circular init problem.
🪄 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: 3b631d22-3da6-4bc3-bd41-ae3f56cb2a0b
📒 Files selected for processing (13)
packages/bruno-app/src/components/Checkbox/StyledWrapper.jspackages/bruno-app/src/components/Checkbox/index.jspackages/bruno-app/src/ui/ActionIcon/StyledWrapper.jspackages/bruno-app/src/ui/Checkbox/StyledWrapper.jspackages/bruno-app/src/ui/Checkbox/index.jspackages/bruno-app/src/ui/InputWrapper/StyledWrapper.jspackages/bruno-app/src/ui/InputWrapper/index.jspackages/bruno-app/src/ui/MaskedInput/StyledWrapper.jspackages/bruno-app/src/ui/MaskedInput/index.jspackages/bruno-app/src/ui/Select/StyledWrapper.jspackages/bruno-app/src/ui/Select/index.jspackages/bruno-app/src/ui/TextInput/StyledWrapper.jspackages/bruno-app/src/ui/TextInput/index.js
💤 Files with no reviewable changes (2)
- packages/bruno-app/src/components/Checkbox/index.js
- packages/bruno-app/src/components/Checkbox/StyledWrapper.js
sanish-bruno
reviewed
Apr 8, 2026
Collaborator
sanish-bruno
left a comment
sanish-bruno
left a comment
There was a problem hiding this comment.
PR Review
Found 4 issue(s): 1 major, 2 minor, 1 nit.
Summary
- Circular import between
InputWrapper/index.jsandInputWrapper/StyledWrapper.js—StyledWrapperimportsINPUT_SIZESfromindex.jswhileindex.jsimportsStyledWrapperfromStyledWrapper.js Selectcomponent doesn't passhtmlFororrequiredtoInputWrapper, so labels aren't linked and required asterisks won't render- Edge case in
Selectkeyboard navigation when all filtered options are disabled - The ActionIcon change seems unrelated to the PR's stated scope
…accessibility improvements - Added useId for unique ID generation in Checkbox and Select components, improving accessibility. - Updated Checkbox to use generated IDs for aria attributes and labels. - Refactored InputWrapper to accept and apply generated IDs for label, description, and error elements. - Modified Select component to include aria attributes for better screen reader support. - Adjusted StyledWrapper imports to reference constants for consistent styling across components.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/bruno-app/src/ui/InputWrapper/index.js (1)
18-30: Add explicitdata-testidpassthrough props for Playwright targets.This component renders stable testable nodes (wrapper/label/description/error), but it currently doesn’t expose test IDs for them.
♻️ Proposed refactor
/** * InputWrapper - Shared form field wrapper for label, description, error * * Used internally by TextInput, Select, MaskedInput, and other form components. @@ * `@param` {string} props.className - Additional CSS class + * `@param` {string} props.testId - Test id for wrapper + * `@param` {string} props.labelTestId - Test id for label + * `@param` {string} props.descriptionTestId - Test id for description + * `@param` {string} props.errorTestId - Test id for error * `@param` {ReactNode} props.children - The actual input element */ -const InputWrapper = ({ label, description, error, htmlFor, required, size = 'md', className, labelId, descriptionId, errorId, children }) => { +const InputWrapper = ({ label, description, error, htmlFor, required, size = 'md', className, labelId, descriptionId, errorId, testId, labelTestId, descriptionTestId, errorTestId, children }) => { return ( - <StyledWrapper className={className} $size={size}> + <StyledWrapper className={className} $size={size} data-testid={testId}> {label && ( - <label id={labelId} className="input-wrapper-label" htmlFor={htmlFor}> + <label id={labelId} className="input-wrapper-label" htmlFor={htmlFor} data-testid={labelTestId}> {label} {required && <span className="input-wrapper-required">*</span>} </label> )} - {description && <div id={descriptionId} className="input-wrapper-description">{description}</div>} + {description && <div id={descriptionId} className="input-wrapper-description" data-testid={descriptionTestId}>{description}</div>} {children} - {error && <div id={errorId} className="input-wrapper-error">{error}</div>} + {error && <div id={errorId} className="input-wrapper-error" data-testid={errorTestId}>{error}</div>} </StyledWrapper> ); };As per coding guidelines: "Add
data-testidto testable elements for Playwright."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/ui/InputWrapper/index.js` around lines 18 - 30, The component InputWrapper currently doesn't pass data-testid attributes; add explicit props (e.g., wrapperTestId, labelTestId, descriptionTestId, errorTestId) to the component signature and pass them through as data-testid on the corresponding elements: set data-testid={wrapperTestId} on <StyledWrapper>, data-testid={labelTestId} on the label element (class "input-wrapper-label"), data-testid={descriptionTestId} on the description div (class "input-wrapper-description"), and data-testid={errorTestId} on the error div (class "input-wrapper-error"); keep existing props like labelId/descriptionId/errorId and children unchanged and make the new props optional.packages/bruno-app/src/ui/Select/index.js (1)
133-144: Document theonChange(null)contract for consumers.Both
allowDeselectandclearablecan invokeonChange(null). Add a note in the JSDoc (line 46) clarifying thatonChangemay receivenullwhen the selection is cleared, so consumers know to handle this case.📝 Suggested JSDoc update
* `@param` {function} props.onChange - Called with the selected value string + * (or null when selection is cleared via allowDeselect or clearable)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/ui/Select/index.js` around lines 133 - 144, Update the component JSDoc for Select to document that the onChange callback may be called with null when the selection is cleared: mention both allowDeselect (used in handleSelect) and the clearable prop can trigger onChange(null), and instruct consumers to handle null as the “no selection” state; reference the onChange prop, allowDeselect, clearable and handleSelect to locate where this behavior is produced and ensure the JSDoc clearly states the null contract for consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/ui/Checkbox/index.js`:
- Around line 73-77: The computed checkedIcon currently uses icon || (...) so a
custom icon can appear when indeterminate is true; change the logic in the
Checkbox component so indeterminate takes precedence: compute checkedIcon by
first checking indeterminate and returning the IconMinus JSX with
iconSize/strokeWidth when true, otherwise use the provided icon or fallback to
IconCheck (preserving iconSize/strokeWidth). Update the expression that defines
checkedIcon (referencing checkedIcon, icon, indeterminate, IconMinus, IconCheck,
iconSize) accordingly.
In `@packages/bruno-app/src/ui/Select/index.js`:
- Line 75: The Select component currently sets allowDeselect = true which makes
clicking the selected option call onChange(null); change the default to
allowDeselect = false in the Select component's prop/default declaration (where
allowDeselect is defined) so existing consumers won't receive null unexpectedly,
and update any related propTypes/defaultProps or JSDoc for the Select component
to reflect the new default and the null contract if you decide to keep true
instead.
- Around line 337-350: The JSX uses option.value as the React key which will
trigger warnings if data contains duplicate value properties; update the key in
the Select item render to a stable combined fallback such as
`${option.value}-${index}` (or otherwise append the provided index) so keys are
unique, and ensure this change is applied in the component that renders the
option div where you reference option.value, index, handleSelect, renderOption
and isSelected/isFocused.
---
Nitpick comments:
In `@packages/bruno-app/src/ui/InputWrapper/index.js`:
- Around line 18-30: The component InputWrapper currently doesn't pass
data-testid attributes; add explicit props (e.g., wrapperTestId, labelTestId,
descriptionTestId, errorTestId) to the component signature and pass them through
as data-testid on the corresponding elements: set data-testid={wrapperTestId} on
<StyledWrapper>, data-testid={labelTestId} on the label element (class
"input-wrapper-label"), data-testid={descriptionTestId} on the description div
(class "input-wrapper-description"), and data-testid={errorTestId} on the error
div (class "input-wrapper-error"); keep existing props like
labelId/descriptionId/errorId and children unchanged and make the new props
optional.
In `@packages/bruno-app/src/ui/Select/index.js`:
- Around line 133-144: Update the component JSDoc for Select to document that
the onChange callback may be called with null when the selection is cleared:
mention both allowDeselect (used in handleSelect) and the clearable prop can
trigger onChange(null), and instruct consumers to handle null as the “no
selection” state; reference the onChange prop, allowDeselect, clearable and
handleSelect to locate where this behavior is produced and ensure the JSDoc
clearly states the null contract for consumers.
🪄 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: a66a1bd5-cd97-4616-abdc-1be71a8a5602
📒 Files selected for processing (10)
packages/bruno-app/src/ui/Checkbox/StyledWrapper.jspackages/bruno-app/src/ui/Checkbox/index.jspackages/bruno-app/src/ui/InputWrapper/StyledWrapper.jspackages/bruno-app/src/ui/InputWrapper/constants.jspackages/bruno-app/src/ui/InputWrapper/index.jspackages/bruno-app/src/ui/MaskedInput/StyledWrapper.jspackages/bruno-app/src/ui/MaskedInput/index.jspackages/bruno-app/src/ui/Select/StyledWrapper.jspackages/bruno-app/src/ui/Select/index.jspackages/bruno-app/src/ui/TextInput/StyledWrapper.js
✅ Files skipped from review due to trivial changes (5)
- packages/bruno-app/src/ui/InputWrapper/constants.js
- packages/bruno-app/src/ui/InputWrapper/StyledWrapper.js
- packages/bruno-app/src/ui/MaskedInput/StyledWrapper.js
- packages/bruno-app/src/ui/Select/StyledWrapper.js
- packages/bruno-app/src/ui/TextInput/StyledWrapper.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/ui/Checkbox/StyledWrapper.js
- packages/bruno-app/src/ui/MaskedInput/index.js
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
2 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.
Description
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Bug Fixes
Chores