-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
Added autoComplete
prop/field passthrough in Portal
#25000
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
autocomplete = 'off'; | ||
autocorrect = 'off'; | ||
autocapitalize = 'off'; | ||
autoComplete = 'off'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the cursor bot has a point here in that adding the autoComplete
prop but always overwriting it for a few input types feels like it's introducing a point of friction worth addressing in this PR. maybe the autoCompletes in this switch should still use the prop if it's passed?
no issue - `autoComplete='off'` was used in our honeypot fields but it was never applied because the `<InputForm>` and `<InputField>` components didn't pass/accept an `autoComplete` prop
f5c732d
to
f845f51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: AutoComplete Prop Overwritten for Specific Inputs
The autoComplete
prop is unconditionally overwritten within the switch
statement for input IDs like input-email
, input-name
, and input-otc
. This causes any autoComplete
value passed via props to be ignored for these specific input types, defeating the purpose of the prop passthrough.
apps/portal/src/components/common/InputField.js#L120-L140
Ghost/apps/portal/src/components/common/InputField.js
Lines 120 to 140 in f845f51
let pattern; | |
switch (id) { | |
case 'input-email': | |
autoComplete = 'off'; | |
autoCorrect = 'off'; | |
autoCapitalize = 'off'; | |
break; | |
case 'input-name': | |
autoComplete = 'off'; | |
autoCorrect = 'off'; | |
break; | |
case 'input-otc': | |
autoComplete = 'one-time-code'; | |
autoCorrect = 'off'; | |
autoCapitalize = 'off'; | |
inputMode = 'numeric'; | |
pattern = '[0-9]*'; | |
placeholder ??= '• • • • • •'; | |
break; | |
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/portal/src/components/common/InputField.js (1)
121-142
: Address the autoComplete prop overwriting issue.As noted in the past review, the switch statement unconditionally overwrites the
autoComplete
prop forinput-email
,input-name
, andinput-otc
cases (lines 123, 128, 132). This defeats the purpose of makingautoComplete
a public prop, since callers cannot override these hardcoded values.Consider using the passed prop value as the default and only falling back to hardcoded values when the prop is not explicitly provided:
switch (id) { case 'input-email': - autoComplete = 'off'; + autoComplete = autoComplete || 'off'; autoCorrect = 'off'; autoCapitalize = 'off'; break; case 'input-name': - autoComplete = 'off'; + autoComplete = autoComplete || 'off'; autoCorrect = 'off'; break; case 'input-otc': - autoComplete = 'one-time-code'; + autoComplete = autoComplete || 'one-time-code'; autoCorrect = 'off'; autoCapitalize = 'off'; inputMode = 'numeric'; pattern = '[0-9]*'; placeholder ??= '• • • • • •'; break; default: break; }This allows callers to explicitly override the defaults when needed (e.g., for honeypot fields) while maintaining sensible defaults for typical use cases.
Based on past review comments.
🧹 Nitpick comments (1)
apps/portal/src/components/common/InputField.test.js (1)
39-47
: Add test for autoComplete override on specific input types.The current tests verify basic autoComplete behavior, but they don't test the interaction with the switch statement that handles specific input IDs like
input-email
,input-name
, andinput-otc
.Consider adding a test to verify that the prop can override (or currently cannot override, which would document the limitation) the hardcoded autoComplete values for these specific input types:
test('applies autoComplete prop for email input type', () => { const {inputEl} = setup({id: 'input-email', autoComplete: 'username'}); expect(inputEl).toHaveAttribute('autoComplete', 'username'); });This would help verify whether the fix suggested in InputField.js (using
autoComplete || 'off'
instead of unconditional assignment) works as intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/portal/src/components/common/InputField.js
(1 hunks)apps/portal/src/components/common/InputField.test.js
(2 hunks)apps/portal/src/components/common/InputForm.js
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup
- GitHub Check: Build & Push
- GitHub Check: Setup
🔇 Additional comments (3)
apps/portal/src/components/common/InputField.js (1)
100-100
: LGTM: autoComplete prop added correctly.The prop is properly declared with a sensible default value of an empty string.
apps/portal/src/components/common/InputForm.js (1)
25-25
: LGTM: autoComplete properly wired through.The
field.autoComplete
value is correctly passed to theInputField
component, enabling the passthrough behavior as intended.apps/portal/src/components/common/InputField.test.js (1)
4-24
: LGTM: Setup function refactored well.The
setup
function now properly accepts and merges props, enabling flexible test configuration. Good refactor.
no issue
autoComplete='off'
was used in our honeypot fields but it was never applied because the<InputForm>
and<InputField>
components didn't pass/accept anautoComplete
prop