fix issue with disabled schedule exec option #5219
Conversation
WalkthroughA new React functional component named Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
| @@ -63,6 +63,7 @@ export default function SplitButton({ | |||
| }; | |||
|
|
|||
| const handleSelect = (option) => { | |||
| if (!option.onSelect) return; | |||
There was a problem hiding this comment.
I noticed this bug while testing. Clicking on the Schedule button in the modal threw an error. This fixes it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
app/packages/operators/src/ExecutionOptionItem.tsx (2)
7-11: Consider extracting inline styles to theme or styled componentsInline styles make it harder to maintain consistent styling across the application.
Consider creating a styled component or adding these styles to the theme:
const TagSpan = styled('span')(({ theme, disabled }) => ({ fontSize: "11px", color: disabled ? theme.text.secondary : theme.custom.primarySoft, marginLeft: "5px" }));
17-20: Add aria-label for better accessibilityThe component should provide proper accessibility attributes for screen readers.
- <div style={{ display: "flex", alignItems: "center" }}> + <div + style={{ display: "flex", alignItems: "center" }} + aria-label={`${label}${tag ? ` ${tag}` : ''}`} + >app/packages/operators/src/components/OperatorExecutionMenu/index.tsx (1)
49-54: Use callback for click handler to prevent memory leaksThe click handler should be memoized to prevent unnecessary re-renders and potential memory leaks.
+ const handleClick = React.useCallback(() => { + if (disabled) return; + onClose?.(); + onOptionClick?.(target); + target.onClick(); + }, [disabled, onClose, onOptionClick, target]); - onClick={() => { - if (disabled) return; - onClose?.(); - onOptionClick?.(target); - target.onClick(); - }} + onClick={handleClick}app/packages/operators/src/SplitButton.tsx (2)
66-68: Improve guard clause implementationThe guard clause for
onSelectcould be more explicit.- if (!option.onSelect) return; + if (typeof option.onSelect !== 'function') return; option.onSelect(); setOpen(false);
Line range hint
146-151: Add aria-label to improve menu item accessibilityThe ExecutionOptionItem implementation in the menu should include accessibility attributes.
primary={ <ExecutionOptionItem label={option.choiceLabel || option.label} tag={option.tag} disabled={option.disabled || !option.onClick} + aria-label={`${option.choiceLabel || option.label}${option.tag ? ` ${option.tag}` : ''}`} /> }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/packages/operators/src/ExecutionOptionItem.tsx(1 hunks)app/packages/operators/src/SplitButton.tsx(3 hunks)app/packages/operators/src/components/OperatorExecutionMenu/index.tsx(2 hunks)app/packages/operators/src/state.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/operators/src/ExecutionOptionItem.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/operators/src/SplitButton.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/operators/src/components/OperatorExecutionMenu/index.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/operators/src/state.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (3)
app/packages/operators/src/components/OperatorExecutionMenu/index.tsx (1)
34-34: Verify disabled state logic aligns with PR objective
The disabled state now correctly considers both isDisabledSchedule and missing onClick handler, which aligns with the PR objective of fixing the schedule execution option.
✅ Verification successful
Disabled state implementation is correctly implemented and aligns with PR objective
The disabled state logic in OperatorExecutionMenu is properly implemented:
- It correctly combines
isDisabledScheduleand missingonClickhandler conditions - The disabled state is consistently propagated to both MenuItem and ExecutionOptionItem
- The click handler properly respects the disabled state
- The implementation aligns with the type definition in state.ts where
isDisabledScheduleis defined as an optional boolean
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the disabled state implementation across the codebase
rg "isDisabledSchedule" --type ts
Length of output: 305
Script:
#!/bin/bash
# Let's check the context around the state definition and usage
rg -B 3 -A 3 "isDisabledSchedule" app/packages/operators/src/state.ts
# Also check how the target prop is defined in the component
ast-grep --pattern 'interface $_ {
$$$
isDisabledSchedule?: $_
$$$
}'
Length of output: 429
Script:
#!/bin/bash
# Let's check how the OperatorExecutionMenu component is using this prop
rg -B 10 -A 5 "disabled={target.isDisabledSchedule" app/packages/operators/src/components/OperatorExecutionMenu/index.tsx
# Also check if there are any tests verifying this behavior
fd "test|spec" --extension ts --extension tsx | xargs rg "isDisabledSchedule"
Length of output: 329
Script:
#!/bin/bash
# Let's try with proper escaping of the curly braces
rg -B 10 -A 5 "disabled=\{target\.isDisabledSchedule" app/packages/operators/src/components/OperatorExecutionMenu/index.tsx
# Let's also check the entire component implementation
cat app/packages/operators/src/components/OperatorExecutionMenu/index.tsx
Length of output: 2646
app/packages/operators/src/state.ts (2)
249-249: LGTM: Clean type extension
The addition of the optional isDisabledSchedule property to OperatorExecutionOption type is well-designed and maintains backward compatibility.
350-350: Verify the disabled schedule implementation
The implementation looks correct. Let's verify that this property is properly handled in all components that consume OperatorExecutionOption.
✅ Verification successful
Let me check the implementation details in these files to verify how isDisabledSchedule is handled.
Implementation of isDisabledSchedule is properly handled
The verification shows that isDisabledSchedule is correctly implemented and consumed:
- It's defined in the
OperatorExecutionOptioninterface instate.ts - It's properly set in the state management logic when scheduling is not available
- It's correctly used in the UI to disable menu items in
OperatorExecutionMenucomponent through thedisabledprop
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all components that consume OperatorExecutionOption to ensure proper handling
# Search for files importing or using OperatorExecutionOption
rg -l "OperatorExecutionOption"
# Search for components rendering schedule-related UI
ast-grep --pattern 'isDisabledSchedule'
Length of output: 404
Script:
#!/bin/bash
# Check implementation details in the relevant files
rg -A 5 -B 5 "isDisabledSchedule"
# Check schedule-related UI components and their props
ast-grep --pattern 'interface OperatorExecutionOption {
$$$
}'
# Check how schedule options are handled in the execution menu
rg -A 10 "schedule" app/packages/operators/src/components/OperatorExecutionMenu/index.tsx
Length of output: 2597
Screen.Recording.2024-12-05.at.8.06.25.AM.mov
This fixes an issue where the schedule option in the
OperatorExecutionButtonViewwas click-able when it should be disabled. It also normalizes the display of the Execution Options in both the modal SplitButton and the OperatorExecutionButton.Summary by CodeRabbit
New Features
ExecutionOptionItemcomponent for improved rendering of execution options.Itemcomponent to streamline theOperatorExecutionMenufunctionality.isDisabledScheduleproperty to indicate scheduling availability.Bug Fixes
handleSelectfunction to prevent issues with undefined actions.Refactor
PrimaryWithTagfunction to simplify theSplitButtoncomponent.