fix: Reverting the change to add debounce to onValueChange for input widgets
#40922
Conversation
WalkthroughThis change removes all debounced input handling and internal state management from various input widgets, making them immediately update their meta properties on input changes. It eliminates the debounce constant, related imports, and delays in Cypress tests, ensuring input values are processed synchronously and directly via props. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InputWidget
participant MetaProperty
User->>InputWidget: Type text (e.g., via barcode/keyboard)
InputWidget->>MetaProperty: Immediately update 'text' prop
User->>InputWidget: Press Enter (onSubmit)
InputWidget->>MetaProperty: onSubmit uses latest 'text' value
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
app/client/src/widgets/RichTextEditorWidget/widget/index.tsx (1)
501-513: 💡 Verification agent🧩 Analysis chain
Performance concern for rich text operations
Rich text editors are typically performance-sensitive due to complex rendering. Removing debounce could cause significant performance issues, especially with large documents or complex formatting.
Verify the performance impact with typical rich text content:
🏁 Script executed:
#!/bin/bash # Check if there are any performance tests for RichTextEditor fd -e test.ts -e spec.ts | xargs rg "RichTextEditor.*performance|RichTextEditor.*debounce" -A 5Length of output: 97
To determine if debouncing is used elsewhere (or needs to be added), let’s search the entire codebase for any
debouncereferences:
🏁 Script executed:
#!/bin/bash # Search for debounce usage across the project rg -n "debounce" -e ".ts" -e ".tsx" -e ".js"Length of output: 87
Let’s retry searching for any debounce usage with proper file globs:
🏁 Script executed:
#!/bin/bash # Search for debounce across all TS, TSX and JS files rg -n "debounce" -g '*.ts' -g '*.tsx' -g '*.js'Length of output: 90350
Debounce
onValueChangeto prevent excessive RichTextEditor updatesRichTextEditorWidget’s
onValueChangefires on every keystroke without any debounce, while most other widgets throttle rapid input updates to improve performance. Please add a debounced wrapper around the text update:• File:
app/client/src/widgets/RichTextEditorWidget/widget/index.tsx
- Import and initialize a debounced updater (e.g. using
lodash/debounce) once (constructor orcomponentDidMount).- Replace the direct call to
updateWidgetMetaProperty("text", …)with the debounced version.Example diff:
import React from "react"; + import debounce from "lodash/debounce"; export default class RichTextEditorWidget extends React.Component<Props, WidgetState> { + private debouncedUpdateText: (text: string) => void; constructor(props: Props) { super(props); + this.debouncedUpdateText = debounce((text: string) => { + this.props.updateWidgetMetaProperty( + "text", + text, + { + triggerPropertyName: "onTextChange", + dynamicString: this.props.onTextChange, + event: { type: EventType.ON_TEXT_CHANGE }, + }, + ); + }, 200); } onValueChange = (text: string) => { if (!this.props.isDirty) { this.props.updateWidgetMetaProperty("isDirty", true); } - - this.props.updateWidgetMetaProperty("text", text, { - triggerPropertyName: "onTextChange", - dynamicString: this.props.onTextChange, - event: { type: EventType.ON_TEXT_CHANGE }, - }); + this.debouncedUpdateText(text); };This mirrors debounce patterns in other widgets (e.g.,
SelectWidget,JSONFormWidget) and will reduce render churn on large or formatted documents.
🧹 Nitpick comments (2)
app/client/src/widgets/CurrencyInputWidget/widget/index.tsx (1)
527-538: Consistent pattern across input widgetsThis follows the same pattern of removing debounce across all input widgets. While consistency is good, the performance implications need to be carefully considered.
Consider implementing a centralized debounce mechanism at a higher level (e.g., in the action dispatcher or meta property update handler) to maintain performance while keeping widgets simple.
app/client/src/widgets/PhoneInputWidget/widget/index.tsx (1)
406-431: Consider extracting formatting decision logicThe implementation correctly preserves conditional formatting behavior. Consider extracting the condition for better readability.
onValueChange = (value: string) => { let formattedValue; + const isAddingCharacters = value && value.length > this.props.text?.length; // Don't format, as value is typed, when user is deleting - if (value && value.length > this.props.text?.length) { + if (isAddingCharacters) { formattedValue = this.getFormattedPhoneNumber(value); } else { formattedValue = value; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Input/Inputv2_spec.js(1 hunks)app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Listv2_BasicClientSideData_spec.js(2 hunks)app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Listv2_Nested_EventBindings_spec.js(2 hunks)app/client/cypress/support/Pages/AggregateHelper.ts(2 hunks)app/client/src/constants/WidgetConstants.tsx(0 hunks)app/client/src/widgets/CurrencyInputWidget/widget/index.tsx(5 hunks)app/client/src/widgets/InputWidget/widget/index.tsx(3 hunks)app/client/src/widgets/InputWidgetV2/widget/index.tsx(5 hunks)app/client/src/widgets/PhoneInputWidget/widget/index.tsx(5 hunks)app/client/src/widgets/RichTextEditorWidget/widget/index.tsx(4 hunks)app/client/src/widgets/wds/WDSCurrencyInputWidget/widget/index.tsx(4 hunks)app/client/src/widgets/wds/WDSInputWidget/widget/index.tsx(4 hunks)app/client/src/widgets/wds/WDSPhoneInputWidget/widget/index.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/constants/WidgetConstants.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test code written using the Cypress test library. Ensure that: - Follow best practices for Cypress code and e2e automation. ...
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Listv2_Nested_EventBindings_spec.jsapp/client/cypress/e2e/Regression/ClientSide/Widgets/Input/Inputv2_spec.jsapp/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Listv2_BasicClientSideData_spec.jsapp/client/cypress/support/Pages/AggregateHelper.ts
🧬 Code Graph Analysis (6)
app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Listv2_Nested_EventBindings_spec.js (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Listv2_BasicClientSideData_spec.js (1)
widgetSelector(6-6)
app/client/src/widgets/wds/WDSCurrencyInputWidget/widget/index.tsx (1)
app/client/src/widgets/BaseWidget.tsx (1)
WidgetState(453-453)
app/client/src/widgets/wds/WDSPhoneInputWidget/widget/index.tsx (1)
app/client/src/widgets/BaseWidget.tsx (1)
WidgetState(453-453)
app/client/src/widgets/RichTextEditorWidget/widget/index.tsx (1)
app/client/src/widgets/BaseWidget.tsx (1)
WidgetState(453-453)
app/client/src/widgets/PhoneInputWidget/widget/index.tsx (1)
app/client/src/widgets/BaseWidget.tsx (1)
WidgetState(453-453)
app/client/src/widgets/InputWidget/widget/index.tsx (1)
app/client/src/widgets/BaseWidget.tsx (1)
WidgetState(453-453)
🪛 Biome (1.9.4)
app/client/src/widgets/wds/WDSInputWidget/widget/index.tsx
[error] 2-2: Do not shadow the global "toString" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
app/client/src/widgets/InputWidgetV2/widget/index.tsx
[error] 21-21: Do not shadow the global "toString" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (11)
app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Listv2_Nested_EventBindings_spec.js (1)
25-25: LGTM! Correctly removes typing delays.The removal of delay options from
.type()calls aligns with reverting the debounce behavior. The test continues to follow best practices with proper selectors.Also applies to: 28-28, 43-43, 49-49
app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Listv2_BasicClientSideData_spec.js (1)
79-79: LGTM! Typing delays removed correctly.The changes properly remove the delay options from input typing, consistent with the PR objective.
Also applies to: 104-104
app/client/cypress/e2e/Regression/ClientSide/Widgets/Input/Inputv2_spec.js (1)
388-388: LGTM! Delay removal maintains test integrity.The typing delay removal is correct and the test still validates that onSubmit receives the complete input value.
app/client/cypress/support/Pages/AggregateHelper.ts (1)
956-956: LGTM! Helper methods updated correctly.Typing delays removed from all affected helper methods, maintaining consistency with the debounce removal.
Also applies to: 959-959, 984-984
app/client/src/widgets/wds/WDSInputWidget/widget/index.tsx (1)
18-18: LGTM! Clean removal of debounce logic.The widget correctly removes all internal state management and debounced handlers. Input updates are now synchronous and the component is simpler.
Also applies to: 186-208, 229-232
app/client/src/widgets/wds/WDSPhoneInputWidget/widget/index.tsx (1)
221-241:❓ Verification inconclusive
Performance concern: Immediate meta property updates without debouncing
The removal of debounce means
onTextChangedwill fire on every keystroke. This could cause performance issues if the handler triggers expensive operations or API calls.Consider the performance impact on downstream consumers of
onTextChanged:
🏁 Script executed:
#!/bin/bash # Search for onTextChanged usage to understand potential performance impact rg -A 5 "onTextChanged.*WDS.*Phone" --type ts --type tsxLength of output: 86
Manual performance validation required: onTextChanged now fires on every keystroke
Static analysis didn’t locate any direct
onTextChangedhandlers (it’s a dynamic event property), so please manually verify that any user-supplied logic bound to this event can handle rapid invocations without causing UI lag or excessive API calls. Consider re-introducing debouncing if needed.• Inspect pages/widgets where
onTextChangedis configured for this phone input
• Confirm no heavy computations or network requests occur on every keystrokeapp/client/src/widgets/InputWidget/widget/index.tsx (1)
880-892:✅ Verification successful
Performance impact on deprecated widget
While this widget is deprecated, removing debounce could still impact existing users. The synchronous
updateWidgetMetaPropertycalls on every keystroke may cause performance degradation.Verify if this deprecated widget is still widely used:
🏁 Script executed:
#!/bin/bash # Check usage of deprecated InputWidget rg "INPUT_WIDGET[\"']" --type ts --type tsx -A 2 | grep -v "INPUT_WIDGET_V2"Length of output: 106
To broaden the search for any remaining references to the deprecated widget, let’s try scanning by filename globs and JSX imports:
🏁 Script executed:
#!/bin/bash # 1. Literal string usage (exclude V2) rg "INPUT_WIDGET" --glob "*.ts" --glob "*.tsx" -A2 | grep -v INPUT_WIDGET_V2 echo "----- JSX usage -----" # 2. Component usage in JSX rg "<InputWidget" --glob "*.tsx" -A2 echo "----- Import usage -----" # 3. Direct imports rg "import.*InputWidget" --glob "*.ts" --glob "*.tsx"Length of output: 73835
No active usage of deprecated InputWidget found
Our search across.ts/.tsxfiles returned only test fixtures and migration scripts referencing “INPUT_WIDGET,” with no JSX imports or runtime usages. Since the widget isn’t instantiated in production code, removing the debounce won’t impact users.app/client/src/widgets/InputWidgetV2/widget/index.tsx (2)
793-793: LGTM!Correctly uses props instead of internal state with appropriate fallback to empty string.
735-758:❓ Verification inconclusive
Verify performance impact of immediate updates
The removal of debouncing means
onTextChangedwill fire on every keystroke. Ensure this doesn't cause performance issues, especially if the handler triggers expensive operations like API calls or complex computations.Run the following script to check for potentially expensive operations in
onTextChangedhandlers:
🏁 Script executed:
#!/bin/bash # Description: Search for onTextChanged event handlers that might perform expensive operations # Look for onTextChanged handlers with API calls or complex logic rg -A 10 'onTextChanged.*:.*\{' --type js --type ts | grep -E '(fetch|axios|api|async|await|forEach|map|reduce|filter)' # Search for widgets using onTextChanged with dynamic bindings ast-grep --pattern 'onTextChanged: $_'Length of output: 162
Verify onTextChanged performance impact
Removing the debounce means
onTextChangednow fires on every keystroke. Static analysis didn’t find any local handlers doing heavy work, but since these callbacks are defined dynamically by consumers, please manually review all places whereonTextChangedis bound (e.g., pages and container components) to ensure they don’t perform expensive operations—API calls, large loops, complex computations, or heavy DOM updates—on each input. If needed, reintroduce a debounce at the consumer level or document this behavior clearly.app/client/src/widgets/PhoneInputWidget/widget/index.tsx (2)
89-89: LGTM!Simplified to use generic
WidgetStatetype, which is appropriate after removing internal state management.
490-490: LGTM!Correctly uses props with appropriate fallback, consistent with the removal of internal state.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/15584098444. |
|
Deploy-Preview-URL: https://ce-40922.dp.appsmith.com |
Description
Reverting the change to add debounce to
onValueChangefor input widgets. (Reverting PR: #40849)Fixes #40912
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15583889634
Commit: 49e570b
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 11 Jun 2025 12:48:50 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit