chore: fix custom widget 's updateModel#38129
Conversation
WalkthroughThe changes made in the pull request involve a modification to the Changes
Suggested labels
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
the
skip-changelog
|
Whoops! Looks like you're using an outdated method of running the Cypress suite. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/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 (1)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (1)
82-84: Consider type safety improvementsWhile the type casting
message as Record<string, unknown>works, consider defining a proper interface for the message structure to improve type safety and maintainability.interface ModelUpdateMessage { model: unknown; // add other expected properties } const handleModelUpdate = (message: ModelUpdateMessage) => { onUpdateModel(message); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx(1 hunks)
🔇 Additional comments (2)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (2)
82-84: Verify parent component compatibility with full message object
The change from passing message.model to passing the entire message object could impact parent components expecting only the model data.
Let's verify the parent component's expectations:
✅ Verification successful
The change is safe and follows the expected type contract
The type definition and implementation show that onUpdateModel expects Record<string, unknown>, which matches exactly with the type being passed. The parent widget correctly merges this data with the existing model, so the change is type-safe and behaviorally correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for onUpdateModel usage patterns
ast-grep --pattern 'onUpdateModel($message)'
# Search for type definitions of onUpdateModel
rg -A 3 'onUpdateModel.*Record<string, unknown>'
Length of output: 1583
82-84: Ensure consistent model update handling
The model update flow should be consistent between direct updates (handleModelUpdate) and model change notifications (useEffect). Verify that both paths handle the model structure similarly.
Let's check the model handling consistency:
Also applies to: 144-153
✅ Verification successful
Model update handling is consistent and well-structured
The model update flow is consistent between direct updates and notifications. The code shows:
- Direct updates via
handleModelUpdateproperly pass through toonUpdateModel - Initial model is correctly passed during iframe initialization
- Message handling is centralized through
IframeMessengerwith proper type safety
The review comment can be closed as the implementation follows good practices for model state management in iframe communication.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all model-related message handling
rg -A 5 'model.*message'
# Look for other instances of model updates
ast-grep --pattern 'model: $model'
Length of output: 130527
Script:
#!/bin/bash
# Search for model update handling in the component
rg -A 5 'onUpdateModel' app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/
# Look for model change handling in useEffect
ast-grep --pattern 'useEffect($$$, [$$$model$$$])'
Length of output: 1663
Script:
#!/bin/bash
# Search for model-related useEffect hooks
rg -B 2 -A 5 'useEffect.*model' app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/
# Look for any other model update handling
ast-grep --pattern 'model.{0,50}update'
Length of output: 143
Script:
#!/bin/bash
# Get broader context around model handling
rg -B 10 -A 10 'handleModelUpdate|onUpdateModel|model.*useEffect' app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
# Look for message handling in useEffect
rg -B 5 -A 5 'useEffect.*message' app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
Length of output: 3089
/ok-to-test tags="@tag.Anvil" Fixes a bug where appsmith.updateModel was not working properly. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Updated the handling of model updates in the Custom Widget, improving the way messages are processed from the iframe. - **Documentation** - Clarified the expected input for the `onUpdateModel` function in relation to the message object. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12294856140> > Commit: cb0ae84 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12294856140&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Anvil` > Spec: > <hr>Thu, 12 Dec 2024 11:12:50 UTC <!-- end of auto-generated comment: Cypress test results -->
2 participants
/ok-to-test tags="@tag.Anvil"
Fixes a bug where appsmith.updateModel was not working properly.
Summary by CodeRabbit
Bug Fixes
Documentation
onUpdateModelfunction in relation to the message object.Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12294856140
Commit: cb0ae84
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Thu, 12 Dec 2024 11:12:50 UTC