Conversation
This commit also fixes broken load more logic
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughUnifies NavItems props into an exported Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Sidebar as AppSidebar
participant Nav as NavItems (Flat/Nested)
participant ProjectHook as useProjectNavigation
participant TRPC as TRPC listProjects
User->>Sidebar: Open sidebar / click "More"
Sidebar->>ProjectHook: request project nav (useInfiniteQuery)
ProjectHook->>TRPC: fetchNextPage / initial pages
TRPC-->>ProjectHook: pages, hasNextPage, nextCursor
ProjectHook-->>Sidebar: project NavItems (with loadMoreAction / #load-more-projects)
Sidebar->>Nav: render items with onLoadMore(item)
alt Click More (projects)
Nav->>Sidebar: onLoadMore(item href:"#load-more-projects")
Sidebar->>ProjectHook: loadMore()
ProjectHook->>TRPC: fetchNextPage()
TRPC-->>ProjectHook: append pages
ProjectHook-->>Sidebar: updated NavItems
Sidebar->>Nav: re-render with NavProps
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ 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 (
|
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
apps/dashboard/lib/trpc/routers/deploy/project/list.ts (4)
113-114: Add a deterministic tie-breaker to orderingWithout a secondary key, pagination boundaries can still be unstable when multiple rows share the same timestamp. Add id as a secondary DESC key to guarantee deterministic ordering across pages.
- orderBy: [desc(schema.projects.updatedAt)], + orderBy: [desc(schema.projects.updatedAt), desc(schema.projects.id)],
46-49: Truthiness check can drop cursor=0 and doesn’t align with fallback logicUse a pure type-check, not a truthiness guard. If you adopt the “fallback to createdAt” approach, consider coalescing in the WHERE as well for consistency.
- if (input.cursor && typeof input.cursor === "number") { - baseConditions.push(lt(schema.projects.updatedAt, input.cursor)); - } + if (typeof input.cursor === "number") { + // If you adopt cursor fallback to createdAt above, consider: + // baseConditions.push(lt(sql`COALESCE(${schema.projects.updatedAt}, ${schema.projects.createdAt})`, input.cursor)); + baseConditions.push(lt(schema.projects.updatedAt, input.cursor)); + }If you decide to coalesce in SQL, import sql from your DB utils and ensure an index exists on (workspace_id, updated_at, id).
106-125: Query performance: consider composite indexes for pagination and route hostname lookupsGiven the WHERE and ORDER BY, consider adding/confirming:
- projects: composite index on (workspace_id, updated_at DESC, id DESC) to support the pagination scan.
- routes: composite index on (workspace_id, project_id, hostname) to accelerate the exists/lookup and the batch hostname fetch.
Also applies to: 139-152
191-197: Error logging: include a safe identifier, avoid logging large error objects in hot pathsUse a lightweight message with a request/workspace identifier to correlate server logs, and log error.stack only at a debug level.
- console.error("Error querying projects:", error); + console.error( + "Error querying projects", + { workspaceId: ctx.workspace.id, userId: ctx.user.id, err: (error as Error)?.message }, + );apps/dashboard/components/navigation/sidebar/workspace-navigations.tsx (1)
16-29: React namespace types used without import; switch to type imports to avoid TS “Cannot find namespace 'React'.”The file references React.ElementType, React.ReactNode, and React.FC but doesn’t import React types. Import them as types (keeps bundle clean) and use the imported names. Also update the NavItem.label/Tag props to use ReactNode consistently.
+import type { ElementType, FC, ReactNode } from "react"; @@ -export type NavItem = { +export type NavItem = { disabled?: boolean; tooltip?: string; - icon: React.ElementType | null; + icon: ElementType | null; href: string; external?: boolean; - label: string | React.ReactNode; + label: string | ReactNode; active?: boolean; - tag?: React.ReactNode; + tag?: ReactNode; hidden?: boolean; items?: NavItem[]; loadMoreAction?: boolean; showSubItems?: boolean; }; @@ -const Tag: React.FC<{ label: string; className?: string }> = ({ label, className }) => ( +const Tag: FC<{ label: string; className?: string }> = ({ label, className }) => (Also applies to: 31-40
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-api-navigation.tsx (2)
20-21: Return undefined when there’s no next page to align with TanStack Query semanticsReturning null works because it’s falsy, but returning undefined is the canonical way and avoids edge-case checks in some helpers.
- getNextPageParam: (lastPage) => lastPage.nextCursor, + getNextPageParam: (lastPage) => lastPage.nextCursor ?? undefined,
92-100: Load-more item looks good; add an a11y label and unify styling across sectionsConsider standardizing the “More” label as a shared component/constant and adding an aria-label for screen readers so “More” is descriptive in context (e.g., “Load more APIs”).
apisItem.items?.push({ - icon: () => null, + icon: () => null, href: "#load-more-apis", - label: <div className="font-normal decoration-dotted underline ">More</div>, + label: <div className="font-normal decoration-dotted underline" aria-label="Load more APIs">More</div>, active: false, loadMoreAction: true, });Optionally centralize this as a shared to reuse styles between APIs, Ratelimits, and Projects.
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-ratelimit-navigation.tsx (2)
93-101: JSX “More” label aligns with widened NavItem.labelConsistent with the APIs and Projects sections. Consider extracting a shared More label component for consistency and a11y as noted in the APIs comment.
19-27: getNextPageParam: prefer undefined when no next pageSame nit as APIs hook.
- getNextPageParam: (lastPage) => lastPage.nextCursor, + getNextPageParam: (lastPage) => lastPage.nextCursor ?? undefined,apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx (3)
153-157: Index sub-item pending state by href, not label.Using label as the key breaks when label is a ReactNode or duplicated. Consider using subItem.href as the record key.
Outside the changed lines, recommended adjustments:
// initialize/use: const [subPending, setSubPending] = useState<Record<string, boolean>>({}); // set: const key = subItem.href ?? `${depth}-${index}`; const updatedPending = { ...subPending, [key]: true }; setSubPending(updatedPending); // read: className={getButtonStyles(subItem.active, subPending[key])} // reset: setTimeout(() => { setSubPending((prev) => ({ ...prev, [key]: false })); }, 300);
195-201: Chevron rotation should reflect actual open state for sub-items.When isSubItem is true, Collapsible uses isChildrenOpen, but the chevron rotates based on isOpen, causing mismatch.
Outside the changed lines, recommended adjustment:
<CaretRight className={cn( "transition-transform duration-200 text-gray-9 !w-[9px] !h-[9px]", - isOpen ? "rotate-90" : "rotate-0", + (isSubItem ? isChildrenOpen : isOpen) ? "rotate-90" : "rotate-0", )} size="sm-bold" />
120-141: Avoid setTimeout for resetting pending; sync to route change.setTimeout risks state updates after unmount and arbitrary delays. Tie reset to pathname changes instead.
Outside the changed lines, recommended adjustment:
// Add an effect: useLayoutEffect(() => { setSubPending({}); }, [pathname]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (9)
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/flat-nav-item.tsx(2 hunks)apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/index.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx(6 hunks)apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-api-navigation.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-projects-navigation.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-ratelimit-navigation.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/app-sidebar/index.tsx(3 hunks)apps/dashboard/components/navigation/sidebar/workspace-navigations.tsx(1 hunks)apps/dashboard/lib/trpc/routers/deploy/project/list.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Use Biome for formatting and linting in TypeScript/JavaScript projects
Prefer named exports over default exports in TypeScript/JavaScript, except for Next.js pages
Files:
apps/dashboard/lib/trpc/routers/deploy/project/list.tsapps/dashboard/components/navigation/sidebar/workspace-navigations.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-ratelimit-navigation.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-projects-navigation.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/index.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-api-navigation.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/index.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/flat-nav-item.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Follow strict TypeScript configuration
Use Zod for runtime validation in TypeScript projects
Files:
apps/dashboard/lib/trpc/routers/deploy/project/list.tsapps/dashboard/components/navigation/sidebar/workspace-navigations.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-ratelimit-navigation.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-projects-navigation.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/index.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-api-navigation.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/index.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsxapps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/flat-nav-item.tsx
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
apps/dashboard/lib/trpc/routers/deploy/project/list.ts
🧠 Learnings (1)
📚 Learning: 2025-07-25T19:09:43.284Z
Learnt from: mcstepp
PR: unkeyed/unkey#3662
File: apps/dashboard/lib/trpc/routers/deployment/list.ts:11-11
Timestamp: 2025-07-25T19:09:43.284Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/list.ts, the listDeployments procedure intentionally queries the versions table rather than a deployments table. The user mcstepp indicated that renaming the table would require a database migration, which was deferred for the current PR focused on UI features.
Applied to files:
apps/dashboard/lib/trpc/routers/deploy/project/list.ts
🧬 Code graph analysis (6)
apps/dashboard/lib/trpc/routers/deploy/project/list.ts (1)
internal/db/src/schema/projects.ts (1)
projects(9-31)
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-projects-navigation.tsx (1)
apps/dashboard/components/navigation/sidebar/workspace-navigations.tsx (1)
NavItem(16-29)
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/index.tsx (1)
apps/dashboard/components/navigation/sidebar/workspace-navigations.tsx (1)
NavItem(16-29)
apps/dashboard/components/navigation/sidebar/app-sidebar/index.tsx (3)
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-projects-navigation.tsx (1)
useProjectNavigation(7-74)apps/dashboard/components/navigation/sidebar/workspace-navigations.tsx (1)
NavItem(16-29)apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/index.tsx (1)
NavItems(12-17)
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx (1)
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/index.tsx (1)
NavProps(5-11)
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/flat-nav-item.tsx (1)
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/index.tsx (1)
NavProps(5-11)
⏰ 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). (3)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
🔇 Additional comments (11)
apps/dashboard/components/navigation/sidebar/workspace-navigations.tsx (1)
42-61: Widening label to ReactNode is fine; confirm no string-only operations downstreamSince label can now be a node, verify that downstream code doesn’t call string methods (e.g., toLowerCase, substring) on label. If needed, render-only components should treat label as ReactNode and avoid string manipulation.
Would you like a quick repo scan script to flag any places where label is assumed to be a string?
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-api-navigation.tsx (1)
64-76: Force showSubItems on APIs parent is intentional; ensure chevron logic matches NavItems/NavPropsYou’re setting showSubItems to true even when sub-items may be filtered. Confirm NavItems and Flat/Nested item components respect showSubItems and onLoadMore(item). Looks aligned with the PR summary.
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-projects-navigation.tsx (3)
38-46: Items merging is correct; ensure the parent Projects item is visible only when the Beta flag is onYou rely on workspace-navigations to hide Projects when betaFeatures.deployments is false. Confirm that enhancedNavItems composition preserves hidden items’ state (your spread-copy will keep the hidden flag off the parent item).
63-67: loadMore guard is good; propagate as onLoadMore(item) consumer expectsEnsure app-sidebar index wires onLoadMore to call the correct hook based on href (#load-more-projects) and that onLoadMore receives the clicked item. From the PR summary, this is already in place.
21-31: No change needed—route uses [projectId], so project.id is correct
The dynamic route directory is named[projectId]underapps/dashboard/app/(app)/projects, so the URL segment and active-state check should indeed useproject.id. Switching toproject.slugwould break the existing route. If you do intend to route by slug in the future, you’d need to rename the dynamic folder from[projectId]to[slug](and update the page’s params accordingly).Likely an incorrect or invalid review comment.
apps/dashboard/components/navigation/sidebar/app-sidebar/index.tsx (3)
27-27: Projects nav hook import looks correct.Import is placed alongside related hooks and maintains locality. No issues.
59-61: Layering projects after ratelimits: confirm intended ordering.You’re composing base → APIs → ratelimits → projects. Ensure this ordering matches UX expectations (e.g., Projects showing after Rate limits). If Projects should appear before Rate limits, swap the composition order.
Would you like a quick patch to reorder while preserving the same enhancement pipeline?
74-75: Good: dependency array updated.Including loadMoreProjects prevents stale closures.
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/flat-nav-item.tsx (1)
5-5: Prop surface consolidation via NavProps looks good.Moving to NavProps keeps Flat and Nested items aligned and reduces duplication.
Also applies to: 10-10
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/index.tsx (1)
12-17: LGTM: central dispatcher pattern.Forwarding onLoadMore to Flat/Nested keeps behavior consistent across tree depths.
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx (1)
27-31: Prop typing aligned with NavProps.The extension with depth/maxDepth/isSubItem is clear and self-contained.
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/flat-nav-item.tsx
Show resolved
Hide resolved
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/index.tsx
Show resolved
Hide resolved
...dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx
Show resolved
Hide resolved
...dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx
Show resolved
Hide resolved
...dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx
Show resolved
Hide resolved
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-projects-navigation.tsx
Show resolved
Hide resolved
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-projects-navigation.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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/dashboard/app/(app)/apis/_components/api-list-client.tsx(3 hunks)apps/dashboard/app/(app)/ratelimits/_components/ratelimit-client.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Use Biome for formatting and linting in TypeScript/JavaScript projects
Prefer named exports over default exports in TypeScript/JavaScript, except for Next.js pages
Files:
apps/dashboard/app/(app)/apis/_components/api-list-client.tsxapps/dashboard/app/(app)/ratelimits/_components/ratelimit-client.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Follow strict TypeScript configuration
Use Zod for runtime validation in TypeScript projects
Files:
apps/dashboard/app/(app)/apis/_components/api-list-client.tsxapps/dashboard/app/(app)/ratelimits/_components/ratelimit-client.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-18T10:28:47.391Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3797
File: apps/dashboard/app/(app)/projects/[projectId]/deployments/components/control-cloud/index.tsx:1-4
Timestamp: 2025-08-18T10:28:47.391Z
Learning: In Next.js App Router, components that use React hooks don't need their own "use client" directive if they are rendered within a client component that already has the directive. The client boundary propagates to child components.
Applied to files:
apps/dashboard/app/(app)/ratelimits/_components/ratelimit-client.tsx
🧬 Code graph analysis (2)
apps/dashboard/app/(app)/apis/_components/api-list-client.tsx (1)
apps/dashboard/components/virtual-table/components/loading-indicator.tsx (1)
LoadMoreFooter(19-206)
apps/dashboard/app/(app)/ratelimits/_components/ratelimit-client.tsx (3)
apps/dashboard/app/(app)/ratelimits/_components/controls/index.tsx (1)
RatelimitListControls(15-30)apps/dashboard/components/virtual-table/components/loading-indicator.tsx (1)
LoadMoreFooter(19-206)apps/dashboard/components/empty-component-spacer.tsx (1)
EmptyComponentSpacer(3-9)
⏰ 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). (3)
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/dashboard/app/(app)/ratelimits/_components/ratelimit-client.tsx (2)
82-86: LGTM — stable keys and clear grid layout.Using namespace.id as the key is correct; grid sizing is consistent with the rest of the app.
4-4: LGTM — footer import and empty/search states read well.
- Importing LoadMoreFooter here is appropriate.
- Empty state toggles based on isSearching are clear and copy is actionable.
- Hiding the snippet/docs during search avoids distraction.
Also applies to: 116-119, 120-144
apps/dashboard/app/(app)/apis/_components/api-list-client.tsx (2)
79-87: LGTM: Footer wiring and pagination integration are correct.
onLoadMore,isFetchingNextPage,hasMore, and the counters are correctly plumbed intoLoadMoreFooter, gated by!isSearching. This aligns with the shared pagination UX.
57-57: No changes needed for totalCount derivationThe
fetchApiOverviewfunction always computes and returns atotalcount unconditionally, even when acursoris provided. This means:
- Every page in the infinite query’s
pagesarray includes a validtotalfield (matching the total number of APIs in the workspace).- Accessing
apisData?.pages[0]?.total || 0safely reflects the true total once the first page loads.Since the TRPC handler guarantees
totalon each invocation, the existing implementation is already resilient and does not require the proposed fallback logic.
apps/dashboard/app/(app)/ratelimits/_components/ratelimit-client.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/ratelimits/_components/ratelimit-client.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/ratelimits/_components/ratelimit-client.tsx
Outdated
Show resolved
Hide resolved
1 participant
What does this PR do?
This PR
Type of change
How should this be tested?
Go to /projects
Create at least 15 projects
See if you can load more from sidebar and when that happens UI should also update projects own load more section. They should match
Do these for both APIs and Ratelimits make sure their load mores work properly.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Improvements
Style
Bug Fixes