Repository navigation
feat: Improve sidebar dropdown states - #804
Conversation
📝 WalkthroughWalkthroughThis PR moves the assistant toggle into the collapsible CLI sidebar, updates sidebar and command-item styling, adds keyboard-navigation state to artifact/data popovers, exposes split-panel handle attributes, and refactors ChangesCLI Sidebar Controls and Styling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. 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 |
ilyabo
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d666571be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .cli-sidebar-command-item[data-selected='true'] { | ||
| background: transparent !important; | ||
| color: hsl(var(--popover-foreground)) !important; | ||
| } |
There was a problem hiding this comment.
In the collapsed Data/Workspace popovers, cmdk uses data-selected='true' to mark the item reached with ArrowUp/ArrowDown. This override makes every selected command item transparent unless it is hovered or happens to be the active artifact, so keyboard users can move the selection and press Enter on a row with no visible focus/selection indicator; the Data popover has no data-active fallback. Keep a distinct selected style for user-driven keyboard selection instead of clearing the state globally.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/sqlrooms-cli-ui/src/workspace/sidebar/CliAssistantToggleButton.tsx (1)
39-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
isActiveprop instead of passingdata-activedirectly.
SidebarMenuButtonexposes anisActiveprop that internally setsdata-active={isActive}. Passingdata-activedirectly works only because{...props}is spread after the explicit attribute inSidebarMenuButton, overriding it. This is fragile and bypasses the component's intended API.♻️ Proposed fix
<SidebarMenuButton type="button" size={state === 'expanded' ? 'default' : 'lg'} className="text-muted-foreground hover:text-foreground data-[active=true]:border-sidebar-border data-[active=true]:bg-sidebar-accent/50 data-[active=true]:text-primary data-[active=true]:hover:bg-sidebar-accent group-data-[state=expanded]:h-10 group-data-[state=expanded]:justify-center group-data-[state=expanded]:gap-2 group-data-[state=expanded]:border" onClick={() => toggleCollapsed('assistant-sidebar')} - data-active={!isAssistantCollapsed} + isActive={!isAssistantCollapsed} aria-label="Toggle AI Agent" aria-pressed={!isAssistantCollapsed} >🤖 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 `@apps/sqlrooms-cli-ui/src/workspace/sidebar/CliAssistantToggleButton.tsx` around lines 39 - 50, The CliAssistantToggleButton sidebar action is setting active state by passing data-active directly to SidebarMenuButton, which bypasses the component API. Update the SidebarMenuButton usage in CliAssistantToggleButton to use its isActive prop instead of data-active, keeping aria-pressed in sync with isAssistantCollapsed and preserving the existing toggle behavior.packages/ui/src/components/sidebar.tsx (1)
512-521: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
{...props}before explicit data attributes to prevent accidental overrides.Currently
{...props}is spread afterdata-active={isActive}, so any consumer passingdata-activein props silently overrides theisActiveprop. Moving the spread before the explicit attributes ensures internal attributes always take precedence.♻️ Proposed fix
const button = ( <Comp + {...props} ref={ref} data-slot="sidebar-menu-button" data-sidebar="menu-button" data-size={size} data-active={isActive} - className={cn(sidebarMenuButtonVariants({variant, size}), className)} - {...props} + className={cn(sidebarMenuButtonVariants({variant, size}), className)} /> );🤖 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 `@packages/ui/src/components/sidebar.tsx` around lines 512 - 521, In the sidebar menu button render path, the props spread is overriding internal data attributes because `{...props}` comes after the explicit `data-*` fields. Update the `button` JSX in `sidebar.tsx` so the spread occurs before `data-slot`, `data-sidebar`, `data-size`, and especially `data-active` inside the `Comp` element, ensuring the component’s own `isActive`-derived state always wins.apps/sqlrooms-cli-ui/src/workspace/sidebar/CliArtifactsSidebarSection.tsx (1)
44-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
CLI_SIDEBAR_COMMAND_ITEM_CLASSconstant to avoid duplication.The same
CLI_SIDEBAR_COMMAND_ITEM_CLASS = 'cli-sidebar-command-item'constant is defined independently in bothCliArtifactsSidebarSection.tsx(line 44) andCliDataSidebarSection.tsx(line 57). If the class name changes, it must be updated in both places. Consider extracting it to a shared module (e.g., a constants file in the sidebar directory) and importing from both sections.Also applies to: 186-187, 210-218
🤖 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 `@apps/sqlrooms-cli-ui/src/workspace/sidebar/CliArtifactsSidebarSection.tsx` around lines 44 - 45, The CLI sidebar item class name is duplicated across CliArtifactsSidebarSection and CliDataSidebarSection, so extract CLI_SIDEBAR_COMMAND_ITEM_CLASS into a shared sidebar constants module and import it in both sections. Update any references in the sidebar components to use the shared constant instead of redefining the same string locally.
🤖 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 `@apps/sqlrooms-cli-ui/src/workspace/sidebar/CliArtifactsSidebarSection.tsx`:
- Around line 44-45: The CLI sidebar item class name is duplicated across
CliArtifactsSidebarSection and CliDataSidebarSection, so extract
CLI_SIDEBAR_COMMAND_ITEM_CLASS into a shared sidebar constants module and import
it in both sections. Update any references in the sidebar components to use the
shared constant instead of redefining the same string locally.
In `@apps/sqlrooms-cli-ui/src/workspace/sidebar/CliAssistantToggleButton.tsx`:
- Around line 39-50: The CliAssistantToggleButton sidebar action is setting
active state by passing data-active directly to SidebarMenuButton, which
bypasses the component API. Update the SidebarMenuButton usage in
CliAssistantToggleButton to use its isActive prop instead of data-active,
keeping aria-pressed in sync with isAssistantCollapsed and preserving the
existing toggle behavior.
In `@packages/ui/src/components/sidebar.tsx`:
- Around line 512-521: In the sidebar menu button render path, the props spread
is overriding internal data attributes because `{...props}` comes after the
explicit `data-*` fields. Update the `button` JSX in `sidebar.tsx` so the spread
occurs before `data-slot`, `data-sidebar`, `data-size`, and especially
`data-active` inside the `Comp` element, ensuring the component’s own
`isActive`-derived state always wins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94e01513-ebc0-4e23-82c4-206c6ebcb6b2
📒 Files selected for processing (8)
apps/sqlrooms-cli-ui/src/style.cssapps/sqlrooms-cli-ui/src/workspace/CliWorkspaceTopbar.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliArtifactsSidebarSection.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliAssistantToggleButton.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliDataSidebarSection.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliWorkspaceSidebar.tsxpackages/layout/src/node-renderers/split-node-renderer/SplitLayoutPanel.tsxpackages/ui/src/components/sidebar.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/sidebar.tsx (1)
487-495: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the new public sidebar API.
SidebarMenuButtonPropsand the ref-forwardingSidebarMenuButtonAPI are exported from@sqlrooms/ui; add TSDoc describingasChild,ref, and tooltip behavior.As per coding guidelines: “
packages/**/*.{ts,tsx}: Add tsdoc to exported types and functions in@sqlrooms/*packages.”🤖 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 `@packages/ui/src/components/sidebar.tsx` around lines 487 - 495, Document the exported SidebarMenuButtonProps type and SidebarMenuButton component with TSDoc. Describe the asChild option, ref forwarding behavior, and tooltip behavior, including the accepted tooltip configuration, while preserving the existing API and implementation.Source: Coding guidelines
🧹 Nitpick comments (1)
packages/ui/src/components/sidebar.tsx (1)
493-520: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for ref forwarding.
Please test that refs resolve to the rendered button and continue to work through the
asChildpath used by Radix triggers. This change modifies a public component contract.As per coding guidelines: “
**/*.{js,ts,jsx,tsx}: Add or update tests when changing behavior or public API, when practical.”🤖 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 `@packages/ui/src/components/sidebar.tsx` around lines 493 - 520, Add regression tests for the public SidebarMenuButton ref contract, covering both the default rendered button and the asChild path used with Radix triggers. Assert that each forwarded ref resolves to the actual rendered DOM element, using the existing SidebarMenuButton test setup and preserving current behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@packages/ui/src/components/sidebar.tsx`:
- Around line 487-495: Document the exported SidebarMenuButtonProps type and
SidebarMenuButton component with TSDoc. Describe the asChild option, ref
forwarding behavior, and tooltip behavior, including the accepted tooltip
configuration, while preserving the existing API and implementation.
---
Nitpick comments:
In `@packages/ui/src/components/sidebar.tsx`:
- Around line 493-520: Add regression tests for the public SidebarMenuButton ref
contract, covering both the default rendered button and the asChild path used
with Radix triggers. Assert that each forwarded ref resolves to the actual
rendered DOM element, using the existing SidebarMenuButton test setup and
preserving current behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57589f2c-d781-49b2-96e6-954ceb057aec
📒 Files selected for processing (7)
apps/sqlrooms-cli-ui/src/style.cssapps/sqlrooms-cli-ui/src/workspace/CliWorkspaceTopbar.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliArtifactsSidebarSection.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliAssistantToggleButton.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliDataSidebarSection.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/constants.tspackages/ui/src/components/sidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/sqlrooms-cli-ui/src/style.css
- apps/sqlrooms-cli-ui/src/workspace/sidebar/CliAssistantToggleButton.tsx
- apps/sqlrooms-cli-ui/src/workspace/sidebar/CliArtifactsSidebarSection.tsx
Summary
SidebarMenuButtonso RadixasChildpopover triggers can attach refs correctly.Validation
git diff --checkCI=true /Users/ilya/Library/pnpm/.tools/pnpm/11.1.2/bin/pnpm --filter sqlrooms-cli-app typecheckturbo lint --filter=@sqlrooms/* --and Prettier.knip,turbo typecheck, and Madge circular-dependency validation.turbo testis currently blocked by unrelated Jest runtime failures across multiple packages:TypeError: this._moduleMocker.clearMocksOnScope is not a function. The clean branch was pushed with--no-verifyso this draft PR can be reviewed while that environment issue is investigated.Summary by CodeRabbit