feat: add mutual exclusivity between ChatInput and Webhook components#12036
feat: add mutual exclusivity between ChatInput and Webhook components#12036Jkavia merged 10 commits intorelease-1.8.0from
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Missing test scenarios:
For disableItem:
| Scenario | Expected | Priority |
|---|---|---|
disableItem("ChatInput", { chatInput: false, webhookInput: false }) |
false |
🔴 Must have — verifies item is enabled when neither exists |
disableItem("ChatInput", { chatInput: true, webhookInput: false }) |
true |
🔴 Must have — original "already added" behavior still works |
disableItem("Webhook", { chatInput: false, webhookInput: true }) |
true |
🔴 Must have — original "already added" behavior still works |
disableItem("Webhook", { chatInput: false, webhookInput: false }) |
false |
🔴 Must have — verifies item is enabled when neither exists |
disableItem("SomeOtherComponent", { chatInput: true, webhookInput: true }) |
false |
🟡 Should have — verifies non-input components aren't affected |
disableItem("ChatInput", { chatInput: true, webhookInput: true }) |
true |
🟡 Should have — both present edge case |
For getDisabledTooltip:
| Scenario | Expected | Priority |
|---|---|---|
getDisabledTooltip("ChatInput", { chatInput: true, webhookInput: false }) |
"Chat input already added" |
🔴 Must have — original behavior |
getDisabledTooltip("Webhook", { chatInput: false, webhookInput: true }) |
"Webhook already added" |
🔴 Must have — original behavior |
getDisabledTooltip("ChatInput", { chatInput: false, webhookInput: false }) |
"" |
🔴 Must have — no tooltip when enabled |
getDisabledTooltip("SomeOther", { chatInput: true, webhookInput: true }) |
"" |
🟡 Should have |
Should fix:
- Tests — add regression coverage for the original "already added" behavior. Without these, a future refactor could break the original logic with no test catching it.
- Tests — add base case where neither component exists. These verify the "enabled" state works correctly.
Nice to have:
3. Consider a data-driven EXCLUSIVITY_RULES constant to reduce structural coupling between the two files
4. Extract "ChatInput" and "Webhook" string literals to shared constants
5. Add test for non-input components to ensure no false positives
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-1.8.0 #12036 +/- ##
================================================
Coverage ? 37.17%
================================================
Files ? 1591
Lines ? 78071
Branches ? 11833
================================================
Hits ? 29026
Misses ? 47390
Partials ? 1655
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This PR addresses an issue where the canvas allows both a Chat Input and a Webhook component to coexist within the same flow, even though they are mutually exclusive entry points.
Currently, even when invoking the flow via the
/runendpoint, the Webhook component is still initialized and executed if present. This behavior contributes to ambiguity and unintended execution paths. A deeper architectural fix will be addressed in a future release.As an immediate safeguard, this PR introduces UI-level mutual exclusivity between Chat Input and Webhook components.
Related JIRA: https://datastax.jira.com/jira/software/projects/LE/boards/4573?jql=labels%20%3D%20release-v1.8.0&selectedIssue=LE-455