feat: Add dynamic property path for sourceData in SelectWidget #34574
Conversation
WalkthroughThe recent updates introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Cypress_Test
participant SelectWidget
User->>+Cypress_Test: Runs `Select_spec.js`
Cypress_Test->>+SelectWidget: Import `propPane`
Cypress_Test->>SelectWidget: Call `propPane.ToggleJSMode("sourcedata", false)`
SelectWidget-->>Cypress_Test: Updates the widget configuration
Cypress_Test-->>User: Test results
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
Widgets Product
rahulbarwal
removed
Select Widget
github-actions
bot
added
Widgets Product
rahulbarwal
removed
Select Widget
github-actions
bot
added
Enhancement
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9709265919. |
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
app/client/src/widgets/SelectWidget/widget/index.tsx (3)
Line range hint
61-63: Remove unnecessary constructor.The constructor in this class is redundant since it only calls
super(props)and does nothing else. Removing it will clean up the code without affecting functionality.- constructor(props: SelectWidgetProps) { - super(props); - }
Line range hint
161-163: Remove redundant double-negation.The use of double-negation (
!!) is unnecessary here as the values will be coerced to boolean naturally in JavaScript. Simplifying this will make the code cleaner and more readable.- const isInvalid = - "isValid" in this.props && !this.props.isValid && !!this.props.isDirty; + const isInvalid = + "isValid" in this.props && !this.props.isValid && this.props.isDirty;
Line range hint
593-593: Replacesuperwith class name in static context.Using
superin a static context can lead to confusion. It's clearer and more correct to use the class name directly.- return !super.getFeatureFlag( + return !SelectWidget.getFeatureFlag(
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/SelectWidget/widget/index.tsx (1 hunks)
Additional context used
Biome
app/client/src/widgets/SelectWidget/widget/index.tsx
[error] 61-63: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 161-163: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 593-593: 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 not posted (1)
app/client/src/widgets/SelectWidget/widget/index.tsx (1)
114-114: Approved addition of dynamic property path.The addition of
dynamicPropertyPathListwith{ key: "sourceData" }aligns with the PR's goal to enhance dynamic data handling in theSelectWidget. This change should facilitate easier integration and management of dynamic data sources.
|
Deploy-Preview-URL: https://ce-34574.dp.appsmith.com |
…tion This is required because if source data does not match with sample data passed for the widget configuration, it disallows switching back JS toggle button
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9711128924. |
|
Deploy-Preview-URL: https://ce-34574.dp.appsmith.com |
rahulbarwal
added
ok-to-test
rahulbarwal
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select_spec.js (2 hunks)
Additional comments not posted (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select_spec.js (2)
12-12: Review of new import statement forpropPane.The import of
propPaneis appropriate for enabling dynamic toggling of JavaScript modes in the properties panel, which aligns with the test case modifications.
106-106: Ensure correct usage and integration ofpropPane.ToggleJSMode.The addition of
propPane.ToggleJSMode("sourcedata", false)is intended to toggle JavaScript mode for thesourcedataproperty of the Select widget. This change is crucial for the test scenario that follows, ensuring that the widget behaves as expected in non-JS mode. It is important to verify that this function call correctly toggles the mode as intended and that it doesn't affect other properties unintentionally.
rahulbarwal
added
ok-to-test
rahulbarwal
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select_spec.js (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select_spec.js
rahulbarwal
deleted the
fix-34568/enable-js-mode-for-sourcedata-in-select-widget
branch
…et (#34574)" (#34618) **/test binding** <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9754334718> > Commit: 2c67d23 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9754334718&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Binding` <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Improved the internal handling of `sourceData` within the Select widget for better performance and maintainability. - **Tests** - Updated test cases for the Select widget to reflect the new handling of `sourceData`. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…ithorg#34574) ## Description Add dynamic property path for sourceData in SelectWidget. Fixes appsmithorg#34568 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Widget" ### 🔍 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/9712372976> > Commit: c5521e4 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9712372976&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Widget` <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced the Select Widget with a new property to support dynamic property paths. - **Tests** - Added new tests to validate the dynamic property paths functionality in the Select Widget. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…et (appsmithorg#34574)" (appsmithorg#34618) **/test binding** <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9754334718> > Commit: 2c67d23 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9754334718&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Binding` <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Improved the internal handling of `sourceData` within the Select widget for better performance and maintainability. - **Tests** - Updated test cases for the Select widget to reflect the new handling of `sourceData`. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Add dynamic property path for sourceData in SelectWidget.
Fixes #34568
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Widget"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9712372976
Commit: c5521e4
Cypress dashboard.
Tags:
@tag.WidgetCommunication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit