-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: Auto-expand filter list when clicking on class in data browser #3101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Auto-expand filter list when clicking on class in data browser #3101
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
📝 WalkthroughWalkthroughSafe-calls to Changes
Sequence Diagram(s)(omitted — change is a localized UI/state update without multi-component sequential flow requiring a diagram) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/components/CategoryList/CategoryList.react.js:
- Around line 178-184: The bug is that the onClick handler for the class link
appends an id to this.state.openClasses while _autoExpandForSelectedFilter
replaces the array (setState({ openClasses: [id] })), causing manual expansions
to be lost; fix _autoExpandForSelectedFilter so it mirrors the onClick behavior
by appending the id if it isn’t already present (e.g., compute newOpen =
this.state.openClasses.includes(id) ? this.state.openClasses :
[...this.state.openClasses, id] and call setState({ openClasses: newOpen })),
ensuring duplicates are avoided and existing open classes are preserved.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/CategoryList/CategoryList.react.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Node 18
- GitHub Check: Docker linux/amd64
🔇 Additional comments (1)
src/components/CategoryList/CategoryList.react.js (1)
178-184: Auto-expand logic implementation looks correct.The implementation properly:
- Checks for filter existence using safe null coalescing:
(c.filters || []).length > 0- Guards against duplicate expansion:
!this.state.openClasses.includes(id)- Preserves existing expanded classes when appending to the array
The guard condition at line 181 prevents redundant state updates when
componentDidUpdatelater calls_autoExpandForSelectedFilter, since it also checks!this.state.openClasses.includes(id)(line 90).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/components/CategoryList/CategoryList.react.js:
- Around line 178-184: The click handler calls this.props.classClicked()
unconditionally and uses a non-functional setState; guard the call by checking
if typeof this.props.classClicked === 'function' before invoking it, replace the
array-spread setState with the functional form this.setState(prev => ({
openClasses: prev.openClasses.includes(id) ? prev.openClasses :
[...prev.openClasses, id] })) to avoid stale state, and add classClicked:
PropTypes.func to the component PropTypes with a short description to document
the callback.
🧹 Nitpick comments (1)
src/components/CategoryList/CategoryList.react.js (1)
90-91: Use functionalsetStatewhen new state depends on previous state.Reading
this.state.openClassesdirectly insidesetStatecan lead to stale state issues if React batches updates. Use the functional form instead.🔎 Suggested fix
if (matchIndex !== -1 && !this.state.openClasses.includes(id)) { - this.setState({ openClasses: [...this.state.openClasses, id] }); + this.setState(prevState => ({ openClasses: [...prevState.openClasses, id] })); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/CategoryList/CategoryList.react.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker linux/amd64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/CategoryList/CategoryList.react.js (1)
178-190: Previous concerns properly addressed!The implementation correctly:
- Guards the
classClickedcallback with a typeof check to prevent runtime errors- Uses functional setState to avoid stale state issues
- Auto-expands only when the class has filters and isn't already expanded
The logic is sound and handles edge cases well.
Optional: Extract handler for readability
Consider extracting the onClick logic into a named method to improve maintainability:
handleClassClick(c, id, link) { if (typeof this.props.classClicked === 'function') { this.props.classClicked(); } // Auto-expand filter list when clicking on a class that has filters if ((c.filters || []).length > 0) { this.setState(prevState => ({ openClasses: prevState.openClasses.includes(id) ? prevState.openClasses : [...prevState.openClasses, id] })); } }Then use:
onClick={() => this.handleClassClick(c, id, link)}This is purely a readability enhancement and not required.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/CategoryList/CategoryList.react.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 2979
File: src/dashboard/Data/Views/Views.react.js:905-912
Timestamp: 2025-09-11T00:15:33.497Z
Learning: In the parse-dashboard codebase, class names are used directly in path segments without URL encoding (e.g., `browser/${className}`) for consistency across methods like handlePointerClick, handlePointerCmdClick, and handleOpenAllPointers.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (2)
src/components/CategoryList/CategoryList.react.js (2)
91-91: LGTM! Functional setState correctly handles state updates.The use of functional setState with
prevStateproperly avoids stale state issues when auto-expanding the filter list, and the guard condition on line 90 prevents duplicate entries.
264-264: PropTypes declaration correctly added.The
classClickedprop is now properly documented with an appropriate type and clear description, addressing the previous review feedback.
# [8.2.0-alpha.17](8.2.0-alpha.16...8.2.0-alpha.17) (2026-01-06) ### Features * Auto-expand filter list when clicking on class in data browser ([#3101](#3101)) ([30a733c](30a733c))
|
🎉 This change has been released in version 8.2.0-alpha.17 |
New Pull Request Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.