fix(playground): align Select All bulk-delete icon with session ⋮ column#13313
Open
keval718 wants to merge 1 commit into
Open
fix(playground): align Select All bulk-delete icon with session ⋮ column#13313keval718 wants to merge 1 commit into
keval718 wants to merge 1 commit into
Conversation
The bulk-delete trash icon in the Select All row sat off-axis from the
``⋮`` MoreMenu icons of the session rows below: lower (the wrapper had
``py-1 mb-1`` while session rows are ``h-8`` with no vertical padding)
and further left (the trash button was ``p-0`` inside an unpadded
column while the SessionMoreMenu trigger is ``h-8 w-8 p-2``).
Mirror the SessionSelector row shape exactly:
- Outer wrapper: ``flex h-8 items-center justify-between`` — no extra
vertical padding, no bottom margin.
- Trash button: ``size="icon" h-8 w-8 p-2 rounded`` — matches the
SessionMoreMenu trigger inner padding so the icon centers in the
same 32×32 column as ``⋮`` below.
- Inner padding moved to the Select All checkbox cluster (``px-2``)
so the left edge still lines up with the SessionSelector internal
padding.
Drive-bys (touched lines biome would flag otherwise):
- Remove unused ``isDefaultSession`` local in the sessions map.
- Sort the alertStore/flowStore imports.
- Convert the Select All click target from ``<div onClick>`` to a
real ``<button type="button" aria-pressed>`` so keyboard users can
toggle it and the a11y lint warning no longer fires.
Adds ``__tests__/chat-sidebar.test.tsx`` covering: button hidden when
nothing selected, ``h-8 w-8 p-2`` button shape, host row uses ``h-8``
with no ``py-*`` / ``mb-*`` padding, and bulk delete fires with the
expected (default-session-excluded) selection.
Contributor
WalkthroughThis PR refactors the ChatSidebar "Select All" and bulk-delete controls to improve visual alignment with session rows. The "Select All" row now uses a dedicated button element with ChangesSelect All and Bulk-Delete UI Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-1.10.0 #13313 +/- ##
==================================================
+ Coverage 55.42% 55.49% +0.07%
==================================================
Files 2179 2179
Lines 205846 205848 +2
Branches 31073 29397 -1676
==================================================
+ Hits 114091 114243 +152
+ Misses 90427 90276 -151
- Partials 1328 1329 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/frontend/src/components/core/playgroundComponent/chat-view/chat-header/components/__tests__/chat-sidebar.test.tsx (1)
25-41: ⚡ Quick winAvoid mocking
SessionSelectorfor this alignment-focused test suite.These tests validate alignment against
SessionSelectorrow behavior, but the local mock replaces that contract with a simplified button. Keep mocks for external dependencies/stores, and render the realSessionSelector(or a higher-fidelity test double) so alignment and interaction assertions track real behavior.As per coding guidelines, "Ensure mocks are used appropriately for external dependencies only, not for core logic".
🤖 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 `@src/frontend/src/components/core/playgroundComponent/chat-view/chat-header/components/__tests__/chat-sidebar.test.tsx` around lines 25 - 41, Remove the local jest.mock call that replaces SessionSelector (the mocked factory returning a simple button) and instead render the real SessionSelector (or a higher-fidelity test double) in chat-sidebar.test.tsx so alignment and interaction assertions exercise the real component behavior; keep mocking only external dependencies/stores, update any test setup to import the actual SessionSelector component and adjust test harness data (e.g., the session prop and data-testid session-row-<session>) so the tests assert against the real DOM structure emitted by SessionSelector rather than the simplified mock.
🤖 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.
Inline comments:
In
`@src/frontend/src/components/core/playgroundComponent/chat-view/chat-header/components/chat-sidebar.tsx`:
- Line 159: The Select All button in chat-sidebar.tsx uses conflicting Tailwind
padding utilities ("px-2" and "p-0") so p-0 cancels the horizontal padding;
update the button's className on the Select All element (the element with
className="flex items-center gap-2 cursor-pointer px-2 bg-transparent border-0
p-0") to remove the conflicting p-0 and instead use explicit vertical padding if
needed (e.g., replace p-0 with py-0 or simply remove p-0) so px-2 remains
effective and row alignment is preserved.
---
Nitpick comments:
In
`@src/frontend/src/components/core/playgroundComponent/chat-view/chat-header/components/__tests__/chat-sidebar.test.tsx`:
- Around line 25-41: Remove the local jest.mock call that replaces
SessionSelector (the mocked factory returning a simple button) and instead
render the real SessionSelector (or a higher-fidelity test double) in
chat-sidebar.test.tsx so alignment and interaction assertions exercise the real
component behavior; keep mocking only external dependencies/stores, update any
test setup to import the actual SessionSelector component and adjust test
harness data (e.g., the session prop and data-testid session-row-<session>) so
the tests assert against the real DOM structure emitted by SessionSelector
rather than the simplified mock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d408783b-ede4-4317-9826-acd3325ca52f
📒 Files selected for processing (2)
src/frontend/src/components/core/playgroundComponent/chat-view/chat-header/components/__tests__/chat-sidebar.test.tsxsrc/frontend/src/components/core/playgroundComponent/chat-view/chat-header/components/chat-sidebar.tsx
| <div className="flex h-8 items-center justify-between"> | ||
| <button | ||
| type="button" | ||
| className="flex items-center gap-2 cursor-pointer px-2 bg-transparent border-0 p-0" |
Contributor
There was a problem hiding this comment.
Fix conflicting padding utilities on the Select All button.
At Line 159, p-0 overrides px-2, so the left padding intended to preserve row alignment is effectively removed.
Suggested fix
- className="flex items-center gap-2 cursor-pointer px-2 bg-transparent border-0 p-0"
+ className="flex items-center gap-2 cursor-pointer px-2 bg-transparent border-0 py-0"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| className="flex items-center gap-2 cursor-pointer px-2 bg-transparent border-0 p-0" | |
| className="flex items-center gap-2 cursor-pointer px-2 bg-transparent border-0 py-0" |
🤖 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
`@src/frontend/src/components/core/playgroundComponent/chat-view/chat-header/components/chat-sidebar.tsx`
at line 159, The Select All button in chat-sidebar.tsx uses conflicting Tailwind
padding utilities ("px-2" and "p-0") so p-0 cancels the horizontal padding;
update the button's className on the Select All element (the element with
className="flex items-center gap-2 cursor-pointer px-2 bg-transparent border-0
p-0") to remove the conflicting p-0 and instead use explicit vertical padding if
needed (e.g., replace p-0 with py-0 or simply remove p-0) so px-2 remains
effective and row alignment is preserved.
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
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.
Summary
The bulk-delete trash icon in the Select All row of the playground sessions sidebar did not line up with the
⋮MoreMenu icons of the session rows below — both vertically (different row heights) and horizontally (different inner-padding shape). This PR aligns the Select All row to the exact same shape and dimensions as aSessionSelectorrow so the icons sit in the same column on the same baseline.Root cause
Old Select All wrapper:
Compared with each
SessionSelectorrow (h-8 flex items-centerwith aSessionMoreMenutrigger ofh-8 w-8 p-2):py-1 mb-1made the Select All row taller and pushed the trash icon's center down relative to the⋮centers in the rows beneath it.p-0and sat in an unpadded host column, so its right edge landed 8 px left of where theSessionMoreMenutrigger's right edge sits (the trigger has its ownp-2).Fix
Mirror the
SessionSelectorrow shape exactly:flex h-8 items-center justify-between— no extra vertical padding, no bottom margin. Row height now matches a session row.<Button size="icon" className="h-8 w-8 p-2 rounded …">directly, matching theSessionMoreMenutrigger's inner padding. The icon centers in the same 32×32 column as⋮.px-2from the outer wrapper onto the Select All checkbox cluster so the left edge still aligns with the SessionSelector's internal left padding.Drive-bys (lines biome would re-flag because they were touched)
isDefaultSessionlocal in the sessions map.useAlertStore/useFlowStoreimports per biomeorganizeImports.<div onClick>to a real<button type="button" aria-pressed>so keyboard users can toggle it and biome's a11y warning no longer fires.Trade-offs / alternatives considered
SessionMoreMenushape keeps the codebase consistent.SessionMoreMenu+ bulk-delete into a sharedRowActionButtonprimitive — the right long-term call once a third call site appears. With only two today it would be premature; covered as a future follow-up.Tests
New
__tests__/chat-sidebar.test.tsx(4 cases):h-8 w-8 p-2shape (regression guard against drift back top-0).h-8with nopy-*/mb-*padding (regression guard against re-introducingpy-1 mb-1).onBulkDeleteSessionswith the checked sessions and excludes the defaultcurrentFlowIdsession.useGetFlowId,useFlowStore,useAlertStore, andSessionSelectorare mocked at the module boundary so the test exercises only the Select All row composition.playgroundComponentjest suite: 140 / 140 ✅@biomejs/biome checkclean on both changed files (0 errors, 0 warnings).make format_frontendclean.Test plan
cd src/frontend && npm start, open playground for any flow.⋮icons in the session rows below; row height matches.Enter/Space→ toggles selection (aria-pressedflips). Previously the<div>was not focusable.Summary by CodeRabbit
Release Notes
Bug Fixes
Tests