feat: remove isFocus from meta state in InputWidgetV2#36843
feat: remove isFocus from meta state in InputWidgetV2#36843rahulbarwal merged 6 commits intoreleasefrom
Conversation
jacquesikot
added
Input Widget
ok-to-test
WalkthroughThe changes made in this pull request focus on the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
github-actions
bot
added
Enhancement
jacquesikot
changed the title
jacquesikot
changed the title
github-actions
bot
added
Enhancement
jacquesikot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/widgets/InputWidgetV2/widget/index.tsx (1)
Line range hint
779-779: EnsureisFocusedis accessed from state instead of propsSince
isFocusedis now managed within the component's state, references should be updated tothis.state.isFocusedinstead ofthis.props.isFocusedin thegetWidgetViewmethod. This ensures the component accurately reflects the focus state.Here's the corrected code:
- showError={!!this.props.isFocused} + showError={!!this.state.isFocused}Making this change helps maintain consistency and prevents potential errors.
🧰 Tools
🪛 Biome
[error] 20-20: 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/widgets/InputWidgetV2/widget/index.tsx (5 hunks)
🧰 Additional context used
🪛 Biome
app/client/src/widgets/InputWidgetV2/widget/index.tsx
[error] 20-20: 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)
[error] 651-651: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/widgets/InputWidgetV2/widget/index.tsx (5 hunks)
🧰 Additional context used
🪛 Biome
app/client/src/widgets/InputWidgetV2/widget/index.tsx
[error] 20-20: 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)
[error] 651-651: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🔇 Additional comments (2)
app/client/src/widgets/InputWidgetV2/widget/index.tsx (2)
303-308: Excellent work on initializing local state in the constructorBy adding a constructor and initializing the
isFocusedstate within the component, you've effectively encapsulated the focus management. This aligns well with React's state management practices and helps reduce unnecessary evaluations.
Line range hint
676-689: Good implementation of focus state handlingYour updated
handleFocusChangemethod effectively manages the focus state internally and correctly triggers theonFocusandonBluractions. This enhancement improves the component's performance and maintains proper encapsulation of state.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/client/src/widgets/InputWidgetV2/widget/index.tsx (2)
676-677: Good job updating the focus handling, students!Your changes to
handleFocusChangealign well with the new state management approach. UsingsetStateto update the focus state is the correct way to handle internal component state in React.However, let's make it even better! Consider using the functional form of
setStateto ensure you're always working with the most up-to-date state:- this.setState({ isFocused: focusState }); + this.setState((prevState) => ({ ...prevState, isFocused: focusState }));This form is especially useful when state updates depend on the previous state, which might be the case in more complex scenarios.
Also applies to: 689-689
651-654: Excellent work on updating the meta properties, class!Your modification of
getMetaPropertiesMapto remove the 'isFocused' property is consistent with the new focus management approach. Usingomitfrom lodash is an efficient way to remove a specific property from an object.To optimize this further, consider memoizing the result of this method if it's called frequently:
static getMetaPropertiesMap(): Record<string, any> { if (!this.memoizedMetaProperties) { const baseMetaProperties = BaseInputWidget.getMetaPropertiesMap(); const rest = omit(baseMetaProperties, ["isFocused"]); this.memoizedMetaProperties = merge(rest, { inputText: "", text: "", }); } return this.memoizedMetaProperties; }This approach can help improve performance by avoiding unnecessary recalculations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/widgets/InputWidgetV2/widget/index.tsx (6 hunks)
🧰 Additional context used
🪛 Biome
app/client/src/widgets/InputWidgetV2/widget/index.tsx
[error] 20-20: 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)
🔇 Additional comments (2)
app/client/src/widgets/InputWidgetV2/widget/index.tsx (2)
303-308: Well done on adding a constructor, class!The addition of a constructor to initialize the
isFocusedstate is a step in the right direction. This change moves the focus management from widget meta properties to internal component state, which is a more React-like approach. It will help in better encapsulation of the component's behavior.Remember, though, that changing state management can have ripple effects. Make sure to update any code that previously relied on the focus state from widget meta properties.
Line range hint
1-938: Class, let's summarize our review!Overall, your changes to the
InputWidgetcomponent are commendable. You've successfully transitioned the focus management from widget meta properties to internal component state, which is a more React-idiomatic approach. This improves the component's encapsulation and should make it easier to reason about its behavior.Key points to remember:
- The new constructor initializes the focus state.
- The
handleFocusChangemethod now updates internal state.- The
getMetaPropertiesMapstatic method has been adjusted to reflect these changes.To further improve your work:
- Consider using the functional form of
setStateinhandleFocusChange.- Look into memoizing the result of
getMetaPropertiesMapfor potential performance gains.- Rename the imported 'toString' function from lodash to avoid shadowing the global property.
Keep up the good work, and don't forget to update any related tests or documentation to reflect these changes!
jacquesikot
added
ok-to-test
…o feat/remove-redundant-props-from-input-evaluations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/widgets/InputWidgetV2/widget/index.tsx (1)
21-21: Extra credit opportunity: Consider renaming lodash importClass, while using lodash's
toStringis a common practice, our static analysis tool has raised a valid point about shadowing the globaltoStringproperty. To earn some extra credit, consider renaming this import for added clarity:- import { isNil, isNumber, merge, toString } from "lodash"; + import { isNil, isNumber, merge, toString as lodashToString } from "lodash";This change would make it explicit that we're using lodash's version of
toStringand avoid any potential confusion with the global property. Remember to update any usage oftoStringtolodashToStringin the rest of the file if you make this change.🧰 Tools
🪛 Biome
[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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/widgets/BaseInputWidget/widget/index.tsx (0 hunks)
- app/client/src/widgets/InputWidgetV2/widget/index.tsx (7 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/widgets/BaseInputWidget/widget/index.tsx
🧰 Additional context used
🪛 Biome
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)
🔇 Additional comments (4)
app/client/src/widgets/InputWidgetV2/widget/index.tsx (4)
303-308: A+ for adding a constructor, class!The addition of this constructor is a great step towards managing the focus state internally. This aligns perfectly with our lesson plan of removing the
isFocusprop trigger from evaluations. Keep up the good work!
675-676: Gold star for updatinghandleFocusChange, students!You've done an excellent job updating this method. By using
setStateto manage the focus internally, we've eliminated the unnecessary evaluation cycle while maintaining the importantonFocusandonBlurfunctionality. This is precisely what we aimed to achieve in our lesson plan!Also applies to: 688-689
651-656: Excellent work ongetMetaPropertiesMap, class!Your revision of this method is spot-on. By removing the
isFocusedproperty and utilizing the base class method, you've not only achieved our goal of eliminating unnecessary evaluations but also demonstrated good code reuse practices. Well done!
651-651: Pop quiz passed: Addressing past feedback!Class, I'm thrilled to see that you've addressed the previous concern about using 'super' in a static method context. By directly referencing
BaseInputWidget.getMetaPropertiesMap(), you've shown great attention to detail and responsiveness to feedback. Keep up this excellent habit of continuous improvement!
jacquesikot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/client/src/widgets/InputWidgetV2/widget/index.tsx (4)
676-677: A step in the right direction, but let's fine-tune it!Your modification to use internal state for
isFocusedis spot on. It aligns perfectly with our goal of reducing unnecessary evaluation cycles. However, we can make a small improvement to enhance readability.Consider extracting the common logic for executing actions into a separate method. This will make the code more DRY and easier to maintain. Here's a suggestion:
handleFocusChange = (focusState: boolean) => { this.setState({ isFocused: focusState }); - if (focusState) { - this.executeAction({ - triggerPropertyName: "onFocus", - dynamicString: this.props.onFocus, - event: { - type: EventType.ON_FOCUS, - }, - }); - } - - if (!focusState) { - this.executeAction({ - triggerPropertyName: "onBlur", - dynamicString: this.props.onBlur, - event: { - type: EventType.ON_BLUR, - }, - }); - } + this.executeFocusAction(focusState); super.handleFocusChange(focusState); }; +executeFocusAction = (focusState: boolean) => { + const actionConfig = focusState + ? { triggerPropertyName: "onFocus", dynamicString: this.props.onFocus, eventType: EventType.ON_FOCUS } + : { triggerPropertyName: "onBlur", dynamicString: this.props.onBlur, eventType: EventType.ON_BLUR }; + + this.executeAction({ + ...actionConfig, + event: { type: actionConfig.eventType }, + }); +};This refactoring will make the
handleFocusChangemethod more concise and easier to understand.Also applies to: 689-697
730-733: Good thinking on resetting the dirty state!Your addition to reset the
isDirtystate whendefaultTextchanges is a smart move. It ensures that the component's state remains consistent with its props. However, we can make this even clearer with a small adjustment.Consider adding a comment explaining why we're resetting the
isDirtystate. This will help future developers (including yourself!) understand the reasoning behind this check. Here's a suggestion:// If defaultText property has changed, reset isDirty to false if ( this.props.defaultText !== prevProps.defaultText && this.state.isDirty ) { + // Reset isDirty when defaultText changes to ensure consistency between props and state this.setState({ isDirty: false }); }This comment will provide valuable context for anyone reviewing or maintaining the code in the future.
756-758: A smart addition to track changes, but let's optimize it!Your implementation to update the
isDirtystate when the input value changes is a great step towards better state management. However, we can make this even more efficient.Consider updating the
isDirtystate only when it changes fromfalsetotrue. This will prevent unnecessary re-renders when the state is already dirty. Here's how you can modify it:- if (!this.state.isDirty) { - this.setState({ isDirty: true }); - } + this.setState((prevState) => ({ + isDirty: prevState.isDirty || true + }));This change ensures that we only update the state when necessary, potentially improving performance, especially for frequently updated inputs.
913-913: An excellent touch for user experience, but let's make it even better!Your modification to show error messages only when the input is focused is a great improvement to the user experience. It prevents overwhelming the user with error messages before they've had a chance to interact with the input.
To enhance accessibility, consider adding an aria-invalid attribute to the input when it's invalid, regardless of focus state. This will help screen readers communicate the input's validity to users. Here's how you can modify the InputComponent props:
<InputComponent // ... other props showError={!!this.state.isFocused} + aria-invalid={isInvalid} // ... rest of the props />This addition will improve the accessibility of your input component, making it more inclusive for all users.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/widgets/BaseInputWidget/widget/index.tsx (0 hunks)
- app/client/src/widgets/InputWidgetV2/widget/index.tsx (10 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/widgets/BaseInputWidget/widget/index.tsx
🧰 Additional context used
🪛 Biome
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)
🔇 Additional comments (2)
app/client/src/widgets/InputWidgetV2/widget/index.tsx (2)
303-309: Well done, class! Let's examine this new addition.The constructor you've added is a great step towards better state management. By initializing
isFocusedandisDirtyas internal state, we're reducing unnecessary evaluation cycles, which was our main objective. This change will help improve the overall performance of our application.
652-657: Excellent work on simplifying our meta properties!By removing
isFocusedandisDirtyfrom the meta properties, we've successfully addressed the issue of unnecessary evaluation cycles. This change will lead to improved performance, especially when multiple input widgets are present on a page. Keep up the good work!
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11343713982. |
|
Deploy-Preview-URL: https://ce-36843.dp.appsmith.com |
jacquesikot
added
ok-to-test
jacquesikot
changed the title
rahulbarwal
deleted the
feat/remove-redundant-props-from-input-evaluations
branch
) ## Description **Problem** A redundant evaluation cycle is run every time a user focuses on an InputWidget component and the isFocused meta state is updated **Root Cause** `isFocus` property of InputWidgetV2 is a meta property and updated through `this.updateWidgetMetaProperty`, thereby changes to these property triggers an evaluation cycle run **Solution** Implement the `isFocus` property as a local state of the InputWidgetV2 component Fixes appsmithorg#36446 ## Automation /ok-to-test tags="@tag.Widget, @tag.Input" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11362334893> > Commit: 80e9089 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11362334893&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Widget, @tag.Input` > Spec: > <hr>Wed, 16 Oct 2024 10:22:00 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced focus management for the InputWidget, allowing for improved internal state handling. - Multi-line text input now supports submitting with keyboard shortcuts for a more streamlined user experience. - **Bug Fixes** - Resolved issues with widget metadata by excluding the `isFocused` property from the meta properties. - Improved internal state management for input changes, reducing reliance on external properties. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Description
Problem
A redundant evaluation cycle is run every time a user focuses on an InputWidget component and the isFocused meta state is updated
Root Cause
isFocusproperty of InputWidgetV2 is a meta property and updated throughthis.updateWidgetMetaProperty, thereby changes to these property triggers an evaluation cycle runSolution
Implement the
isFocusproperty as a local state of the InputWidgetV2 componentFixes #36446
Automation
/ok-to-test tags="@tag.Widget, @tag.Input"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11362334893
Commit: 80e9089
Cypress dashboard.
Tags:
@tag.Widget, @tag.InputSpec:
Wed, 16 Oct 2024 10:22:00 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
isFocusedproperty from the meta properties.