chore: fix onStateChange prop bug in sidebar#37710
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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
|
jsartisan
changed the title
github-actions
bot
added
the
skip-changelog
jsartisan
changed the title
![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 (2)
app/client/packages/design-system/widgets/src/components/Sidebar/stories/ControlledStateSidebar.tsx (2)
14-16: Consider adding prop validationWhile the implementation is clean, consider adding prop validation to ensure required props are provided.
export const ControlledStateSidebar = (props: ControlledStateSidebarProps) => { const { children, ...sidebarProps } = props; + if (!children) { + throw new Error('Children are required for ControlledStateSidebar'); + } const [state, setState] = useState<SidebarState>("collapsed");
23-26: Consider making styles configurableThe hardcoded styles might limit component reusability. Consider making these styles configurable through props.
+ type StyleProps = { + containerHeight?: string; + borderStyle?: string; + }; type ControlledStateSidebarProps = SidebarProps & { children: React.ReactNode; + styleProps?: StyleProps; }; // In the component const { children, styleProps, ...sidebarProps } = props; // In the style prop style={{ - height: "50vh", - border: "1px solid var(--color-bd-elevation-1)", + height: styleProps?.containerHeight ?? "50vh", + border: styleProps?.borderStyle ?? "1px solid var(--color-bd-elevation-1)", }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/stories/ControlledStateSidebar.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx(2 hunks)
🔇 Additional comments (6)
app/client/packages/design-system/widgets/src/components/Sidebar/stories/ControlledStateSidebar.tsx (2)
1-12: LGTM! Clean imports and proper type definitions
The imports are well-organized and the type extension is properly implemented.
16-16: Verify initial state requirement
The sidebar is initialized as "collapsed". Ensure this aligns with the product requirements.
Also applies to: 20-22
✅ Verification successful
The previous pattern didn't work because it was too specific. Let's try a broader search to understand the sidebar state management patterns and requirements across the codebase.
Let's try without the typescript file type flag since it's not recognized. Also adding a search for documentation that might specify the requirements.
Initial state is correctly implemented with proper fallback mechanism
The implementation is correct as the component accepts a defaultState prop in SidebarProvider which serves as the source of truth. The "collapsed" state in the story is just for demonstration purposes and doesn't affect the actual component's behavior. The SidebarProvider properly handles state initialization through props:
defaultStateprop for uncontrolled usagestateprop for controlled usage- Internal state management via
useStatewith proper type safety
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other sidebar implementations to verify consistent initial states
ast-grep --pattern 'useState<SidebarState>($initial)'
Length of output: 55
Script:
#!/bin/bash
# Search for SidebarState type definition and usage
rg -t typescript "SidebarState" -A 3
# Search for any sidebar state initialization patterns
rg -t typescript "useState.*sidebar" -i
Length of output: 148
Script:
#!/bin/bash
# Search for SidebarState type definition and usage
rg "SidebarState" -A 3
# Search for any sidebar state initialization patterns
rg "useState.*sidebar" -i
# Look for any documentation or stories related to sidebar state
fd -e md -e mdx -X rg "sidebar.*state|initial.*sidebar" -i
Length of output: 5046
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx (2)
36-37: LGTM! This fixes the state synchronization issue.
The change ensures consistent state updates by always updating local state before notifying parent components via onStateChange.
36-37: Verify state updates in both controlled and uncontrolled modes.
Let's ensure the changes work correctly in all scenarios:
✅ Verification successful
State management implementation verified and working correctly
The implementation correctly handles both controlled and uncontrolled modes:
- Controlled mode is properly demonstrated in
ControlledStateSidebar.tsxwith external state management - Types in
types.tscorrectly define the state interface and optional props for controlled mode - The state update logic in
SidebarProvider.tsxproperly syncs internal and external state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usage of SidebarProvider to verify both controlled and uncontrolled modes
# Search for SidebarProvider usage
rg -A 5 "SidebarProvider" --type tsx
# Search for onStateChange prop usage
rg -A 5 "onStateChange.*SidebarProvider" --type tsx
Length of output: 146
Script:
#!/bin/bash
# Let's try with correct file extensions and search patterns
# Search for SidebarProvider usage
rg -A 5 "SidebarProvider" --type ts --type tsx --type jsx
# Search for state-related props in SidebarProvider
rg -A 5 "(state|setState|onStateChange).*SidebarProvider" --type ts --type tsx --type jsx
# Find the test file for SidebarProvider
fd "SidebarProvider.test" --type f
# Look for any stories or examples
fd "SidebarProvider.stories" --type f
Length of output: 278
Script:
#!/bin/bash
# Let's try with just the basic grep command without file type filters
rg "SidebarProvider"
# Also search for the file directly
fd "SidebarProvider"
# Look for test files in the same directory
cd app/client/packages/design-system/widgets/src/components/Sidebar/src/
ls -la
Length of output: 3164
Script:
#!/bin/bash
# Let's examine the controlled state example and stories
cat app/client/packages/design-system/widgets/src/components/Sidebar/stories/ControlledStateSidebar.tsx
# Check the types to understand the state interface
cat app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts
# Look at the context implementation
cat app/client/packages/design-system/widgets/src/components/Sidebar/src/context.tsx
Length of output: 2289
app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx (2)
4-6: LGTM! Clean import organization
Import consolidation and new component import look good.
131-139: Verify the controlled state behavior
The story structure looks good, but since this PR fixes an onStateChange bug, we should verify the controlled state implementation.
Let's check the ControlledStateSidebar implementation:
✅ Verification successful
The controlled state implementation looks good
The ControlledStateSidebar component correctly implements state management and properly passes the state and state updater to SidebarProvider:
- Uses useState hook to manage collapsed/expanded state
- Properly connects setState as the onStateChange handler
- Follows the controlled component pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ControlledStateSidebar implementation and its usage of onStateChange
# Find the ControlledStateSidebar implementation
ast-grep --pattern 'export const ControlledStateSidebar = $_' -A 20
# Look for onStateChange usage in the component
rg "onStateChange" --type ts --type tsx
Length of output: 162824
Script:
#!/bin/bash
# Let's try a different approach to find the ControlledStateSidebar implementation and onStateChange usage
# Search for onStateChange in the specific file path
rg "onStateChange" "app/client/packages/design-system/widgets/src/components/Sidebar/stories/ControlledStateSidebar.tsx"
# Also look for state management related code in the same file
rg "useState|state" "app/client/packages/design-system/widgets/src/components/Sidebar/stories/ControlledStateSidebar.tsx"
# Look for SidebarProvider props to understand what it accepts
rg "SidebarProvider" "app/client/packages/design-system/widgets/src/components/Sidebar/"
Length of output: 2622
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/client/packages/design-system/widgets/src/components/Sidebar/stories/ControlledStateSidebar.tsx (1)
24-24: Consider making height configurableThe fixed height of "50vh" might limit component reusability. Consider making it a prop with a default value.
+ type ControlledStateSidebarProps = SidebarProps & { + children: React.ReactNode; + height?: string; + }; export const ControlledStateSidebar = (props: ControlledStateSidebarProps) => { - const { children, ...sidebarProps } = props; + const { children, height = "50vh", ...sidebarProps } = props; // ... style={{ - height: "50vh", + height, border: "1px solid var(--color-bd-elevation-1)", }}app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx (1)
131-139: Consider enhancing story documentation and props.While the implementation is correct, consider adding:
- Story args to document available props
- Story description explaining the controlled state behavior
export const WithControlledState: Story = { + args: { + // Document available props here + }, + parameters: { + docs: { + description: { + story: "Demonstrates sidebar with externally controlled state", + }, + }, + }, render: (args) => { return ( <ControlledStateSidebar {...args}>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/stories/ControlledStateSidebar.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx(2 hunks)
🔇 Additional comments (5)
app/client/packages/design-system/widgets/src/components/Sidebar/stories/ControlledStateSidebar.tsx (2)
1-9: LGTM! Imports are well-organized and properly typed.
14-34: LGTM! Clean implementation of controlled component pattern.
The component effectively implements the controlled state pattern with proper prop spreading and state management.
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx (1)
36-37: LGTM! State management logic improvement
The new state update sequence ensures consistent state management by updating local state first, followed by the optional external state handler.
Let's verify this fixes the bug mentioned in the PR title:
✅ Verification successful
State management change is correct and properly implemented
The code changes ensure proper state synchronization between internal state and controlled state pattern. The implementation correctly:
- Updates internal state first via
_setState - Propagates changes to parent via
setStatePropcallback - Maintains consistency in both controlled and uncontrolled modes
- Handles state updates in the correct order to prevent race conditions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test cases or usages of onStateChange
rg -l "onStateChange.*sidebar" --type ts --type tsx
# Look for any existing issues or PRs related to this bug
gh issue list --search "onStateChange sidebar"
gh pr list --search "onStateChange sidebar"
Length of output: 285
Script:
#!/bin/bash
# Let's try searching without the file type restriction
rg -l "onStateChange.*sidebar"
# Search for the SidebarProvider component usage
rg -l "SidebarProvider"
# Look for test files related to Sidebar
fd "Sidebar.*test" --type f
# Check if there are any existing tests for the component
ast-grep --pattern 'describe("Sidebar$$$'
Length of output: 800
Script:
#!/bin/bash
# Let's check the implementation details of SidebarProvider and its usage
cat app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx
# Check the controlled state example
cat app/client/packages/design-system/widgets/src/components/Sidebar/stories/ControlledStateSidebar.tsx
# Check the types file for onStateChange definition
cat app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts
Length of output: 4555
app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx (2)
4-6: LGTM! Clean import organization.
The consolidated imports improve code organization while maintaining functionality.
134-136: Verify test coverage for ControlledStateSidebar.
Ensure the new ControlledStateSidebar component has adequate test coverage for state management scenarios.
| type ControlledStateSidebarProps = SidebarProps & { | ||
| children: React.ReactNode; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing SidebarProps type import
The type SidebarProps is used but not explicitly imported. Consider adding it to the named imports.
import {
Sidebar,
SidebarTrigger,
SidebarProvider,
type SidebarState,
+ type SidebarProps,
} from "../src/index";Committable suggestion skipped: line range outside the PR's diff.
jsartisan
force-pushed
the
chore/fix-sidebar-onStateChange
branch
from
aaa2612 to
20a4c3d
Compare
/ok-to-test tags="@tag.Anvil" <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Introduced a new `ControlledStateSidebar` component for better management of sidebar state. - Added a new story, `WithControlledState`, to demonstrate the `ControlledStateSidebar` functionality. - **Bug Fixes** - Improved the state update flow in the `SidebarProvider` for more reliable state management. - **Documentation** - Enhanced organization of import statements in the sidebar stories for better readability. <!-- 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/12030926462> > Commit: 20a4c3d > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12030926462&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Anvil` > Spec: > <hr>Tue, 26 Nov 2024 13:11:30 UTC <!-- end of auto-generated comment: Cypress test results -->
3 participants
/ok-to-test tags="@tag.Anvil"
Summary by CodeRabbit
Summary by CodeRabbit
New Features
ControlledStateSidebarcomponent for better management of sidebar state.WithControlledState, to demonstrate theControlledStateSidebarfunctionality.Bug Fixes
SidebarProviderfor more reliable state management.Documentation
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12030926462
Commit: 20a4c3d
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Tue, 26 Nov 2024 13:11:30 UTC