fix(Switch widget): sync isSwitchedOn meta on programmatic changes#41749
fix(Switch widget): sync isSwitchedOn meta on programmatic changes#41749itsmejay80 wants to merge 2 commits intoappsmithorg:releasefrom
Conversation
…hanges Mirrors the existing CheckboxWidget behavior so external updates to defaultSwitchState (setValue, bound expressions) propagate to the isSwitchedOn meta property. Without this, bindings reading Switch.isSwitchedOn kept returning the stale meta value once the user had toggled the switch at least once. Fixes appsmithorg#41566 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughUpdated the Switch widget to detect external Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/client/src/widgets/SwitchWidget/widget/index.tsx`:
- Around line 437-440: The stale-meta synchronization is incorrectly gated by
the presence of an action handler; remove the onChange guard so meta updates run
whenever isSwitchedOn changes. In the componentDidUpdate (or the method
containing the if block) replace the condition that checks both
this.props.isSwitchedOn !== prevProps.isSwitchedOn && this.props.onChange with a
check that only compares this.props.isSwitchedOn !== prevProps.isSwitchedOn so
meta synchronization executes even when onChange is not set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13dbf9d9-82a0-4e3a-bf70-c9cedc65dcd3
📒 Files selected for processing (1)
app/client/src/widgets/SwitchWidget/widget/index.tsx
There was a problem hiding this comment.
Pull request overview
Fixes a stale-binding issue where Switch1.isSwitchedOn meta can fall out of sync with programmatic updates to the switch state after the user has interacted with the widget.
Changes:
- Adds
componentDidUpdatelogic inSwitchWidgetto sync theisSwitchedOnmeta value when the rendered switch state changes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Sync meta when isSwitchedOn changes programmatically so dependents don't see a stale value. | ||
| if ( | ||
| this.props.isSwitchedOn !== prevProps.isSwitchedOn && | ||
| this.props.onChange | ||
| ) { | ||
| this.props.updateWidgetMetaProperty( | ||
| "isSwitchedOn", | ||
| this.props.isSwitchedOn, | ||
| ); |
There was a problem hiding this comment.
The sync block is gated by this.props.onChange, but onChange is optional and often unset in the repro (programmatic setValue/bindings can update defaultSwitchState without configuring the Switch’s own onChange). In that case this code never runs and isSwitchedOn meta can still go stale. Remove the && this.props.onChange guard (and, if you want to avoid extra dispatches, consider keying the sync specifically off defaultSwitchState changes instead of isSwitchedOn changes).
| // Sync meta when isSwitchedOn changes programmatically so dependents don't see a stale value. | ||
| if ( | ||
| this.props.isSwitchedOn !== prevProps.isSwitchedOn && | ||
| this.props.onChange | ||
| ) { | ||
| this.props.updateWidgetMetaProperty( | ||
| "isSwitchedOn", | ||
| this.props.isSwitchedOn, | ||
| ); |
There was a problem hiding this comment.
This change fixes a regression scenario (programmatic updates after a manual toggle) but there doesn’t appear to be an automated test covering it. Please add a Cypress regression that: toggles the Switch once directly (to set meta), then changes it via Switch1.setValue(...)/another widget, and asserts dependents bound to {{Switch1.isSwitchedOn}} update correctly.
- Remove the `&& this.props.onChange` condition so the meta sync also runs for Switches that only act as inputs to bindings (no onChange action configured) — this was the actual repro in appsmithorg#41566. - Add a Switch_spec regression covering: user toggles once to set meta, then programmatic defaultSwitchState change, then assert that a dependent bound to Switch.isSwitchedOn updates. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Thanks for the review. Addressed both items in 7f655f5:
Note: |
Description
Fixes a stale-binding bug in the Switch widget. Once a user interacts with the widget at least once, its
isSwitchedOnmeta takes precedence over the deriveddefaultSwitchState. Any subsequent programmatic change todefaultSwitchState(viaSwitch1.setValue(...), a bound default state expression, or a ToggleButton driving the switch) updateddefaultSwitchStatebut left the stale meta in place. Widgets bound toSwitch1.isSwitchedOnkept reading the old value.This PR adds a small
componentDidUpdateblock inSwitchWidgetthat syncs the meta value wheneverisSwitchedOnchanges, mirroring the existing behavior inCheckboxWidget.componentDidUpdate(app/client/src/widgets/CheckboxWidget/widget/index.tsx).Fixes #41566
Automation
/ok-to-test tags="@tag.Switch, @tag.Binding"
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Testing
Manual repro from the issue:
Switch, aToggleButton, and aButtonon the canvas.Button.disabledto{{Switch1.isSwitchedOn}}.ToggleButton.onChangeto{{Switch1.setValue(!Switch1.isSwitchedOn)}}.Switchonce directly, then toggle viaToggleButton— before the fix,Button.disabledgoes stale; after the fix it follows the switch consistently.Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit