-
Notifications
You must be signed in to change notification settings - Fork 46.2k
fix(frontend): allow empty values in number inputs and fix AnyOfField toggle #11661
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: dev
Are you sure you want to change the base?
fix(frontend): allow empty values in number inputs and fix AnyOfField toggle #11661
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
|
Thank you for fixing the INTEGER input type issue! This is definitely a needed correction since "account" is not a valid HTML input type. However, I noticed an unrelated change in your PR that wasn't mentioned in the description: - [Flag.CHAT]: true,
+ [Flag.CHAT]: false,This change to disable the CHAT feature flag doesn't seem related to the input type fix described in the PR. Could you either:
Once that's addressed, this PR should be ready to merge. The main fix looks good and will correctly fix integer input behavior. |
| @@ -51,7 +51,7 @@ export const TextInputWidget = (props: WidgetProps) => { | |||
| handleChange: (v: string) => (v === "" ? undefined : Number(v)), | |||
| }, | |||
| [InputType.INTEGER]: { | |||
| htmlType: "account", | |||
| htmlType: "number", | |||
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.
@majiayu000 We can’t solve this issue this way. To fix it properly, we need to allow empty values in the number input, and we also need to fix the anyOfField file so it doesn’t close the input when its value is null.
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.
Hi @Abhi1992002, thanks for the review!
I believe this PR actually addresses both points you mentioned:
-
Allow empty values in number input: The
handleChangefunction inTextInputWidget.tsxalready handles this - when the input is empty (v === ""), it returnsundefinedinstead of trying to convert an empty string to a number. This allows users to clear the number input. -
Fix anyOfField not closing when value is null: The change in
useAnyOfField.tsxfromformData !== null && formData !== undefinedto justformData !== nullmeans:undefined(empty input) → field stays enabled ✓null(explicitly toggled off) → field disabled ✓
The htmlType: "account"→"number" fix was also necessary because "account" is not a valid HTML input type.
Regarding the use-get-flag.ts comment - I don't see any changes to that file in this PR. Could you clarify what you meant?
Please let me know if I misunderstood your feedback!
| @@ -48,7 +48,7 @@ const mockFlags = { | |||
| [Flag.AGENT_FAVORITING]: false, | |||
| [Flag.MARKETPLACE_SEARCH_TERMS]: DEFAULT_SEARCH_TERMS, | |||
| [Flag.ENABLE_PLATFORM_PAYMENT]: false, | |||
| [Flag.CHAT]: true, | |||
| [Flag.CHAT]: false, | |||
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.
We don’t need it — you can remove it.
ecd1b12 to
c9d7355
Compare
|
Thanks for this fix! The PR looks good and addresses an important issue with integer inputs. Before we can merge:
This is a valuable fix that will ensure integer inputs have proper HTML semantics and validation. Once the test plan is completed, this should be ready to merge. |
c9d7355 to
1b5cd32
Compare
|
Addressed feedback: removed unrelated Flag.CHAT change from the branch and updated the test plan checkboxes. |
… toggle
1. Fix INTEGER input htmlType from 'account' to 'number' (typo fix)
2. Fix useAnyOfField isEnabled logic to only check for explicit null
- Previously checked for both null AND undefined, causing input to
disappear when user cleared a number field
- Now only checks for null (set by toggle off), allowing undefined
(empty input) to keep the field visible
Fixes Significant-Gravitas#11594
Signed-off-by: majiayu000 <[email protected]>
1b5cd32 to
af820a5
Compare
CI Status UpdateThe test failures are in Failing tests:
This PR's changes:
The failing tests are about the marketplace agent page functionality, not the input renderer components. Could a maintainer please re-run the failing tests? |
|
/rerun |
|
The 3 failing marketplace E2E tests ( This PR only modifies:
The failing tests are marketplace-related E2E tests that don't interact with the form widgets changed in this PR. Triggered a rerun to confirm. |
Summary
This PR fixes two related issues with number/integer inputs in the frontend:
HTMLType typo fix: INTEGER input type was incorrectly mapped to
htmlType: 'account'(which is not a valid HTML input type) instead ofhtmlType: 'number'.AnyOfField toggle fix: When a user cleared a number input field, the input would disappear because
useAnyOfFieldchecked for bothnullANDundefinedinisEnabled. This PR changes it to only check for explicitnull(set by toggle off), allowingundefined(empty input) to keep the field visible.Root cause analysis
When a user cleared a number input:
handleChangereturnedundefined(becausev === "" ? undefined : Number(v))useAnyOfField,isEnabled = formData !== null && formData !== undefinedbecamefalseFix
Changed
useAnyOfField.tsxline 67:This way:
formDataisnull→isEnabledisfalse(input hidden) ✓formDataisundefined→isEnabledistrue(input visible) ✓Test plan
type="number"Fixes #11594
🤖 AI-assisted development disclaimer: This PR was developed with assistance from Claude Code.