feat: implement disabled functionality in the property pane section and control #39939
Conversation
…dd-disabled-property-to-property-pane-config
…enabled" This reverts commit 9e6417d.
WalkthroughThe changes extend the property pane functionality by introducing new optional properties ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Section as PropertySection
participant Callback as shouldDisableSection Callback
User->>Section: Render component with props
Section->>Callback: Evaluate disabled state using widget properties
Callback-->>Section: Return true/false
alt Disabled True
Section->>Section: Apply disabled styling and tooltip with help text
Section-->>User: Render non-interactive UI
else Disabled False
Section->>Section: Render interactive UI normally
Section-->>User: Enable full interaction
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🪧 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
|
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
github-actions
bot
added
Property Pane
jacquesikot
added
Query & Widgets Pod
github-actions
bot
added
Property Pane
![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
🧹 Nitpick comments (4)
app/client/src/pages/Editor/PropertyPane/PropertySection.test.tsx (1)
6-6: Use optional chaining for safer property access.The disabled property check could be improved with optional chaining to avoid potential errors if the property is undefined.
- const isSectionDisabled = props.disabled && props.disabled(props.widgetProps, props.propertyPath || ""); + const isSectionDisabled = props.disabled?.(props.widgetProps, props.propertyPath || "");🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/pages/Editor/PropertyPane/PropertyControl.test.tsx (1)
7-7: Use optional chaining for safer property access.Similar to the PropertySection mock, using optional chaining would make this property check safer.
- const isDisabled = props.disabled ? props.disabled(props.widgetProperties, props.propertyName) : false; + const isDisabled = props.disabled?.(props.widgetProperties, props.propertyName) ?? false;app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx (1)
93-114: Use optional chaining for safer property access.The current implementation might throw errors if
props.disabledis undefined.- const isControlDisabled = props.disabled && props.disabled(widgetProperties, props.propertyName); + const isControlDisabled = props.disabled?.(widgetProperties, props.propertyName);🧰 Tools
🪛 Biome (1.9.4)
[error] 111-111: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/pages/Editor/PropertyPane/PropertySection.tsx (1)
117-119: Use optional chaining for safer property access.The current implementation might throw errors if
props.disabledis undefined.- const isSectionDisabled = props.disabled && props.disabled(widgetProps, props.propertyPath || ""); + const isSectionDisabled = props.disabled?.(widgetProps, props.propertyPath || "");🧰 Tools
🪛 Biome (1.9.4)
[error] 118-118: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/src/constants/PropertyControlConstants.tsx(2 hunks)app/client/src/pages/Editor/PropertyPane/PropertyControl.test.tsx(1 hunks)app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx(7 hunks)app/client/src/pages/Editor/PropertyPane/PropertyControlsGenerator.tsx(1 hunks)app/client/src/pages/Editor/PropertyPane/PropertySection.test.tsx(1 hunks)app/client/src/pages/Editor/PropertyPane/PropertySection.tsx(6 hunks)app/client/src/pages/Editor/PropertyPane/helpers.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx (1)
app/client/src/pages/Editor/PropertyPane/PropertySection.tsx (1)
DisabledContext(102-102)
🪛 Biome (1.9.4)
app/client/src/pages/Editor/PropertyPane/PropertySection.test.tsx
[error] 6-6: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/pages/Editor/PropertyPane/PropertySection.tsx
[error] 118-118: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx
[error] 111-111: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (19)
app/client/src/pages/Editor/PropertyPane/PropertyControlsGenerator.tsx (1)
63-64: Looks good - disabled properties added to PropertySection.The new properties implement the disabled functionality in the property pane section as intended. This allows sections to be disabled and provide contextual help text.
app/client/src/pages/Editor/PropertyPane/PropertySection.test.tsx (6)
5-33: MockPropertySection implementation looks appropriate.The mock implementation correctly simulates the section's behavior when disabled, including CSS classes and help text display.
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
37-57: Test setup looks good.The test configuration with separate mock functions and a utility for default props makes the tests clean and maintainable.
59-69: First test case is properly structured.Correctly tests that the section renders normally when not disabled.
71-86: Disabled section test case is well implemented.Properly tests that when disabled, the section has the appropriate CSS classes and cursor style.
88-113: Help text test cases are well structured.Both help text test cases (showing when disabled and not showing when not disabled) are properly implemented.
115-138: Toggle functionality tests are comprehensive.Tests for both disabled and enabled click handling on the section title are well implemented.
app/client/src/pages/Editor/PropertyPane/helpers.tsx (1)
63-71: Good refactoring of object construction.Extracting the object construction to a named variable improves code readability. The comment about passing through the disabled property provides helpful context.
app/client/src/pages/Editor/PropertyPane/PropertyControl.test.tsx (2)
6-23: MockPropertyControl implementation looks good.The mock properly simulates the control's behavior when disabled, including visual indicators and help text.
27-98: Test cases are comprehensive and well-structured.All test cases for the PropertyControl component (normal rendering, disabled rendering, help text display) are properly implemented and cover the necessary scenarios.
app/client/src/constants/PropertyControlConstants.tsx (2)
56-67: LGTM: Well-documented disabled functionality for property sections.The addition of
disabledanddisabledHelpTextproperties toPropertyPaneSectionConfigis well-documented with clear JSDoc comments explaining their purpose and usage.
220-233: LGTM: Consistent implementation of disabled functionality for property controls.This implementation mirrors the section configuration approach, maintaining consistency in the codebase with proper documentation.
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx (3)
1-67: Import DisabledContext to enable context-aware disabled state.The addition of DisabledContext import allows the PropertyControl to be aware of its parent section's disabled state.
892-896: LGTM: Conditional help text based on disabled state.The component correctly prioritizes showing the disabled help text when the control is disabled.
926-938: LGTM: Apply appropriate visual styling for disabled state.Adding the cursor-not-allowed and reduced opacity classes provides clear visual feedback for disabled controls.
app/client/src/pages/Editor/PropertyPane/PropertySection.tsx (4)
87-90: LGTM: Property interface extension for disabled functionality.The PropertySectionProps interface has been properly extended with the new disabled properties.
101-102: LGTM: Create DisabledContext for propagating disabled state to children.Creating a context for disabled state allows child components to be aware of their parent section's disabled state without prop drilling.
189-196: LGTM: Apply visual styling and behavior changes for disabled sections.The implementation correctly applies styling changes and disables the click handler when the section is disabled.
232-240: LGTM: Conditionally wrap disabled sections with tooltips.The tooltip implementation provides helpful context when a section is disabled.
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/components/propertyControls/CodeEditorControl.tsx (1)
40-42: Consider parameterizing the theme rather than hardcoding.The properties are now passed directly to LazyCodeEditor which improves readability. However, the theme is now hardcoded to EditorTheme.LIGHT instead of being passed through props. This might limit theme customization.
- theme={EditorTheme.LIGHT} + theme={this.props.theme || EditorTheme.LIGHT}Also applies to: 45-47, 52-52
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/components/propertyControls/CodeEditorControl.tsx(1 hunks)
🔇 Additional comments (4)
app/client/src/components/propertyControls/CodeEditorControl.tsx (4)
1-1: Import updates look good.The imports have been updated to include EditorTheme, properly type the component, and import LazyCodeEditor correctly.
Also applies to: 5-5, 9-14
21-21: Props destructuring properly includes the new disabled property.The disabled property is now correctly destructured from props.
28-33: Good implementation of the disabled state conversion.The conversion from a function-based disabled property to a boolean value is implemented correctly. This ensures proper compatibility between the PropertyPaneControlConfig's function-based disabled property and LazyCodeEditor's boolean isReadOnly prop.
34-36: Proper handling of maxHeight property.The maxHeight is correctly converted to a string when present in controlConfig. This ensures proper type handling when passing to LazyCodeEditor.
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
…x cyclic dependency
| // Context is being provided to re-render anything that subscribes to this context on open and close | ||
| export const CollapseContext = createContext<boolean>(false); | ||
| // Context for propagating the disabled state from section to child controls | ||
| export const DisabledContext = createContext<boolean>(false); |
There was a problem hiding this comment.
Where are we providing value to this context?
There was a problem hiding this comment.
Thanks for this pick, I have moved away from using the disabled context and the isDisabled is computed inside the PropertyControl and propertySection independently
| @@ -83,6 +80,9 @@ interface PropertySectionProps { | |||
| // TODO: Fix this the next time the file is edited | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| hidden?: (props: any, propertyPath: string) => boolean; | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
|
|
||
| return ( | ||
| <LazyCodeEditor | ||
| AIAssisted |
There was a problem hiding this comment.
Nice work refactoring this!
| const props: Partial<ControlProps> = {}; | ||
| // PropertyPaneControlConfig's disabled is a function (props: any, propertyPath: string) => boolean | ||
| // while LazyCodeEditor expects a boolean. Convert function to boolean result. | ||
| const isReadOnly = |
There was a problem hiding this comment.
Can we use the word isDisabled rather than isReadOnly? It's better to follow the same nomenclature throughout.
| * @param propertyPath - Path to the widget property | ||
| * @returns - True if the section should be disabled, false otherwise | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
Can we remove this? Let's not use any; you can prefer using unknown but since these are WidgetProps, do you think we can use that without any cyclic dependency?
There was a problem hiding this comment.
I have tried widget props and I still get a lot of errors on CI - build failure due to type check.
…nitions for disabled and hidden properties in property controls
github-actions
bot
added
Table Widget V2
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx (2)
115-116: Use optional chaining for safer code.The static analysis tool suggests using optional chaining instead of the logical AND operator for better code safety. This would eliminate potential undefined behavior.
- const isControlDisabled = - props.disabled && props.disabled(widgetProperties, props.propertyName); + const isControlDisabled = + props.disabled?.(widgetProperties, props.propertyName);🧰 Tools
🪛 Biome (1.9.4)
[error] 116-116: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
895-897: Consider providing a default disabled message.When a control is disabled but no
disabledHelpTextis provided, the help text becomes empty. This might confuse users about why the control is disabled. Consider providing a generic default message.- const helpText = isControlDisabled - ? props.disabledHelpText || "" + const helpText = isControlDisabled + ? props.disabledHelpText || "This control is currently disabled"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
app/client/src/components/propertyControls/CodeEditorControl.tsx(1 hunks)app/client/src/constants/PropertyControlConstants.tsx(2 hunks)app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx(4 hunks)app/client/src/pages/Editor/PropertyPane/PropertyPaneContexts.tsx(1 hunks)app/client/src/pages/Editor/PropertyPane/PropertySection.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/src/components/propertyControls/CodeEditorControl.tsx
- app/client/src/pages/Editor/PropertyPane/PropertySection.tsx
- app/client/src/constants/PropertyControlConstants.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx
[error] 116-116: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx (1)
930-930: Good implementation of disabled state visual feedback.The conditional class for the disabled state provides good visual feedback with cursor and opacity changes, improving the user experience.
app/client/src/pages/Editor/PropertyPane/PropertyPaneContexts.tsx (1)
1-4:✅ Verification successful
CollapseContext implementation looks good.
The context is well-implemented with a sensible default value. The comment provides clarity on its purpose.
However, based on past comments, there seems to have been a decision to move away from using a context for disabled state. Please ensure this context is actually being utilized in the codebase.
🏁 Script executed:
#!/bin/bash # Find usages of CollapseContext in the codebase echo "Searching for imports of CollapseContext..." rg -l "import.*CollapseContext.*from" --type tsx --type ts echo "Searching for usages of CollapseContext.Provider..." rg "CollapseContext\.Provider" --type tsx --type tsLength of output: 380
Below is a new script to re-verify the usage of CollapseContext using file glob patterns for TypeScript and TSX files. Please re-run this script to confirm whether the context is actually used in the codebase:
Once you have the output, please review if any components or files are importing or providing the context. If no usages are found, double-check whether the move away from using context for disabled state is intentional or if further refactoring is needed.
🏁 Script executed:
#!/bin/bash echo "Searching for imports of CollapseContext..." rg "import.*CollapseContext.*from" -g "*.tsx" -g "*.ts" echo "Searching for usages of CollapseContext.Provider..." rg "CollapseContext\.Provider" -g "*.tsx" -g "*.ts"Length of output: 954
CollapseContext is implemented correctly, and its usage in the codebase has been verified.
- The context, defined in
app/client/src/pages/Editor/PropertyPane/PropertyPaneContexts.tsxwith a sensible default value, is imported in multiple components.- In particular,
PropertySection.tsxutilizesCollapseContext.Provider, confirming that this context isn't redundant.Please ensure that using this context aligns with the intended design regarding the disabled state.
…in PropertyPaneSectionConfig to use unknown type
…in PropertyPaneSectionConfig to use Record<string, unknown> type
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/client/src/constants/PropertyControlConstants.tsx (1)
224-225: 🛠️ Refactor suggestionInconsistent type usage for props parameter.
While the
hiddenproperty inPropertyPaneSectionConfigwas updated to useRecord<string, unknown>, thedisabledproperty here still usesany. For consistency and type safety, this should also be updated.- // eslint-disable-next-line @typescript-eslint/no-explicit-any - disabled?: (props: any, propertyPath: string) => boolean; + disabled?: (props: Record<string, unknown>, propertyPath: string) => boolean;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/constants/PropertyControlConstants.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (3)
app/client/src/constants/PropertyControlConstants.tsx (3)
53-53: Good improvement in type safety.Replacing
anywithRecord<string, unknown>improves type safety while maintaining flexibility. This is a good step toward reducing the use ofanythroughout the codebase.
59-64: Solid implementation of the disabled functionality.The addition of the
disabledcallback anddisabledHelpTextproperties to the section config is well-documented and clearly implements the requested feature. The typed parameter ensures consistency with the updatedhiddenproperty.
227-230: Well-documented tooltip property.The
disabledHelpTextproperty is clearly documented to explain its purpose and behavior. This will make it easier for other developers to understand and implement correctly.
| @@ -205,6 +214,20 @@ export interface PropertyPaneControlConfig { | |||
| // TODO: Fix this the next time the file is edited | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| hidden?: (props: any, propertyPath: string) => boolean; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update this property to use more specific typing.
Similar to line 53, this property should be updated from any to Record<string, unknown> for consistency across the interface and improved type safety.
- // TODO: Fix this the next time the file is edited
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- hidden?: (props: any, propertyPath: string) => boolean;
+ hidden?: (props: Record<string, unknown>, propertyPath: string) => boolean;Committable suggestion skipped: line range outside the PR's diff.
…in PropertySection to use Record<string, unknown> type
…in PropertySection to use generic type T
…pertyPaneSectionConfig and PropertySection to use any type
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx (3)
1069-1079: 🛠️ Refactor suggestionPrevent editing when control is disabled.
The edit button's click handler should respect the disabled state of the control.
<Button className={clsx( `${config.label}`, "edit-control flex items-center justify-center text-center h-7 w-7", `t--edit-control-${config.label}`, )} isIconButton kind="tertiary" + isDisabled={isControlDisabled} onClick={() => { + if (isControlDisabled) return; setIsRenaming(true); AnalyticsUtil.logEvent( "CUSTOM_WIDGET_EDIT_EVENT_CLICKED", { widgetId: widgetProperties.widgetId, }, ); }} size="sm" startIcon="pencil-line" />
1082-1113: 🛠️ Refactor suggestionPrevent deletion when control is disabled.
The delete button's click handler should also respect the disabled state of the control.
<Button className={clsx( `${config.label}`, "delete-control flex items-center justify-center text-center h-7 w-7", `t--delete-control-${config.label}`, )} isIconButton kind="tertiary" + isDisabled={isControlDisabled} onClick={() => { + if (isControlDisabled) return; if ( config.controlConfig && typeof config.controlConfig.onDelete === "function" ) { const updates = config.controlConfig.onDelete(widgetProperties); onBatchUpdateProperties(updates); } onDeleteProperties([config.propertyName]); AnalyticsUtil.logEvent( "CUSTOM_WIDGET_DELETE_EVENT_CLICKED", { widgetId: widgetProperties.widgetId, }, ); }} size="sm" startIcon="trash" />
1114-1126: 🛠️ Refactor suggestionPrevent adding actions when control is disabled.
The add action button should also respect the disabled state of the control when applicable.
{!isDynamic && config.controlType === "ACTION_SELECTOR" && ( <Button className={clsx( `${config.label}`, "add-action flex items-center justify-center text-center h-7 w-7", `t--add-action-${config.label}`, )} isIconButton kind="tertiary" + isDisabled={isControlDisabled} onClick={() => { + if (isControlDisabled) return; setShowEmptyBlock(true) }} startIcon="plus" /> )}
🧹 Nitpick comments (2)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx (2)
108-110: Use optional chaining for safer function access.The current implementation checks if
props.disabledexists and then calls it, but it could be simplified using optional chaining.- const isControlDisabled = - props.disabled && props.disabled(widgetProperties, props.propertyName); + const isControlDisabled = props.disabled?.(widgetProperties, props.propertyName);🧰 Tools
🪛 Biome (1.9.4)
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
923-923: Consider adding ARIA attributes for better accessibility.While the visual styling for disabled state (cursor-not-allowed and opacity) is helpful, adding ARIA attributes would improve accessibility for screen readers.
- className={`t--property-control-wrapper t--property-control-${className} group relative ${isControlDisabled ? "cursor-not-allowed opacity-50" : ""}`} + className={`t--property-control-wrapper t--property-control-${className} group relative ${isControlDisabled ? "cursor-not-allowed opacity-50" : ""}`} + aria-disabled={isControlDisabled ? "true" : "false"}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx (2)
888-892: LGTM! Conditional help text implementation.The implementation correctly handles displaying different help text based on the disabled state of the control.
1130-1146:❓ Verification inconclusive
Forward the disabled state to child controls.
You should pass the disabled state to the child controls created by PropertyControlFactory to ensure the entire control hierarchy respects the disabled state.
🏁 Script executed:
#!/bin/bash # Check if PropertyControlFactory handles disabled state rg -A 5 'createControl' app/client/src/utils/PropertyControlFactory.tsLength of output: 157
I'm going to run an additional verification script to locate the
PropertyControlFactoryfile in case it exists elsewhere in the repo so we can confirm whether the disabled state is being forwarded. Please hold on.
🏁 Script executed:
#!/bin/bash # Search for any file named PropertyControlFactory with a .ts extension in the repository fd -t f -e ts 'PropertyControlFactory'Length of output: 38
Forward Disabled State to Child Controls
The current implementation in
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx(lines 1130–1146) calls
PropertyControlFactory.createControl(...)but does not pass a disabled state. This omission might cause child controls to ignore the disabled setting. Since we couldn’t locate the corresponding implementation in a file named “PropertyControlFactory” (or similar) in the repository, please manually verify the expected behavior of the child controls regarding the disabled state. If they are designed to accept a disabled flag, update the options passed tocreateControl(for example, by includingdisabled: props.disabled) to ensure consistency throughout the control hierarchy.
- Action Item: Confirm if the child controls expect a disabled parameter and, if so, modify the call accordingly.
app/client/src/components/propertyControls/CodeEditorControl.tsx
Outdated
Show resolved
Hide resolved
…dd-disabled-property-to-property-pane-config
…WidgetV2 configuration
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/pages/Editor/PropertyPane/PropertySection.test.tsx (1)
21-49: Improve conditional logic with optional chaining.The conditional logic for determining if a section is disabled should use optional chaining to prevent potential runtime errors.
const isSectionDisabled = - props.shouldDisableSection && - props.shouldDisableSection(props.widgetProps, props.propertyPath || ""); + props.shouldDisableSection?.(props.widgetProps, props.propertyPath || "");🧰 Tools
🪛 Biome (1.9.4)
[error] 23-24: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx (1)
108-111: Use optional chaining for shouldDisableSection.Using optional chaining will make the code more robust by preventing potential errors if shouldDisableSection is undefined.
const isControlDisabled = - props.shouldDisableSection && - props.shouldDisableSection(widgetProperties, props.propertyName); + props.shouldDisableSection?.(widgetProperties, props.propertyName);🧰 Tools
🪛 Biome (1.9.4)
[error] 109-110: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
app/client/src/components/propertyControls/CodeEditorControl.tsx(1 hunks)app/client/src/constants/PropertyControlConstants.tsx(2 hunks)app/client/src/pages/Editor/PropertyPane/PropertyControl.test.tsx(1 hunks)app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx(4 hunks)app/client/src/pages/Editor/PropertyPane/PropertyControlsGenerator.tsx(1 hunks)app/client/src/pages/Editor/PropertyPane/PropertySection.test.tsx(1 hunks)app/client/src/pages/Editor/PropertyPane/PropertySection.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/src/pages/Editor/PropertyPane/PropertyControlsGenerator.tsx
- app/client/src/pages/Editor/PropertyPane/PropertyControl.test.tsx
- app/client/src/pages/Editor/PropertyPane/PropertySection.tsx
- app/client/src/components/propertyControls/CodeEditorControl.tsx
🧰 Additional context used
🧬 Code Definitions (1)
app/client/src/pages/Editor/PropertyPane/PropertySection.test.tsx (2)
app/client/src/components/propertyControls/CodeEditorControl.tsx (1)
render(17-62)app/client/src/pages/Editor/PropertyPane/PropertySection.tsx (1)
PropertySection(95-231)
🪛 Biome (1.9.4)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx
[error] 109-110: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/pages/Editor/PropertyPane/PropertySection.test.tsx
[error] 23-24: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (12)
app/client/src/pages/Editor/PropertyPane/PropertySection.test.tsx (8)
1-19: Good test setup and interface definition.The interface MockPropertySectionProps is well-defined with all necessary properties to test the disabled functionality.
51-53: Good mock implementation.The Jest mock correctly returns the MockPropertySection component with the same props passed in.
77-88: Clean and focused test case.This test verifies that sections render normally when not disabled. Good usage of expectations to confirm absence of disabled styling.
90-106: Comprehensive disabled section test.The test properly checks both the cursor-not-allowed and opacity-50 classes when the section is disabled.
108-121: Good tooltip test implementation.This test verifies that the disabled help text appears when a section is disabled. Consider also testing if the tooltip appears in the right position.
123-133: Good negative test case.Properly verifies that help text isn't shown when the section isn't disabled using queryByTestId instead of getByTestId.
135-143: Good interaction test.This test verifies that clicking on the section title triggers the toggle function when not disabled.
145-158: Comprehensive negative interaction test.This test confirms that clicking a disabled section doesn't trigger the toggle function - important for validating the disabled behavior.
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx (2)
889-893: Good conditional help text implementation.The help text is correctly prioritized to show disabled text when the control is disabled, falling back to other text when not disabled.
924-924: Good disabled styling implementation.The ControlWrapper className correctly applies cursor-not-allowed and opacity-50 when the control is disabled, providing clear visual indication to users.
app/client/src/constants/PropertyControlConstants.tsx (2)
56-68: Good property section disabling functionality.The shouldDisableSection function and disabledHelpText properties are well-documented with clear JSDoc comments explaining their purpose.
Note: Consider using a more specific type than
anyfor the props parameter in shouldDisableSection to improve type safety.
221-234: Good property control disabling functionality.The shouldDisableSection function and disabledHelpText properties match the implementation in the section config, maintaining consistency across the codebase.
Note: For consistency with other similar functions in this file, consider using a more specific type than
anyfor the props parameter.
jacquesikot
deleted the
feat/add-disabled-property-to-property-pane-config
branch
🔍 Problem
Currently, the property pane does not support disabling entire sections or individual controls. This leads to several issues:
💡 Solution
This PR introduces the ability to:
disabledcallback inPropertyPaneSectionConfig.disabledcallback inPropertyPaneControlConfig.disabledHelpTextproperty for both sections and controls.🔑 Key Implementation Details
CollapseContextto a dedicated contexts file for better organisation and cyclic dependency prevention.🤔 Why This Solution?
This approach provides several benefits:
✔ Backward Compatibility – Optional properties ensure existing code remains unaffected.
✔ Flexible Implementation – The callback approach allows for dynamic disabling logic.
✔ Consistent UX – Clear visual indicators and tooltips improve user experience.
✔ Performance Optimized – Using React Context prevents prop drilling and reduces unnecessary re-renders.
🧪 Tests
Comprehensive tests have been added to ensure the new functionality works as expected:
📌
PropertySection.test.tsx– Tests section rendering in normal and disabled states.📌
PropertyControl.test.tsx– Tests control rendering in normal and disabled states.✅ Tests Verify:
Fixes #39940
Automation
/ok-to-test tags="@tag.Binding, @tag.IDE, @tag.Sanity, @tag.Widget"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14191720905
Commit: 285d46d
Cypress dashboard.
Tags:
@tag.Binding, @tag.IDE, @tag.Sanity, @tag.WidgetSpec:
Tue, 01 Apr 2025 10:55:54 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Tests
Refactor
Chores
Documentation