-
Notifications
You must be signed in to change notification settings - Fork 194
fix(web): Improve /repos page performance #677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis PR refactors the repositories listing page to implement URL-driven pagination and search, introduces a new InputGroup UI component system, updates repository data fetching with filter and pagination support, and restructures test data injection to create per-repo job records with randomized statuses. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/app/[domain]/repos/components/reposTable.tsx (1)
151-156: Typo: "Lastest" should be "Latest".{ accessorKey: "latestJobStatus", size: 150, - header: "Lastest status", + header: "Latest status", cell: ({ row }) => getStatusBadge(row.getValue("latestJobStatus")), },
🧹 Nitpick comments (3)
packages/db/tools/scripts/inject-repo-data.ts (1)
37-91: Inconsistent indentation breaks code style.Lines 37-91 use 4-space indentation while the surrounding code uses 8-space indentation (inside the
runfunction body). This appears to be an accidental de-indent.packages/web/src/app/[domain]/repos/page.tsx (1)
90-98: UsePrisma.QueryModeenum for type safety.The
mode: 'insensitive'string literal works but using the Prisma enum provides better type safety and IDE support:const whereClause: Prisma.RepoWhereInput = { orgId: org.id, ...(search && { OR: [ - { name: { contains: search, mode: 'insensitive' } }, - { displayName: { contains: search, mode: 'insensitive' } } + { name: { contains: search, mode: Prisma.QueryMode.insensitive } }, + { displayName: { contains: search, mode: Prisma.QueryMode.insensitive } } ] }), };packages/web/src/app/[domain]/repos/components/reposTable.tsx (1)
306-326: Cleanup function may prematurely hide the loading indicator.The cleanup function sets
setIsPendingSearch(false)which runs on every re-render before the new effect runs. This could cause a brief flicker where the loader disappears and reappears. Consider only clearing the pending state when the navigation completes:useEffect(() => { setIsPendingSearch(true); const timer = setTimeout(() => { const params = new URLSearchParams(searchParams.toString()); if (searchValue) { params.set('search', searchValue); } else { params.delete('search'); } params.set('page', '1'); router.replace(`${pathname}?${params.toString()}`); setIsPendingSearch(false); }, 300); - return () => { - clearTimeout(timer); - setIsPendingSearch(false); - }; + return () => clearTimeout(timer); // eslint-disable-next-line react-hooks/exhaustive-deps }, [searchValue]);The pending state will be reset when the navigation completes and the component re-renders with new data.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/db/tools/scripts/inject-repo-data.ts(2 hunks)packages/web/src/app/[domain]/repos/components/reposTable.tsx(4 hunks)packages/web/src/app/[domain]/repos/page.tsx(2 hunks)packages/web/src/components/ui/input-group.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/web/src/components/ui/input-group.tsx (4)
packages/web/src/lib/utils.ts (1)
cn(24-26)packages/web/src/components/ui/button.tsx (1)
Button(56-56)packages/web/src/components/ui/input.tsx (1)
Input(22-22)packages/web/src/components/ui/textarea.tsx (1)
Textarea(22-22)
packages/web/src/app/[domain]/repos/page.tsx (5)
packages/web/src/lib/utils.ts (1)
isServiceError(438-444)packages/web/src/lib/serviceError.ts (1)
ServiceErrorException(16-20)packages/web/src/app/[domain]/repos/components/reposTable.tsx (1)
ReposTable(281-481)packages/web/src/actions.ts (1)
sew(44-57)packages/web/src/withAuthV2.ts (1)
withOptionalAuthV2(43-67)
🔇 Additional comments (5)
packages/web/src/app/[domain]/repos/page.tsx (2)
81-86: Thestatusparameter is accepted but not used in filtering.The
statusparameter is passed togetReposWithLatestJobbut thewhereClausedoesn't filter by it. This aligns with the PR TODO "Fix status filtering" but worth noting for tracking.
43-47: TheisFirstTimeIndexlogic looks correct for single-job fetch.The filter on
repo.jobsworks correctly sincetake: 1fetches only the latest job. The logic correctly identifies repos that haven't been indexed yet but have a pending/in-progress job.packages/web/src/components/ui/input-group.tsx (1)
11-37: Well-designed InputGroup wrapper with comprehensive styling.The use of CSS
has-[]selectors for focus/error state propagation and alignment variants is a clean approach that keeps the components loosely coupled while maintaining visual cohesion.packages/web/src/app/[domain]/repos/components/reposTable.tsx (2)
300-304: Nice UX: keyboard shortcut for search focus.The
/hotkey for focusing the search input is a good developer-friendly UX pattern commonly seen in documentation sites and GitHub.
272-288: Clean URL-driven state management.The prop-based approach with
currentPage,pageSize,totalCount,initialSearch, andinitialStatusproperly separates server-provided state from client-side URL manipulation. This aligns well with Next.js App Router patterns.
| function InputGroupInput({ | ||
| className, | ||
| ...props | ||
| }: React.ComponentProps<"input">) { | ||
| return ( | ||
| <Input | ||
| data-slot="input-group-control" | ||
| className={cn( | ||
| "flex-1 h-10 px-3 py-2 text-base md:text-sm rounded-none border-0 bg-transparent shadow-none focus-visible:ring-0 focus-visible:ring-offset-0 dark:bg-transparent placeholder:text-muted-foreground disabled:cursor-not-allowed disabled:opacity-50", | ||
| className | ||
| )} | ||
| {...props} | ||
| /> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Height mismatch between InputGroup and InputGroupInput.
InputGroup sets h-9 (36px) on line 18, but InputGroupInput applies h-10 (40px) on line 139. This could cause the input to overflow the container or create visual inconsistency.
<Input
data-slot="input-group-control"
className={cn(
- "flex-1 h-10 px-3 py-2 text-base md:text-sm rounded-none border-0 bg-transparent shadow-none focus-visible:ring-0 focus-visible:ring-offset-0 dark:bg-transparent placeholder:text-muted-foreground disabled:cursor-not-allowed disabled:opacity-50",
+ "flex-1 h-full px-3 py-2 text-base md:text-sm rounded-none border-0 bg-transparent shadow-none focus-visible:ring-0 focus-visible:ring-offset-0 dark:bg-transparent placeholder:text-muted-foreground disabled:cursor-not-allowed disabled:opacity-50",
className
)}
{...props}
/>📝 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.
| function InputGroupInput({ | |
| className, | |
| ...props | |
| }: React.ComponentProps<"input">) { | |
| return ( | |
| <Input | |
| data-slot="input-group-control" | |
| className={cn( | |
| "flex-1 h-10 px-3 py-2 text-base md:text-sm rounded-none border-0 bg-transparent shadow-none focus-visible:ring-0 focus-visible:ring-offset-0 dark:bg-transparent placeholder:text-muted-foreground disabled:cursor-not-allowed disabled:opacity-50", | |
| className | |
| )} | |
| {...props} | |
| /> | |
| ) | |
| } | |
| function InputGroupInput({ | |
| className, | |
| ...props | |
| }: React.ComponentProps<"input">) { | |
| return ( | |
| <Input | |
| data-slot="input-group-control" | |
| className={cn( | |
| "flex-1 h-full px-3 py-2 text-base md:text-sm rounded-none border-0 bg-transparent shadow-none focus-visible:ring-0 focus-visible:ring-offset-0 dark:bg-transparent placeholder:text-muted-foreground disabled:cursor-not-allowed disabled:opacity-50", | |
| className | |
| )} | |
| {...props} | |
| /> | |
| ) | |
| } |
🤖 Prompt for AI Agents
packages/web/src/components/ui/input-group.tsx around lines 131 to 145: the
input height class h-10 on InputGroupInput conflicts with the parent
InputGroup's h-9 set earlier, causing visual mismatch; change the
InputGroupInput class to use h-9 (or remove the explicit height and let it
inherit the parent's height) so the control matches the container height, and
update any padding or alignment if needed to keep vertical centering consistent.
TODO:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.