feat: improve responsiveness of /api/[apiID] page #3092
Conversation
revogabe
requested review from
perkinsjr,
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@revogabe is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis pull request implements comprehensive responsive layout enhancements across dashboard components. It updates various flex and sizing classes to adjust component arrangements based on screen size. Key changes include modifying charts, tables, buttons, navigation elements, and filter popovers to support both mobile and desktop views. In particular, some popovers are now conditionally rendered as drawers on mobile using media query checks, and new dependency support (vaul) has been added. All changes focus solely on UI and layout responsiveness without affecting the underlying business logic. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as DatetimePopover/FiltersPopover
participant M as useMediaQuery
participant D as Drawer
participant P as Popover
U->>C: Click toggle/open filter
C->>M: Check viewport width
alt Mobile view
C->>D: Render Drawer variant
U->>D: Interact/select option
D->>C: onOpenChange callback closes Drawer
else Desktop view
C->>P: Render Popover variant
U->>P: Interact/select option
P->>C: onApply closes Popover
end
Assessment against linked issues
Suggested labels
Suggested reviewers
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
github-actions
bot
added
Bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/dashboard/components/logs/datetime/datetime-popover.tsx (2)
127-190: Well-implemented mobile drawer component with appropriate animations.The mobile implementation using the
Drawercomponent provides a better user experience for small screens. The toggle button with the chevron animation gives clear visual feedback, and the motion animations for showing/hiding content add polish to the interface.However, there's a potential issue with the conditional rendering approach:
Consider using CSS-based hiding instead of conditional rendering for the
DateTimeSuggestionscomponent to prevent remounting. This would improve performance by avoiding unnecessary re-renders:-{timeRangeOpen && ( - <motion.div - initial={{ opacity: 0 }} - animate={{ opacity: 1 }} - className="flex w-full" - > - <DateTimeSuggestions options={suggestions} onChange={handleSuggestionChange} /> - </motion.div> -)} +<motion.div + initial={{ opacity: 0 }} + animate={{ opacity: timeRangeOpen ? 1 : 0 }} + className={cn("flex w-full", !timeRangeOpen && "hidden")} +> + <DateTimeSuggestions options={suggestions} onChange={handleSuggestionChange} /> +</motion.div>
158-186: Consider optimizing the calendar animation approach.The animation on the calendar looks good, but it might be more performant to use CSS-based visibility transitions rather than animating opacity to zero while still rendering the component.
Consider this optimization to improve performance:
-<motion.div - animate={timeRangeOpen ? { opacity: 0 } : { opacity: 1 }} - className="w-full" -> +<div + className={cn("w-full transition-opacity duration-200 ease-out", + timeRangeOpen ? "opacity-0 pointer-events-none" : "opacity-100")} +>
📜 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 (21)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-datetime/index.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/index.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/index.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/logs-client.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx(3 hunks)apps/dashboard/components/dashboard/create-key-button.tsx(1 hunks)apps/dashboard/components/keyboard-button.tsx(1 hunks)apps/dashboard/components/logs/checkbox/filters-popover.tsx(5 hunks)apps/dashboard/components/logs/control-cloud/index.tsx(1 hunks)apps/dashboard/components/logs/datetime/datetime-popover.tsx(4 hunks)apps/dashboard/components/logs/filter-operator-input/index.tsx(2 hunks)apps/dashboard/components/logs/llm-search/components/search-input.tsx(1 hunks)apps/dashboard/components/logs/llm-search/index.tsx(1 hunks)apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx(3 hunks)apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx(1 hunks)apps/dashboard/components/navigation/navbar.tsx(1 hunks)apps/dashboard/components/ui/drawer.tsx(1 hunks)apps/dashboard/components/virtual-table/index.tsx(1 hunks)apps/dashboard/package.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
apps/dashboard/app/(app)/apis/[apiId]/_overview/logs-client.tsx (2)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (1)
KeysOverviewLogsCharts(9-118)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx (1)
KeysOverviewLogsTable(26-213)
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (1)
apps/dashboard/components/logs/chart/utils/calculate-timepoints.ts (1)
calculateTimePoints(1-9)
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (1)
apps/dashboard/components/logs/chart/utils/calculate-timepoints.ts (1)
calculateTimePoints(1-9)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/index.tsx (2)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-search/index.tsx (1)
LogsSearch(7-63)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/index.tsx (1)
LogsFilters(11-135)
apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx (5)
apps/dashboard/components/navigation/navbar.tsx (1)
Navbar(42-55)internal/icons/src/icons/gauge.tsx (1)
Gauge(15-51)apps/dashboard/components/navbar-popover.tsx (1)
QuickNavPopover(36-218)apps/www/components/ui/badge.tsx (1)
Badge(33-33)apps/dashboard/components/dashboard/create-key-button.tsx (1)
CreateKeyButton(6-29)
apps/dashboard/components/logs/checkbox/filters-popover.tsx (3)
apps/dashboard/components/ui/drawer.tsx (1)
Drawer(58-68)apps/dashboard/components/keyboard-button.tsx (1)
KeyboardButton(11-38)internal/icons/src/icons/caret-right.tsx (1)
CaretRight(15-37)
apps/dashboard/components/logs/datetime/datetime-popover.tsx (4)
apps/dashboard/components/ui/drawer.tsx (1)
Drawer(58-68)internal/icons/src/icons/chevron-down.tsx (1)
ChevronDown(15-34)apps/dashboard/lib/utils.ts (1)
cn(5-7)apps/dashboard/components/logs/datetime/suggestions.tsx (1)
DateTimeSuggestions(14-124)
🔇 Additional comments (41)
apps/dashboard/components/dashboard/create-key-button.tsx (1)
6-9: Enhanced Readability with Multi-line Type DefinitionThe change to expand the props parameter into a multi-line format significantly improves readability and aligns with common TypeScript best practices. There are no functional changes, and the implementation remains correct.
apps/dashboard/components/logs/control-cloud/index.tsx (1)
145-151: Responsive UI Adjustment: Hiding Filter Controls on Mobile
The addition of themax-md:hiddenclass ensures that the filter control buttons (i.e. "Clear filters" and "Focus filters") are hidden on medium screens and below, which aligns well with the PR's objective of enhancing mobile responsiveness. This modification is consistent with similar changes across the codebase that adjust visibility based on screen size.apps/dashboard/components/virtual-table/index.tsx (1)
113-113: Good improvement for responsive layoutChanging from
h-fulltoflex-1is a smart adjustment for the empty state container. This allows the component to grow and adapt based on available space rather than being constrained to a fixed height, which improves how it renders on smaller mobile screens.apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (3)
142-142: Good responsive improvement for the chart headerAdding
max-md:w-fullensures the metric container takes full width on mobile screens, preventing layout issues and improving readability of the metric values.
275-275: Good horizontal scrolling implementation for timestamp labelsThe addition of
max-md:gap-4 max-md:overflow-x-autois a smart solution for handling timestamps on small screens. The increased gap improves readability, while the overflow property enables horizontal scrolling when timestamps can't all fit on the screen width.
282-282: Proper timestamp formatting for mobile viewSetting
max-md:w-44 max-md:whitespace-nowrapensures consistent width and prevents ugly text wrapping for timestamp labels on mobile displays. This maintains the clean appearance of the chart timeline.apps/dashboard/components/logs/filter-operator-input/index.tsx (3)
44-44: Nice responsive update for the main container.The change from a fixed width to a responsive layout with
max-md:flex-col w-full md:w-[500px]improves mobile usability by stacking elements vertically on smaller screens while maintaining the original width on medium and larger screens.
45-45: Good responsive adaptation for the options container.Converting from a fixed width to
w-full md:w-[180px]allows the options to take full width on mobile devices while maintaining the original width on larger screens, improving readability and touch targets.
74-74: Appropriate responsive adjustment for the input area.The change to
w-full md:w-[320px]ensures the input area utilizes the full screen width on mobile devices while maintaining its original width on larger screens, making it easier to use on touch devices.apps/dashboard/package.json (1)
107-107: Good addition of Vaul for mobile-friendly drawers.Adding the Vaul library (
^0.9.0) is appropriate for implementing drawer components that replace popovers on mobile devices. Drawers provide a better user experience on small screens by utilizing more space and offering improved touch interaction.apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-datetime/index.tsx (1)
88-90: Smart UI simplification for mobile views.Adding the
max-md:hiddenclass to hide the datetime text on smaller screens reduces clutter while preserving functionality through the calendar icon. This creates a cleaner, more compact UI that works better on mobile devices.apps/dashboard/app/(app)/apis/[apiId]/_overview/logs-client.tsx (1)
27-30: Good structural change for stacked charts and table.Adding a wrapper div with
flex flex-colaround the charts and table components ensures they stack properly in a vertical layout. This matches the PR's goal of stacking charts to improve visibility on smaller screens, working in conjunction with the responsive changes in theKeysOverviewLogsChartscomponent (which already hasflex-col md:flex-rowlayout adjustments).apps/dashboard/components/navigation/navbar.tsx (1)
70-71: Improved spacing for better mobile responsivenessThe reduction of gap space between breadcrumb items (from
gap-3togap-2) and removal of right margin creates a more compact layout that fits better on smaller screens.apps/dashboard/components/logs/llm-search/index.tsx (3)
127-127: Good addition of flex-1 for responsive layoutAdding
flex-1allows this container to grow and properly fill available space, which is essential for responsive designs.
130-130: Excellent responsive width implementationThe change from fixed width (
w-80) to responsive width (w-full flex-1 md:w-80) ensures the component spans full width on mobile while maintaining a specific width on larger screens.
137-137: Proper responsive container sizingAdding
w-full flex-1to the inner container creates consistent responsive behavior and ensures proper space utilization on smaller screens.apps/dashboard/components/keyboard-button.tsx (1)
25-25: Smart decision to hide keyboard shortcuts on mobileAdding
max-md:hiddento keyboard shortcuts is a good UX decision since keyboard shortcuts are rarely used on mobile devices and removing them saves valuable screen space.apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/index.tsx (1)
125-125: Good space optimization for mobile viewHiding the "Filter" text on mobile screens while keeping the icon visible is an effective way to preserve functionality while optimizing for limited screen space.
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (2)
277-277: Great mobile optimization!Adding
max-md:gap-4andmax-md:overflow-x-autoensures proper spacing and enables horizontal scrolling on mobile devices when timestamp content overflows, improving usability.
284-284: Good handling of timestamp display on mobile.The addition of
max-md:w-44andmax-md:whitespace-nowrapensures timestamps maintain consistent width and prevent awkward wrapping on smaller screens.apps/dashboard/components/logs/llm-search/components/search-input.tsx (1)
43-43: Text truncation improves mobile display.Adding the
truncateclass to the input ensures long text doesn't overflow the container, displaying an ellipsis when needed. This maintains layout integrity on smaller screens.apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx (4)
9-9: Good addition of responsive utility.The
useMediaQueryhook provides a clean way to implement conditional rendering based on viewport size.
30-30: Effective responsive breakpoint detection.Defining
isMobilewith the 768px breakpoint aligns with standard medium device breakpoints and provides a clear boolean flag for conditional rendering.
32-59: Well-implemented conditional rendering for mobile.The restructuring of the navbar with conditional rendering of breadcrumb links based on
isMobilestate effectively simplifies the UI for mobile devices, removing unnecessary navigation elements.
89-102: Improved responsive layout for actions section.The updated class names for the Navbar.Actions and Badge components create a more balanced layout on mobile. The truncation of the API ID text and non-shrinking copy button ensure critical elements remain accessible.
apps/dashboard/components/ui/drawer.tsx (1)
1-69: Well-implemented Drawer component for mobile interactions.This new drawer component based on the
vaullibrary provides an excellent mobile-friendly alternative to popovers. The implementation follows React best practices:
- Clean component structure with proper separation of concerns
- Proper ref forwarding for DOM access
- Well-defined props using React's type system
- Consistent styling with the application's design system
This aligns perfectly with the PR objective of replacing popovers with more suitable components for mobile interactions.
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx (1)
188-209: Improved empty state responsivenessThe changes to the empty state UI improve mobile responsiveness by:
- Setting a fixed height on mobile and adaptive height on larger screens
- Centering content horizontally with
mx-autoandmax-w-[400px]- Aligning text and buttons centrally on mobile while maintaining left alignment on desktop
This creates a more balanced and usable layout on smaller screens.
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/index.tsx (1)
8-27: Well-structured responsive controls layoutThe refactored layout creates a more adaptable UI for different screen sizes:
- Uses
flex-1to allow the search area to expand appropriately- Aligns filters and datetime controls to the right on mobile with
max-md:justify-end- Hides the refresh button on mobile with
max-md:hiddento save space- Maintains proper spacing with consistent gap values
These changes work well with the responsive components from the relevant code snippets like
LogsFiltersand create a cohesive mobile experience.apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (1)
85-116: Effective responsive chart layout strategyThe charts now stack vertically on mobile and display side-by-side on larger screens:
- Using
flex-col md:flex-rowto change orientation based on screen size- Setting
w-full md:w-1/2to ensure charts take full width on mobile- Adding fixed heights with
max-md:h-72to maintain proper proportionsThis stacked approach significantly improves chart readability on mobile devices without sacrificing the desktop experience.
apps/dashboard/components/logs/checkbox/filters-popover.tsx (4)
1-8: Good implementation of responsive UI patternsThe addition of the
Drawercomponent anduseMediaQueryhook are appropriate for implementing mobile-specific UI patterns.
30-31: Proper mobile detection implementationUsing a media query with a 768px breakpoint aligns with standard MD breakpoint patterns in the codebase and ensures consistent responsive behavior.
82-126: Excellent mobile UX with conditional drawer renderingReplacing popovers with drawers on mobile is an excellent UX decision:
- Drawers are more touch-friendly and utilize mobile screen space better
- The implementation maintains the same functionality while adapting to different form factors
- The drawer header is simplified compared to the popover header for a cleaner mobile experience
This approach aligns perfectly with the PR's objective to improve mobile responsiveness.
151-282: Consistent responsive pattern for nested filter itemsThe
FilterItemcomponent similarly adapts to mobile by using nested drawers instead of popovers:
- Maintains visual and functional consistency with parent components
- Preserves all keyboard navigation and accessibility features
- Uses identical content between mobile and desktop versions to ensure feature parity
This implementation shows attention to detail in maintaining a consistent user experience across device types.
apps/dashboard/components/logs/datetime/datetime-popover.tsx (8)
4-4: Appropriate imports added for responsive design implementation.The new imports provide necessary elements for implementing the responsive design pattern:
Drawercomponent for mobile views,ChevronDownicon for the toggle button,motionfor animations, anduseMediaQueryhook for detecting viewport size. The code correctly imports these dependencies for the new responsive functionality.Also applies to: 7-10, 12-12
40-41: Good implementation of responsive layout detection.Using the
useMediaQueryhook with a standard 768px breakpoint for mobile detection follows responsive design best practices. The additionaltimeRangeOpenstate is appropriately used to control the visibility of the time range filter section on mobile devices.
95-95: Improvement to user experience by closing popover after selection.Adding
setOpen(false)ensures the popover closes immediately after a suggestion is selected, improving the user experience by removing the need for an additional close action.
138-147: Well-designed toggle button with clear visual feedback.The toggle button implementation is clean and provides good visual feedback with the rotating chevron icon. The button uses appropriate styling for mobile interfaces and clearly indicates its purpose with the "Filter by time range" text.
162-185: Good reuse of DateTime component between mobile and desktop views.The DateTime component is reused effectively between mobile and desktop views with appropriate class modifications. This maintains consistency in the date/time selection experience while adapting to different screen sizes.
191-229: Well-structured desktop view with appropriate styling.The desktop implementation maintains the existing Popover pattern but with improved styling. The structure is clean and the alignment with the mobile view implementation ensures a consistent user experience across devices.
236-239: Improved PopoverHeader with better spacing.The PopoverHeader component has been adjusted with better spacing and sizing for the keyboard shortcut button, improving the overall appearance and usability.
127-230:❓ Verification inconclusive
Verify keyboard accessibility in mobile view.
The mobile drawer implementation needs to maintain the same level of keyboard accessibility as the desktop popover version. Ensure that focus management and keyboard navigation work properly in the mobile view.
Test keyboard navigation in the mobile view to verify that focus is properly managed when opening/closing the drawer and interacting with elements inside it. Specifically check:
- Tab navigation works correctly
- Escape key closes the drawer
- Focus returns to the trigger element when the drawer is closed
🏁 Script executed:
#!/bin/bash # Look for keyboard accessibility implementations in the drawer component ast-grep --pattern $'function\ .*Drawer.*\(.*\)\ \{ .* \$$$ .*useEffect.* .*\}' # Check for focus management implementations rg -A 10 "useFocusTrap|FocusTrap|trapFocus|manageFocus" --type tsLength of output: 156
Keyboard Accessibility in Mobile Drawer – Manual Verification Required
Please verify that the mobile drawer implementation in
apps/dashboard/components/logs/datetime/datetime-popover.tsxdelivers the same keyboard accessibility as the desktop popover. In particular, ensure that:
- Tab Navigation: All interactive elements inside the drawer are reachable via the Tab key.
- Escape Key Functionality: Pressing Escape properly closes the drawer.
- Focus Management: When the drawer is closed, focus correctly returns to the trigger element.
Since our automated search did not locate explicit focus management code, please manually test these behaviors to confirm compliance with accessibility standards.
chronark
changed the title
|
- submitting a filter should close the drawer - changing between time range and calendar has some annoying layout shift - there seem to be some weird scroll bars
|
fix: 1,2,3. Waiting for ideas on point 5 and about point 4 I mentioned that I followed the doc but was not successful in populating the API with fake data, to check how the table is working cc: @chronark |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/outcome-filter.tsx (1)
5-7: Add JSDoc for the new prop.
Defining anonOpenChangecallback is a nice touch to handle state changes externally. Consider adding JSDoc or comments describing when and how this callback should be triggered for future maintainers.type OutcomesFilterProps = { onOpenChange?: () => void; + /** + * Callback fired whenever the open state of the filter changes. + * Useful for programmatic control of UI elements that depend on filter visibility. + */ };apps/dashboard/components/logs/checkbox/filter-checkbox.tsx (1)
51-51: Enhance the onOpenChange signature for future expansions.
Consider passing an argument (e.g., a boolean indicating the next open state) toonOpenChangeso calling contexts know whether the filter is about to open or close.- onOpenChange?: () => void; + onOpenChange?: (isOpen: boolean) => void;apps/dashboard/components/logs/checkbox/filters-popover.tsx (3)
2-7: Use import type for purely type-based imports.
Static analysis indicates some named imports here are only used as types. Switching toimport type { ... }helps tree-shaking and clarity.-import { Drawer } from "@/components/ui/drawer"; -import { - Popover, - PopoverContent, - PopoverTrigger, -} from "@/components/ui/popover";+import type { Drawer } from "@/components/ui/drawer"; +import type { + Popover, + PopoverContent, + PopoverTrigger, +} from "@/components/ui/popover";
11-20: Check for dynamic imports or code splitting.
You’re pulling in multiple new items (e.g.,Dispatch,SetStateAction,useEffect, etc.). Rendering drawers vs. popovers conditionally might be a good candidate for dynamic import or code splitting if load time is a concern.🧰 Tools
🪛 Biome (1.9.4)
[error] 11-20: Some named imports are only used as types.
This import is only used as a type.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.(lint/style/useImportType)
43-46: Supplement getFilterCount usage.
getFilterCount = (field) => activeFilters.filter(...)is straightforward. If you later need to handle multiple forms of counts (e.g., advanced conditions), consider a more robust counting or grouping method.apps/dashboard/components/logs/datetime/datetime-popover.tsx (6)
1-2: Include a top-level comment or docstring.
Since"use client"must appear at the top and you’re significantly customizing the component, a brief docstring about the purpose ofDatetimePopovercan be helpful.
4-9: Ensure consistent use of Drawer vs. Popover.
TheDrawerimport and other UI elements in lines 4–9 are consistent with your new pattern. Confirm that you aren’t duplicating code already handled in other popovers/drawers to avoid maintainability issues.
27-31: Readable multiline function signature.
The updatedonDateTimeChangesignature is more readable. Consider adding a more descriptive doc comment for each parameter.
147-212: Smooth transitions in Drawer.Content.
Usingmotion.divfor fade in/out is functional. Double-check that re-opening the drawer resets or retains prior user selections (whichever is intended).
213-255: Ensure stable state in Popover variant.
The popover uses a straightforwardPopoverContent. Confirm you’re not missing transitions, or that you don’t need them for consistency with the drawer approach.
261-265: Document keyboard shortcut.
Line 265 references aKeyboardButtonwith shortcut “T.” Document this in your user-facing help or developer notes to avoid overlooked shortcuts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/index.tsx(10 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/outcome-filter.tsx(2 hunks)apps/dashboard/components/logs/checkbox/filter-checkbox.tsx(8 hunks)apps/dashboard/components/logs/checkbox/filters-popover.tsx(9 hunks)apps/dashboard/components/logs/datetime/datetime-popover.tsx(8 hunks)apps/dashboard/components/logs/datetime/suggestions.tsx(3 hunks)apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx(8 hunks)apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx(10 hunks)packages/api/src/openapi.d.ts(39 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/components/logs/datetime/suggestions.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx
- apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/index.tsx
- apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/dashboard/components/logs/checkbox/filter-checkbox.tsx (2)
apps/dashboard/components/logs/validation/filter.types.ts (2)
FilterValue(49-62)FilterOperator(7-7)apps/dashboard/components/logs/checkbox/hooks/index.ts (1)
useCheckboxState(12-100)
🪛 Biome (1.9.4)
apps/dashboard/components/logs/checkbox/filters-popover.tsx
[error] 11-20: Some named imports are only used as types.
This import is only used as a type.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.
(lint/style/useImportType)
🔇 Additional comments (28)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/outcome-filter.tsx (2)
9-9: Ensure onOpenChange is handled properly.
TheOutcomesFilternow destructures the optionalonOpenChangeprop. Verify downstream components or parents correctly implement or ignore this callback, preventing unexpected undefined behavior.
34-34: Confirm correct passthrough of onOpenChange.
You are forwardingonOpenChangedirectly toFilterCheckbox. Confirm that the checkbox component invokes this callback only at appropriate times (e.g., after filter application) to ensure consistent UI states.apps/dashboard/components/logs/checkbox/filter-checkbox.tsx (8)
16-22: Optional operators are well-defined.
These refined type definitions (ExtractOperatorandExtractValue) elegantly handle the conditional extraction of types fromFilterValue. This aids readability and type safety.
26-26: Confirm TFilterValue constraints.
UsingTFilterValue extends FilterValue<any, any, any>is a flexible constraint but might mask some type errors if you plan to enforce more specific data structures later.
56-57: Validate extended generics approach.
Declaring the component with<TFilterValue extends FilterValue<any, any, any>>ensures flexible usage, but confirm that more specialized filter constraints aren’t needed for advanced scenarios (e.g., numeric filters).
74-75: Confirm destructured props usage.
You destructureonOpenChangehere but do not appear to invoke it within the main component logic aside fromhandleApplyFilter. Double-check if you intend to use it in other user interactions (like toggling checkboxes) to close or open external popovers.
77-84: Review the usage of useCheckboxState.
Nice usage ofuseCheckboxState<TItem, TFilterValue>. Ensure that:
- The provided
options,filters, andfilterFieldalign with the final shape of the data.- Returns and side effects from the hook remain stable, especially given extra logic in
FilterCheckbox.
111-112: Check callback dependencies.
YourhandleSingleSelectioncallback depends oncheckboxes,handleCheckboxChange, andallowDeselection. It looks correct, but re-check for any missing or extraneous dependencies to prevent stale closures or unnecessary re-renders.
134-135: Validate callback dependencies for handleCheckboxClick.
Same approach: ensure all relevant variables are included in the dependency array. Overlooking changes toselectionModeor new local states can introduce subtle bugs.
190-199: Close popover upon filter application.
InvokingonOpenChange?.()after updating filters is a logical approach to close the filter UI. Confirm all usage paths (multiple or single selection) handle filter finalization consistently.apps/dashboard/components/logs/checkbox/filters-popover.tsx (6)
35-36: Well-defined open state props.
Exposingopen: booleanandonOpenChangeis helpful for controlling popovers both internally and externally. This approach ensures minimal friction when hooking into other UI flows.
48-48: Check viewport threshold.
(max-width: 768px)is a common breakpoint, but confirm in your design system if a different threshold is more appropriate for drawer vs. popover usage.
58-58: Keyboard shortcut toggling.
Great job adding an “F” shortcut to open or close filters. Verify it doesn’t conflict with other shortcuts.
74-99: Keyboard controls for arrow navigation.
This logic ensures that users can navigate filters with arrow keys. Confirm all states (e.g.,activeFilter) are updated properly and no edge cases exist (like pressing ArrowDown with no items).
104-146: Responsive drawer approach.
Your mobile-oriented drawer ensures a dedicated layout without cramming popover logic. This is a sound approach for usability. Just confirm the transitions (open → close → re-open) do not cause layout flickers or unexpected re-renders.
172-172: Re-check isMobile usage in nested items.
You branch at multiple levels (FilterItem / FilterPopover). Confirm consistent usage ofisMobileso the drawer vs. popover logic doesn’t accidentally overlap or create redundancy.apps/dashboard/components/logs/datetime/datetime-popover.tsx (5)
11-12: Validate new utility usage.
cnandprocessTimeFiltersfrom@/lib/utilsare integrated. Confirm yourprocessTimeFiltershandles edge cases (e.g., invalid dates) to avoid run-time errors.
14-14: Leverage motion for smooth transitions.
Usingframer-motionis an excellent way to create subtle animations. Keep an eye on performance if running multiple transitions for large sets of time-based components.
16-17: ValidateuseMediaQueryfallback.
In some server-side rendering or older browsers,usehooks-tsmight require polyfills. Confirm you have appropriate fallbacks so the drawer remains accessible.
48-49: Verify correct breakpoint.
As with the filters popover, confirm that(max-width: 768px)is your target for shifting from a popover to a drawer.
141-146: Handle toggling states carefully.
timeRangeOpentoggles within the drawer. Double-check that toggling between custom date/time pickers and suggestions doesn’t cause partially applied filters or lost state.packages/api/src/openapi.d.ts (7)
9-10: Improved type definition clarity with better parentheses.The XOR and OneOf type definitions have been enhanced with better parentheses placement, making the precedence of operations clearer while maintaining the same functionality.
570-570: Simplified code property format.The code property in the V1KeysVerifyKeyResponse interface has been reformatted to a single line for better readability while maintaining all the same enum values.
598-602: Consistent formatting for PermissionQuery type.The PermissionQuery type has been reformatted to use the OneOf utility type in a more consistent way, maintaining the same functionality with a cleaner syntax.
667-682: Improved array type notation for ratelimits.The ratelimits property has been reformatted to use the more modern TypeScript array notation (
Type[]instead ofArray<Type>), which is consistent with the rest of the codebase.
708-709: Simplified parameters object structure.The parameters object has been simplified to an empty object, which is appropriate since there are no global parameters defined in this OpenAPI spec.
1686-1706: Consistent array object formatting throughout the file.Multiple array response types have been reformatted with consistent indentation and property structure throughout the file, improving code readability without changing functionality.
Also applies to: 2973-2979, 3423-3571, 4457-4474
4765-4765: Simplified type definitions for query parameters.The groupBy and orderBy parameters in the getVerifications operation have been reformatted to more concise and readable type definitions.
Also applies to: 4767-4767
|
check this out, we just made the seeding easier and it should work as documented now |
chronark
added
Feature
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/dashboard/hooks/use-responsive.tsx (3)
1-45: Good implementation of a responsive hook for device detectionThe
useResponsivehook is a well-structured solution for detecting device types across the application. It initializes correctly with server-side rendering support and properly handles cleanup.A few suggestions for improvement:
const onResize = () => { + // Debounce or throttle could be added here to prevent excessive re-renders const w = window.innerWidth; const newDevice: DeviceType = w <= BREAKPOINTS.mobile ? 'mobile' : w <= BREAKPOINTS.tablet ? 'tablet' : 'desktop'; setDevice(newDevice); };Consider adding a debounce mechanism to prevent excessive re-renders during window resizing, especially on mobile devices where this can impact performance. You could use a utility like lodash's debounce or a custom implementation.
7-10: Consider exporting breakpoint constantsThe breakpoint values are currently only accessible within this file, but they may be useful in other parts of the application for consistent responsive behavior.
Consider exporting the breakpoints to make them reusable:
-const BREAKPOINTS = { +export const BREAKPOINTS = { mobile: 768, tablet: 1024, };
13-19: Extract device detection logic to avoid duplicationThe device detection logic is duplicated between the initial state and the resize handler.
Extract the device detection logic to a reusable function:
+const getDeviceType = (width: number): DeviceType => { + if (width <= BREAKPOINTS.mobile) return 'mobile'; + if (width <= BREAKPOINTS.tablet) return 'tablet'; + return 'desktop'; +}; export function useResponsive() { const [device, setDevice] = useState<DeviceType>(() => { if (typeof window === 'undefined') { return 'desktop'; } - const w = window.innerWidth; - if (w <= BREAKPOINTS.mobile) { return 'mobile'; } - if (w <= BREAKPOINTS.tablet) { return 'tablet'; } - return 'desktop'; + return getDeviceType(window.innerWidth); });Then reuse this function in the resize handler as well.
apps/dashboard/components/logs/checkbox/filters-popover.tsx (1)
204-296: Consider reducing code duplication in the conditional renderingThe component implements conditional rendering well, but there's significant duplication between the mobile and desktop render paths.
Extract common elements like button content and filter count display to reduce duplication:
// Extract common button content const buttonContent = ( <> <div className="flex gap-2 items-center"> {shortcut && ( <KeyboardButton shortcut={shortcut} modifierKey="⌘" role="presentation" aria-haspopup="true" title={`Press '⌘${shortcut?.toUpperCase()}' to toggle ${label} options`} /> )} <span className="text-[13px] text-accent-12 font-medium">{label}</span> </div> <div className="flex items-center gap-1.5"> {filterCount > 0 && ( <div className="bg-gray-6 rounded size-4 text-[11px] font-medium text-accent-12 text-center flex items-center justify-center"> {filterCount} </div> )} <Button variant="ghost" size="icon" tabIndex={-1} className="size-5 [&_svg]:size-2"> <CaretRight className="text-gray-7 group-hover:text-gray-10" /> </Button> </div> </> ); // Then use in both render pathsThis would reduce maintenance overhead and make future changes easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/index.tsx(7 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/outcome-filter.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx(3 hunks)apps/dashboard/components/logs/checkbox/filter-checkbox.tsx(3 hunks)apps/dashboard/components/logs/checkbox/filters-popover.tsx(7 hunks)apps/dashboard/components/logs/datetime/datetime-popover.tsx(4 hunks)apps/dashboard/components/logs/datetime/suggestions.tsx(1 hunks)apps/dashboard/components/ui/drawer.tsx(1 hunks)apps/dashboard/components/virtual-table/index.tsx(6 hunks)apps/dashboard/hooks/use-responsive.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/components/logs/datetime/suggestions.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/outcome-filter.tsx
- apps/dashboard/components/virtual-table/index.tsx
- apps/dashboard/components/ui/drawer.tsx
- apps/dashboard/components/logs/checkbox/filter-checkbox.tsx
- apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/index.tsx
- apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx
- apps/dashboard/components/logs/datetime/datetime-popover.tsx
- apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx
🔇 Additional comments (4)
apps/dashboard/components/logs/checkbox/filters-popover.tsx (4)
92-138: Good responsive implementation but consider auto-closing after filter submissionThe conditional rendering based on device type is well-implemented. However, according to the PR comments, there was feedback about closing the drawer automatically after filter submission.
Ensure the drawer automatically closes when filters are applied. Based on the PR objectives, chronark suggested that "submitting a filter should automatically close the drawer to improve usability."
You've added the
onCloseprop toFilterItemwhich is passed to nested items, but verify that when a filter is actually applied (not just selected), the drawer closes automatically.
283-283: Ensure consistent z-index across desktop and mobile implementationsYou've added
z-50to the PopoverContent for desktop, but there's no equivalent z-index in the mobile implementation.Verify that the Drawer appears correctly above other UI elements on mobile. If needed, add a consistent z-index to both implementations.
<Drawer.Content> - <div className="flex flex-col w-full"> + <div className="flex flex-col w-full z-50">
108-108: Fix for nested drawers requiring multiple clicks to closeAccording to PR objectives, chronark mentioned: "we have nested drawers, but i need to click twice to close the drawer. that's confusing." Your implementation addresses this by passing the
onCloseprop to nested items, which is a good approach.Passing the
onClosecallback to nested drawer items ensures users can close all drawers with a single action, improving usability.
30-31: Good external state control implementationMaking the component's open state controllable from parent components is a good pattern that increases flexibility.
The external state control allows for better integration with parent components and addresses the previous critical bug where
setOpenwasn't defined.Also applies to: 38-39
… and popover depending on responsiveness
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/dashboard/components/logs/checkbox/filters-popover.tsx (1)
127-127:⚠️ Potential issueFix string literal passed as boolean
There's a string literal
"false"being used where a boolean value is expected.- const { children, asChild = "false", ...closeProps } = props; + const { children, asChild = false, ...closeProps } = props;
🧹 Nitpick comments (1)
apps/dashboard/lib/create-context.tsx (1)
15-15: Consider adding a more specific explanation for the biome-ignoreThe
biome-ignorecomment includes a placeholder<explanation>rather than an actual explanation of why the exhaustive dependencies lint rule is being ignored.- // biome-ignore lint/correctness/useExhaustiveDependencies: <explanation> + // biome-ignore lint/correctness/useExhaustiveDependencies: Only re-memoize when the context object reference changes
📜 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 (5)
apps/dashboard/components/logs/checkbox/filters-popover.tsx(6 hunks)apps/dashboard/components/logs/llm-search/components/search-input.tsx(1 hunks)apps/dashboard/components/ui/drover.tsx(1 hunks)apps/dashboard/lib/create-context.tsx(1 hunks)apps/dashboard/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard/components/logs/llm-search/components/search-input.tsx
- apps/dashboard/package.json
🔇 Additional comments (10)
apps/dashboard/lib/create-context.tsx (1)
3-34: Well-structured context creation utility with strong typingThis is a solid implementation of a typed React context utility that provides several benefits:
- Strong TypeScript typing support
- Built-in error handling with descriptive messages
- Proper memoization to prevent unnecessary re-renders
- Support for both required and optional (defaultContext) contexts
I particularly like the error message in line 29 that guides developers on proper context usage.
apps/dashboard/components/logs/checkbox/filters-popover.tsx (4)
28-29: Good implementation of controlled component patternThe component correctly implements the controlled component pattern by:
- Adding
openandonOpenChangeprops to the component interface- Using these props to control the component's state
- Properly delegating the state control to the parent component
This is consistent with React best practices for composable components.
Also applies to: 36-37, 49-49
90-108: Great refactoring to use the new Drover componentThe component has been successfully refactored to use the new
Drovercomponent system, which provides responsive behavior for different device sizes. The structure is clean and maintains the same functionality while adding mobile responsiveness.
112-117: Good separation of header componentExtracting the header into a separate component makes the code more maintainable. The responsive classes (
px-4 pt-3 md:px-3 md:py-1) effectively handle different device sizes, and hiding the keyboard shortcut on mobile withmax-md:hiddenis a thoughtful touch.
172-218: Successful migration of nested filtering UI to Drover systemThe nested filter item has been properly migrated to use the
Drover.Nestedcomponent structure. The z-index (z-50) ensures proper stacking of nested elements.One suggestion to consider: The hardcoded z-index (
z-50) could potentially conflict with other UI elements. Consider using a more systematic approach to z-index management across the application.apps/dashboard/components/ui/drover.tsx (5)
1-54: Well-designed responsive UI component with proper context usageThe
Drover.Rootcomponent is well-implemented with:
- Clean separation of concerns
- Effective use of the custom context created with the
createContextutility- Proper state management with
useControllableState- Responsive rendering based on device type
This approach of conditionally rendering either a drawer (mobile) or popover (desktop) is an elegant solution to the mobile responsiveness issues mentioned in the PR objectives.
67-79: Clean implementation of conditional trigger componentThe
Triggercomponent appropriately forwards refs and props while conditionally rendering the correct trigger type based on the device. This maintains a consistent API regardless of the underlying implementation.
94-115: Good styling approach for responsive contentThe
Contentcomponent applies styles conditionally based on device type, ensuring appropriate appearance in both mobile and desktop contexts. The use of thecnutility for class name composition keeps the code clean.
158-183: Well-implemented nested component with proper root coordinationThe
Nestedcomponent correctly coordinates with the root component, ensuring that closing a nested component can optionally close the root. This helps create a cohesive UX for complex nested UI elements.
189-198: Clean component composition patternUsing
Object.assignto create a namespaced component API is a clean pattern that makes the component usage more intuitive and organized. This approach is consistent with many popular React component libraries.
|
Updates applied: implemented @chronark feedback, fixed bugs, added a responsive component that toggles between Drawer and Popover, and created a custom media‑query hook—no magic numbers, just mobile, tablet, or desktop. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/dashboard/components/ui/drover.tsx (1)
127-127: Fix string literal in asChild default valueThere's a string literal
"false"being used where a boolean value is expected for theasChildprop.- const { children, asChild = "false", ...closeProps } = props; + const { children, asChild = false, ...closeProps } = props;
🧹 Nitpick comments (4)
apps/dashboard/components/ui/drover.tsx (4)
95-95: Remove unused destructured propertyThe
onAnimationEndprop is destructured but never used in the component.- const { children, onKeyDown, className, onAnimationEnd, ...contentProps } = props; + const { children, onKeyDown, className, ...contentProps } = props;
158-184: Add displayName to Nested componentAll other components have a displayName set for better debugging, but the Nested component is missing one.
}; +Nested.displayName = NESTED_NAME; + /* ----------------------------------------------------------------------------
159-159: Remove unused onDrag propThe
onDragprop is destructured but never used in the component.- const { children, open: openProp, onOpenChange, defaultOpen, onDrag, ...nestedProps } = props; + const { children, open: openProp, onOpenChange, defaultOpen, ...nestedProps } = props;
85-92: Consider more specific type handling for ContentPropsThe
ContentPropstype is defined as a union ofPopoverContentProps | DrawerContentProps, which could lead to type safety issues when passing props specific to one component but not the other. Consider using discriminated unions or conditional types to ensure type safety.type ContentProps = { [K in keyof (PopoverContentProps & DrawerContentProps)]: K extends keyof PopoverContentProps & keyof DrawerContentProps ? PopoverContentProps[K] | DrawerContentProps[K] : never; } & { isMobile?: boolean };Or a simpler approach using conditional types:
type ContentProps = { isMobile?: boolean; } & ( | ({ isMobile: true } & DrawerContentProps) | ({ isMobile?: false } & PopoverContentProps) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/components/ui/drover.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/dashboard/components/ui/drover.tsx (3)
apps/dashboard/lib/create-context.tsx (1)
createContext(3-34)apps/dashboard/hooks/use-responsive.tsx (1)
useResponsive(12-49)apps/dashboard/components/ui/drawer.tsx (1)
Drawer(58-68)
🔇 Additional comments (2)
apps/dashboard/components/ui/drover.tsx (2)
36-54: LGTM - Nice responsive approachThe root component elegantly determines which UI element to render based on the device type, providing a smooth responsive experience. This is a clean solution to the mobile screen overflow issues mentioned in the PR.
177-177: Great attention to UX detailThe implementation of the
onClosecallback that closes the root drover when a nested one is closed is a nice touch for improving user experience, preventing multiple UI layers from staying open.
What does this PR do?
I adjusted the mobile responsiveness of the /api/[apiID] page. I also updated the filter and calendar buttons to improve the mobile experience by replacing popovers (which were overflowing the screen) with more suitable components.
Additionally, I stacked the charts for better visibility on smaller screens and made a few other minor UI adjustments.
Fixes #3020
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
/api/[apiID]page, especially when there is data/logs to analyze.Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Screencast.From.2025-04-08.16-52-14.mp4
Summary by CodeRabbit
New Features
Style
Chores