Feat/sidebar adjustments #1743
Feat/sidebar adjustments #1743
Conversation
- Fix z-index issue: Change dropdown menu z-index from z-10 to z-50 to display above form containers - Add external link icons: Display ExternalLink icons for Docs, Terms, Privacy & Cookies, and Homepage items - Add Display type dialog: Implement display type selection dialog matching the settings UI design - Improve Help submenu width: Adjust width from 250px to 180px for better text-icon spacing - Fix dropdown menu state management: Add proper state handling to prevent conflicts between dropdown and dialog The navigation rail menu now properly displays above other UI elements and provides a consistent experience with external link indicators and display type configuration.
- Replace local state with URL search params for display type management - Add useRouter and useSearchParams to NavigationRailFooterMenu for URL state control - Update useUIState hook to read/write display type from URL parameters - Enable seamless sync between sidebar Display type dialog and main content view - Support browser history and bookmarkable display states (?view=carousel) - Remove localStorage dependency in favor of URL state management The display type selection now works correctly across all components, with changes in the navigation rail immediately reflected in the main content area.
- Extract HELP_ITEMS constant to module level for better organization - Unify MENU_ITEM_CLASS constant to reduce CSS class duplication across 6 usage points - Add as const assertion for type safety on help items array - Improve code readability and maintainability without UI changes This refactoring reduces redundancy, improves consistency, and makes future styling changes easier to implement by centralizing repeated CSS class definitions.
|
|
Finished running flow.
|
WalkthroughURL-driven view state: the carousel/list view is now derived from the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Footer as NavigationRail Footer Menu
participant Dialog as Display Type Dialog
participant Hook as useUIState
participant Router as Next.js Router
participant UI as Stage View
User->>Footer: open menu
Footer->>Dialog: open "Display type"
User->>Dialog: select "Carousel" or "List"
Dialog->>Hook: setIsCarouselView(value)
alt value == carousel
Hook->>Router: push ?view=carousel (no-scroll)
else
Hook->>Router: push ? (remove view)
end
Router-->>Hook: updated searchParams
Hook-->>UI: isCarouselView derived from URL
UI-->>User: render selected view
sequenceDiagram
autonumber
participant Server as getSidebarData
participant Teams as teams service
Server->>Teams: fetchCurrentTeam()
Teams-->>Server: currentTeam
Server->>Teams: isProPlan(currentTeam)
Teams-->>Server: boolean
Server-->>Caller: sidebar data { ..., planName: "Pro plan"/"Free plan" }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
This comment was marked as outdated.
This comment was marked as outdated.
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (18)
apps/studio.giselles.ai/app/stage/loading.tsx (1)
5-5: Good move to theme variables; consider dvh + reduced-motion for skeletons
- Using
var(--color-stage-background)is spot on. On mobile Safari,h-screencan cause viewport jump; considermin-h-[100dvh]to account for dynamic toolbars.- For motion-sensitive users, prefer
motion-safe:animate-pulseover unconditionalanimate-pulse.- Optional a11y: mark the region as loading with
aria-busy(and optionallyaria-live="polite").Apply:
- <div className="flex h-screen bg-[var(--color-stage-background)]"> + <div className="flex min-h-[100dvh] bg-[var(--color-stage-background)]" aria-busy="true" aria-live="polite">- <div className="w-[200px] bg-[var(--color-stage-background)] border-r border-white/10 animate-pulse"> + <div className="w-[200px] bg-[var(--color-stage-background)] border-r border-white/10 motion-safe:animate-pulse">Also applies to: 7-7
apps/studio.giselles.ai/app/stage/ui/navigation-rail/menu-button.tsx (1)
18-18: Adopts stage sidebar tokens — add focus-visible ring and allow aria-label passthroughThe color tokens look good. Two improvements:
- Add a visible keyboard focus outline using the new stage key color.
- The component currently blocks
aria-label(and most native button props). Allowing passthrough props improves a11y for icon-only buttons.Apply focus styles:
- className={clsx( - "group size-8 text-[var(--color-stage-sidebar-text)] hover:text-[var(--color-stage-sidebar-text-hover)] transition-colors rounded flex items-center justify-center", - className, - )} + className={clsx( + "group size-8 text-[var(--color-stage-sidebar-text)] hover:text-[var(--color-stage-sidebar-text-hover)] transition-colors rounded flex items-center justify-center", + "focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-[var(--color-stage-key)] focus-visible:ring-offset-2 focus-visible:ring-offset-[var(--color-stage-background)]", + className, + )}For prop passthrough (outside the selected lines), a minimal revision:
export function MenuButton({ onClick, children, className, ...rest }: React.ButtonHTMLAttributes<HTMLButtonElement>) { return ( <button type="button" onClick={onClick} className={/* same clsx as above */} {...rest} // enables aria-label, title, disabled, etc. > {children} </button> ); }apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-collapsed.tsx (2)
29-30: Icon color theming looks correct; consider keyboard parity for hover swapUsing
--color-stage-sidebar-text(-hover)is consistent with the new tokens. Optional: mirror the hover swap for keyboard focus so non-pointer users see the same cue.Apply:
- <GiselleIcon className="size-6 text-[var(--color-stage-sidebar-text-hover)] stroke-1 group-hover:hidden" /> - <PanelLeftOpenIcon className="size-6 text-[var(--color-stage-sidebar-text)] stroke-1 hidden group-hover:block" /> + <GiselleIcon className="size-6 text-[var(--color-stage-sidebar-text-hover)] stroke-1 group-hover:hidden group-focus-within:hidden" /> + <PanelLeftOpenIcon className="size-6 text-[var(--color-stage-sidebar-text)] stroke-1 hidden group-hover:block group-focus-within:block" />
48-49: Unify fallback skeleton background with stage themeThe Suspense fallback still uses
bg-black-800. For consistency with the rest of the PR (and custom themes), usebg-[var(--color-stage-background)].Apply:
- <div className="w-full bg-black-800 animate-pulse h-full rounded-md" /> + <div className="w-full bg-[var(--color-stage-background)] motion-safe:animate-pulse h-full rounded-md" />apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx (4)
27-27: Verifygroup-hover:hiddenactually has a.groupancestor.The GiselleIcon uses
group-hover:hidden, but I don't see a.groupancestor in this header block. If no ancestor hasgroup, the hover style won’t trigger.Proposed minimal fix (add
.groupto the wrapper on Line 25):-<div className="flex items-center justify-start w-full"> +<div className="group flex items-center justify-start w-full">
29-31: Use the base text token for non-interactive label; reserve “hover” token for hover states.
Stagelabel usestext-[var(--color-stage-sidebar-text-hover)]. If the label isn’t interactive, prefer the base token to match the theming model and only shift to the hover token on interactive elements.-<p className="text-[var(--color-stage-sidebar-text-hover)] text-[13px] font-semibold"> +<p className="text-[var(--color-stage-sidebar-text)] text-[13px] font-semibold">
34-37: Cursor affordance: usecursor-pointerfor a clickable collapse button.
cursor-w-resizesuggests drag-resizing. If the action is a click-to-collapse,cursor-pointeris clearer.- className="cursor-w-resize" + className="cursor-pointer"
34-39: Add an accessible label to the collapse button.Improves screen-reader UX.
- <MenuButton - onClick={() => onCollapseButtonClick()} - className="cursor-w-resize" - > + <MenuButton + onClick={() => onCollapseButtonClick()} + className="cursor-pointer" + aria-label="Collapse sidebar" + >apps/studio.giselles.ai/app/stage/query.ts (1)
6-8: Handle team fetch failures gracefully and fetch in parallel to reduce latency.
- If
fetchCurrentTeam()throws, the current code would crash this call path.- Fetch account/team concurrently to shave a round-trip.
export async function getSidebarData() { - const accountInfo = await getAccountInfo(); - const currentTeam = await fetchCurrentTeam(); - const isPro = isProPlan(currentTeam); + const [accountInfo, currentTeam] = await Promise.all([ + getAccountInfo(), + // Tolerate errors: treat plan as unknown instead of failing the whole request. + fetchCurrentTeam().catch(() => null as const), + ]); + const isPro = currentTeam ? isProPlan(currentTeam) : false; return { displayName: accountInfo.displayName ?? undefined, email: accountInfo.email ?? undefined, avatarUrl: accountInfo.avatarUrl ?? undefined, - planName: isPro ? "Pro plan" : "Free plan", + // If team is unavailable, leave undefined to avoid misreporting the plan. + planName: currentTeam ? (isPro ? "Pro plan" : "Free plan") : undefined, }; }Also applies to: 13-13
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-list-item.tsx (1)
11-19: Add accessible name when collapsed (icon-only).When
variant === "collapsed", the link renders only an icon; without an accessible name, screen readers may announce an unlabeled link.<Link href={props.href} - className="text-[var(--color-stage-sidebar-text)] text-sm flex items-center py-0.5 hover:text-[var(--color-stage-sidebar-text-hover)] rounded-lg px-1" + className="text-[var(--color-stage-sidebar-text)] text-sm flex items-center py-0.5 hover:text-[var(--color-stage-sidebar-text-hover)] rounded-lg px-1" + aria-label={props.variant === "collapsed" ? props.label : undefined} + title={props.variant === "collapsed" ? props.label : undefined} >Optional: add a focus-visible style for keyboard users.
- className="text-[var(--color-stage-sidebar-text)] text-sm flex items-center py-0.5 hover:text-[var(--color-stage-sidebar-text-hover)] rounded-lg px-1" + className="text-[var(--color-stage-sidebar-text)] text-sm flex items-center py-0.5 hover:text-[var(--color-stage-sidebar-text-hover)] rounded-lg px-1 focus-visible:outline focus-visible:outline-2 focus-visible:outline-[var(--color-stage-key)] focus-visible:outline-offset-2"apps/studio.giselles.ai/app/stage/ui/navigation-rail/types.ts (1)
7-8: NarrowplanNameto a union for type-safety.Since only “Pro plan”/“Free plan” are produced today, consider a tagged type to avoid typos and ease future additions.
+export type PlanName = "Pro plan" | "Free plan"; + export interface UserDataForNavigationRail { displayName: string | undefined; email: string | undefined; avatarUrl: string | undefined; - planName: string | undefined; + planName: PlanName | undefined; }apps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsx (2)
22-30: Preserve pathname when updating query params.Pushing only
?queryrelies on relative resolution; usingusePathname()avoids edge cases (nested paths, basePath). Also keeps intent explicit.-import { useRouter, useSearchParams } from "next/navigation"; +import { usePathname, useRouter, useSearchParams } from "next/navigation"; @@ - const setIsCarouselView = (value: boolean) => { + const setIsCarouselView = (value: boolean) => { const params = new URLSearchParams(searchParams.toString()); if (value) { params.set("view", "carousel"); } else { params.delete("view"); } - router.push(`?${params.toString()}`, { scroll: false }); + const pathname = usePathname(); + router.push(`${pathname}?${params.toString()}`, { scroll: false }); };
22-30: Optional: avoid fragment-only updates creating duplicate history entries.If users toggle frequently, consider
router.replaceto avoid history spam. You’re intentionally supporting history, so keeppushif that’s desired.- router.push(`${pathname}?${params.toString()}`, { scroll: false }); + router.push(`${pathname}?${params.toString()}`, { scroll: false }); // or router.replace(...)apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (5)
68-79: Preserve path/basePath when pushing query paramsUsing
router.push("?…")relies on implicit resolution against the current URL. UsingusePathname()is more robust (keeps basePath, locale, and nested routes intact).Apply these diffs:
- Imports
- import { useRouter, useSearchParams } from "next/navigation"; + import { usePathname, useRouter, useSearchParams } from "next/navigation";
- Initialize pathname
- const router = useRouter(); + const router = useRouter(); + const pathname = usePathname();
- Push with pathname and update deps
- router.push(`?${params.toString()}`, { scroll: false }); + router.push(`${pathname}?${params.toString()}`, { scroll: false });- [router, searchParams], + [pathname, router, searchParams],
167-176: Add noreferrer to external links for privacy and consistencyYou already use
rel="noopener". Appendnoreferrerto prevent referrer leakage to external destinations.Apply these diffs:
- rel="noopener" + rel="noopener noreferrer"Also applies to: 200-209
216-218: Avoid nested interactive elements; render SignOutButton as the item via asChildPlacing a button inside a non-asChild
DropdownMenu.Itemnests interactive elements and can confuse focus/ARIA and event handling. Let the Item render your button directly.- {/* Logout */} - <DropdownMenuPrimitive.Item className={MENU_ITEM_CLASS}> - <SignOutButton className="text-[14px]">Log out</SignOutButton> - </DropdownMenuPrimitive.Item> + {/* Logout */} + <DropdownMenuPrimitive.Item className={MENU_ITEM_CLASS} asChild> + <SignOutButton className="w-full text-[14px]">Log out</SignOutButton> + </DropdownMenuPrimitive.Item>
244-301: Potential UI lag: RadioGroup value derives from URL stateBecause
valuecomes fromuseSearchParams(), the visual selection may briefly lag until the router updates. If you notice this, keep a localselectedViewstate for instant feedback while still pushing the URL on change.Example approach (minimal):
// Local state next to dialog open state const [selectedView, setSelectedView] = useState<"list" | "carousel">(isCarouselView ? "carousel" : "list"); // Keep in sync when URL changes or dialog opens useEffect(() => { setSelectedView(isCarouselView ? "carousel" : "list"); }, [isCarouselView, isDisplayDialogOpen]); // RadioGroup <RadioGroup value={selectedView} onValueChange={(value) => { setSelectedView(value as "list" | "carousel"); setIsCarouselView(value === "carousel"); // keep immediate URL sync if desired }} />
331-336: Microcopy nit: “Continue” might imply deferred applyThe selection applies immediately on radio change. Consider “Done” or “Close” to better reflect the action.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
apps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsx(2 hunks)apps/studio.giselles.ai/app/stage/layout.tsx(1 hunks)apps/studio.giselles.ai/app/stage/loading.tsx(1 hunks)apps/studio.giselles.ai/app/stage/query.ts(1 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/menu-button.tsx(1 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-list-item.tsx(1 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-collapsed.tsx(1 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx(1 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx(3 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/types.ts(1 hunks)internal-packages/ui/style.css(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.{ts,tsx}: Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoidanytypes
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames (e.g.,user-profile.ts)
Use camelCase for variables, functions, and methods (e.g.,userEmail)
Use prefixes likeis,has,can,shouldfor boolean variables and functions for clarity
Use verbs or verb phrases that clearly indicate purpose for function naming (e.g.,calculateTotalPrice(), notprocess())If breaking changes are introduced in new AI SDK versions, update code to accommodate those changes
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/menu-button.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-collapsed.tsxapps/studio.giselles.ai/app/stage/layout.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-list-item.tsxapps/studio.giselles.ai/app/stage/query.tsapps/studio.giselles.ai/app/stage/ui/navigation-rail/types.tsapps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsxapps/studio.giselles.ai/app/stage/loading.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.tsx: Use functional components with React hooks
Use PascalCase for React components and classes (e.g.,UserProfile)
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/menu-button.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-collapsed.tsxapps/studio.giselles.ai/app/stage/layout.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-list-item.tsxapps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsxapps/studio.giselles.ai/app/stage/loading.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
All filenames should use kebab-case (lowercase with hyphens)
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/menu-button.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-collapsed.tsxapps/studio.giselles.ai/app/stage/layout.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-list-item.tsxapps/studio.giselles.ai/app/stage/query.tsapps/studio.giselles.ai/app/stage/ui/navigation-rail/types.tsapps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsxapps/studio.giselles.ai/app/stage/loading.tsxinternal-packages/ui/style.cssapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
**/*.{js,jsx,ts,tsx}: React components and classes should use PascalCase
Variables, functions, and methods should use camelCase
Use verbs or verb phrases for function names; names should clearly indicate what the function does; avoid ambiguous names that could lead to misuse
Use nouns or noun phrases for variable names; names should describe what the variable represents; avoid single-letter variables except in very short scopes
Use prefixes like 'is', 'has', 'can', 'should' for both variables and functions returning boolean values; make the true/false meaning clear
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/menu-button.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-collapsed.tsxapps/studio.giselles.ai/app/stage/layout.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-list-item.tsxapps/studio.giselles.ai/app/stage/query.tsapps/studio.giselles.ai/app/stage/ui/navigation-rail/types.tsapps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsxapps/studio.giselles.ai/app/stage/loading.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
🧠 Learnings (3)
📚 Learning: 2025-07-21T22:29:07.662Z
Learnt from: CR
PR: giselles-ai/giselle#0
File: .cursor/rules/edit-workspace-tour.mdc:0-0
Timestamp: 2025-07-21T22:29:07.662Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/workspace-tour/workspace-tour.tsx : Update the `TourGlobalStyles` component in `workspace-tour.tsx` for animation changes
Applied to files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/menu-button.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-collapsed.tsxapps/studio.giselles.ai/app/stage/layout.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-list-item.tsxapps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsxapps/studio.giselles.ai/app/stage/loading.tsx
📚 Learning: 2025-07-21T22:29:07.662Z
Learnt from: CR
PR: giselles-ai/giselle#0
File: .cursor/rules/edit-workspace-tour.mdc:0-0
Timestamp: 2025-07-21T22:29:07.662Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/workspace-tour/workspace-tour.tsx : Modify CSS positioning in step components in `workspace-tour.tsx` to adjust step positioning
Applied to files:
apps/studio.giselles.ai/app/stage/layout.tsx
📚 Learning: 2025-07-21T22:29:07.662Z
Learnt from: CR
PR: giselles-ai/giselle#0
File: .cursor/rules/edit-workspace-tour.mdc:0-0
Timestamp: 2025-07-21T22:29:07.662Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/workspace-tour/workspace-tour.tsx : Modify the `CARD_STYLES` constants in `workspace-tour.tsx` to change step styling
Applied to files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx
🧬 Code graph analysis (4)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-collapsed.tsx (1)
internal-packages/workflow-designer-ui/src/icons/giselle-icon.tsx (1)
GiselleIcon(3-34)
apps/studio.giselles.ai/app/stage/query.ts (2)
apps/studio.giselles.ai/app/(main)/settings/account/actions.ts (1)
getAccountInfo(53-75)apps/studio.giselles.ai/services/teams/utils.ts (1)
isProPlan(4-8)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx (2)
internal-packages/workflow-designer-ui/src/icons/giselle-icon.tsx (1)
GiselleIcon(3-34)apps/studio.giselles.ai/app/stage/ui/navigation-rail/menu-button.tsx (1)
MenuButton(3-26)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (3)
internal-packages/ui/components/popover.tsx (1)
PopoverContent(4-15)internal-packages/ui/components/dialog.tsx (1)
DialogFooter(59-69)apps/studio.giselles.ai/lib/utils.ts (1)
cn(4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
apps/studio.giselles.ai/app/stage/layout.tsx (1)
9-9: LGTM — background now sourced from stage themeConsistent with the rest of the PR and keeps layout and loading aligned. No functional concerns.
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (3)
118-124: z-index bump and width behavior look correct
z-50addresses the submenu stacking issue, and using Radix Portal ensures the layer escapes overflow contexts. The trigger-width binding is a nice touch for the expanded variant.
28-53: Good extraction of HELP_ITEMS and shared MENU_ITEM_CLASSMoving these to module scope reduces duplication and increases consistency. The
as constassertion provides stronger typing of item shapes.
54-62: Verify React/Next versions foruse()supportI wasn’t able to determine exact React or Next versions from your manifests (both are using
workspace:/catalog:protocols in package.json and the lock file didn’t expose explicit versions). React 19+ is required to use the experimentaluse()hook inside a Client Component, and your Next version must bundle that support. Please confirm you’re on React ≥ 19.0.0 (and a compatible Next.js release). If not, you’ll need to resolve the promise in a Server Component and pass the data down as a plain prop.Call sites using
use()on a Promise—ensure each is either in a Server Component or you’re on React 19+:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx(lines 61–62)apps/studio.giselles.ai/app/stage/acts/[actId]/ui/sidebar.tsx(line 41)apps/studio.giselles.ai/app/(main)/apps/page.tsx(line 14)Suggested fallback diff if you need to switch to resolved props:
-export function NavigationRailFooterMenu({ - user: userPromise, - variant, -}: { - user: Promise<UserDataForNavigationRail>; - variant: NavigationRailState; -}) { - const user = use(userPromise); +export function NavigationRailFooterMenu({ + user, + variant, +}: { + user: UserDataForNavigationRail; + variant: NavigationRailState; +}) { + // user is resolved upstream in a Server Component
| @@ -1,10 +1,15 @@ | |||
| import { fetchCurrentTeam, isProPlan } from "@/services/teams"; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm barrel exports exist for @/services/teams.
Importing both fetchCurrentTeam and isProPlan from the package root implies a barrel. Please verify the index exports to avoid runtime/import errors.
Run from repo root:
🏁 Script executed:
#!/bin/bash
# Verify exports in services/teams
rg -nP 'export\s+(?:\{[^}]*\}|const|function|type|interface)\s+isProPlan|fetchCurrentTeam' -- apps/studio.giselles.ai/services
rg -nP 'from\s+"@/services/teams"' -C2Length of output: 13617
Add missing barrel exports in services/teams
It looks like index.ts in apps/studio.giselles.ai/services/teams only re-exports fetchCurrentTeam, but you’re importing both fetchCurrentTeam and isProPlan (and elsewhere also fetchUserTeams) from "@/services/teams". That will cause unresolved-module errors at runtime.
Please update apps/studio.giselles.ai/services/teams/index.ts to include:
--- a/apps/studio.giselles.ai/services/teams/index.ts
+++ b/apps/studio.giselles.ai/services/teams/index.ts
export { fetchCurrentTeam } from "./fetch-current-team";
+export { isProPlan } from "./utils";
+export { fetchUserTeams } from "./fetch-user-teams";• Verify that ./utils.ts exports isProPlan.
• Verify that there is a fetch-user-teams.ts (or equivalent) exporting fetchUserTeams.
• After adding these barrel exports, confirm all imports from "@/services/teams" resolve without errors.
📝 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.
| import { fetchCurrentTeam, isProPlan } from "@/services/teams"; | |
| // apps/studio.giselles.ai/services/teams/index.ts | |
| export { fetchCurrentTeam } from "./fetch-current-team"; | |
| export { isProPlan } from "./utils"; | |
| export { fetchUserTeams } from "./fetch-user-teams"; |
🤖 Prompt for AI Agents
In apps/studio.giselles.ai/services/teams/index.ts around the current exports
(affecting imports in apps/studio.giselles.ai/app/stage/query.ts line 1), the
barrel file only re-exports fetchCurrentTeam but callers import isProPlan and
fetchUserTeams as well; update the barrel to export isProPlan (verify it is
exported from ./utils.ts) and export fetchUserTeams from its module (e.g.,
./fetch-user-teams.ts or the actual filename), and run a TypeScript build or IDE
import resolution to confirm all imports from "@/services/teams" resolve without
errors.
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
Outdated
Show resolved
Hide resolved
- Create custom SimpleChevronLeft/Right icons with 1.5px stroke width - Replace PanelLeftCloseIcon and PanelLeftOpenIcon with minimal double chevron design - Reduce icon size from size-6 to size-5 for better visual balance - Add proper accessibility attributes (role, aria-label) - Maintain existing hover behavior and cursor styles
|
Finished running flow.
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
🔍 QA Testing Assistant by Giselle📋 Manual QA ChecklistBased on the changes in this PR, here are the key areas to test manually:
✨ Prompt for AI AgentsUse the following prompts with Cursor or Claude Code to automate E2E testing: 📝 E2E Test Generation Prompt |
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/simple-chevron-icons.tsx (4)
17-19: Make icons decorative by default; move the accessible name to the button.Placing aria-label on the SVG ties these icons to a specific meaning (“Collapse/Expand sidebar”). It’s more robust to:
- Mark SVGs as decorative (aria-hidden) and let the parent control (button) provide the accessible name, or
- Accept an overridable aria-label via props (which you already do by spreading props last), but don’t set a semantic default here.
Proposed change (keeps consumer override possible):
- role="img" - aria-label="Collapse sidebar" + aria-hidden="true" + focusable="false" {...props}Apply to both icons at the indicated lines.
Also applies to: 37-39
21-22: Optional: Keep stroke width visually consistent when scaled.If these icons are frequently sized via CSS (e.g., size-4 to size-7), consider non-scaling strokes for crisper rendering:
- <path d="M11 17l-5-5 5-5" /> - <path d="M18 17l-5-5 5-5" /> + <path vectorEffect="non-scaling-stroke" d="M11 17l-5-5 5-5" /> + <path vectorEffect="non-scaling-stroke" d="M18 17l-5-5 5-5" />Repeat for the right icon’s paths.
Also applies to: 41-42
3-5: Type nit: className is already in SVG props.IconProps extends ComponentProps<"svg">, which already includes className?: string. You can drop the explicit className to avoid redundancy:
-interface IconProps extends ComponentProps<"svg"> { - className?: string; -} +type IconProps = ComponentProps<"svg">;This keeps the surface area minimal.
7-9: (Optional) forwardRef for better interoperability.If these icons might be measured/focused/animated by parent components, consider forwardRef<SVGSVGElement, IconProps>. No behavior change otherwise.
Also applies to: 27-29
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx (4)
36-37: Pass the handler directly; avoid the no-op lambda.Slightly cleaner and avoids re-creating a function on each render:
- onClick={() => onCollapseButtonClick()} + onClick={onCollapseButtonClick}
35-41: Give the collapse button an explicit accessible name.Per the icon comment, label the interactive control instead of the SVG. Once MenuButton forwards standard button props (see suggested change below), set an aria-label here:
<MenuButton - onClick={onCollapseButtonClick} + onClick={onCollapseButtonClick} + aria-label="Collapse sidebar" className="cursor-w-resize" > <SimpleChevronLeft className="size-5 text-[var(--color-stage-sidebar-text)]" /> </MenuButton>Support change in MenuButton (outside this file) to forward native props:
// apps/studio.giselles.ai/app/stage/ui/navigation-rail/menu-button.tsx export function MenuButton({ onClick, children, className, ...rest }: React.ButtonHTMLAttributes<HTMLButtonElement>) { return ( <button type="button" className={clsx( "group size-8 text-[var(--color-stage-sidebar-text)] hover:text-[var(--color-stage-sidebar-text-hover)] transition-colors rounded flex items-center justify-center", className, )} onClick={onClick} {...rest} > {children} </button> ); }
28-28: Minor: stroke-1 has no effect on a fill-only SVG.GiselleIcon fills paths with currentColor; stroke-1 won’t affect rendering. You can drop it:
- <GiselleIcon className="size-6 text-[var(--color-stage-sidebar-text-hover)] stroke-1 group-hover:hidden shrink-0" /> + <GiselleIcon className="size-6 text-[var(--color-stage-sidebar-text-hover)] group-hover:hidden shrink-0" />
37-37: Cursor style nit.cursor-w-resize implies a resize affordance; this is a toggle button. cursor-pointer is clearer:
- className="cursor-w-resize" + className="cursor-pointer"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-collapsed.tsx(3 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx(3 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/simple-chevron-icons.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-collapsed.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.{ts,tsx}: Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoidanytypes
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames (e.g.,user-profile.ts)
Use camelCase for variables, functions, and methods (e.g.,userEmail)
Use prefixes likeis,has,can,shouldfor boolean variables and functions for clarity
Use verbs or verb phrases that clearly indicate purpose for function naming (e.g.,calculateTotalPrice(), notprocess())If breaking changes are introduced in new AI SDK versions, update code to accommodate those changes
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/simple-chevron-icons.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.tsx: Use functional components with React hooks
Use PascalCase for React components and classes (e.g.,UserProfile)
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/simple-chevron-icons.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
All filenames should use kebab-case (lowercase with hyphens)
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/simple-chevron-icons.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
**/*.{js,jsx,ts,tsx}: React components and classes should use PascalCase
Variables, functions, and methods should use camelCase
Use verbs or verb phrases for function names; names should clearly indicate what the function does; avoid ambiguous names that could lead to misuse
Use nouns or noun phrases for variable names; names should describe what the variable represents; avoid single-letter variables except in very short scopes
Use prefixes like 'is', 'has', 'can', 'should' for both variables and functions returning boolean values; make the true/false meaning clear
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/simple-chevron-icons.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx
🧠 Learnings (1)
📚 Learning: 2025-07-21T22:29:07.662Z
Learnt from: CR
PR: giselles-ai/giselle#0
File: .cursor/rules/edit-workspace-tour.mdc:0-0
Timestamp: 2025-07-21T22:29:07.662Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/workspace-tour/workspace-tour.tsx : Update the `TourGlobalStyles` component in `workspace-tour.tsx` for animation changes
Applied to files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx
🧬 Code graph analysis (1)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx (3)
internal-packages/workflow-designer-ui/src/icons/giselle-icon.tsx (1)
GiselleIcon(3-34)apps/studio.giselles.ai/app/stage/ui/navigation-rail/menu-button.tsx (1)
MenuButton(3-26)apps/studio.giselles.ai/app/stage/ui/navigation-rail/simple-chevron-icons.tsx (1)
SimpleChevronLeft(7-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/simple-chevron-icons.tsx (1)
7-25: Solid, reusable chevron icons with sensible defaults.Clean SVGs, correct viewBox, currentColor usage, and props-spread order lets consumer overrides win. Naming and file casing follow the guidelines.
Also applies to: 27-45
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx (1)
13-13: Swap to internal chevron icons: LGTM.Importing and using SimpleChevronLeft aligns with the new shared icon set and theming tokens.
Also applies to: 39-39
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx
Outdated
Show resolved
Hide resolved
- Add 'use client' directives to client components using Next.js hooks - Improve color contrast for stage-sidebar-text to meet WCAG AA standards - Rename --color-stage-key to --color-stage-accent for consistency - Add group class to enable group-hover functionality 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsx (1)
1-1: "use client" added — correct and necessary for App Router hooksMarking this module as a Client Component is required when using
useRouter/useSearchParams. Good catch.
🧹 Nitpick comments (3)
apps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsx (3)
3-3: ImportusePathnameto build full, stable URLs on navigationTo avoid edge cases with relative
?querypushes and to preserve the current pathname/basePath/locale, importusePathnamefor use in the setter below.-import { useRouter, useSearchParams } from "next/navigation"; +import { useRouter, useSearchParams, usePathname } from "next/navigation";
9-11: DerivingisCarouselViewfrom the URL is a solid move; also capturepathnameURL-driven state makes the view shareable and history-friendly. Add
pathnamenow so the setter can construct absolute URLs (and avoid pushing just "?...").const searchParams = useSearchParams(); const router = useRouter(); -const isCarouselView = searchParams.get("view") === "carousel"; +const pathname = usePathname(); +const isCarouselView = searchParams.get("view") === "carousel";
24-32: Avoid redundant history entries and trailing “?”; push full path + queryTwo improvements:
- No-op when the requested value matches the current one (prevents extra history entries).
- Build the URL using
pathnameand only append?when there are params, avoiding URLs like/stage?.-const setIsCarouselView = (value: boolean) => { - const params = new URLSearchParams(searchParams.toString()); - if (value) { - params.set("view", "carousel"); - } else { - params.delete("view"); - } - router.push(`?${params.toString()}`, { scroll: false }); -}; +const setIsCarouselView = (value: boolean) => { + // No-op if nothing changes; avoids redundant history entries + if (isCarouselView === value) return; + + const params = new URLSearchParams(searchParams.toString()); + if (value) { + params.set("view", "carousel"); + } else { + params.delete("view"); + } + const qs = params.toString(); + router.push(qs ? `${pathname}?${qs}` : pathname, { scroll: false }); +};Optional: if your page uses hash fragments, consider preserving them by appending
window.location.hashwhen present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsx(2 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx(3 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx(3 hunks)internal-packages/ui/style.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-expanded.tsx
- internal-packages/ui/style.css
- apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.{ts,tsx}: Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoidanytypes
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames (e.g.,user-profile.ts)
Use camelCase for variables, functions, and methods (e.g.,userEmail)
Use prefixes likeis,has,can,shouldfor boolean variables and functions for clarity
Use verbs or verb phrases that clearly indicate purpose for function naming (e.g.,calculateTotalPrice(), notprocess())If breaking changes are introduced in new AI SDK versions, update code to accommodate those changes
Files:
apps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.tsx: Use functional components with React hooks
Use PascalCase for React components and classes (e.g.,UserProfile)
Files:
apps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
All filenames should use kebab-case (lowercase with hyphens)
Files:
apps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
**/*.{js,jsx,ts,tsx}: React components and classes should use PascalCase
Variables, functions, and methods should use camelCase
Use verbs or verb phrases for function names; names should clearly indicate what the function does; avoid ambiguous names that could lead to misuse
Use nouns or noun phrases for variable names; names should describe what the variable represents; avoid single-letter variables except in very short scopes
Use prefixes like 'is', 'has', 'can', 'should' for both variables and functions returning boolean values; make the true/false meaning clear
Files:
apps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsx
🧠 Learnings (1)
📚 Learning: 2025-07-21T22:29:07.662Z
Learnt from: CR
PR: giselles-ai/giselle#0
File: .cursor/rules/edit-workspace-tour.mdc:0-0
Timestamp: 2025-07-21T22:29:07.662Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/workspace-tour/workspace-tour.tsx : Update the `TourGlobalStyles` component in `workspace-tour.tsx` for animation changes
Applied to files:
apps/studio.giselles.ai/app/stage/(top)/hooks/use-ui-state.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: Cursor Bugbot
There was a problem hiding this comment.
@kaochannel154 When I checked the operation, it seemed that I couldn't change the display format by selecting the radio button in the View Style modal.
Could you please check if you can reproduce this issue locally?
cannot-click-carousel.mp4
|
@shige It's going to take a while to load, but it should work...! I know it's bad UX, but do you know the reason?? 2025-08-25.21.24.14.mov |
|
@kaochannel154 I understand that this has an impact on the speed at which the app loads! If the View Style changes in the background when you switch the radio button, I thought that the "Continue" button might not be necessary.I thought that showing only an X or Close button would be better instead. I was confused because I mistakenly thought that the View Style changed when you pressed the Continue button. |
- Removed Continue button from both PC and mobile View Style dialogs - Removed X close icon from dialog headers - Simplified dialog footer to only show Cancel button - Applied changes to both settings-dialog.tsx and navigation-rail-footer-menu.tsx 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
I removed the continue button and the X button! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsx (1)
41-52: Mobile modal lacks dialog semantics and focus management—screen readers will miss this dialogThe custom mobile overlay is not an actual dialog (no role="dialog", no aria-modal, no labelledby, no focus trap, no Escape handling). This impacts accessibility and keyboard users. If keeping the custom overlay, minimally add dialog semantics and basic close interactions; ideally, reuse the same Dialog component as desktop for consistency and a11y.
Apply this minimal patch for semantics and basic interactions:
- <div className="fixed inset-0 z-50 bg-black/60 flex items-center justify-center p-4"> + <div + className="fixed inset-0 z-50 bg-black/60 flex items-center justify-center p-4" + role="dialog" + aria-modal="true" + aria-labelledby="settings-dialog-title" + onKeyDown={(e) => e.key === "Escape" && handleClose()} + tabIndex={-1} + onClick={handleClose} + > - <div className="relative z-10 w-[90vw] max-w-[500px] max-h-[90vh] overflow-y-auto rounded-[12px] p-6 shadow-xl focus:outline-none"> + <div + className="relative z-10 w-[90vw] max-w-[500px] max-h-[90vh] overflow-y-auto rounded-[12px] p-6 shadow-xl focus:outline-none" + onClick={(e) => e.stopPropagation()} + > @@ - <div className="mb-6"> - <h2 className="text-[20px] font-medium text-white-400 tracking-tight font-sans"> + <div className="mb-6"> + <h2 + id="settings-dialog-title" + className="text-[20px] font-medium text-white-400 tracking-tight font-sans" + > View Style </h2> </div> @@ - <div className="mt-6 flex justify-end"> + <div className="mt-6 flex justify-end"> <button type="button" onClick={handleClose} className={cn(buttonVariants({ variant: "link" }))} aria-label="Cancel" + autoFocus > Cancel </button> </div>If you want, I can refactor this to use the shared for mobile (full-screen variant) so focus trapping, aria, and Escape handling are handled for you.
Also applies to: 59-136, 137-146
🧹 Nitpick comments (8)
apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsx (2)
53-57: Reinstate a close (X) control in headers for both mobile and desktopPer the UX feedback in the PR comments, removing “Continue” is good, but users still expect an explicit close affordance in the header. Add a close button (X) next to the title.
Apply this patch to add a close button (mobile):
- <div className="mb-6"> - <h2 - id="settings-dialog-title" - className="text-[20px] font-medium text-white-400 tracking-tight font-sans" - > - View Style - </h2> - </div> + <div className="mb-6 flex items-center justify-between"> + <h2 + id="settings-dialog-title" + className="text-[20px] font-medium text-white-400 tracking-tight font-sans" + > + View Style + </h2> + <button + type="button" + onClick={handleClose} + aria-label="Close" + className="inline-flex items-center justify-center rounded-md p-1 text-white-400 hover:text-white-200 hover:bg-white/10" + > + <X className="h-4 w-4" /> + </button> + </div>And similarly for desktop:
- <div className="mb-6"> - <DialogTitle className="text-[20px] font-medium text-white-400 tracking-tight font-sans"> - View Style - </DialogTitle> - </div> + <div className="mb-6 flex items-center justify-between"> + <DialogTitle className="text-[20px] font-medium text-white-400 tracking-tight font-sans"> + View Style + </DialogTitle> + <button + type="button" + onClick={handleClose} + aria-label="Close" + className="inline-flex items-center justify-center rounded-md p-1 text-white-400 hover:text-white-200 hover:bg-white/10" + > + <X className="h-4 w-4" /> + </button> + </div>Add the missing import:
import { X } from "lucide-react";Confirm this aligns with shige’s suggestion (close-only, no Continue). I can also wire this to your design system’s DialogClose if available.
Also applies to: 157-161
62-71: Associate section label with its radio group for better a11yTie the “Display Type” label to the RadioGroup via aria-labelledby so screen readers announce the group properly.
Apply for mobile:
- <Label className="text-white-800 font-medium text-[12px] leading-[20.4px] font-geist"> + <Label id="mobile-display-type-label" className="text-white-800 font-medium text-[12px] leading-[20.4px] font-geist"> Display Type </Label> - <RadioGroup + <RadioGroup + aria-labelledby="mobile-display-type-label" value={isCarouselView ? "carousel" : "list"} onValueChange={(value) => setIsCarouselView(value === "carousel") }Apply for desktop:
- <Label className="text-white-800 font-medium text-[12px] leading-[20.4px] font-geist"> + <Label id="desktop-display-type-label" className="text-white-800 font-medium text-[12px] leading-[20.4px] font-geist"> Display Type </Label> - <RadioGroup + <RadioGroup + aria-labelledby="desktop-display-type-label" value={isCarouselView ? "carousel" : "list"} onValueChange={(value) => setIsCarouselView(value === "carousel")}Also applies to: 165-171
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (6)
93-116: Add an accessible name to the trigger button (especially when collapsed)When variant is "collapsed", the trigger shows only an avatar—no text. Provide an aria-label so screen readers announce what this button does.
Apply:
- <button - className="w-full hover:bg-ghost-element-hover h-full rounded-md cursor-pointer outline-none p-1.5 flex items-center gap-2" - type="button" - > + <button + className="w-full hover:bg-ghost-element-hover h-full rounded-md cursor-pointer outline-none p-1.5 flex items-center gap-2" + type="button" + aria-label={ + variant === "expanded" + ? "Open user menu" + : `Open user menu for ${user.displayName ?? user.email ?? "account"}` + } + >
170-178: Use rel="noopener noreferrer" for external linksYou already have target="_blank" with rel="noopener". Add “noreferrer” for privacy and to cover older browsers.
Apply:
- <a + <a href={item.href} target="_blank" - rel="noopener" + rel="noopener noreferrer" className="w-full flex items-center justify-between" > @@ - <a + <a href="https://giselles.ai" target="_blank" - rel="noopener" + rel="noopener noreferrer" className="w-full flex items-center justify-between" >Also applies to: 203-211
217-220: Avoid nested interactive elements inside a menu itemDropdownMenuPrimitive.Item renders a focusable element. Nesting a button inside can create conflicting interaction semantics. Use
asChildand move classes to the child.Apply:
- <DropdownMenuPrimitive.Item className={MENU_ITEM_CLASS}> - <SignOutButton className="text-[14px]">Log out</SignOutButton> - </DropdownMenuPrimitive.Item> + <DropdownMenuPrimitive.Item asChild> + <SignOutButton className={cn(MENU_ITEM_CLASS, "text-[14px]")}> + Log out + </SignOutButton> + </DropdownMenuPrimitive.Item>
70-81: Preserve pathname (and avoid dropping fragments) when updating the query paramPushing only
?querycan drop the URL hash and is a bit implicit. Prefer composing with pathname for clarity and stability.Apply:
-import { useRouter, useSearchParams } from "next/navigation"; +import { usePathname, useRouter, useSearchParams } from "next/navigation"; @@ - const router = useRouter(); - const searchParams = useSearchParams(); + const router = useRouter(); + const pathname = usePathname(); + const searchParams = useSearchParams(); @@ - router.push(`?${params.toString()}`, { scroll: false }); + router.push(`${pathname}?${params.toString()}`, { scroll: false });Optional (if you want to retain hash as well):
// after computing params const hash = typeof window !== "undefined" ? window.location.hash : ""; router.push(`${pathname}?${params.toString()}${hash}`, { scroll: false });Also applies to: 14-15, 65-67
53-55: Keyboard highlight state for menu itemsConsider styling the Radix-highlighted state for keyboard users in addition to hover.
Apply:
-const MENU_ITEM_CLASS = - "text-text outline-none cursor-pointer hover:bg-ghost-element-hover rounded-[4px] px-[8px] py-[6px] text-[14px]"; +const MENU_ITEM_CLASS = + "text-text outline-none cursor-pointer hover:bg-ghost-element-hover data-[highlighted]:bg-ghost-element-hover rounded-[4px] px-[8px] py-[6px] text-[14px]";
107-114: Guard empty planName to avoid an empty second lineWhen planName is undefined, the second line renders empty space. Only show the plan line if present.
Apply:
- <div className="flex flex-col min-w-0 flex-1 text-left"> + <div className="flex flex-col min-w-0 flex-1 text-left"> <p className="truncate text-text-muted text-sm"> {user.displayName ?? user.email} </p> - <p className="truncate text-[var(--color-stage-accent)] text-[10px]"> - {user.planName} - </p> + {user.planName ? ( + <p className="truncate text-[var(--color-stage-accent)] text-[10px]"> + {user.planName} + </p> + ) : null} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsx(4 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.{ts,tsx}: Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoidanytypes
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames (e.g.,user-profile.ts)
Use camelCase for variables, functions, and methods (e.g.,userEmail)
Use prefixes likeis,has,can,shouldfor boolean variables and functions for clarity
Use verbs or verb phrases that clearly indicate purpose for function naming (e.g.,calculateTotalPrice(), notprocess())If breaking changes are introduced in new AI SDK versions, update code to accommodate those changes
Files:
apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.tsx: Use functional components with React hooks
Use PascalCase for React components and classes (e.g.,UserProfile)
Files:
apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
All filenames should use kebab-case (lowercase with hyphens)
Files:
apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
**/*.{js,jsx,ts,tsx}: React components and classes should use PascalCase
Variables, functions, and methods should use camelCase
Use verbs or verb phrases for function names; names should clearly indicate what the function does; avoid ambiguous names that could lead to misuse
Use nouns or noun phrases for variable names; names should describe what the variable represents; avoid single-letter variables except in very short scopes
Use prefixes like 'is', 'has', 'can', 'should' for both variables and functions returning boolean values; make the true/false meaning clear
Files:
apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsxapps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
🧬 Code graph analysis (1)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (3)
internal-packages/ui/components/popover.tsx (1)
PopoverContent(4-15)internal-packages/ui/components/dialog.tsx (1)
DialogFooter(59-69)apps/studio.giselles.ai/lib/utils.ts (1)
cn(4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsx (1)
246-255: Cancel-only footer matches the new URL-synced behavior—LGTMRemoving “Continue” is consistent with immediate application on radio change and reduces confusion. The sticky DialogFooter keeps the affordance visible.
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (3)
84-91: Open-state gating between dropdown and dialog is solidDisabling the dropdown while the dialog is open avoids focus and layering conflicts; this is a clean solution.
121-126: z-index bump on menu content fixes stacking—LGTMUsing z-50 on Content addresses the reported stacking issue over form containers and aligns with the PR objective.
63-63: Verified: Project uses React 19.1.1 and Next 15.3.4, which supportuse()in Client Components
- The pnpm lockfile shows React v19.1.1 and React-DOM v19.1.1.
- Next.js is pinned at v15.3.4.
- React 19 includes the new
use()hook (allowing reading promises or contexts in client components) and integrates with Suspense and error boundaries (blog.saeloun.com)No further action required—keeping
const user = use(userPromise);in a Client Component is safe with these versions.
…dialogs - Removed Cancel button from DialogFooter in both dialogs - Kept X close button in dialog header for closing - Applied changes to both settings-dialog.tsx and navigation-rail-footer-menu.tsx 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…tent - Changed dialog overlay z-index from z-50 to z-[9999] - Changed dialog content z-index from z-50 to z-[10000] - Applied to both shared dialog component and mobile settings dialog - Ensures dialogs appear above all other page elements 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (1)
1-1: Client directive added — good catch and resolutionThe "use client" directive addresses the prior build-time issue flagged in earlier review comments. No further action needed here.
🧹 Nitpick comments (6)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (6)
93-116: Add an accessible name to the trigger buttonIn collapsed mode the trigger shows only an avatar, so screen readers have no accessible name. Add aria-label to the button to improve a11y.
- <button + <button + aria-label="Open user menu" className="w-full hover:bg-ghost-element-hover h-full rounded-md cursor-pointer outline-none p-1.5 flex items-center gap-2" type="button" >
163-185: Harden external links, avoid blank tab on mailto
- Use rel="noopener noreferrer" for external links to prevent reverse-tabnabbing and avoid leaking referrer.
- For mailto links, opening a new tab is undesirable; render without target/rel.
- {item.external ? ( + {item.external ? ( <a href={item.href} - target="_blank" - rel="noopener" + target={item.href.startsWith("mailto:") ? undefined : "_blank"} + rel={item.href.startsWith("mailto:") ? undefined : "noopener noreferrer"} className="w-full flex items-center justify-between" >
202-212: Add noreferrer to Homepage external linkSmall security/privacy tweak for consistency with the Help items.
<a href="https://giselles.ai" target="_blank" - rel="noopener" + rel="noopener noreferrer" className="w-full flex items-center justify-between" >
218-220: Avoid nested interactive elements inside Radix ItemWrap SignOutButton with asChild to delegate focus/keyboard handling to your button and avoid nested interactive semantics.
- <DropdownMenuPrimitive.Item className={MENU_ITEM_CLASS}> - <SignOutButton className="text-[14px]">Log out</SignOutButton> - </DropdownMenuPrimitive.Item> + <DropdownMenuPrimitive.Item className={MENU_ITEM_CLASS} asChild> + <SignOutButton className="w-full text-[14px]">Log out</SignOutButton> + </DropdownMenuPrimitive.Item>
232-238: Add an accessible name to the dialog close buttonScreen readers should announce the intent of the X button.
- <button + <button type="button" + aria-label="Close dialog" onClick={() => setIsDisplayDialogOpen(false)} className="p-1 rounded-lg hover:bg-white/10 transition-colors" >
70-81: Wrap router.push in a transition to reduce perceived latencyUpdating search params triggers a navigation and can feel janky. Mark the update as a transition so React keeps the UI responsive while the route refreshes. This may address the “takes a while to load” UX comment without changing your URL-driven state design.
-const [isDisplayDialogOpen, setIsDisplayDialogOpen] = useState(false); -const [dropdownOpen, setDropdownOpen] = useState(false); +const [isDisplayDialogOpen, setIsDisplayDialogOpen] = useState(false); +const [dropdownOpen, setDropdownOpen] = useState(false); +const [isPending, startTransition] = useTransition();- router.push(`?${params.toString()}`, { scroll: false }); + startTransition(() => { + router.push(`?${params.toString()}`, { scroll: false }); + });And add the import:
-import { use, useCallback, useState } from "react"; +import { use, useCallback, useState, useTransition } from "react";Also applies to: 67-69, 15-15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsx(0 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.{ts,tsx}: Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoidanytypes
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames (e.g.,user-profile.ts)
Use camelCase for variables, functions, and methods (e.g.,userEmail)
Use prefixes likeis,has,can,shouldfor boolean variables and functions for clarity
Use verbs or verb phrases that clearly indicate purpose for function naming (e.g.,calculateTotalPrice(), notprocess())If breaking changes are introduced in new AI SDK versions, update code to accommodate those changes
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.tsx: Use functional components with React hooks
Use PascalCase for React components and classes (e.g.,UserProfile)
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
All filenames should use kebab-case (lowercase with hyphens)
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
**/*.{js,jsx,ts,tsx}: React components and classes should use PascalCase
Variables, functions, and methods should use camelCase
Use verbs or verb phrases for function names; names should clearly indicate what the function does; avoid ambiguous names that could lead to misuse
Use nouns or noun phrases for variable names; names should describe what the variable represents; avoid single-letter variables except in very short scopes
Use prefixes like 'is', 'has', 'can', 'should' for both variables and functions returning boolean values; make the true/false meaning clear
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
🧬 Code graph analysis (1)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (1)
internal-packages/ui/components/popover.tsx (1)
PopoverContent(4-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: check
🔇 Additional comments (6)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (6)
30-55: Good extraction and reuse of menu constantsPulling HELP_ITEMS and MENU_ITEM_CLASS to module scope with as const improves readability, type safety, and consistency across the menu. Nice cleanup.
119-126: z-index bump resolves stacking conflictsSwitching the dropdown content to z-50 is the right fix for the overlap with form containers. Verified alignment logic per variant looks sane.
138-148: Nice interplay: closing dropdown before opening dialogExplicitly closing the menu before opening the Display Type dialog prevents focus traps and interaction conflicts. Solid UX detail.
241-304: URL-backed View Style selection looks correct
- Radio reflects isCarouselView.
- onValueChange updates the URL immediately via setIsCarouselView.
- Border highlight logic matches the current selection.
This achieves the PR goal of synchronizing the dialog with main content and browser history.
226-239: Note on product decision: dialog close controlThe code currently includes an explicit X close button, which differs from the PR thread that mentioned removing the X. If keeping the X is intentional after reconsideration, ignore this; otherwise confirm alignment with the latest UX decision.
63-67: Action Required: Verify React Version and Suspense Boundary
- apps/studio.giselles.ai/package.json
– React dependency resolves to “catalog:” (no concrete version)- apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
– Uses"use client"anduse(userPromise)but no<Suspense>wrapper was detected around this component or its consumersPlease confirm that your workspace is running React 19 or newer (required for
use()in Client Components) and that a<Suspense>boundary surrounds the tree where<NavigationRailFooterMenu>is rendered.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal-packages/ui/components/dialog.tsx (2)
21-22: Optional: use viewport-relative max-heights for more predictable sizingPercentages on a fixed-position element are relative to the viewport in most cases, but
vhcommunicates intent more clearly and avoids surprises in nested contexts.- "data-[size=default]:w-[500px] data-[size=default]:max-h-[75%]", - "data-[size=wide]:w-[800px] data-[size=wide]:max-h-[85%]", + "data-[size=default]:w-[500px] data-[size=default]:max-h-[75vh]", + "data-[size=wide]:w-[800px] data-[size=wide]:max-h-[85vh]",
16-16: Centralize z-index layering to replace magic valuesWe ran an inventory of z-index usages across TS/TSX/CSS and found numerous hard-coded values—e.g.
z-0,z-10,z-50,z-[9999],z-[10000], and evenz-2147483647—leading to inconsistent stacking and potential conflicts (see script output).Rather than bumping to
z-[9999]for one overlay, it’s safer to define a shared set of z-index tokens in your Tailwind config so every layer is predictable:• In
tailwind.config.jsundertheme.extend.zIndex:module.exports = { theme: { extend: { zIndex: { navigation: 50, popover: 60, dropdown: 70, 'dialog-overlay': 80, dialog: 90, } } } }• Update the dialog component (internal-packages/ui/components/dialog.tsx):
- <DialogPrimitive.Overlay className="fixed inset-0 bg-black/60 z-[9999]" /> + <DialogPrimitive.Overlay className="fixed inset-0 bg-black/60 z-dialog-overlay" /> - <DialogPrimitive.Content - className="fixed left-[50%] top-[50%] … z-[10000] overflow-y-auto …" - > + <DialogPrimitive.Content + className="fixed left-[50%] top-[50%] … z-dialog overflow-y-auto …" + >• (Optional) If you prefer CSS variables, you can mirror these tokens with
--z-dialog-overlayand--z-dialog, but Tailwind theme tokens integrate more naturally with existing classnames.By centralizing z-index values, you’ll prevent future “stacking wars” (e.g. tooltips vs. dropdowns vs. modals) and make it trivial to adjust layering globally. I’m happy to help migrate this across the codebase or open a follow-up PR—just let me know!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsx(1 hunks)internal-packages/ui/components/dialog.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.{ts,tsx}: Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoidanytypes
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames (e.g.,user-profile.ts)
Use camelCase for variables, functions, and methods (e.g.,userEmail)
Use prefixes likeis,has,can,shouldfor boolean variables and functions for clarity
Use verbs or verb phrases that clearly indicate purpose for function naming (e.g.,calculateTotalPrice(), notprocess())If breaking changes are introduced in new AI SDK versions, update code to accommodate those changes
Files:
internal-packages/ui/components/dialog.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.tsx: Use functional components with React hooks
Use PascalCase for React components and classes (e.g.,UserProfile)
Files:
internal-packages/ui/components/dialog.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
All filenames should use kebab-case (lowercase with hyphens)
Files:
internal-packages/ui/components/dialog.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
**/*.{js,jsx,ts,tsx}: React components and classes should use PascalCase
Variables, functions, and methods should use camelCase
Use verbs or verb phrases for function names; names should clearly indicate what the function does; avoid ambiguous names that could lead to misuse
Use nouns or noun phrases for variable names; names should describe what the variable represents; avoid single-letter variables except in very short scopes
Use prefixes like 'is', 'has', 'can', 'should' for both variables and functions returning boolean values; make the true/false meaning clear
Files:
internal-packages/ui/components/dialog.tsx
| "fixed left-[50%] top-[50%] translate-y-[-50%] translate-x-[-50%] z-[10000] overflow-y-auto overflow-x-hidden outline-none", | ||
| "data-[size=default]:w-[500px] data-[size=default]:max-h-[75%]", | ||
| "data-[size=wide]:w-[800px] data-[size=default]:max-h-[85%]", |
There was a problem hiding this comment.
Bug: data-[size=default] mistakenly used twice; wide max-height is never applied
On Line 22, data-[size=default]:max-h-[85%] looks like a typo. It overrides the default’s earlier max-h-[75%] and leaves the wide variant without its intended max-h. This will change the default dialog’s height and make wide behave the same as default.
Apply this diff to fix the variant:
- "data-[size=default]:w-[500px] data-[size=default]:max-h-[75%]",
- "data-[size=wide]:w-[800px] data-[size=default]:max-h-[85%]",
+ "data-[size=default]:w-[500px] data-[size=default]:max-h-[75%]",
+ "data-[size=wide]:w-[800px] data-[size=wide]:max-h-[85%]",📝 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.
| "fixed left-[50%] top-[50%] translate-y-[-50%] translate-x-[-50%] z-[10000] overflow-y-auto overflow-x-hidden outline-none", | |
| "data-[size=default]:w-[500px] data-[size=default]:max-h-[75%]", | |
| "data-[size=wide]:w-[800px] data-[size=default]:max-h-[85%]", | |
| "fixed left-[50%] top-[50%] translate-y-[-50%] translate-x-[-50%] z-[10000] overflow-y-auto overflow-x-hidden outline-none", | |
| "data-[size=default]:w-[500px] data-[size=default]:max-h-[75%]", | |
| "data-[size=wide]:w-[800px] data-[size=wide]:max-h-[85%]", |
🤖 Prompt for AI Agents
In internal-packages/ui/components/dialog.tsx around lines 20 to 22, the
tailwind variant on line 22 mistakenly uses data-[size=default]:max-h-[85%]
which overrides the earlier default max-h and prevents the wide variant from
getting its intended max-h; change that token to data-[size=wide]:max-h-[85%] so
default keeps max-h-[75%] and wide gets max-h-[85%].
- Removed unused DialogFooter imports from both dialog files - Removed unused cn utility and buttonVariants imports - Fixed formatting by removing extra blank lines - No design changes, only linting fixes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (1)
53-60: Avoid suspending the entire footer on userPromise; moveuse(userPromise)under Suspense to fix slow load UXCalling
use(userPromise)at the top-level suspends the whole menu until user resolves, which aligns with the “slow load/bad UX” comment. Wrap only the trigger’s user-dependent UI in a Suspense boundary and resolve the promise inside that subtree. This keeps the rest (menu/open-state, z-index, etc.) interactive immediately.Apply these diffs:
-import { use, useCallback, useState } from "react"; +import { Suspense, use, useCallback, useState } from "react";- const user = use(userPromise);- <DropdownMenuPrimitive.Trigger asChild> - <button - className="w-full hover:bg-ghost-element-hover h-full rounded-md cursor-pointer outline-none p-1.5 flex items-center gap-2" - type="button" - > - <div className="size-8 flex items-center justify-center shrink-0"> - <AvatarImage - className="rounded-full" - avatarUrl={user.avatarUrl ?? null} - width={24} - height={24} - alt={user.displayName || user.email || "User"} - /> - </div> - {variant === "expanded" && ( - <div className="flex flex-col min-w-0 flex-1 text-left"> - <p className="truncate text-text-muted text-sm"> - {user.displayName ?? user.email} - </p> - <p className="truncate text-[var(--color-stage-accent)] text-[10px]"> - {user.planName} - </p> - </div> - )} - </button> - </DropdownMenuPrimitive.Trigger> + <DropdownMenuPrimitive.Trigger asChild> + <Suspense + fallback={ + <button + className="w-full hover:bg-ghost-element-hover h-full rounded-md cursor-pointer outline-none p-1.5 flex items-center gap-2" + type="button" + > + <div className="size-8 rounded-full bg-white/10 animate-pulse" /> + {variant === "expanded" && ( + <div className="flex flex-col min-w-0 flex-1 text-left"> + <div className="h-4 w-24 bg-white/10 rounded animate-pulse mb-1" /> + <div className="h-3 w-12 bg-white/10 rounded animate-pulse" /> + </div> + )} + </button> + } + > + <UserTrigger userPromise={userPromise} variant={variant} /> + </Suspense> + </DropdownMenuPrimitive.Trigger>Add this small internal component (anywhere in the file, e.g., below the export):
function UserTrigger({ userPromise, variant, }: { userPromise: Promise<UserDataForNavigationRail>; variant: NavigationRailState; }) { const user = use(userPromise); return ( <button className="w-full hover:bg-ghost-element-hover h-full rounded-md cursor-pointer outline-none p-1.5 flex items-center gap-2" type="button" > <div className="size-8 flex items-center justify-center shrink-0"> <AvatarImage className="rounded-full" avatarUrl={user.avatarUrl ?? null} width={24} height={24} alt={user.displayName || user.email || "User"} /> </div> {variant === "expanded" && ( <div className="flex flex-col min-w-0 flex-1 text-left"> <p className="truncate text-text-muted text-sm"> {user.displayName ?? user.email} </p> {user.planName ? ( <p className="truncate text-[var(--color-stage-accent)] text-[10px]"> {user.planName} </p> ) : null} </div> )} </button> ); }Also applies to: 90-114
♻️ Duplicate comments (1)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (1)
1-1: Good catch: Client Component directive is presentThe prior issue is resolved; the file correctly opts into client mode.
🧹 Nitpick comments (5)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (5)
27-52: Nice extraction of constants; minor naming nitPulling HELP_ITEMS and MENU_ITEM_CLASS up improves readability and reuse. Small naming nit: per our guidelines, variables should use camelCase. Consider renaming MENU_ITEM_CLASS -> menuItemClass for consistency.
Apply this minimal change and update references:
-const MENU_ITEM_CLASS = +const menuItemClass = "text-text outline-none cursor-pointer hover:bg-ghost-element-hover rounded-[4px] px-[8px] py-[6px] text-[14px]";Example usage updates:
- className={`${MENU_ITEM_CLASS} flex items-center justify-between`} + className={`${menuItemClass} flex items-center justify-between`}
67-77: Avoid pushing a bare “?” when removing the view param; include pathnameWhen params become empty,
router.push(\?${params}`)` yields the path with a trailing “?”. Use pathname to push a clean URL and preserve any future hash.-import { useRouter, useSearchParams } from "next/navigation"; +import { usePathname, useRouter, useSearchParams } from "next/navigation";- router.push(`?${params.toString()}`, { scroll: false }); + const pathname = usePathname(); + const qs = params.toString(); + router.push(qs ? `${pathname}?${qs}` : pathname, { scroll: false });
167-176: Harden external links: addnoreferrerFor target="_blank" links, include
rel="noopener noreferrer"to prevent referrer leakage and strengthen security.- rel="noopener" + rel="noopener noreferrer"Also applies to: 199-209
215-217: Avoid interactive-in-interactive; preferasChildor handle viaonSelectWrapping a button inside a Radix Menu.Item (role=menuitem) can produce nested interactive semantics. Prefer making the Item adopt the button via
asChild, provided SignOutButton forwards refs and passes props, or handle the action from the Item’sonSelect.Option A (preferred, if SignOutButton supports it):
-<DropdownMenuPrimitive.Item className={MENU_ITEM_CLASS}> - <SignOutButton className="text-[14px]">Log out</SignOutButton> -</DropdownMenuPrimitive.Item> +<DropdownMenuPrimitive.Item className={menuItemClass} asChild> + <SignOutButton className="text-[14px]">Log out</SignOutButton> +</DropdownMenuPrimitive.Item>Option B (fallback):
<DropdownMenuPrimitive.Item className={menuItemClass} onSelect={(e) => { e.preventDefault(); // call sign-out here or expose a callback prop from SignOutButton }} > Log out </DropdownMenuPrimitive.Item>
303-318: Disabled Font selector: add a short hint or tooltip?Minor UX polish: consider a helper text or title attribute explaining “Coming Soon” so keyboard/AT users get feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsx(1 hunks)apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/studio.giselles.ai/app/stage/(top)/settings-dialog.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.{ts,tsx}: Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoidanytypes
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames (e.g.,user-profile.ts)
Use camelCase for variables, functions, and methods (e.g.,userEmail)
Use prefixes likeis,has,can,shouldfor boolean variables and functions for clarity
Use verbs or verb phrases that clearly indicate purpose for function naming (e.g.,calculateTotalPrice(), notprocess())If breaking changes are introduced in new AI SDK versions, update code to accommodate those changes
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.tsx: Use functional components with React hooks
Use PascalCase for React components and classes (e.g.,UserProfile)
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
All filenames should use kebab-case (lowercase with hyphens)
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
**/*.{js,jsx,ts,tsx}: React components and classes should use PascalCase
Variables, functions, and methods should use camelCase
Use verbs or verb phrases for function names; names should clearly indicate what the function does; avoid ambiguous names that could lead to misuse
Use nouns or noun phrases for variable names; names should describe what the variable represents; avoid single-letter variables except in very short scopes
Use prefixes like 'is', 'has', 'can', 'should' for both variables and functions returning boolean values; make the true/false meaning clear
Files:
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx
🧬 Code graph analysis (1)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (1)
internal-packages/ui/components/popover.tsx (1)
PopoverContent(4-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx (5)
81-88: Dropdown/Dialog coordination looks solidGuarding open state with
!isDisplayDialogOpenprevents focus/interaction conflicts between the menu and dialog.
118-123: Layering and submenu width addressedRaising z-index to 50 and constraining Help submenu to 180px should resolve prior overlay and spacing issues.
Also applies to: 156-158
225-236: Inconsistent with PR comments: Close (X) still present in the dialog headerPR thread says both “Continue” and “Close (X)” were removed. The X button is still rendered. If you intend to remove manual close, rely on outside click/Esc via onOpenChange.
- <div className="flex items-center justify-between mb-6"> - <DialogTitle className="text-[20px] font-medium text-white-400 tracking-tight font-sans"> - View Style - </DialogTitle> - <button - type="button" - onClick={() => setIsDisplayDialogOpen(false)} - className="p-1 rounded-lg hover:bg-white/10 transition-colors" - > - <X className="w-5 h-5 text-white-400" /> - </button> - </div> + <div className="flex items-center justify-between mb-6"> + <DialogTitle className="text-[20px] font-medium text-white-400 tracking-tight font-sans"> + View Style + </DialogTitle> + </div>
243-301: URL-backed View Style selection LGTMRadioGroup reflects the
viewparam and updates it immediately. Borders toggle correctly and removal of action buttons simplifies UX.
12-14: Verify React 19+ & Next.js compatibility foruse()in Client ComponentsThe app’s
package.jsonunderapps/studio.giselles.aiusescatalog:placeholders for both React and Next.js versions, so the actual installed versions aren’t visible here. The newuse()hook in Client Components is only supported starting in React 19+ and requires a matching Next.js version (e.g. Next 14+). Please confirm your effective versions by inspecting the lockfile (e.g.pnpm-lock.yaml) or running:pnpm list react nextIf you’re not on React 19+ and a compatible Next.js release, these imports will break at runtime.
Review all occurrences of
use()imports in your Client Components and adjust as needed:
apps/studio.giselles.ai/app/(main)/apps/page.tsx(line 4):
import { Suspense, use } from "react";apps/studio.giselles.ai/app/stage/acts/[actId]/ui/sidebar.tsx(line 23):
import { use, useCallback, useEffect, useState } from "react";apps/studio.giselles.ai/app/stage/ui/navigation-rail/navigation-rail-footer-menu.tsx(line 14):
import { use, useCallback, useState } from "react";If necessary, upgrade React/Next or refactor these hooks out until you’re on a supported version.
There was a problem hiding this comment.
Note
I've also written here the details of the synchronous communication.
Thank you for implementing various features into the Sidebar (Navigation Rail)! The Stage top page with the Sidebar requires quite complex data streaming and state management, and the current implementation is causing unintended delays, so I will rewrite the UI implemented in this pull request to make it work. I will create a new pull request, and close this one once it's merged.
User description
Summary
Improved navigation rail sidebar functionality with z-index fixes, external link indicators, working display type synchronization, and code maintainability enhancements. The sidebar menu now properly displays above other UI elements and provides seamless display type switching between list and carousel views.
Related Issue
N/A - UI/UX improvements and bug fixes based on user feedback
Changes
z-10toz-50to display above form containers?view=carousel)HELP_ITEMSconstant to module levelMENU_ITEM_CLASSconstant to reduce CSS duplication across 6 usage pointsas constassertionsTesting
pnpm test)pnpm check-types)pnpm build-sdk)pnpm tidy)/stage?view=carousel)Other Information
PR Type
Enhancement
Description
Implement URL-based display type synchronization between sidebar and main content
Add user plan information display in navigation rail
Replace navigation icons with custom chevron design
Enhance dropdown menu with external link indicators and improved styling
Diagram Walkthrough
File Walkthrough
12 files
Add user plan information to sidebar dataAdd planName field to user data interfaceAdd CSS variables for stage sidebar stylingReplace local state with URL-based display type managementUpdate background color to use CSS variableUpdate loading page background colorsUpdate button styling with CSS variablesUpdate navigation item text colorsReplace panel icon with custom chevronReplace panel icon and update text stylingComplete dropdown menu rewrite with display type dialogCreate custom chevron icons for navigationSummary by CodeRabbit
New Features
Style
UI