-
Notifications
You must be signed in to change notification settings - Fork 17
fix(TopicData): reset filters on partition change #2567
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
Conversation
|
bugbot run |
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.
Pull Request Overview
This PR refactors the partition-change flow so that TopicDataControls delegates filter resets to its parent, ensuring filters are cleared whenever the user selects a new partition.
- Introduce a new
handlePartitionChangeprop onTopicDataControlsand remove its internal reset logic. - Implement
handlePartitionChangeinTopicDatato update the selected partition and callresetFilters(). - Wire the new handler through the component tree.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/containers/Tenant/Diagnostics/TopicData/TopicDataControls/TopicDataControls.tsx | Added handlePartitionChange prop, removed internal partition change logic, updated Select.onUpdate |
| src/containers/Tenant/Diagnostics/TopicData/TopicData.tsx | Defined handlePartitionChange callback calling handleSelectedPartitionChange and resetFilters |
Comments suppressed due to low confidence (3)
src/containers/Tenant/Diagnostics/TopicData/TopicDataControls/TopicDataControls.tsx:44
- The
handlePartitionChangeprop is marked optional, but the partition select always calls it. Consider making this prop required (remove the?) or providing a no-op default to avoid potential runtime errors when it's not passed.
handlePartitionChange?: (value: string[]) => void;
src/containers/Tenant/Diagnostics/TopicData/TopicData.tsx:202
- Add unit tests to verify that selecting a new partition triggers
resetFilters()and clears offsets/timestamps as expected.
const handlePartitionChange = React.useCallback(
src/containers/Tenant/Diagnostics/TopicData/TopicDataControls/TopicDataControls.tsx:74
- [nitpick] For consistency with other handlers like
handleSelectedColumnsUpdate, consider renaminghandlePartitionChangetohandleSelectedPartitionChangeto make the naming pattern clearer.
onUpdate={handlePartitionChange}
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.
Bug: Missing Dependency Causes Stale Closures
The renderControls useCallback is missing handlePartitionChange from its dependency array. handlePartitionChange is passed as a prop to TopicDataControls within renderControls, violating React's exhaustive deps rule. This can lead to stale closures.
src/containers/Tenant/Diagnostics/TopicData/TopicData.tsx#L261-L273
ydb-embedded-ui/src/containers/Tenant/Diagnostics/TopicData/TopicData.tsx
Lines 261 to 273 in 327e9f8
| ); | |
| }, [ | |
| columnsToSelect, | |
| controlsKey, | |
| endOffset, | |
| partitions, | |
| partitionsError, | |
| partitionsLoading, | |
| scrollToOffset, | |
| setColumns, | |
| startOffset, | |
| truncated, | |
| ]); |
BugBot free trial expires on July 22, 2025
Learn more in the Cursor dashboard.
Was this report helpful? Give feedback by reacting with 👍 or 👎
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: ✅
Current: 85.12 MB | Main: 85.12 MB
Diff: 0.28 KB (-0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information