refactor(forms): migrate Input to TS - #7762
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
talissoncosta
changed the title
Docker builds report
|
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/environment-permission-test.pw.ts › Environment Permission Tests › Environment-level permissions control access to features, identities, and segments @enterprise
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Failed testsfirefox › tests/change-request-test.pw.ts › Change Request Tests › Change requests can be created, approved, and published with four-eyes approval @enterprise
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Failed testsfirefox › tests/environment-permission-test.pw.ts › Environment Permission Tests › Environment-level permissions control access to features, identities, and segments @enterprise
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Failed testsfirefox › tests/environment-permission-test.pw.ts › Environment Permission Tests › Environment-level permissions control access to features, identities, and segments @enterprise
Details
|
Visual Regression19 screenshots compared. See report for details. |
2f2cfcb to
418973a
Compare
Mirror the login form's actual rightComponent usage (Forgot password link next to the Password field) instead of a made-up API key example.
…htComponent The login form placed 'Forgot password' in the password field's label row via InputGroup's rightComponent escape hatch — an uncommon pattern. Move it to a right-aligned link below the field (the conventional placement). HomePage was the sole rightComponent consumer, so remove the prop from InputGroup entirely and its Storybook story. Behaviour: same link/handler, just relocated. No E2E selects it by position.
The Input.tsx migration removed the last MaskedInput usage; drop the dependency (and its orphaned inputmask-core transitive) from package.json and the lockfile. Its React ^16 peer range is also incompatible with our React 19. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fa8d6bd to
8a4c307
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02e16119-3f7f-441d-832f-96a718c81729
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (44)
frontend/documentation/components/FieldError.stories.tsxfrontend/documentation/components/FieldLabel.stories.tsxfrontend/documentation/components/InputGroup.stories.tsxfrontend/documentation/components/LabelWithTooltip.stories.tsxfrontend/package.jsonfrontend/web/components/BreadcrumbSeparator.tsxfrontend/web/components/ChangeRequestsSetting.tsxfrontend/web/components/GroupSelect.tsxfrontend/web/components/PermissionsTabs.tsxfrontend/web/components/RegexTester.tsxfrontend/web/components/RolesSelect.tsxfrontend/web/components/base/LabelWithTooltip.tsxfrontend/web/components/base/forms/FieldError.tsxfrontend/web/components/base/forms/FieldLabel.tsxfrontend/web/components/base/forms/Input.jsfrontend/web/components/base/forms/Input.tsxfrontend/web/components/base/forms/InputGroup.jsfrontend/web/components/base/forms/InputGroup.tsxfrontend/web/components/import-export/ImportPage.tsxfrontend/web/components/inspect-permissions/InspectPermissions.tsxfrontend/web/components/inspect-permissions/ProjectPermissions.tsxfrontend/web/components/metrics/EnvironmentMetric.tsxfrontend/web/components/modals/ChangeEmailAddress.tsxfrontend/web/components/modals/ConfirmDeleteAccount.tsxfrontend/web/components/modals/CreateEditIntegrationModal.tsxfrontend/web/components/modals/CreateGroup.tsxfrontend/web/components/modals/CreateSegmentRulesTabForm.tsxfrontend/web/components/modals/create-feature/components/FeatureNameInput.tsxfrontend/web/components/onboarding/OnboardingAccountForm.tsxfrontend/web/components/pages/AccountSettingsPage.tsxfrontend/web/components/pages/GitHubSetupPage.tsxfrontend/web/components/pages/HomePage.tsxfrontend/web/components/pages/UsersAndPermissionsPage.tsxfrontend/web/components/pages/organisation-settings/tabs/sso/saml/modals/CreateSAML.tsxfrontend/web/components/pages/sdk-keys/SDKKeysPage.tsxfrontend/web/components/segments/Rule/components/RuleConditionValueInput.tsxfrontend/web/components/tables/TableFilterOptions.tsxfrontend/web/components/tables/TableSearchFilter.tsxfrontend/web/components/tables/TableTagFilter.tsxfrontend/web/components/tables/TableValueFilter.tsxfrontend/web/components/tags/AddEditTags.tsxfrontend/web/components/tags/CreateEditTag.tsxfrontend/web/styles/components/_input.scssfrontend/web/styles/project/_forms.scss
💤 Files with no reviewable changes (15)
- frontend/web/components/base/LabelWithTooltip.tsx
- frontend/web/components/pages/sdk-keys/SDKKeysPage.tsx
- frontend/web/components/base/forms/InputGroup.js
- frontend/documentation/components/LabelWithTooltip.stories.tsx
- frontend/web/components/modals/create-feature/components/FeatureNameInput.tsx
- frontend/package.json
- frontend/web/components/onboarding/OnboardingAccountForm.tsx
- frontend/web/components/pages/organisation-settings/tabs/sso/saml/modals/CreateSAML.tsx
- frontend/web/components/RegexTester.tsx
- frontend/web/components/modals/ChangeEmailAddress.tsx
- frontend/web/components/tags/CreateEditTag.tsx
- frontend/web/components/modals/ConfirmDeleteAccount.tsx
- frontend/web/components/modals/CreateGroup.tsx
- frontend/web/components/pages/AccountSettingsPage.tsx
- frontend/web/components/base/forms/Input.js
MCPIntegration still imported the removed LabelWithTooltip, breaking the frontend bundle build. Replace it with the new FieldLabel, which owns the label + info-tooltip pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 013fbede-9e8e-4445-9ec4-ba51c3fe6d0c
📒 Files selected for processing (1)
frontend/web/components/integrations/MCPIntegration.tsx
- Input: password reveal toggle is now a <button> with aria-label and aria-pressed (was a non-focusable span, unreachable by keyboard). - InputGroup: extract the legacy isValid union into a named InputGroupValidity type. - _input.scss: dark textarea uses --color-border-default so the border is visible (was --color-surface-default, blending into the fill). - _input.scss / _forms.scss: satisfy stylelint empty-line-before-comment. - InputGroup story: type meta against the component. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/web/components/base/forms/Input.tsx (1)
53-54: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftNew form components not organised per the one-folder-per-component guideline. Both
InputandInputGroupare new components added flat intofrontend/web/components/base/forms/rather than each living in its own folder with a barrelindex.tsand co-locatedComponentName.scss/subcomponents.
frontend/web/components/base/forms/Input.tsx#L53-L54: move intoforms/Input/Input.tsxwith aforms/Input/index.tsbarrel and update imports to reference the folder path.frontend/web/components/base/forms/InputGroup.tsx#L62-L62: move intoforms/InputGroup/InputGroup.tsxwith aforms/InputGroup/index.tsbarrel and update imports to reference the folder path.As per coding guidelines,
frontend/web/components/**/*.{ts,tsx,scss}requires "Each new component must live in its own folder with a barrelindex.ts, aComponentName/ComponentName.tsxfile, co-locatedComponentName.scss... and imports should use the component folder path rather than the inner file."Source: Coding guidelines
frontend/web/components/base/forms/InputGroup.tsx (2)
54-57: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract
error's inline union into a named type.
error?: string | string[]is an inline union that should follow the same treatment already applied toisValid(InputGroupValidity).As per coding guidelines, `frontend/**/*.{ts,tsx}` must extract inline union types into named types (for example, use `type Status = 'A' | 'B'` instead of an inline union).♻️ Proposed fix
+type InputGroupError = string | string[] + inputProps?: Record<string, any> & { - error?: string | string[] + error?: InputGroupError name?: string }Source: Coding guidelines
129-134: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftCustom
componentstill bypasses label/error wiring.The
componentbranch renders the consumer-supplied node as-is, withoutid,aria-invalid, oraria-describedby. Whenidis generated internally (useId()), the rendered label/error cannot be associated with the custom control. This was flagged previously and remains unresolved.
♻️ Duplicate comments (2)
frontend/web/components/base/forms/Input.tsx (1)
76-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSync
typewithtypeProp, still unresolved.
useState(typeProp)only captures the initial value; subsequent changes to thetypeprop (e.g. dynamic type switching by a consumer) will not update the rendered<input>. This was already flagged in a previous review round and remains unfixed.Deriving the type instead of storing it removes the desync entirely and simplifies the toggle logic:
🐛 Proposed fix
- const [type, setType] = useState(typeProp) + const [revealed, setRevealed] = useState(false) + const type = typeProp === 'password' && revealed ? 'text' : typePropThen update the toggle button:
- aria-label={type === 'password' ? 'Show password' : 'Hide password'} - aria-pressed={type === 'text'} + aria-label={revealed ? 'Hide password' : 'Show password'} + aria-pressed={revealed} ... - onClick={() => setType(type === 'password' ? 'text' : 'password')} + onClick={() => setRevealed((r) => !r)}and the icon name:
- name={type === 'password' ? 'eye' : 'eye-off'} + name={revealed ? 'eye-off' : 'eye'}frontend/web/components/base/forms/InputGroup.tsx (1)
135-156: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInternal ref is still overridden by the consumer-prop spread.
refis set before{...restInputProps}for both thetextareaandInputbranches. IfinputPropscontains arefkey, it silently overwrites the internal callback ref, breakingInputGroup.focus(). This exact issue was flagged in a previous review round and remains unfixed.🐛 Proposed fix
<textarea - ref={(c) => { - inputRef.current = c - }} {...(restInputProps as React.TextareaHTMLAttributes<HTMLTextAreaElement>)} + ref={(c) => { + inputRef.current = c + }} disabled={disabled}<Input - ref={(c) => { - inputRef.current = c - }} {...restInputProps} + ref={(c) => { + inputRef.current = c + }} isValid={
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e3b8fde5-bbfb-420a-8ee3-4c82895b51be
📒 Files selected for processing (5)
frontend/documentation/components/InputGroup.stories.tsxfrontend/web/components/base/forms/Input.tsxfrontend/web/components/base/forms/InputGroup.tsxfrontend/web/styles/components/_input.scssfrontend/web/styles/project/_forms.scss
Switching the reveal toggle to a <button> reset its color, so the icon's currentColor fill went dark and vanished on the dark input surface. Give the Icon an explicit theme-aware fill (colorIconDefault) so it stays visible in both themes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
.input-icon-right:hover still used the raw $primary (purple-600, fixed across themes). Use --color-icon-action so the hover follows the theme — purple-400 in dark for better contrast — completing the input colour tokenisation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/web/styles/components/_input.scss (1)
122-129: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the dedicated disabled icon token.
The disabled right-side icon currently uses
--color-text-disabled, whilefrontend/web/styles/_tokens.scssdefines--color-icon-disabled. The values currently match, but separate theme values would not propagate correctly.Suggested fix
& + .input-icon-right { path { - fill: var(--color-text-disabled); + fill: var(--color-icon-disabled); opacity: $btn-disabled-opacity; } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0b65858f-9718-4fb6-a07d-e4c69b8e6bd8
📒 Files selected for processing (1)
frontend/web/styles/components/_input.scss
label and .label-switch used $body-color (light) with separate .dark overrides ($white / $text-icon-light). Collapse both into var(--color-text-default) — same resolved values in each theme — and delete the two .dark blocks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hey @Zaimwa9, now it is ready for review. |
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
A few comments, main ones to fix would be the default size of the search icon and mostly the forgot password positioning, otherwise feels good
With FieldLabel the tooltip triggers from the info icon, so hiding the icon just removed the only trigger — hideTooltipIcon had become a confusing alias for "no tooltip". Drop it; the single caller (FeatureValueTab) now expresses the same intent by conditionally passing `tooltip`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the inline nested ternary with getValueTooltip(hasVariations, isEdit) so the JSX reads cleanly and the logic is in one named place. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The unified iconWidthBySize map had default: undefined, so default-size search/password icons fell back to the icon's intrinsic 22px — the search icon lost its previous explicit 20px (spotted in review). Give the map a real default of 20 and look it up for the no-size case too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
It was typed string, but every caller passes a boolean toggle (HomePage enableAutoComplete: true) and the `autocomplete` prop already covers explicit tokens. Type it boolean and resolve it to 'on'/'off' for the native attribute. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Input had three ways to set autocomplete: the native `autoComplete`, a lowercase `autocomplete` alias, and an `enableAutoComplete` boolean resolved to on/off. Drop the two custom props and let the native `autoComplete` string flow through — also removes the resolver clobbering a caller's `autoComplete`. Call sites: HomePage `enableAutoComplete: true` -> `autoComplete: 'on'`; CreateEditIntegrationModal `autocomplete=` -> `autoComplete=`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #7758
Migrates the
Inputbase component (behind ~94 call sites) fromInput.jsto a typed function component,Input.tsx. Stacked on #7745 (base branchfeat/label-variant-key-ui) — will retarget tomainonce that merges.useImperativeHandleexposing the E2E-guardedfocus()(no-op under E2E, matching the original).react-maskedinputdependency and the checkbox/radio delegation (the one caller renders<Checkbox>directly).colorIconDangertoken instead of a hardcoded hex.InputEvent/KeyboardEventannotations onInputhandlers (React infers the synthetic events) and fixes a few loose usages typing surfaced (JSX in the stringtitle,readonly→readOnly, non-booleanisValid, a booleanvalue).Out of scope (follow-ups): the wrapper-less redesign with
PasswordInput/SearchInput, anInputvariantAPI, theautocomplete→autoCompletealias removal, and the repo-wideInputEvent-annotation cleanup.How did you test this code?
npm run typecheck: 0 new errors vs the base branch (the typedInputremoved one pre-existing error; 976 → 975).npm run lint: clean on all changed files.