Collapse long connector topic lists#1849
Open
rahulbsw wants to merge 6 commits into
Open
Conversation
kapybro
Bot
added
status/triage
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds collapse/expand behavior for connector topic lists in the Topics detail view and the TopicsCell table cell, including ToggleButtonWrapper styled-components, configurable collapsed thresholds, toggle state, conditional rendering, and tests for collapsed, threshold, expanded, and empty cases. ChangesTopic List Collapse/Expand Behavior
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/components/Connect/List/ConnectorsTable/connectorsColumns/cells/__tests__/TopicsCell.spec.tsx (1)
34-53: ⚡ Quick winConsider adding edge-case test coverage.
Similar to the Topics test, consider adding tests for:
- When
topics.length <= COLLAPSED_TOPICS_COUNT(button should not appear)- When
topicsis empty or undefinedThis would ensure the component handles all states gracefully.
🧪 Proposed additional test cases
it('does not show toggle button when topics count is below threshold', () => { const cellPropsSmall = { row: { original: { ...connectors[0], topics: ['topic-1', 'topic-2', 'topic-3'], }, }, } as CellContext<FullConnectorInfo, unknown>; render( <WithRoute path={clusterConnectorsPath()}> <TopicsCell {...cellPropsSmall} /> </WithRoute>, { initialEntries: [clusterConnectorsPath(clusterName)] } ); expect(screen.getByText('topic-1')).toBeInTheDocument(); expect(screen.queryByRole('button')).not.toBeInTheDocument(); }); it('handles undefined topics gracefully', () => { const cellPropsEmpty = { row: { original: { ...connectors[0], topics: undefined, }, }, } as CellContext<FullConnectorInfo, unknown>; render( <WithRoute path={clusterConnectorsPath()}> <TopicsCell {...cellPropsEmpty} /> </WithRoute>, { initialEntries: [clusterConnectorsPath(clusterName)] } ); expect(screen.queryByRole('button')).not.toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/Connect/List/ConnectorsTable/connectorsColumns/cells/__tests__/TopicsCell.spec.tsx` around lines 34 - 53, Add edge-case tests for TopicsCell: when topics.length <= COLLAPSED_TOPICS_COUNT and when topics is undefined/empty. In the TopicsCell.spec.tsx add two tests using the existing render helper (or renderComponent) that render TopicsCell with row.original.topics set to a short array (<= COLLAPSED_TOPICS_COUNT) and to undefined/[] and assert that topic items render but the toggle button (role='button' or label 'Show X more') does not exist; reference TopicsCell and COLLAPSED_TOPICS_COUNT to construct the test inputs and assertions.frontend/src/components/Connect/Details/Topics/__tests__/Topics.spec.tsx (1)
46-67: ⚡ Quick winConsider adding edge-case test coverage.
The current test validates the happy path well, but consider adding tests for:
- When
topics.length <= COLLAPSED_TOPICS_COUNT(button should not appear)- When
topicsis empty or undefinedThese would improve confidence that the component handles all states correctly.
🧪 Proposed additional test cases
it('does not show toggle button when topics count is below threshold', () => { (useConnector as jest.Mock).mockImplementation(() => ({ data: { ...connector, topics: Array.from({ length: 5 }, (_, i) => `topic-${i + 1}`), }, })); render( <WithRoute path={clusterConnectConnectorTopicsPath()}> <Topics /> </WithRoute>, { initialEntries: [ clusterConnectConnectorTopicsPath( clusterName, connectName, connectorName ), ], } ); expect(screen.queryByRole('button')).not.toBeInTheDocument(); }); it('handles empty topics gracefully', () => { (useConnector as jest.Mock).mockImplementation(() => ({ data: { ...connector, topics: [], }, })); render( <WithRoute path={clusterConnectConnectorTopicsPath()}> <Topics /> </WithRoute>, { initialEntries: [ clusterConnectConnectorTopicsPath( clusterName, connectName, connectorName ), ], } ); expect(screen.getByText('No topics found')).toBeInTheDocument(); expect(screen.queryByRole('button')).not.toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/Connect/Details/Topics/__tests__/Topics.spec.tsx` around lines 46 - 67, Add two edge-case tests for the Topics component: (1) verify that when topics.length <= COLLAPSED_TOPICS_COUNT the toggle button is not rendered by mocking useConnector to return a small topics array and asserting no button appears (reference: Topics component and COLLAPSED_TOPICS_COUNT, test file Topics.spec.tsx), and (2) verify that when topics is empty or undefined the component renders the empty-state text (e.g., "No topics found") and does not render the toggle button by mocking useConnector to return topics: [] or undefined and asserting the empty-state message and absence of the button (reference: useConnector mock and Topics component).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/components/Connect/Details/Topics/__tests__/Topics.spec.tsx`:
- Around line 46-67: Add two edge-case tests for the Topics component: (1)
verify that when topics.length <= COLLAPSED_TOPICS_COUNT the toggle button is
not rendered by mocking useConnector to return a small topics array and
asserting no button appears (reference: Topics component and
COLLAPSED_TOPICS_COUNT, test file Topics.spec.tsx), and (2) verify that when
topics is empty or undefined the component renders the empty-state text (e.g.,
"No topics found") and does not render the toggle button by mocking useConnector
to return topics: [] or undefined and asserting the empty-state message and
absence of the button (reference: useConnector mock and Topics component).
In
`@frontend/src/components/Connect/List/ConnectorsTable/connectorsColumns/cells/__tests__/TopicsCell.spec.tsx`:
- Around line 34-53: Add edge-case tests for TopicsCell: when topics.length <=
COLLAPSED_TOPICS_COUNT and when topics is undefined/empty. In the
TopicsCell.spec.tsx add two tests using the existing render helper (or
renderComponent) that render TopicsCell with row.original.topics set to a short
array (<= COLLAPSED_TOPICS_COUNT) and to undefined/[] and assert that topic
items render but the toggle button (role='button' or label 'Show X more') does
not exist; reference TopicsCell and COLLAPSED_TOPICS_COUNT to construct the test
inputs and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6bd91a6-7a13-4a54-a646-c186b1539ac6
📒 Files selected for processing (6)
frontend/src/components/Connect/Details/Topics/Topics.styled.tsfrontend/src/components/Connect/Details/Topics/Topics.tsxfrontend/src/components/Connect/Details/Topics/__tests__/Topics.spec.tsxfrontend/src/components/Connect/List/ConnectorsTable/connectorsColumns/cells/TopicsCell.styled.tsfrontend/src/components/Connect/List/ConnectorsTable/connectorsColumns/cells/TopicsCell.tsxfrontend/src/components/Connect/List/ConnectorsTable/connectorsColumns/cells/__tests__/TopicsCell.spec.tsx
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #1306
Summary
Testing
Summary by CodeRabbit
New Features
Tests