fix(playground): hide session-row checkbox until hover or selection#13314
Open
keval718 wants to merge 1 commit into
Open
fix(playground): hide session-row checkbox until hover or selection#13314keval718 wants to merge 1 commit into
keval718 wants to merge 1 commit into
Conversation
The selectable-row checkbox was always rendered at full opacity, adding
visual noise to every session in the sidebar. Hide it by default and
reveal it only when the row is hovered, while keeping checked boxes
permanently visible so users can still see what they have selected
without re-hovering each row.
Implementation:
- Apply ``invisible group-hover:visible`` to the icon when the row is
not selected; the row already carries the ``group`` class, so the
utility resolves without further plumbing.
- Skip those classes when ``isSelected`` so a checked box stays
visible regardless of pointer location.
- Keep the wrapping ``w-4 h-4`` column so the row layout does not
jump on hover/unhover. ``visibility: hidden`` also disables pointer
events so a stray click on the hidden column cannot toggle
selection.
- Add ``transition-opacity`` for a smooth reveal.
The Select All checkbox at the top of the multi-select region remains
always visible — it is the entry-point affordance for multi-select.
Adds ``__tests__/session-selector-checkbox-visibility.test.tsx`` (5
cases): hidden default + hover utility wiring, always-visible-on-
selected, layout-jump guard via reserved column, click toggles
selection without bubbling to row toggleVisibility, and showCheckbox
false renders nothing.
Contributor
WalkthroughThis PR introduces a hover-reveal checkbox visibility feature for the ChangesCheckbox Visibility Behavior
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 6 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (6 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. ❌ Your project check has failed because the head coverage (51.71%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release-1.10.0 #13314 +/- ##
==================================================
+ Coverage 55.42% 55.52% +0.09%
==================================================
Files 2179 2179
Lines 205846 205850 +4
Branches 31073 29401 -1672
==================================================
+ Hits 114091 114297 +206
+ Misses 90427 90224 -203
- 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
🤖 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/__tests__/session-selector-checkbox-visibility.test.tsx`:
- Around line 1-37: This test uses Jest/@testing-library instead of the
project's Playwright-based frontend tests and also mocks internal UI components
(SessionMoreMenu, SessionRename) which violates guidelines; either (A) rewrite
the test as a Playwright test that mounts the SessionSelector component and
asserts checkbox visibility via real DOM interactions, removing Jest-specific
helpers (fireEvent/render/screen) and jest.mock usages, or (B) if you intend to
keep a unit test with Jest, stop mocking internal components by removing the
mocks for SessionMoreMenu and SessionRename and render the real implementations
while keeping mocks only for external dependencies like useUpdateSessionName,
useVoiceStore and useSessionHasMessages; update the test filename/naming to
match the approved pattern and ensure all mocks reference the same symbol names
(SessionSelector, SessionMoreMenu, SessionRename, useUpdateSessionName,
useVoiceStore, useSessionHasMessages) so the correct modules are exercised.
🪄 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: 40da7dce-ac0c-4069-bdab-7b20abc62c18
📒 Files selected for processing (2)
src/frontend/src/components/core/playgroundComponent/chat-view/chat-header/components/__tests__/session-selector-checkbox-visibility.test.tsxsrc/frontend/src/components/core/playgroundComponent/chat-view/chat-header/components/session-selector.tsx
Comment on lines
+1
to
+37
| import { fireEvent, render, screen } from "@testing-library/react"; | ||
| import { SessionSelector } from "../session-selector"; | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Minimal mocks — keep the test focused on the checkbox visibility logic. | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| // Override the global no-op mock so we can read the className passed to | ||
| // the icon and assert the hover/visibility utility classes are wired. | ||
| jest.mock("@/components/common/genericIconComponent", () => ({ | ||
| __esModule: true, | ||
| default: ({ name, className }: { name: string; className?: string }) => ( | ||
| <span data-testid={`icon-${name}`} className={className} /> | ||
| ), | ||
| })); | ||
|
|
||
| jest.mock("@/controllers/API/queries/messages/use-rename-session", () => ({ | ||
| useUpdateSessionName: () => ({ mutate: jest.fn() }), | ||
| })); | ||
|
|
||
| type VoiceState = { setNewSessionCloseVoiceAssistant: jest.Mock }; | ||
| jest.mock("@/stores/voiceStore", () => ({ | ||
| useVoiceStore: <TResult,>(selector: (state: VoiceState) => TResult) => | ||
| selector({ setNewSessionCloseVoiceAssistant: jest.fn() }), | ||
| })); | ||
|
|
||
| jest.mock("../../hooks/use-session-has-messages", () => ({ | ||
| useSessionHasMessages: () => true, | ||
| })); | ||
|
|
||
| jest.mock("../session-more-menu", () => ({ | ||
| SessionMoreMenu: () => <div data-testid="session-more-menu" />, | ||
| })); | ||
|
|
||
| jest.mock("../session-rename", () => ({ | ||
| SessionRename: () => <div data-testid="session-rename" />, | ||
| })); |
Contributor
There was a problem hiding this comment.
Test framework does not match coding guidelines.
The coding guidelines require frontend test files to use Playwright, but this test uses Jest with @testing-library/react. Additionally, the test mocks internal UI components (SessionMoreMenu and SessionRename at lines 31-37) rather than rendering them, which may indicate poor test design.
Consider either:
- Rewriting this test using Playwright to comply with the guidelines, or
- If Jest/Testing Library is acceptable for unit tests in this codebase, removing the mocks for internal UI components (lines 31-37) and rendering the real components instead. Mocks should be reserved for external dependencies like API hooks and stores.
As per coding guidelines, frontend test files should follow the naming convention *.test.ts or *.test.tsx using Playwright. Additionally, ensure mocks are used appropriately for external dependencies only, not for core logic or internal UI components.
🤖 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__/session-selector-checkbox-visibility.test.tsx`
around lines 1 - 37, This test uses Jest/@testing-library instead of the
project's Playwright-based frontend tests and also mocks internal UI components
(SessionMoreMenu, SessionRename) which violates guidelines; either (A) rewrite
the test as a Playwright test that mounts the SessionSelector component and
asserts checkbox visibility via real DOM interactions, removing Jest-specific
helpers (fireEvent/render/screen) and jest.mock usages, or (B) if you intend to
keep a unit test with Jest, stop mocking internal components by removing the
mocks for SessionMoreMenu and SessionRename and render the real implementations
while keeping mocks only for external dependencies like useUpdateSessionName,
useVoiceStore and useSessionHasMessages; update the test filename/naming to
match the approved pattern and ensure all mocks reference the same symbol names
(SessionSelector, SessionMoreMenu, SessionRename, useUpdateSessionName,
useVoiceStore, useSessionHasMessages) so the correct modules are exercised.
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 selectable-row checkbox in the playground sessions sidebar was rendered at full opacity on every row, adding persistent visual noise to a UI that mostly shows session names. This PR hides the checkbox by default and reveals it only when the row is hovered, while keeping already-checked boxes permanently visible so the user can still see their selection without re-hovering each row.
Visibility rules
invisible) — column space reserved, no layout jumpgroup-hover:visible)text-status-red) — noinvisible, nogroup-hover:gatingThe Select All checkbox at the top of the multi-select region stays always visible because it is the entry-point affordance for multi-select — without it, the user has no way to discover the feature.
Implementation
session-selector.tsxcheckbox icon className:groupclass, sogroup-hover:visibleresolves without any extra plumbing.invisible(CSSvisibility: hidden) reserves the 16×16 column and disables pointer events, so a stray click on the hidden column cannot accidentally toggle selection.transition-opacitysmooths the reveal.w-4 h-4 flex-shrink-0column is unchanged, so the row layout never shifts on hover/unhover or on selection state change.Trade-offs / alternatives considered
selectedSessions.size > 0prop / mode that the parent has to thread down. Skipped for now — current behaviour matches the user's exact ask; revisit if QA reports multi-select friction.opacity-0+pointer-events-noneinstead ofinvisible— semantically equivalent for this case but more verbose and produces no transition unlessopacityis on atransition-propertylist.invisibleis the single-utility shadcn pattern used bySessionMoreMenu(invisible group-hover:visible), so keeping the same idiom across both row-affordances is the better consistency choice.Tests
New
__tests__/session-selector-checkbox-visibility.test.tsx(5 cases):invisible+group-hover:visible; row carries thegroupclass so the utility resolves.invisiblenorgroup-hover:visible;text-status-redpresent (always-visible regression guard).w-4 h-4(layout-jump regression guard).toggleVisibility(event propagation guard).showCheckbox={false}→ no checkbox column rendered at all.Locally overrode the global
genericIconComponentmock so the icon renders a<span>with className, letting tests assert the utility classes are wired correctly (the global jest setup mocks it tonullfor the rest of the suite).playgroundComponentjest suite: 141 / 141 ✅@biomejs/biome checkclean on both changed files (0 errors; 3 pre-existing a11y warnings on the edit-mode wrapper and other rows are untouched).make format_frontendclean.Test plan
cd src/frontend && npm start, open playground for any flow.⋮MoreMenu.Summary by CodeRabbit
Bug Fixes
Tests