refactor: DirectoryPicker -- vertical list layout instead of card grid (OPE-117)#275
Merged
DevanshuNEU merged 7 commits intoMar 2, 2026
Conversation
… (OPE-117) Card grid was hard to scan with 25+ packages -- cards wrapped unpredictably, proportional sizing added visual noise. New layout: clean vertical list, one row per package. - Checkbox on left, package name, file count, function estimate - Selected rows highlighted with bg-primary/5 + bold name - Dividers between rows, hover highlight - Staggered fade-in animation preserved (30ms per row) - Removed maxFiles/proportional sizing logic (not needed) Same props, same behavior, much easier to scan.
|
@DevanshuNEU is attempting to deploy a commit to the Dev's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces DirectoryPicker's card-grid with a sortable vertical row list (per-row checkboxes, SortButton, PackageRow, memoized sortedDirs), removes maxFiles logic; DashboardHome now provides functionLimit from a new useUserUsage React Query hook that fetches user usage/limits from the backend. Changes
Sequence Diagram(s)sequenceDiagram
participant Dashboard as DashboardHome
participant Hook as useUserUsage
participant API as Backend API
participant UI as DirectoryPicker
Dashboard->>Hook: call useUserUsage(apiKey, userId)
Hook->>API: fetch /user/usage (with auth)
API-->>Hook: return usage + limits
Hook-->>Dashboard: provide usage (react-query cache)
Dashboard->>UI: render DirectoryPicker(functionLimit = usage.limits.max_functions_per_repo)
UI->>UI: sort and render `sortedDirs` rows, handle row toggles
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/DirectoryPicker.tsx (1)
221-223: Use consistent numeric formatting for file counts.Line 222 prints raw
file_count, while nearby metrics are localized. Consider applyingtoLocaleString()for consistency/readability.Small consistency patch
- <span className="text-xs text-muted-foreground tabular-nums w-20 text-right"> - {dir.file_count} files - </span> + <span className="text-xs text-muted-foreground tabular-nums w-20 text-right"> + {dir.file_count.toLocaleString()} files + </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/DirectoryPicker.tsx` around lines 221 - 223, The file count display in DirectoryPicker.tsx prints raw dir.file_count; update the JSX that renders the span (the element showing "{dir.file_count} files") to format the number with locale-aware separators (e.g., use dir.file_count.toLocaleString()) so it matches the localized formatting used elsewhere in this component; ensure the call handles undefined/null safely if dir.file_count can be missing and keep the trailing "files" text unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/DirectoryPicker.tsx`:
- Around line 205-215: In PackageRow replace the outer <button> that wraps
Checkbox and text with a non-nested approach: remove the <button> and render a
focusable container (e.g., a <div> with role="button" and tabIndex={0}) that
applies the same className and calls onToggle on click and onKeyDown (handling
Enter/Space) for keyboard activation; ensure the Checkbox
(CheckboxPrimitive.Root / Checkbox component) is not rendered as an interactive
child of that container (make it a sibling or render a non-interactive
visual-only checkbox) and preserve the isSelected-based styling logic so
selection state and accessibility (aria-checked/aria-labels) remain correct.
---
Nitpick comments:
In `@frontend/src/components/DirectoryPicker.tsx`:
- Around line 221-223: The file count display in DirectoryPicker.tsx prints raw
dir.file_count; update the JSX that renders the span (the element showing
"{dir.file_count} files") to format the number with locale-aware separators
(e.g., use dir.file_count.toLocaleString()) so it matches the localized
formatting used elsewhere in this component; ensure the call handles
undefined/null safely if dir.file_count can be missing and keep the trailing
"files" text unchanged.
- Header row: Package, Files, Functions labels above the list - Click any column header to sort by that field - Default: sorted by file count descending (largest first) - Click again to toggle ascending/descending - Active sort column highlighted with ArrowUpDown icon - SortButton component handles toggle logic Makes it easy to find specific packages in repos with 30+ dirs.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/components/DirectoryPicker.tsx (1)
267-290:⚠️ Potential issue | 🟠 MajorFix invalid interactive nesting in
PackageRow.
Checkboxis interactive and is nested inside an interactive row<button>, which is invalid semantics and can cause accessibility issues.Suggested fix
function PackageRow({ dir, isSelected, onToggle, }: { dir: DirectoryEntry isSelected: boolean onToggle: () => void }) { return ( - <button + <div + role="checkbox" + aria-checked={isSelected} + tabIndex={0} onClick={onToggle} + onKeyDown={(e) => { + if (e.key === ' ' || e.key === 'Enter') { + e.preventDefault() + onToggle() + } + }} className={cn( - 'flex items-center gap-3 w-full px-6 py-2.5 text-left transition-colors', + 'flex items-center gap-3 w-full px-6 py-2.5 text-left transition-colors cursor-pointer', isSelected ? 'bg-primary/5' : 'hover:bg-muted/50', )} > - <Checkbox checked={isSelected} tabIndex={-1} className="pointer-events-none" /> + <Checkbox checked={isSelected} aria-hidden tabIndex={-1} className="pointer-events-none" /> <span className={cn( 'text-sm flex-1 truncate', isSelected ? 'text-foreground font-medium' : 'text-muted-foreground', )}> {dir.name} </span> <span className="text-xs text-muted-foreground tabular-nums w-20 text-right"> {dir.file_count} files </span> <span className="text-xs text-muted-foreground tabular-nums w-24 text-right"> ~{dir.estimated_functions.toLocaleString()} fn </span> - </button> + </div> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/DirectoryPicker.tsx` around lines 267 - 290, The row currently nests an interactive Checkbox inside a <button> which is invalid; replace the outer <button> with a non-button container that provides equivalent keyboard/ARIA behavior: change the element to a <div> (or other non-interactive element) with role="button", tabIndex={0}, aria-pressed={isSelected} (or aria-checked if preferred), keep onClick={onToggle} and add an onKeyDown handler that triggers onToggle on Enter/Space so keyboard users can activate it; preserve the existing className usage (cn(...)), keep Checkbox, isSelected and dir usage as-is, and ensure the Checkbox remains interactive (do not set pointer-events-none) or if you keep pointer-events-none instead add an onClick on the Checkbox that stops propagation so clicks on the Checkbox don’t double-trigger onToggle.
🧹 Nitpick comments (1)
frontend/src/components/DirectoryPicker.tsx (1)
240-253: Use shadcnButtonfor sortable headers instead of native<button>.This keeps interaction and styling behavior aligned with the shared UI primitives.
Suggested refactor
function SortButton({ label, sortKey, current, asc, onToggle, className, }: { label: string sortKey: SortKey current: SortKey asc: boolean onToggle: (key: SortKey) => void className?: string }) { const active = current === sortKey return ( - <button + <Button + type="button" + variant="ghost" + size="sm" onClick={() => onToggle(sortKey)} className={cn( - 'flex items-center gap-1 hover:text-foreground transition-colors', + 'h-auto justify-start px-0 py-0 flex items-center gap-1 hover:text-foreground transition-colors', active ? 'text-foreground font-medium' : 'text-muted-foreground', className, )} > {label} {active && ( <ArrowUpDown className="w-3 h-3" /> )} - </button> + </Button> ) }As per coding guidelines "frontend/src/components/**/*.{tsx,ts}: Use shadcn/ui components over custom UI in React components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/DirectoryPicker.tsx` around lines 240 - 253, Replace the native <button> used for the sortable header (the element with onClick={() => onToggle(sortKey)} that renders {label} and <ArrowUpDown /> when active) with the shared shadcn Button component; import Button from your UI primitives, pass the same onClick, className (preserving cn(...) classes and the active conditional), and children (label and conditional <ArrowUpDown />), and ensure any accessibility attributes (e.g., aria-pressed/aria-sort) are preserved or added on the Button; update the import list to include Button and remove the native button usage in the DirectoryPicker sortable header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/DirectoryPicker.tsx`:
- Around line 267-290: The row currently nests an interactive Checkbox inside a
<button> which is invalid; replace the outer <button> with a non-button
container that provides equivalent keyboard/ARIA behavior: change the element to
a <div> (or other non-interactive element) with role="button", tabIndex={0},
aria-pressed={isSelected} (or aria-checked if preferred), keep
onClick={onToggle} and add an onKeyDown handler that triggers onToggle on
Enter/Space so keyboard users can activate it; preserve the existing className
usage (cn(...)), keep Checkbox, isSelected and dir usage as-is, and ensure the
Checkbox remains interactive (do not set pointer-events-none) or if you keep
pointer-events-none instead add an onClick on the Checkbox that stops
propagation so clicks on the Checkbox don’t double-trigger onToggle.
---
Nitpick comments:
In `@frontend/src/components/DirectoryPicker.tsx`:
- Around line 240-253: Replace the native <button> used for the sortable header
(the element with onClick={() => onToggle(sortKey)} that renders {label} and
<ArrowUpDown /> when active) with the shared shadcn Button component; import
Button from your UI primitives, pass the same onClick, className (preserving
cn(...) classes and the active conditional), and children (label and conditional
<ArrowUpDown />), and ensure any accessibility attributes (e.g.,
aria-pressed/aria-sort) are preserved or added on the Button; update the import
list to include Button and remove the native button usage in the DirectoryPicker
sortable header.
- Removed redundant description text (UI is self-explanatory) - Select all + package count on one line (was taking full section) - Column header row tighter (py-1.5, subtle bg-muted/30 background) - Header padding reduced from p-6 to px-6 py-4 - Less vertical space eaten before ScrollArea = scroll actually works
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/components/DirectoryPicker.tsx (1)
238-252: Sort direction is not visually communicated to users.The
SortButtonreceives theascprop but only renders a staticArrowUpDownicon when active. Users cannot tell whether the current sort is ascending or descending. Consider using directional icons likeArrowUp/ArrowDownor rotating the icon.Proposed fix using directional icons
-import { FolderGit2, X, Files, FunctionSquare, ArrowUpDown } from 'lucide-react' +import { FolderGit2, X, Files, FunctionSquare, ArrowUp, ArrowDown } from 'lucide-react'Then in
SortButton:{label} {active && ( - <ArrowUpDown className="w-3 h-3" /> + asc ? <ArrowUp className="w-3 h-3" /> : <ArrowDown className="w-3 h-3" /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/DirectoryPicker.tsx` around lines 238 - 252, The current SortButton (in DirectoryPicker.tsx) always renders a static ArrowUpDown icon when active, so it doesn't convey direction; update the rendering logic in the button that uses props current, sortKey and the asc prop to display a directional icon: if asc is true render ArrowUp (or a rotated ArrowUpDown), otherwise render ArrowDown, and keep the same sizing/className behavior (e.g., replace the conditional block that currently renders ArrowUpDown with a conditional that selects ArrowUp or ArrowDown based on the asc prop); also consider adding an accessible label/aria-label that includes "ascending" or "descending" to reflect the current sort direction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/components/DirectoryPicker.tsx`:
- Around line 238-252: The current SortButton (in DirectoryPicker.tsx) always
renders a static ArrowUpDown icon when active, so it doesn't convey direction;
update the rendering logic in the button that uses props current, sortKey and
the asc prop to display a directional icon: if asc is true render ArrowUp (or a
rotated ArrowUpDown), otherwise render ArrowDown, and keep the same
sizing/className behavior (e.g., replace the conditional block that currently
renders ArrowUpDown with a conditional that selects ArrowUp or ArrowDown based
on the asc prop); also consider adding an accessible label/aria-label that
includes "ascending" or "descending" to reflect the current sort direction.
…OPE-118) New useUserUsage hook calls GET /api/v1/users/usage (React Query, 60s stale). Returns actual max_functions_per_repo for the user's tier from backend. DashboardHome now passes usage?.limits?.max_functions_per_repo to DirectoryPicker instead of TIER_FUNCTION_LIMITS[userTier]. Removed: TIER_FUNCTION_LIMITS import, TierName import, rawTier/userTier derivation from session metadata. All gone from DashboardHome. If user is on Pro (20K limit), budget bar shows X/20,000. If admin bumps them to Enterprise (500K), it updates on next page load. No frontend deploy needed when tier limits change on backend.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/hooks/useCachedQuery.ts (1)
195-211: Keep this hook aligned with the dual-layer cache pattern used in this module.This new hook skips the localStorage layer while other hooks in
useCachedQuery.tsuse React Query + localStorage for warm starts and consistency. Please align it (or document why this endpoint is intentionally memory-only).Based on learnings: Applies to frontend/src/hooks/**/*.ts : Frontend fetches via React Query hooks in
hooks/useCachedQuery.tswith dual-layer caching (React Query + localStorage).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useCachedQuery.ts` around lines 195 - 211, The hook useUserUsage currently skips the module's dual-layer cache (React Query + localStorage); modify useUserUsage to follow the same pattern as other hooks in this file by reading cached JSON from localStorage using the same queryKey (['user','usage']) as initialData or initialDataUpdated, and in the query's onSuccess persist the fetched payload (from fetchWithAuth/API_URL) back to localStorage (serialize safely and handle nulls). Keep enabled: !!apiKey and staleTime: 60_000, and ensure you use the same storage key/versioning convention used elsewhere in this module when referencing localStorage so warm starts are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/hooks/useCachedQuery.ts`:
- Around line 197-210: The query key for the usage query currently uses a static
key ['user','usage'] which can leak previous user's limits; change the queryKey
in useCachedQuery (the object containing queryKey, queryFn, enabled, staleTime)
to include a stable user identifier (e.g. ['user','usage', userId]) and ensure
the call site in DashboardHome passes session?.user?.id (or null/undefined when
unauthenticated) into the hook so fetchWithAuth remains scoped per user and
prevents cross-account cache bleed.
---
Nitpick comments:
In `@frontend/src/hooks/useCachedQuery.ts`:
- Around line 195-211: The hook useUserUsage currently skips the module's
dual-layer cache (React Query + localStorage); modify useUserUsage to follow the
same pattern as other hooks in this file by reading cached JSON from
localStorage using the same queryKey (['user','usage']) as initialData or
initialDataUpdated, and in the query's onSuccess persist the fetched payload
(from fetchWithAuth/API_URL) back to localStorage (serialize safely and handle
nulls). Keep enabled: !!apiKey and staleTime: 60_000, and ensure you use the
same storage key/versioning convention used elsewhere in this module when
referencing localStorage so warm starts are consistent.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/dashboard/DashboardHome.tsxfrontend/src/hooks/useCachedQuery.ts
…e scroll Radix ScrollArea's Viewport uses h-full inside overflow-hidden Root, which fights with flex-1 in a flex column container. The list never scrolled because the Viewport expanded to fit all content. Native div with overflow-y-auto + flex-1 + min-h-0 works correctly. The browser handles scroll natively without Radix's abstraction layer.
flex-1 + min-h-0 + overflow-y-auto was not constraining the list. Replaced with explicit maxHeight: min(400px, 50vh). Simple, works.
1. PackageRow: replaced nested button>checkbox with div[role=checkbox] and visual-only checkbox indicator. Keyboard Enter/Space handled. 2. file_count now uses toLocaleString() matching estimated_functions 3. useUserUsage query key includes userId to prevent cache bleed between users on shared devices
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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)
frontend/src/components/DirectoryPicker.tsx (1)
159-167:⚠️ Potential issue | 🟠 MajorDon’t gate function-limit enforcement on truthiness.
functionLimitchecks currently treat0as “no limit,” which can allow confirmations that should be blocked.Proposed fix
- {functionLimit && ( + {typeof functionLimit === 'number' && functionLimit > 0 && ( <BudgetBar current={stats.functions} limit={functionLimit} /> )} ... - overLimit={!!functionLimit && stats.functions > functionLimit} + overLimit={typeof functionLimit === 'number' && stats.functions > functionLimit}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/DirectoryPicker.tsx` around lines 159 - 167, The code treats functionLimit truthily so a value of 0 is interpreted as "no limit"; update checks to explicitly test for presence (e.g., functionLimit !== undefined/null or typeof functionLimit === "number") before rendering BudgetBar and computing overLimit. Change the BudgetBar conditional (now "functionLimit &&") to an explicit presence check and change the overLimit expression passed to PickerFooter (now "!!functionLimit && stats.functions > functionLimit") to use the same explicit presence check with stats.functions > functionLimit; search for other uses of functionLimit in this component and replace truthy checks with an explicit numeric/presence check to correctly enforce a zero limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/DirectoryPicker.tsx`:
- Around line 239-251: Replace the raw HTML button in DirectoryPicker.tsx with
the shadcn Button component and wire ARIA state: use Button instead of <button>
for the control that currently calls onToggle(sortKey), keep label and
ArrowUpDown icon, add aria-pressed={active} and an aria-label or
aria-describedby that includes the current sort direction (e.g.,
ascending/descending from your sort state) so screen readers get the active
direction, and preserve the existing className handling and onClick handler
(onToggle and sortKey) to maintain behavior.
---
Outside diff comments:
In `@frontend/src/components/DirectoryPicker.tsx`:
- Around line 159-167: The code treats functionLimit truthily so a value of 0 is
interpreted as "no limit"; update checks to explicitly test for presence (e.g.,
functionLimit !== undefined/null or typeof functionLimit === "number") before
rendering BudgetBar and computing overLimit. Change the BudgetBar conditional
(now "functionLimit &&") to an explicit presence check and change the overLimit
expression passed to PickerFooter (now "!!functionLimit && stats.functions >
functionLimit") to use the same explicit presence check with stats.functions >
functionLimit; search for other uses of functionLimit in this component and
replace truthy checks with an explicit numeric/presence check to correctly
enforce a zero limit.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/DirectoryPicker.tsxfrontend/src/components/dashboard/DashboardHome.tsxfrontend/src/hooks/useCachedQuery.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/dashboard/DashboardHome.tsx
- frontend/src/hooks/useCachedQuery.ts
This was referenced Mar 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Replace the card grid in DirectoryPicker with a clean vertical list. One row per package: checkbox, name, file count, function estimate.
Why
Card grid was hard to scan with 25+ packages. Cards wrapped unpredictably, proportional sizing added visual noise. A list is cleaner, faster to read, and matches patterns like the admin user table.
Before (card grid)
Cards of varying sizes wrapping across rows. Hard to compare packages.
After (vertical list)
Clean rows with aligned columns. Checkbox, name, file count, functions. Easy to scan 30+ packages at a glance.
Changes
1 file changed, 26 insertions, 31 deletions. Net -5 lines.
Closes OPE-117
Summary by CodeRabbit
New Features
Accessibility
Style