Repository navigation
feat: Add shadcn Sidebar component - #400
Merged
Merged
Conversation
Co-authored-by: Ilya Boyandin <[email protected]>
|
Cursor Agent can help with this pull request. Just |
Contributor
📝 WalkthroughWalkthroughAdds a responsive, stateful Sidebar component suite and a useIsMobile hook to the UI package, updates theme tokens, and refactors the multi-room example to use a Sidebar-driven layout with room switching and a TableSchemaTree; RoomContent now accepts roomId. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant RoomPage as RoomPage
participant SidebarProv as SidebarProvider
participant RoomsAPI as getRoomsList/getRoom
participant Schema as TableSchemaTree
User->>RoomPage: Open room page (roomId)
RoomPage->>SidebarProv: Mount with roomId (provide context)
SidebarProv->>RoomsAPI: getRoomsList()
RoomsAPI-->>SidebarProv: rooms list
SidebarProv->>RoomPage: expose selectedRoom, onSelect, onAdd
User->>SidebarProv: select room / add room
SidebarProv->>RoomPage: navigate(roomId)
RoomPage->>Schema: refreshTableSchemas(roomId)
Schema-->>RoomPage: schemaTrees / loading state
RoomPage->>User: render sidebar, schema tree, table UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Co-authored-by: Ilya Boyandin <[email protected]>
Co-authored-by: Ilya Boyandin <[email protected]>
ilyabo
marked this pull request as ready for review
February 27, 2026 18:40
ilyabo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/components/sidebar.tsx (1)
503-518: Consider avoiding parameter reassignment.The
tooltipparameter is reassigned from a string to an object. While this works, it can be slightly cleaner to use a separate variable for the normalized value:♻️ Optional refactor
if (!tooltip) { return button; } - if (typeof tooltip === 'string') { - tooltip = { - children: tooltip, - }; - } + const tooltipProps = typeof tooltip === 'string' ? { children: tooltip } : tooltip; return ( <Tooltip> <TooltipTrigger asChild>{button}</TooltipTrigger> - <TooltipContent side="right" align="center" hidden={state !== 'collapsed' || isMobile} {...tooltip} /> + <TooltipContent side="right" align="center" hidden={state !== 'collapsed' || isMobile} {...tooltipProps} /> </Tooltip> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/sidebar.tsx` around lines 503 - 518, The code reassigns the incoming tooltip param (string -> object); instead, create a new normalized variable (e.g., const normalizedTooltip or tooltipProps) to hold the normalized object and leave the original tooltip untouched. In the component around the Tooltip return path (references: tooltip param, button, TooltipTrigger, TooltipContent, state, isMobile), replace the reassignment with a conditional that sets normalizedTooltip = typeof tooltip === 'string' ? { children: tooltip } : tooltip and then spread normalizedTooltip into <TooltipContent ... {...normalizedTooltip} /> while keeping the early return for falsy tooltip.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/components/sidebar.tsx`:
- Around line 503-518: The code reassigns the incoming tooltip param (string ->
object); instead, create a new normalized variable (e.g., const
normalizedTooltip or tooltipProps) to hold the normalized object and leave the
original tooltip untouched. In the component around the Tooltip return path
(references: tooltip param, button, TooltipTrigger, TooltipContent, state,
isMobile), replace the reassignment with a conditional that sets
normalizedTooltip = typeof tooltip === 'string' ? { children: tooltip } :
tooltip and then spread normalizedTooltip into <TooltipContent ...
{...normalizedTooltip} /> while keeping the early return for falsy tooltip.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
examples/multi-room/package.jsonexamples/multi-room/src/RoomPage.tsxpackages/ui/src/components/sidebar.tsxpackages/ui/src/hooks/use-mobile.tspackages/ui/src/index.tspackages/ui/tailwind-preset.css
Co-authored-by: Ilya Boyandin <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/multi-room/src/RoomPage.tsx`:
- Around line 120-131: The room naming is off-by-one because onAddRoom uses
roomOptions.length (which includes the synthetic fallback) to compute the new
name; change the logic in onAddRoom to base the name on the real rooms array
(use rooms.length + 1) so the first created room is "Room 1" even when
roomOptions contains the fallback; update the name generation in onAddRoom to
reference rooms rather than roomOptions (symbols: onAddRoom, roomOptions,
rooms).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
examples/multi-room/package.jsonexamples/multi-room/src/RoomPage.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/multi-room/package.json
Co-authored-by: Ilya Boyandin <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/multi-room/src/RoomPage.tsx (1)
462-465:⚠️ Potential issue | 🟠 MajorEscape table identifiers in SQL query.
selectedTableis directly interpolated into the SQL query. If a table name contains", the query breaks and opens identifier-injection risk.Use the existing
escapeIdfunction from@sqlrooms/duckdbto properly escape identifiers:🔧 Proposed fix
-import {QueryDataTable} from '@sqlrooms/data-table'; +import {QueryDataTable} from '@sqlrooms/data-table'; +import {escapeId} from '@sqlrooms/duckdb'; function TableBrowser({ @@ {selectedTable ? ( <QueryDataTable key={selectedTable} - query={`SELECT * FROM "${selectedTable}"`} + query={`SELECT * FROM ${escapeId(selectedTable)}`} fontSize="text-xs" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/multi-room/src/RoomPage.tsx` around lines 462 - 465, selectedTable is interpolated directly into the SQL string passed to QueryDataTable, which allows identifier-breaking characters and identifier-injection; fix it by importing and using the escapeId function from `@sqlrooms/duckdb` to wrap the selectedTable when building the query (update the query prop passed to QueryDataTable to use escapeId(selectedTable) instead of raw selectedTable) so table identifiers are properly quoted/escaped and injection is prevented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/multi-room/src/RoomPage.tsx`:
- Around line 94-118: RoomContent never calls room.initialize(), so it may never
become initialized; add a useEffect that calls the room.initialize() method on
mount (and when roomId changes) before relying on the initialized flag. Grab the
initialize function from the same store (e.g., const initialize = useRoomStore(s
=> s.room.initialize)) and invoke it inside a useEffect with [roomId,
initialize] (use void/async handling and optional catch/logging) so the room
lifecycle is triggered prior to calling refreshTableSchemas or inspecting
tables.
---
Outside diff comments:
In `@examples/multi-room/src/RoomPage.tsx`:
- Around line 462-465: selectedTable is interpolated directly into the SQL
string passed to QueryDataTable, which allows identifier-breaking characters and
identifier-injection; fix it by importing and using the escapeId function from
`@sqlrooms/duckdb` to wrap the selectedTable when building the query (update the
query prop passed to QueryDataTable to use escapeId(selectedTable) instead of
raw selectedTable) so table identifiers are properly quoted/escaped and
injection is prevented.
Comment on lines
+94
to
118
| function RoomContent({roomId, roomName}: {roomId: string; roomName: string}) { | ||
| const navigate = useNavigate(); | ||
| const rooms = getRoomsList(); | ||
| const initialized = useRoomStore((s) => s.room.initialized); | ||
| const tables = useRoomStore((s) => s.db.tables); | ||
| const schemaTrees = useRoomStore((s) => s.db.schemaTrees); | ||
| const isRefreshing = useRoomStore((s) => s.db.isRefreshingTableSchemas); | ||
| const refreshTableSchemas = useRoomStore((s) => s.db.refreshTableSchemas); | ||
| const [selectedTable, setSelectedTable] = useState<string | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| if (initialized && tables.length > 0 && !selectedTable) { | ||
| if (!initialized) return; | ||
| void refreshTableSchemas(); | ||
| }, [initialized, refreshTableSchemas, roomId]); | ||
|
|
||
| useEffect(() => { | ||
| if (!initialized) return; | ||
| if (tables.length === 0) { | ||
| setSelectedTable(null); | ||
| return; | ||
| } | ||
| if (!selectedTable || !tables.some((table) => table.table.table === selectedTable)) { | ||
| setSelectedTable(tables[0].table.table); | ||
| } | ||
| }, [initialized, tables, selectedTable]); |
Contributor
There was a problem hiding this comment.
Call room.initialize() on mount before relying on initialized.
RoomContent gates schema refresh on initialized, but this component never triggers room.initialize(). That can leave the room stuck uninitialized depending on lifecycle timing.
🔧 Proposed fix
function RoomContent({roomId, roomName}: {roomId: string; roomName: string}) {
const navigate = useNavigate();
const rooms = getRoomsList();
const initialized = useRoomStore((s) => s.room.initialized);
+ const initializeRoom = useRoomStore((s) => s.room.initialize);
const tables = useRoomStore((s) => s.db.tables);
const schemaTrees = useRoomStore((s) => s.db.schemaTrees);
const isRefreshing = useRoomStore((s) => s.db.isRefreshingTableSchemas);
const refreshTableSchemas = useRoomStore((s) => s.db.refreshTableSchemas);
const [selectedTable, setSelectedTable] = useState<string | null>(null);
+ useEffect(() => {
+ void initializeRoom();
+ }, [initializeRoom, roomId]);
+
useEffect(() => {
if (!initialized) return;
void refreshTableSchemas();
}, [initialized, refreshTableSchemas, roomId]);As per coding guidelines, "Always call room.initialize() during component mount to orchestrate automatic slice initialization with lifecycle hooks".
📝 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
| function RoomContent({roomId, roomName}: {roomId: string; roomName: string}) { | |
| const navigate = useNavigate(); | |
| const rooms = getRoomsList(); | |
| const initialized = useRoomStore((s) => s.room.initialized); | |
| const tables = useRoomStore((s) => s.db.tables); | |
| const schemaTrees = useRoomStore((s) => s.db.schemaTrees); | |
| const isRefreshing = useRoomStore((s) => s.db.isRefreshingTableSchemas); | |
| const refreshTableSchemas = useRoomStore((s) => s.db.refreshTableSchemas); | |
| const [selectedTable, setSelectedTable] = useState<string | null>(null); | |
| useEffect(() => { | |
| if (initialized && tables.length > 0 && !selectedTable) { | |
| if (!initialized) return; | |
| void refreshTableSchemas(); | |
| }, [initialized, refreshTableSchemas, roomId]); | |
| useEffect(() => { | |
| if (!initialized) return; | |
| if (tables.length === 0) { | |
| setSelectedTable(null); | |
| return; | |
| } | |
| if (!selectedTable || !tables.some((table) => table.table.table === selectedTable)) { | |
| setSelectedTable(tables[0].table.table); | |
| } | |
| }, [initialized, tables, selectedTable]); | |
| function RoomContent({roomId, roomName}: {roomId: string; roomName: string}) { | |
| const navigate = useNavigate(); | |
| const rooms = getRoomsList(); | |
| const initialized = useRoomStore((s) => s.room.initialized); | |
| const initializeRoom = useRoomStore((s) => s.room.initialize); | |
| const tables = useRoomStore((s) => s.db.tables); | |
| const schemaTrees = useRoomStore((s) => s.db.schemaTrees); | |
| const isRefreshing = useRoomStore((s) => s.db.isRefreshingTableSchemas); | |
| const refreshTableSchemas = useRoomStore((s) => s.db.refreshTableSchemas); | |
| const [selectedTable, setSelectedTable] = useState<string | null>(null); | |
| useEffect(() => { | |
| void initializeRoom(); | |
| }, [initializeRoom, roomId]); | |
| useEffect(() => { | |
| if (!initialized) return; | |
| void refreshTableSchemas(); | |
| }, [initialized, refreshTableSchemas, roomId]); | |
| useEffect(() => { | |
| if (!initialized) return; | |
| if (tables.length === 0) { | |
| setSelectedTable(null); | |
| return; | |
| } | |
| if (!selectedTable || !tables.some((table) => table.table.table === selectedTable)) { | |
| setSelectedTable(tables[0].table.table); | |
| } | |
| }, [initialized, tables, selectedTable]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/multi-room/src/RoomPage.tsx` around lines 94 - 118, RoomContent
never calls room.initialize(), so it may never become initialized; add a
useEffect that calls the room.initialize() method on mount (and when roomId
changes) before relying on the initialized flag. Grab the initialize function
from the same store (e.g., const initialize = useRoomStore(s =>
s.room.initialize)) and invoke it inside a useEffect with [roomId, initialize]
(use void/async handling and optional catch/logging) so the room lifecycle is
triggered prior to calling refreshTableSchemas or inspecting tables.
Co-authored-by: Ilya Boyandin <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/multi-room/README.md (1)
8-8: Consider more neutral, evergreen documentation language.The phrase "powerful new Sidebar component pattern" uses subjective marketing language. Technical documentation typically benefits from more neutral, factual descriptions. The word "new" will also become outdated over time.
📝 Suggested improvement
-A multi-room application demonstrating how to manage multiple independent data workspaces with proper room store lifecycle management and the powerful new Sidebar component pattern. Features include: +A multi-room application demonstrating how to manage multiple independent data workspaces with proper room store lifecycle management and a Sidebar component for navigation. Features include:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/multi-room/README.md` at line 8, Replace the subjective marketing phrase "powerful new Sidebar component pattern" with neutral, evergreen wording: describe the Sidebar as a component or pattern and state its function factually (e.g., "the Sidebar component pattern for managing workspace navigation and context"), removing "powerful" and "new" and keeping the rest of the sentence intact to read: "A multi-room application demonstrating how to manage multiple independent data workspaces with proper room store lifecycle management and the Sidebar component pattern."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/multi-room/README.md`:
- Line 8: Replace the subjective marketing phrase "powerful new Sidebar
component pattern" with neutral, evergreen wording: describe the Sidebar as a
component or pattern and state its function factually (e.g., "the Sidebar
component pattern for managing workspace navigation and context"), removing
"powerful" and "new" and keeping the rest of the sentence intact to read: "A
multi-room application demonstrating how to manage multiple independent data
workspaces with proper room store lifecycle management and the Sidebar component
pattern."
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/media/examples/multi-room.mp4is excluded by!**/*.mp4
📒 Files selected for processing (2)
docs/examples.mdexamples/multi-room/README.md
✅ Files skipped from review due to trivial changes (1)
- docs/examples.md
Merged
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
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.
Add a shadcn-style sidebar to
@sqlrooms/uiand integrate it into the multi-room example to enable project switching and display the schema tree.Summary by CodeRabbit
New Features
Chores
Documentation