Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughConsolidates UI primitives into the internal UI package: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
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 |
MichaelUnkey
requested review from
Flo4604,
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/dashboard/components/logs/checkbox/filter-item.tsx (1)
2-2: Using Drover from@unkey/uiis good; consider avoiding deepcnimportPulling
Button,Drover, andKeyboardButtonfrom@unkey/uimatches the shared UI pattern and doesn’t change how this component behaves.One follow‑up to consider (not blocking here):
cnis currently imported from@unkey/ui/src/lib/utils, which reaches into an internal source path of the UI package. If/whencnis exposed via the main@unkey/uientry (or a dedicated helper export), it would be cleaner to switch to that to keep app code decoupled from internal file layout.Before changing the
cnimport, please confirm whether@unkey/uiactually re‑exports it; if not, it may be better to keep the current import until such an export exists.internal/ui/src/hooks/use-mobile.tsx (1)
1-22:useIsMobilehook is SSR‑safe and correctly manages the media query listenerThe hook keeps
windowusage insideuseEffect, initializes from the current viewport, and removes thematchMedialistener on unmount, which is all correct.If you ever revisit it, a tiny simplification would be to use
mql.matchesinsideonChangeand for the initial value:const mql = window.matchMedia(`(max-width: ${breakpoint - 1}px)`); const onChange = () => setIsMobile(mql.matches); setIsMobile(mql.matches);Functionally it’s equivalent but avoids recomputing via
window.innerWidth.internal/ui/src/lib/create-context.tsx (2)
20-30: Consider using explicitundefinedcheck instead of truthy check.The type constraint
ContextValueType extends object | nullallowsnullas a valid context value. The current truthy checkif (context)would treat an explicitly providednullvalue as missing context.While the fallback to
defaultContextmay handle some cases, usingcontext !== undefinedwould be more precise:function useContext(consumerName: string) { const context = React.useContext(Context); - if (context) { + if (context !== undefined) { return context; } if (defaultContext !== undefined) { return defaultContext; } // if a defaultContext wasn't specified, it's a required context. throw new Error(`\`${consumerName}\` must be used within \`${rootComponentName}\``); }
14-15: Add meaningful explanation to the biome-ignore comment.The placeholder
<explanation>should describe why the lint rule is being suppressed. This helps future maintainers understand the reasoning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/dashboard/components/logs/checkbox/filter-item.tsx(1 hunks)apps/dashboard/components/logs/checkbox/filters-popover.tsx(1 hunks)apps/dashboard/components/logs/datetime/datetime-popover.tsx(1 hunks)apps/dashboard/package.json(0 hunks)internal/ui/package.json(2 hunks)internal/ui/src/components/drawer.tsx(2 hunks)internal/ui/src/components/drover.tsx(1 hunks)internal/ui/src/hooks/use-mobile.tsx(1 hunks)internal/ui/src/index.ts(1 hunks)internal/ui/src/lib/create-context.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/package.json
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3292
File: apps/dashboard/lib/vault.ts:60-78
Timestamp: 2025-06-02T11:09:05.843Z
Learning: The vault implementation in `apps/dashboard/lib/vault.ts` is a duplicate of the vault package from `api` and should be kept consistent with the original implementation.
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3292
File: apps/dashboard/lib/vault.ts:80-97
Timestamp: 2025-06-02T11:08:56.397Z
Learning: The vault.ts file in apps/dashboard/lib/vault.ts is a duplicate of the vault package from the `api` directory and should be kept consistent with that original implementation.
Learnt from: p6l-richard
Repo: unkeyed/unkey PR: 2085
File: apps/www/components/ui/button.tsx:39-46
Timestamp: 2024-10-23T16:33:02.143Z
Learning: In `apps/www/components/ui/` directory, components are based on shadcn UI patterns, including their typing conventions.
📚 Learning: 2025-01-30T20:38:00.058Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2825
File: apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-datetime/index.tsx:0-0
Timestamp: 2025-01-30T20:38:00.058Z
Learning: In the logs dashboard, keyboard shortcuts that toggle UI elements (like popovers) should be implemented in the component that owns the state being toggled, not in the presentational wrapper components. For example, the 'T' shortcut for toggling the datetime filter is implemented in DatetimePopover, not in LogsDateTime.
Applied to files:
apps/dashboard/components/logs/checkbox/filters-popover.tsxapps/dashboard/components/logs/datetime/datetime-popover.tsx
📚 Learning: 2025-04-24T14:34:30.621Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3115
File: apps/dashboard/components/logs/checkbox/filters-popover.tsx:33-55
Timestamp: 2025-04-24T14:34:30.621Z
Learning: In the ShortcutActivator component within filters-popover.tsx, the purpose is to track keys separately for each filter item, providing a registration mechanism for shortcuts passed to it rather than enforcing specific key combinations like option+shift+key.
Applied to files:
apps/dashboard/components/logs/checkbox/filters-popover.tsx
📚 Learning: 2024-12-03T14:07:45.173Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2143
File: apps/dashboard/components/ui/group-button.tsx:21-31
Timestamp: 2024-12-03T14:07:45.173Z
Learning: In the `ButtonGroup` component (`apps/dashboard/components/ui/group-button.tsx`), avoid suggesting the use of `role="group"` in ARIA attributes.
Applied to files:
apps/dashboard/components/logs/checkbox/filters-popover.tsx
📚 Learning: 2025-08-25T13:05:22.441Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3834
File: internal/ui/src/components/form/input.tsx:130-131
Timestamp: 2025-08-25T13:05:22.441Z
Learning: The Input component in internal/ui/src/components/form/input.tsx is designed to support interactive right icons (like X buttons for deletion) by enabling pointer events on the right icon container, allowing clicks to be handled by the icon rather than passed through to the input.
Applied to files:
apps/dashboard/components/logs/checkbox/filter-item.tsx
📚 Learning: 2024-10-23T16:25:33.113Z
Learnt from: p6l-richard
Repo: unkeyed/unkey PR: 2085
File: apps/www/components/glossary/terms-stepper-mobile.tsx:16-20
Timestamp: 2024-10-23T16:25:33.113Z
Learning: In the `apps/www/components/glossary/terms-stepper-mobile.tsx` file, avoid suggesting to extract the term navigation logic into a custom hook, as the user prefers to keep the component straightforward.
Applied to files:
internal/ui/src/components/drover.tsx
📚 Learning: 2025-08-18T10:28:47.391Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 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:
internal/ui/src/components/drover.tsxapps/dashboard/components/logs/datetime/datetime-popover.tsx
📚 Learning: 2025-01-22T16:51:59.978Z
Learnt from: MichaelUnkey
Repo: unkeyed/unkey PR: 2810
File: internal/ui/src/components/date-time/components/time-split.tsx:10-14
Timestamp: 2025-01-22T16:51:59.978Z
Learning: The DateTime component in internal/ui/src/components/date-time/components/time-split.tsx already includes sufficient validation through handleChange and handleBlur functions, making additional runtime validation unnecessary.
Applied to files:
apps/dashboard/components/logs/datetime/datetime-popover.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). (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Build / Build
🔇 Additional comments (5)
internal/ui/src/components/drawer.tsx (1)
9-9: Drawer namespace + relative utils import look consistentUsing a relative import for
cndecouples this component from app aliases, and exposingDraweras a namespace withRoot/Trigger/Content/...matches how it’s consumed (<Drawer.Root>, etc.). No behavioral changes from these lines.Also applies to: 58-65
apps/dashboard/components/logs/checkbox/filters-popover.tsx (1)
2-2: Importing Drover/KeyboardButton from@unkey/uialigns with the shared UI surfaceSwitching these imports to
@unkey/uikeeps logs UI on the shared component library without changing howDroverorKeyboardButtonare used.Please confirm that
@unkey/ui’s public index indeed re‑exports bothDroverandKeyboardButton(e.g., by checkinginternal/ui/src/index.tsand verifying the built package).apps/dashboard/components/logs/datetime/datetime-popover.tsx (1)
7-17: Drawer import move to@unkey/uilooks correct; watch mobile behaviorUsing
Drawerfrom@unkey/uikeeps this popover in sync with the shared UI package while preserving the existingDrawer.Root/Trigger/Contentusage on mobile. This should be a no‑op behaviorally, but it’s worth sanity‑checking the mobile datetime drawer (open/close, overlay, scroll) after the move.When testing, specifically verify on a small viewport that:
- The drawer still slides up from the bottom.
- Background scrolling is correctly locked while open.
- Closing via outside tap, ESC, and the “Apply” button works as before.
internal/ui/src/index.ts (1)
18-19: ExportingDrawerandDroverfrom the UI barrel is aligned with app usageAdding
export * from "./components/drawer";andexport * from "./components/drover";cleanly exposes these primitives to consumers like the dashboard without changing their APIs.After building the UI package, please confirm that the generated entry file still contains these exports and that
import { Drawer, Drover } from "@unkey/ui";works from another package without relying on TypeScript path aliases.internal/ui/src/components/drover.tsx (1)
6-10: Relative imports for Drover’s dependencies are appropriate inside@unkey/uiPointing
Drawer,useIsMobile,createContext,cn, and the Popover primitives to local relative paths avoids circular/self‑imports through@unkey/uiand keeps this component self‑contained. The Root/Trigger/Content/Nested logic remains the same.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/ui/src/lib/create-context.tsx (1)
9-23: Memoization fix correctly depends on actual prop values (previous issue resolved)Switching from memoizing the spread props object to deriving dependencies from the individual non‑
childrenprop values meansuseMemonow correctly reuses the context value when those props are referentially stable, addressing the earlier concern where the memo never hit due to object identity changes. The dynamic dependency array is a reasonable tradeoff for a generic helper, as long as callers keep the context props shape stable (no frequent key churn); no changes required from my side.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx(2 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/deployments-list.tsx(1 hunks)apps/dashboard/components/logs/datetime/datetime-popover.tsx(2 hunks)apps/dashboard/components/ui/sidebar.tsx(1 hunks)apps/dashboard/components/virtual-table/index.tsx(1 hunks)apps/dashboard/hooks/use-mobile.tsx(1 hunks)apps/dashboard/package.json(0 hunks)internal/ui/src/components/drover.tsx(2 hunks)internal/ui/src/hooks/use-mobile.tsx(1 hunks)internal/ui/src/lib/create-context.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/ui/src/hooks/use-mobile.tsx
- internal/ui/src/components/drover.tsx
- apps/dashboard/components/logs/datetime/datetime-popover.tsx
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: p6l-richard
Repo: unkeyed/unkey PR: 2085
File: apps/www/components/ui/button.tsx:39-46
Timestamp: 2024-10-23T16:33:02.143Z
Learning: In `apps/www/components/ui/` directory, components are based on shadcn UI patterns, including their typing conventions.
📚 Learning: 2025-07-25T19:11:00.208Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/lib/trpc/routers/deployment/getOpenApiDiff.ts:110-147
Timestamp: 2025-07-25T19:11:00.208Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/getOpenApiDiff.ts, the user mcstepp prefers to keep mock data fallbacks in POC/demonstration code for simplicity, even if it wouldn't be production-ready. This aligns with the PR being work-in-progress for demonstration purposes.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsxapps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/deployments-list.tsx
📚 Learning: 2025-09-23T17:39:59.820Z
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 4009
File: apps/dashboard/app/(app)/[workspace]/apis/[apiId]/_overview/components/table/components/override-indicator.tsx:88-97
Timestamp: 2025-09-23T17:39:59.820Z
Learning: The useWorkspaceNavigation hook in the Unkey dashboard guarantees that a workspace exists. If no workspace is found, the hook redirects the user to create a new workspace. Users cannot be logged in without a workspace, and new users must create one to continue. Therefore, workspace will never be null when using this hook.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx
📚 Learning: 2025-09-22T18:44:56.279Z
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 4009
File: apps/dashboard/app/(app)/[workspace]/apis/[apiId]/_overview/components/table/components/log-details/index.tsx:4-5
Timestamp: 2025-09-22T18:44:56.279Z
Learning: In the Unkey dashboard, the workspace hook (useWorkspace) provides security validation by checking database access and user authorization to the workspace, with 10-minute caching for performance. Using URL params (useParams) for workspace slug would bypass this security validation and allow unauthorized access attempts. Always use the workspace hook for workspace-scoped navigation and handle loading states properly rather than switching to URL parameters.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx
📚 Learning: 2025-07-25T19:09:43.284Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 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/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/deployments-list.tsx
📚 Learning: 2025-07-28T20:38:53.244Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/app/(app)/projects/[projectId]/diff/[...compare]/components/client.tsx:322-341
Timestamp: 2025-07-28T20:38:53.244Z
Learning: In apps/dashboard/app/(app)/projects/[projectId]/diff/[...compare]/components/client.tsx, mcstepp prefers to keep hardcoded endpoint logic in the getDiffType function during POC phases for demonstrating diff functionality, rather than implementing a generic diff algorithm. This follows the pattern of keeping simplified implementations for demonstration purposes.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/deployments-list.tsx
📚 Learning: 2024-10-23T16:20:19.324Z
Learnt from: p6l-richard
Repo: unkeyed/unkey PR: 2085
File: apps/www/components/glossary/terms-rolodex-desktop.tsx:26-37
Timestamp: 2024-10-23T16:20:19.324Z
Learning: When reviewing React components in this project, avoid suggesting manual memoization with `useMemo` for performance optimizations, as the team prefers to rely on the React compiler to handle such optimizations.
Applied to files:
internal/ui/src/lib/create-context.tsx
📚 Learning: 2024-11-08T11:44:42.947Z
Learnt from: unrenamed
Repo: unkeyed/unkey PR: 2652
File: apps/www/components/particles.tsx:88-90
Timestamp: 2024-11-08T11:44:42.947Z
Learning: In React TypeScript components, if a function memoized with `useCallback` is not called elsewhere in the component, and a `useEffect` is used to invoke it, do not suggest removing the `useEffect` as it is necessary to execute the function.
Applied to files:
internal/ui/src/lib/create-context.tsx
📚 Learning: 2024-11-13T14:58:01.321Z
Learnt from: unrenamed
Repo: unkeyed/unkey PR: 2660
File: apps/play/app/page-bk.tsx:24-24
Timestamp: 2024-11-13T14:58:01.321Z
Learning: In React, refs created with useRef do not change between renders, so including them in the dependency array of useEffect hooks is unnecessary.
Applied to files:
internal/ui/src/lib/create-context.tsx
📚 Learning: 2024-10-23T16:25:33.113Z
Learnt from: p6l-richard
Repo: unkeyed/unkey PR: 2085
File: apps/www/components/glossary/terms-stepper-mobile.tsx:16-20
Timestamp: 2024-10-23T16:25:33.113Z
Learning: In the `apps/www/components/glossary/terms-stepper-mobile.tsx` file, avoid suggesting to extract the term navigation logic into a custom hook, as the user prefers to keep the component straightforward.
Applied to files:
apps/dashboard/components/virtual-table/index.tsx
🧬 Code graph analysis (3)
apps/dashboard/components/ui/sidebar.tsx (2)
apps/dashboard/hooks/use-mobile.tsx (1)
useIsMobile(8-26)internal/ui/src/hooks/use-mobile.tsx (1)
useIsMobile(8-26)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx (2)
apps/dashboard/hooks/use-mobile.tsx (1)
useIsMobile(8-26)internal/ui/src/hooks/use-mobile.tsx (1)
useIsMobile(8-26)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/deployments-list.tsx (2)
apps/dashboard/hooks/use-mobile.tsx (1)
useIsMobile(8-26)internal/ui/src/hooks/use-mobile.tsx (1)
useIsMobile(8-26)
⏰ 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). (4)
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Test Packages / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
🔇 Additional comments (8)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx (2)
104-104: LGTM - Defensive default for isMobile prop.The default parameter
isMobile = falseprovides a safe fallback ensuring desktop behavior if the prop is ever omitted.
229-230: LGTM - Hydration-safe mobile detection.Passing
{ defaultValue: false }ensures the initial SSR render matches the client hydration by defaulting to desktop view.apps/dashboard/components/ui/sidebar.tsx (1)
67-68: LGTM - Consistent hydration-safe initialization.The
{ defaultValue: false }pattern matches the approach used across other components in this PR, ensuring stable SSR hydration for the sidebar.apps/dashboard/components/virtual-table/index.tsx (1)
68-69: LGTM - Hydration-safe mobile detection for table height.Defaulting to desktop ensures SSR/hydration consistency. The table height will update correctly once the effect runs on mobile devices.
apps/dashboard/hooks/use-mobile.tsx (1)
3-25: LGTM - Well-implemented hydration-safe media query hook.The implementation correctly:
- Accepts a
defaultValuefor SSR-safe initial state- Guards against server-side execution with the
windowcheck- Immediately syncs to the actual viewport state via
mql.matchesafter mount- Properly cleans up the event listener
This is the standard pattern for avoiding hydration mismatches with media queries in React/Next.js.
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/deployments-list.tsx (1)
49-50: LGTM - Hydration-safe compact view detection.Correctly uses both the custom
breakpointfor the compact view threshold anddefaultValue: falseto ensure wide view during SSR, preventing hydration mismatches.internal/ui/src/lib/create-context.tsx (2)
3-8: GenericcreateContexthelper looks solid and aligns with common patternsThe generic factory, type constraints, and
useContexterror handling mirror well-known Radix-style helpers and give a nice, typed API ([Provider, useContext]). The handling ofdefaultContextplus the descriptive error message for missing providers looks correct and safe.
25-38:useContextsemantics and displayName are clear and ergonomicThe
useContexthelper correctly distinguishes between “no provider” (error when nodefaultContext) and validnullvalues, and theProvider.displayNameimproves DevTools debugging. This looks good as-is.
|
wtf is a drover |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx (1)
11-11: Potential build breaker: confirm@unkey/uiexportsuseIsMobilefrom the package entrypoint.This import will fail at compile time if
useIsMobileisn’t re-exported from the public@unkey/uientry.#!/bin/bash set -euo pipefail # Find likely entrypoints fd -a '^(index|main)\.(ts|tsx|js|mjs|cjs)$' internal/ui src packages apps 2>/dev/null || true echo "---- search for re-export of useIsMobile ----" rg -n --hidden -S 'export\s*\{\s*[^}]*\buseIsMobile\b[^}]*\}' internal/ui src packages 2>/dev/null || true rg -n --hidden -S 'export\s+\*\s+from\s+["'\''][^"'\'']*use-mobile[^"'\'']*["'\'']' internal/ui src packages 2>/dev/null || true echo "---- search for hook definition (sanity) ----" rg -n --hidden -S 'function\s+useIsMobile\b|export\s+function\s+useIsMobile\b' internal/ui src packages 2>/dev/null || true
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx (1)
99-107: MakeNavbarContentProps.isMobileoptional (or drop the default) to keep types honest.
isMobile = falseimplies the prop can be omitted, but the interface currently requires it.interface NavbarContentProps { apiId: string; keyspaceId?: string; keyId?: string; activePage?: { href: string; text: string; }; workspace: Workspace; - isMobile: boolean; + isMobile?: boolean; layoutData: ApiLayoutData; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx(3 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/deployments-list.tsx(2 hunks)apps/dashboard/components/logs/datetime/datetime-popover.tsx(2 hunks)apps/dashboard/components/ui/sidebar.tsx(2 hunks)apps/dashboard/components/virtual-table/index.tsx(2 hunks)apps/dashboard/hooks/use-mobile.tsx(0 hunks)apps/dashboard/package.json(0 hunks)internal/ui/src/components/drover.tsx(2 hunks)internal/ui/src/index.ts(2 hunks)
💤 Files with no reviewable changes (2)
- apps/dashboard/package.json
- apps/dashboard/hooks/use-mobile.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/dashboard/components/ui/sidebar.tsx
- apps/dashboard/components/logs/datetime/datetime-popover.tsx
- apps/dashboard/components/virtual-table/index.tsx
- apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/deployments-list.tsx
- internal/ui/src/index.ts
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-07-25T19:11:00.208Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/lib/trpc/routers/deployment/getOpenApiDiff.ts:110-147
Timestamp: 2025-07-25T19:11:00.208Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/getOpenApiDiff.ts, the user mcstepp prefers to keep mock data fallbacks in POC/demonstration code for simplicity, even if it wouldn't be production-ready. This aligns with the PR being work-in-progress for demonstration purposes.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx
📚 Learning: 2024-10-23T16:25:33.113Z
Learnt from: p6l-richard
Repo: unkeyed/unkey PR: 2085
File: apps/www/components/glossary/terms-stepper-mobile.tsx:16-20
Timestamp: 2024-10-23T16:25:33.113Z
Learning: In the `apps/www/components/glossary/terms-stepper-mobile.tsx` file, avoid suggesting to extract the term navigation logic into a custom hook, as the user prefers to keep the component straightforward.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsxinternal/ui/src/components/drover.tsx
📚 Learning: 2025-06-24T13:29:10.129Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3401
File: apps/dashboard/app/(app)/logs/filters.query-params.ts:10-0
Timestamp: 2025-06-24T13:29:10.129Z
Learning: The `buildQueryParams` function in `apps/dashboard/app/(app)/logs/filters.query-params.ts` calls `useFilters()` hook inside it, but this is valid because the function is only called from within other React hooks, maintaining the Rules of Hooks compliance.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx
📚 Learning: 2025-09-23T17:39:59.820Z
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 4009
File: apps/dashboard/app/(app)/[workspace]/apis/[apiId]/_overview/components/table/components/override-indicator.tsx:88-97
Timestamp: 2025-09-23T17:39:59.820Z
Learning: The useWorkspaceNavigation hook in the Unkey dashboard guarantees that a workspace exists. If no workspace is found, the hook redirects the user to create a new workspace. Users cannot be logged in without a workspace, and new users must create one to continue. Therefore, workspace will never be null when using this hook.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx
📚 Learning: 2025-09-23T17:40:44.944Z
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 4009
File: apps/dashboard/app/(app)/[workspace]/authorization/roles/navigation.tsx:26-40
Timestamp: 2025-09-23T17:40:44.944Z
Learning: In the Unkey dashboard authorization section navigation components, the maintainer prefers to wrap entire navbars in Suspense rather than scoping Suspense to individual components, even if it blocks the whole navigation during loading.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx
📚 Learning: 2025-09-22T18:44:56.279Z
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 4009
File: apps/dashboard/app/(app)/[workspace]/apis/[apiId]/_overview/components/table/components/log-details/index.tsx:4-5
Timestamp: 2025-09-22T18:44:56.279Z
Learning: In the Unkey dashboard, the workspace hook (useWorkspace) provides security validation by checking database access and user authorization to the workspace, with 10-minute caching for performance. Using URL params (useParams) for workspace slug would bypass this security validation and allow unauthorized access attempts. Always use the workspace hook for workspace-scoped navigation and handle loading states properly rather than switching to URL parameters.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx
📚 Learning: 2025-07-28T20:38:53.244Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/app/(app)/projects/[projectId]/diff/[...compare]/components/client.tsx:322-341
Timestamp: 2025-07-28T20:38:53.244Z
Learning: In apps/dashboard/app/(app)/projects/[projectId]/diff/[...compare]/components/client.tsx, mcstepp prefers to keep hardcoded endpoint logic in the getDiffType function during POC phases for demonstrating diff functionality, rather than implementing a generic diff algorithm. This follows the pattern of keeping simplified implementations for demonstration purposes.
Applied to files:
internal/ui/src/components/drover.tsx
📚 Learning: 2025-08-18T10:28:47.391Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 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:
internal/ui/src/components/drover.tsx
🧬 Code graph analysis (2)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx (1)
internal/ui/src/hooks/use-mobile.tsx (1)
useIsMobile(8-26)
internal/ui/src/components/drover.tsx (1)
internal/ui/src/hooks/use-mobile.tsx (1)
useIsMobile(8-26)
⏰ 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). (5)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx (1)
229-231: Hydration mismatch mitigation looks good; verify no “mobile flash” regression on real devices.
useIsMobile({ defaultValue: false })is a reasonable SSR-safe default; please sanity-check on an actual mobile viewport that the breadcrumbs/actions don’t visibly flicker in a problematic way on first paint.internal/ui/src/components/drover.tsx (2)
6-10: Import move looks fine; double-check Popover API parity after switching to./dialog/popover.
This refactor is safe ifPopover/PopoverTrigger/PopoverContentfrom./dialog/popoverpreserve the same behavioral expectations (portals, focus/blur interactions, and prop names) as before.
38-53: Hydration-safedefaultValue: falseis good; verify the Popover→Drawer swap doesn’t glitch whileopen=trueon mobile.
BecauseisMobilecan flip totrueafter mount,RootComponentcan change component type while the drover is open; please sanity-check focus trapping, dismissal, and “open state” continuity on mobile (and when resizing across the breakpoint).
3 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps us understand why this PR exists
Type of change
How should this be tested?
Usage Locations
Datetime mobile only
Filter popover all locations use changes
Ensure filter and datetime functions work as they did before
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated