Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
📝 Walkthrough## Walkthrough
This set of changes updates keyboard shortcut handling and display across several dashboard components. The `useKeyboardShortcut` hook is refactored for improved parsing, configurability, and reliability, now supporting explicit key codes and a disabled state. Multiple components update their keyboard shortcut combinations, especially shifting from meta/ctrl-based shortcuts to option+shift-based ones, and update UI labels to match. The filter popover and filter item logic are modularized, with enhanced keyboard navigation, focus management, and shortcut registration. Visual adjustments are made to button focus rings, and shortcut display logic is made more robust. Additional keyboard accessibility improvements were made in filter-related components.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| apps/dashboard/hooks/use-keyboard-shortcut.tsx | Refactored hook for robust shortcut parsing, explicit key code matching, new `disabled` option, improved modifier handling, and better error handling. Updated signature and added supporting types/functions. |
| apps/dashboard/hooks/use-keyboard-shortcut.test.tsx | Added comprehensive unit tests for `useKeyboardShortcut` hook and `parseShortcutString` utility, covering shortcut parsing, event handling, options, and edge cases. |
| apps/dashboard/components/logs/checkbox/filter-item.tsx | Introduced new `FilterItem` React component for interactive filter items with popover UI, focus management, keyboard handling, and accessibility. Exported `FilterItem` and `FilterItemConfig` types. |
| apps/dashboard/components/logs/checkbox/filters-popover.tsx | Refactored to use the new `FilterItem` component. Added `ShortcutActivator` for per-item shortcut registration. Enhanced keyboard navigation, focus management, and shortcut handling. Updated accessibility and UI. |
| apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/index.tsx | Changed filter item shortcut keys from lowercase to uppercase ("E", "M", "P"). Added `shortcutLabel` property to each filter item object. |
| apps/dashboard/components/keyboard-button.tsx | Added optional chaining to `shortcut?.toUpperCase()` to prevent errors when shortcut is falsy. |
| apps/dashboard/components/logs/control-cloud/index.tsx | Changed keyboard shortcuts for clearing/focusing filters from meta+key to option+shift+key. Updated displayed shortcut labels. |
| apps/dashboard/components/logs/control-cloud/control-pill.tsx | Updated Button focus ring styling: changed color class and added offset for visual adjustment. |
| apps/dashboard/components/logs/live-switch-button/index.tsx | Changed live toggle shortcut from meta+L to option+shift+L. Updated tooltip and displayed shortcut label. Added `KeyboardButton` for shortcut display. |
| apps/dashboard/components/logs/refresh-button/index.tsx | Changed refresh shortcut from ctrl+R to option+shift+R. Updated hook usage, tooltip, and displayed shortcut label. |
| apps/dashboard/components/navbar-popover.tsx | Changed QuickNavPopover toggle shortcut from ctrl+key to option+shift+key. Updated shortcut display in header. |
| apps/dashboard/components/logs/checkbox/filter-checkbox.tsx | Replaced keyboard event handler to stop propagation and manage focus explicitly on toggle keys. Wrapped click handlers to stop propagation. Added explicit IDs for checkbox inputs. |
| apps/dashboard/components/logs/filter-operator-input/index.tsx | Added keyboard navigation for filter options list with arrow keys, Enter/Space for selection, and Tab to move focus to textarea. Introduced refs for options and textarea. |
| apps/dashboard/components/logs/checkbox/hooks/index.ts | Merged toggle logic into expanded `handleKeyDown`. Improved arrow key navigation scoped to local checkbox labels with wrap-around. Removed 'h','j','k','l' keys from navigation. Added error handling for focus. |
| apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx | Adjusted empty state container and content alignment and sizing classes for key verification logs empty state UI. |
| apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx | Replaced `useResponsive` hook with `useIsMobile` for mobile detection. |
| apps/dashboard/components/logs/datetime/datetime-popover.tsx | Replaced `useResponsive` with `useIsMobile`. Simplified mobile drawer UI by removing fixed height and nested overflow wrappers. Replaced conditional rendering with CSS-based show/hide toggling for time range suggestions and DateTime component. Updated toggle button styling. |
| apps/dashboard/components/navigation/navbar.tsx | Increased horizontal gap spacing between breadcrumb items from `gap-2` to `gap-3`. |
| apps/dashboard/components/ui/drover.tsx | Replaced `useResponsive` with `useIsMobile` for mobile detection in `Root` component. |
| apps/dashboard/components/virtual-table/index.tsx | Replaced `useResponsive` with `useIsMobile` for mobile detection. |
| apps/dashboard/hooks/use-responsive.tsx | Deleted `useResponsive` hook that tracked device type by window width and returned flags for mobile, tablet, desktop. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant UI_Component
participant useKeyboardShortcut
participant Callback
User->>UI_Component: Presses keyboard shortcut
UI_Component->>useKeyboardShortcut: Registers shortcut with callback
useKeyboardShortcut->>UI_Component: Listens for keydown events
User->>UI_Component: Triggers keydown event
UI_Component->>useKeyboardShortcut: Handles event, matches shortcut
useKeyboardShortcut->>Callback: Invokes callback if match
Callback->>UI_Component: Executes action (e.g., open popover, refresh)Possibly related PRs
Suggested labels
Suggested reviewers
|
|
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: 2
🧹 Nitpick comments (4)
apps/dashboard/hooks/use-keyboard-shortcut.tsx (1)
33-134: Consider consolidating or refactoring the large mapping logic.
While the extensive switch/case ingetKeyNameToCodeis functional and explicit, you might consider consolidating mappings into a lookup table or employing a library to reduce verbosity and ease future maintenance.apps/dashboard/components/keyboard-button.tsx (1)
31-31: Consider providing a fallback label whenshortcutis undefined.
In cases whereshortcutis falsy, displaying an empty<kbd>might be unclear to users. Including a short placeholder (e.g., "N/A") can improve usability.Also applies to: 35-35
apps/dashboard/components/logs/checkbox/filter-item.tsx (2)
35-44: Solid synchronization of local and parent states.
Storingopenlocally but syncing it in auseEffectensures external controls (viaisActive) can override local state as needed. This pattern is appropriate for custom popover control, though pay attention to potential double-render ifisActivechanges frequently.
45-72: Thoughtful focus management.
Focusing the popover trigger or content based onisFocusedandisActivefosters keyboard accessibility. The fallback focus on the container is also a nice touch. Verify ignoring the lint rule for missing dependencies won't cause stale references.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/index.tsx(1 hunks)apps/dashboard/components/keyboard-button.tsx(1 hunks)apps/dashboard/components/logs/checkbox/filter-item.tsx(1 hunks)apps/dashboard/components/logs/checkbox/filters-popover.tsx(3 hunks)apps/dashboard/components/logs/control-cloud/control-pill.tsx(1 hunks)apps/dashboard/components/logs/control-cloud/index.tsx(3 hunks)apps/dashboard/components/logs/live-switch-button/index.tsx(3 hunks)apps/dashboard/components/logs/refresh-button/index.tsx(2 hunks)apps/dashboard/components/navbar-popover.tsx(2 hunks)apps/dashboard/hooks/use-keyboard-shortcut.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/index.tsx (2)
apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/status-filter.tsx (1)
StatusFilter(41-70)apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/methods-filter.tsx (1)
MethodsFilter(18-35)
apps/dashboard/components/logs/control-cloud/index.tsx (2)
apps/dashboard/hooks/use-keyboard-shortcut.tsx (1)
useKeyboardShortcut(227-305)apps/dashboard/components/keyboard-button.tsx (1)
KeyboardButton(11-38)
apps/dashboard/components/logs/refresh-button/index.tsx (3)
apps/dashboard/hooks/use-keyboard-shortcut.tsx (1)
useKeyboardShortcut(227-305)internal/icons/src/icons/refresh-3.tsx (1)
Refresh3(15-38)apps/dashboard/components/keyboard-button.tsx (1)
KeyboardButton(11-38)
apps/dashboard/components/logs/checkbox/filter-item.tsx (4)
apps/dashboard/components/logs/checkbox/filters-popover.tsx (1)
FilterItemConfig(15-21)apps/dashboard/components/keyboard-button.tsx (1)
KeyboardButton(11-38)internal/ui/src/components/button.tsx (1)
Button(443-443)internal/icons/src/icons/caret-right.tsx (1)
CaretRight(15-37)
apps/dashboard/components/logs/checkbox/filters-popover.tsx (3)
apps/dashboard/components/logs/checkbox/filter-item.tsx (2)
FilterItemConfig(9-15)FilterItem(24-184)apps/dashboard/hooks/use-keyboard-shortcut.tsx (1)
useKeyboardShortcut(227-305)apps/dashboard/components/keyboard-button.tsx (1)
KeyboardButton(11-38)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (31)
apps/dashboard/hooks/use-keyboard-shortcut.tsx (3)
1-11: Good introduction of type definitions and descriptive doc comments.
TheKeyCombointerface comprehensively captures the relevant details (key, code, and modifiers). The accompanying doc comments clearly explain each property.
15-23: New 'disabled' option is well integrated.
Addingdisabled?: booleanto the options provides a straightforward mechanism to turn off the listener, enhancing flexibility.
227-305: Well-implemented effect and matching logic.
TheuseKeyboardShortcuthook correctly handles typical use cases, including ignoring inputs/contentEditable and matching physical key codes. The code is clear, and merging user-provided options with defaults is concise.apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/index.tsx (1)
14-15: Updated shortcuts from lowercase to uppercase appear valid.
The change to "E," "M," and "P" aligns with the parser's logic, as it normalizes to lowercase internally. However, verify there is no overlap with the existing "Press 'F' to toggle filters" instruction (line 48) or other shortcuts.Also applies to: 21-22, 28-29
apps/dashboard/components/keyboard-button.tsx (1)
31-31: Optional chaining prevents runtime errors.
Safely usingshortcut?.toUpperCase()ensures no crash whenshortcutis null or undefined. This is a clean and effective fix.Also applies to: 35-35
apps/dashboard/components/logs/control-cloud/control-pill.tsx (1)
81-81: Improved focus state styling for better accessibility.The focus ring styling has been enhanced with a better color contrast (
focus:ring-accent-9instead of the previous accent-7) and added offset (focus:ring-offset-1), which will improve visibility during keyboard navigation. This change complements the keyboard shortcut improvements in the PR.apps/dashboard/components/navbar-popover.tsx (2)
70-72: Keyboard shortcut updated to use option+shift combination.The shortcut has been updated from a Ctrl/Meta key combination to option+shift, addressing the PR objective of preventing conflicts with browser and OS default shortcuts.
229-229: Updated shortcut display to match new key combination.The visual indicator of the keyboard shortcut has been updated to show the new option+shift combination with proper symbols (⌥+⇧), providing users with accurate visual cues.
apps/dashboard/components/logs/control-cloud/index.tsx (4)
27-43: Updated "clear filters" shortcut to use option+shift combination.The keyboard shortcut for clearing filters has been changed from using meta/ctrl to option+shift+d, which aligns with the PR's goal of preventing conflicts with browser and OS shortcuts.
45-47: Updated "focus filters" shortcut to use option+shift combination.The keyboard shortcut for focusing filters has been changed from using meta/ctrl to option+shift+c, which aligns with the PR's goal of preventing conflicts with browser and OS shortcuts.
147-147: Updated visual indicator for "clear filters" shortcut.The keyboard shortcut indicator has been updated to show the new option+shift combination with proper symbols (⌥+⇧+D), providing users with accurate visual cues.
150-150: Updated visual indicator for "focus filters" shortcut.The keyboard shortcut indicator has been updated to show the new option+shift combination with proper symbols (⌥+⇧+C), providing users with accurate visual cues.
apps/dashboard/components/logs/live-switch-button/index.tsx (4)
1-1: Added KeyboardButton component for visual shortcut display.The KeyboardButton component is now imported to provide a visual representation of the keyboard shortcut, improving the user interface.
13-13: Updated live toggle shortcut to use option+shift combination.The keyboard shortcut for toggling live updates has been changed from using meta+l to option+shift+l, which aligns with the PR's goal of preventing conflicts with browser and OS shortcuts.
20-20: Updated button tooltip text to reflect new shortcut combination.The tooltip for the live toggle button has been updated to show the new option+shift key combination, ensuring consistent communication to users.
35-35: Added visual shortcut indicator to live toggle button.A KeyboardButton component has been added to visually display the keyboard shortcut (⌥+⇧+L), making the shortcut more discoverable for users.
apps/dashboard/components/logs/refresh-button/index.tsx (2)
44-47: Looks good migrating to option+shift+r.
Ensuringdisabled: !isEnabledprevents the shortcut from firing when refresh is unavailable, which aligns well with the overall logic of this PR.
68-68: Consistent with the assigned shortcut.
Using<KeyboardButton shortcut="⌥+⇧+R" />cleanly displays the actual key combination.apps/dashboard/components/logs/checkbox/filter-item.tsx (4)
24-34: Good separation of props for popover logic.
DefiningFilterItemPropsto extend the base config withisFocused,isActive, and a callback to set the active filter is a clear approach, promoting reusability.
74-91: Escape key handler logic is correct.
Stopping propagation prevents closing parent popovers and ensures focus management flows back up properly.
93-117: Graceful handling of outside clicks and open state changes.
Calling the parent callback if the popover closes unexpectedly (e.g., clicking outside) helps keep states in sync. ThehandleTriggerClicktoggles activation cleanly.
119-185: Robust popover structure.
ThePopoverTriggerandPopoverContentwrap is standard and consistent. Usingpointer-events-nonewithin nested elements ensures only the main trigger is clickable. Overall, the approach is well-structured and easy to maintain.apps/dashboard/components/logs/checkbox/filters-popover.tsx (9)
29-49: Ingenious shortcut activation workaround.
Rendering theShortcutActivatorcomponents so each item’s hook runs at the top level avoids manual listeners. This is slightly “hacky,” but a clean solution, given React’s hook rules.
55-57: Ensures filter count defaults to the active filters.
Providing a defaultgetFilterCountavoids errors in scenarios where the function is not supplied.
62-63: Remembering the last focused index.
TrackinglastFocusedIndexis helpful for returning focus after closing a nested popover, enhancing consistent keyboard navigation.
79-91: Shortcut “f” toggles the main popover.
Preventing default stops browser find-in-page conflicts. Good approach for preventing hijacking.
93-106: Activating child filters from global shortcuts.
handleActivateFilteropenly reuses the logic to setopenand immediately focuses the correct item. This is a well-structured callback.
113-132: Conditional arrow handling.
Allowing arrow keys to exit from a child filter popover if it's active but ignoring them if an input is focused is thoughtful. This fosters more intuitive navigation.
135-170: Keyboard navigation in the main filter list.
ManagingArrowDown,ArrowUp,Enter,ArrowRight, andEscapefor movement and activation seamlessly. This is consistent with typical menu or list controls.
175-186: Activators provide a central, consolidated approach.
Rendering these hidden components at the top ensures all shortcuts remain registered while simplifying the main UI code.
216-218: Enhanced filter header.
Labeling “Filter by...” clarifies the popover’s purpose. Displaying the “F” shortcut is consistent with the override logic.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/dashboard/hooks/use-keyboard-shortcut.tsx (1)
136-215: Well-implemented parsing with thorough validationThe
parseShortcutStringfunction provides robust validation and normalized output. This is critical for the reliability of keyboard shortcuts across the application.
🧹 Nitpick comments (2)
apps/dashboard/hooks/use-keyboard-shortcut.tsx (2)
33-134: Great implementation of comprehensive key mapping!The
getKeyNameToCodefunction provides an extensive and well-structured mapping from user-friendly key names to KeyboardEvent.code values, with excellent categorization for different key types.For minor optimization, consider replacing the nested switch statements with a single mapping object:
export const getKeyNameToCode = (keyName: string): string | null => { const lowerKey = keyName.toLowerCase(); + const keyMappings: Record<string, string> = { + // Letters and digits handled separately + + // Control keys + 'enter': 'Enter', + 'tab': 'Tab', + 'space': 'Space', + 'esc': 'Escape', + 'escape': 'Escape', + 'backspace': 'Backspace', + 'delete': 'Delete', + + // Arrow keys + 'up': 'ArrowUp', + 'arrowup': 'ArrowUp', + // ...remaining mappings + }; + + // Handle letters + if (lowerKey.length === 1 && lowerKey >= "a" && lowerKey <= "z") { + return `Key${lowerKey.toUpperCase()}`; + } + + // Handle digits + if (lowerKey.length === 1 && lowerKey >= "0" && lowerKey <= "9") { + return `Digit${lowerKey}`; + } + + // Handle function keys + if (lowerKey.startsWith("f") && !Number.isNaN(Number.parseInt(lowerKey.substring(1), 10))) { + const fNum = Number.parseInt(lowerKey.substring(1), 10); + if (fNum >= 1 && fNum <= 12) { + return `F${fNum}`; + } + } + + // Look up from mapping + if (keyMappings[lowerKey]) { + return keyMappings[lowerKey]; + } console.warn( `[useKeyboardShortcut] Could not map key name "${keyName}" to a standard KeyboardEvent.code...` ); return null; };This would make the function more maintainable and potentially more efficient.
237-238: Add explanation for ignore directiveThe
biome-ignoredirective is missing an explanation. Add a clear reason for why the dependency rule is being ignored here.- // biome-ignore lint/correctness/useExhaustiveDependencies: <explanation> + // biome-ignore lint/correctness/useExhaustiveDependencies: callback is already listed as a dependency
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/components/logs/refresh-button/index.tsx(2 hunks)apps/dashboard/hooks/use-keyboard-shortcut.test.tsx(1 hunks)apps/dashboard/hooks/use-keyboard-shortcut.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/components/logs/refresh-button/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: autofix
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/dashboard/hooks/use-keyboard-shortcut.tsx (1)
281-297: Good upgrade to use code-based matchingSwitching from
e.keytoe.codefor matching is a significant improvement, as it provides more reliable behavior especially with modifier keys like Alt/Option which can produce different character outputs on different keyboard layouts.apps/dashboard/hooks/use-keyboard-shortcut.test.tsx (3)
5-34: Well-designed test helper functionThe
dispatchKeyEventhelper function provides a clean way to simulate keyboard events with various modifier states, making the tests more readable and maintainable.
36-111: Comprehensive test coverage for basic functionalityThese tests thoroughly verify the hook's behavior for various key combinations, ensuring that callbacks are triggered correctly and only when the exact key combination is pressed.
299-491: Excellent test coverage for parseShortcutStringThis comprehensive set of tests addresses the previous review comment requesting test coverage for the parsing function. The tests cover normal cases, edge cases, error handling, and warn messages.
|
@ogzhanolguncu |
|
I don’t have a good suggestion though |
|
also it feels a little awkward that I can’t do arrows/j/k in this menu maybe we should allow that and remap keys to something else? |
|
like I intuitively tried |
|
how do I select any of these via keyboard? |
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
Code looks fine, but I found some usability issues mentioned in this thread
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
lgtm, but please fix the conflicts @ogzhanolguncu
|
sure |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/dashboard/components/logs/checkbox/filters-popover.tsx (3)
100-112: Consider adding a comment explaining the setTimeout usage.While the setTimeout with 0ms delay is a common pattern to defer execution to the next tick, adding a comment explaining why this is necessary would improve code clarity.
useKeyboardShortcut( "f", () => { setOpen((prev) => { const newState = !prev; if (newState && items.length > 0) { + // Use setTimeout to ensure DOM is updated before setting focus setTimeout(() => setFocusedIndex(0), 0); } return newState; }); }, { preventDefault: true, ignoreInputs: true }, );
114-127: Consider adding a comment for setTimeout in handleActivateFilter.Similar to the previous comment, adding an explanation for the setTimeout usage would improve code clarity.
const handleActivateFilter = useCallback( (id: string) => { setOpen(true); + // Use setTimeout to ensure the popover is rendered before setting focus setTimeout(() => { setActiveFilter(id); const index = items.findIndex((i) => i.id === id); if (index !== -1) { setFocusedIndex(index); setLastFocusedIndex(index); } }, 0); }, [items, setOpen], );
156-191: Consider adding ARIA keyboard interactions for menu navigation.While the keyboard navigation works well, consider enhancing accessibility by adding ARIA properties to better communicate the menu structure to screen readers. This aligns with the PR's focus on improving keyboard interactions.
// In the JSX part (around line 220) -<div className="flex flex-col gap-1 w-full p-2" role="menu"> +<div + className="flex flex-col gap-1 w-full p-2" + role="menu" + aria-orientation="vertical" +> {items.map((item, index) => ( <FilterItem key={item.id} {...item} filterCount={getFilterCount(item.id)} isFocused={focusedIndex === index} isActive={activeFilter === item.id} setActiveFilter={setActiveFilter} + role="menuitem" + aria-selected={focusedIndex === index} /> ))} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/dashboard/components/keyboard-button.tsx(1 hunks)apps/dashboard/components/logs/checkbox/filter-checkbox.tsx(3 hunks)apps/dashboard/components/logs/checkbox/filter-item.tsx(1 hunks)apps/dashboard/components/logs/checkbox/filters-popover.tsx(2 hunks)apps/dashboard/components/logs/control-cloud/index.tsx(3 hunks)apps/dashboard/components/logs/filter-operator-input/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/dashboard/components/logs/checkbox/filter-checkbox.tsx
- apps/dashboard/components/keyboard-button.tsx
- apps/dashboard/components/logs/filter-operator-input/index.tsx
- apps/dashboard/components/logs/checkbox/filter-item.tsx
- apps/dashboard/components/logs/control-cloud/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/dashboard/components/logs/checkbox/filters-popover.tsx (10)
21-21: Good addition of shortcutLabel to support display customization.Adding the optional
shortcutLabelproperty to the FilterItemConfig type allows for more flexibility in displaying shortcuts, helping to accommodate the new option+shift+key combinations mentioned in the PR objectives.
33-55: Smart use of a memoized null-rendering component for shortcut registration.This is a clever pattern to handle dynamic shortcut registration. The component doesn't render anything but allows you to use React hooks for each filter item without manual event listener management. The comments clearly explain this "hacky" but effective workaround.
67-67: Improved focus management with lastFocusedIndex.Adding the
lastFocusedIndexstate variable enhances the user experience by properly tracking and restoring focus when navigating between filters, which addresses some of the navigation issues mentioned in the PR comments.
70-84: Well-implemented controlled/uncontrolled state pattern.Good implementation of the controlled/uncontrolled state pattern with clear separation of concerns. This makes the component more flexible and reusable.
86-92: Proper cleanup of state when popover closes.This useEffect hook ensures all state is properly reset when the popover closes, preventing stale state from affecting subsequent interactions.
134-139: Good detection of input focus state.This properly identifies when an input element is focused, which helps prevent keyboard shortcuts from interfering with typing - directly addressing the hijacking issues mentioned in the PR objectives.
140-153: Improved handling for active filter navigation.The logic for handling arrow keys when a filter item popover is active has been improved, making it clearer and addressing the navigation issues mentioned in the PR comments.
195-217: Properly updated Drover components with necessary props.The Drover components are properly updated with the necessary props for the new controlled/uncontrolled pattern. Good addition of the outline-none class for accessibility.
197-207: Elegant solution for dynamic shortcut registration.The ShortcutActivator components are elegantly integrated, creating a clean way to dynamically register shortcuts for each filter item. This approach helps address the shortcut conflicts mentioned in the PR objectives.
220-231:Details
❓ Verification inconclusive
Consider verifying keyboard navigation with screen readers.
The keyboard navigation logic looks solid, but it would be worthwhile to verify how this works with screen readers, especially since the PR comments mentioned navigation difficulties.
🏁 Script executed:
#!/bin/bash # Look for any ARIA-related attributes or roles in related filter components echo "Checking for ARIA attributes in filter components..." rg -n -A2 -B2 "aria-" --glob "**/*filter*.tsx" echo "Checking for role attributes in filter components..." rg -n -A2 -B2 "role=" --glob "**/*filter*.tsx" echo "Checking for keyboard handling in related components..." rg -n -A5 -B5 "handleKeyDown|onKeyDown" --glob "**/*filter*.tsx"Length of output: 11214
Keyboard navigation ARIA attributes in place – please test with screen readers
The components already include the appropriate
roleandaria-attributes plusonKeyDownhandlers:• apps/dashboard/components/logs/checkbox/filters-popover.tsx (≈ lines 216–224):
–<div role="menu">container withonKeyDown={handleKeyDown}
• apps/dashboard/components/logs/checkbox/filter-item.tsx (≈ lines 123–131):
–role="menuitem",aria-haspopup="true",aria-expanded={open}
• apps/dashboard/components/logs/checkbox/filter-checkbox.tsx (≈ lines 197–201):
–aria-checked={…},onKeyDown={(e) => handleKeyDown…}on labelsNo code changes are required—please perform manual testing with a screen reader (e.g., NVDA, VoiceOver) to confirm focus management and announcements meet your accessibility goals.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/components/logs/checkbox/filter-item.tsx(1 hunks)apps/dashboard/components/logs/checkbox/filters-popover.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/components/logs/checkbox/filter-item.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/components/logs/checkbox/filters-popover.tsx (3)
70-86: Excellent controlled component implementationThe implementation of controlled vs. uncontrolled state is well done. This pattern follows React best practices by supporting both external state management and internal fallback state.
131-194: Improved keyboard navigation logicThe keyboard navigation handling is well-structured with clear separation between:
- Child popover navigation (limited to ArrowLeft)
- Main filter list navigation (arrow keys)
- Filter activation (Enter/ArrowRight)
- Popover dismissal (Escape)
This addresses some of the navigation issues mentioned in the PR comments.
196-237: Well-structured rendering with proper accessibilityThe render method is well organized with appropriate ARIA roles and focus management. The pattern of rendering null ShortcutActivator components for each item is clever, though unconventional.
ogzhanolguncu
force-pushed
the
fix-keyboard-hijack
branch
from
1dd8253 to
70105bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/dashboard/components/logs/checkbox/filter-item.tsx (2)
113-113: Add explanation for thebiome-ignorecommentThe
biome-ignorecomment is missing the explanation for why the linting rule is being ignored. Always include explanations for linter suppressions to help other developers understand your reasoning.- {/* biome-ignore lint/a11y/useKeyWithClickEvents: <explanation> */} + {/* biome-ignore lint/a11y/useKeyWithClickEvents: Click events are handled separately, keyboard navigation is managed by parent component */}
52-67: Consider handling focus return when popover closesWhile you've implemented focus management for when the popover opens, there's no explicit logic to return focus when it closes. Although the parent component might handle this via
isFocused, it's good practice to explicitly handle focus restoration within the component.You could add another effect to handle focus restoration:
// Focus content when drover becomes active and open // biome-ignore lint/correctness/useExhaustiveDependencies: no need to react for label useEffect(() => { if (isActive && open && contentRef.current) { // Find and focus the first focusable element within the content const focusableElements = contentRef.current.querySelectorAll<HTMLElement>( 'button, [href], input:not([type="hidden"]), select, textarea, [tabindex]:not([tabindex="-1"])', ); if (focusableElements.length > 0) { focusableElements[0].focus({ preventScroll: true }); } else { // Fallback: focus the content container itself if nothing else is focusable contentRef.current.focus({ preventScroll: true }); } } }, [isActive, open, label]); // Depend on isActive and open + // Return focus to trigger when popover closes + useEffect(() => { + // If was previously active and open, but now closed + if (!open && !isActive && itemRef.current) { + // Return focus to the trigger + itemRef.current.focus({ preventScroll: true }); + } + }, [open, isActive]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/components/logs/checkbox/filter-item.tsx(1 hunks)apps/dashboard/components/logs/checkbox/filters-popover.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/components/logs/checkbox/filters-popover.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Packages / Test ./internal/clickhouse
🔇 Additional comments (1)
apps/dashboard/components/logs/checkbox/filter-item.tsx (1)
1-171: Overall implementation looks good with a few concernsThe FilterItem component has good structure with clear separation of concerns between state management, focus handling, and UI rendering. The component handles keyboard navigation for the Escape key and provides good accessibility attributes.
However, based on the PR comments about keyboard navigation issues, it's worth ensuring this implementation addresses those concerns. Specifically:
- The tooltip reference to Command key doesn't match the PR's move to option+shift+key
- Space/Enter key handling for selection may need improvement
- Verify if arrow key navigation within menus works as expected
These issues should be addressed to ensure the keyboard shortcut changes don't negatively impact usability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/dashboard/components/logs/datetime/datetime-popover.tsx (1)
132-177: Improved mobile UI with simplified toggle and visibility logicThe mobile drawer layout has been significantly improved with:
- Better height constraints using
max-h-[80vh]instead of fixed height- Cleaner toggle button with improved visual feedback
- Simplified visibility toggling using CSS classes instead of animation components
These changes align with the PR's goals of improving UI/UX while fixing keyboard navigation issues.
Consider adding a smooth transition when showing/hiding content to improve the user experience:
- <div className={cn("w-full", !timeRangeOpen && "hidden")}> + <div className={cn("w-full transition-all duration-200 ease-in-out", { + "opacity-100 max-h-96 overflow-visible": timeRangeOpen, + "opacity-0 max-h-0 overflow-hidden": !timeRangeOpen + })}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx(2 hunks)apps/dashboard/components/logs/datetime/datetime-popover.tsx(3 hunks)apps/dashboard/components/navigation/navbar.tsx(1 hunks)apps/dashboard/components/ui/drover.tsx(2 hunks)apps/dashboard/components/virtual-table/index.tsx(2 hunks)apps/dashboard/hooks/use-responsive.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/hooks/use-responsive.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/dashboard/components/navigation/navbar.tsx
- apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx (1)
apps/dashboard/hooks/use-mobile.tsx (1)
useIsMobile(5-19)
apps/dashboard/components/virtual-table/index.tsx (1)
apps/dashboard/hooks/use-mobile.tsx (1)
useIsMobile(5-19)
apps/dashboard/components/ui/drover.tsx (1)
apps/dashboard/hooks/use-mobile.tsx (1)
useIsMobile(5-19)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test Packages / Test ./apps/dashboard
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/dashboard/components/ui/drover.tsx (1)
3-3: Cleaner responsiveness hook usageThe change from
useResponsivetouseIsMobilesimplifies the code while maintaining the same functionality. This direct hook usage is more focused on the specific need to detect mobile viewports.Also applies to: 38-38
apps/dashboard/components/virtual-table/index.tsx (1)
1-1: Good migration to focused mobile detection hookChanging from
useResponsiveto the focuseduseIsMobilehook maintains the same mobile detection functionality with a simpler implementation. The hook's return value continues to be used consistently throughout the component.Also applies to: 64-64
apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx (1)
8-8: Consistent implementation of mobile detectionThe switch to
useIsMobilemaintains the same functionality while aligning with the codebase-wide refactoring pattern. The boolean value continues to be used appropriately for conditional rendering.Also applies to: 32-32
apps/dashboard/components/logs/datetime/datetime-popover.tsx (1)
7-7: Consistent implementation of mobile detectionSwitching to
useIsMobilealigns with the pattern established in other components while maintaining the same functionality for responsive UI detection.Also applies to: 39-39
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/dashboard/components/logs/checkbox/filter-item.tsx (2)
135-135: Update keyboard shortcut tooltip to match actual implementationThe tooltip references
${shortcut?.toUpperCase()}without mentioning the modifier keys. According to the PR objectives, shortcuts are being changed to useoption+shift+keycombinations. This creates inconsistency between the displayed tooltip and the actual keyboard shortcut implementation.- title={`Press '${shortcut?.toUpperCase()}' to toggle ${label} options`} + title={`Press 'option+shift+${shortcut?.toUpperCase()}' to toggle ${label} options`}
114-128: Improve keyboard accessibility by handling key events on the triggerThe trigger div handles click events but lacks keyboard event handlers for Enter and Space keys. This prevents users from activating the filter item using keyboard navigation.
<div ref={itemRef} className={cn( "flex w-full items-center px-2 py-1.5 justify-between rounded-lg group cursor-pointer", "hover:bg-gray-3 data-[state=open]:bg-gray-3", "focus:outline-none focus:ring-2 focus:ring-accent-7", isFocused && !isActive ? "bg-gray-4" : "", isActive ? "bg-gray-3" : "", )} tabIndex={-1} role="menuitem" aria-haspopup="true" aria-expanded={open} onClick={handleTriggerClick} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + handleTriggerClick(); + } + }} >
🧹 Nitpick comments (3)
apps/dashboard/components/logs/checkbox/filter-item.tsx (3)
69-84: Extend keyboard support for nested menu navigationCurrently, the nested Drover only handles the Escape key for closing the menu, but doesn't implement handlers for arrow keys or Enter/Space for selection in the nested content. Users reported difficulties with keyboard navigation in the PR comments.
Consider implementing a more comprehensive keyboard navigation system that:
- Handles ArrowUp/Down for navigating nested menu items
- Supports Enter/Space for activating selected items
- Maintains focus when navigating through nested submenus
This can be addressed in a future iteration as mentioned in your response to a previous similar comment.
45-50: Consider simplifying focus management with a combined useEffectThe component has multiple useEffect hooks managing focus in different conditions, which increases complexity and could lead to unexpected behavior.
Consider combining related focus management logic into a single useEffect that handles all focus scenarios, making the code more maintainable and reducing the risk of conflicting focus management.
- // biome-ignore lint/correctness/useExhaustiveDependencies: no need to react for label - useEffect(() => { - if (isFocused && !isActive && itemRef.current) { - // Only focus trigger if not active - itemRef.current.focus({ preventScroll: true }); - } - }, [isFocused, isActive, label]); // Depend on isActive too - - // Focus content when drover becomes active and open - // biome-ignore lint/correctness/useExhaustiveDependencies: no need to react for label - useEffect(() => { - if (isActive && open && contentRef.current) { - // Find and focus the first focusable element within the content - const focusableElements = contentRef.current.querySelectorAll<HTMLElement>( - 'button, [href], input:not([type="hidden"]), select, textarea, [tabindex]:not([tabindex="-1"])', - ); - if (focusableElements.length > 0) { - focusableElements[0].focus({ preventScroll: true }); - } else { - // Fallback: focus the content container itself if nothing else is focusable - contentRef.current.focus({ preventScroll: true }); - } - } - }, [isActive, open, label]); // Depend on isActive and open + // biome-ignore lint/correctness/useExhaustiveDependencies: no need to react for label + useEffect(() => { + if (isFocused && !isActive && itemRef.current) { + // Only focus trigger if not active + itemRef.current.focus({ preventScroll: true }); + } else if (isActive && open && contentRef.current) { + // Find and focus the first focusable element within the content + const focusableElements = contentRef.current.querySelectorAll<HTMLElement>( + 'button, [href], input:not([type="hidden"]), select, textarea, [tabindex]:not([tabindex="-1"])', + ); + if (focusableElements.length > 0) { + focusableElements[0].focus({ preventScroll: true }); + } else { + // Fallback: focus the content container itself if nothing else is focusable + contentRef.current.focus({ preventScroll: true }); + } + } + }, [isFocused, isActive, open, label]); // Depend on all relevant state
160-165: Consider adding explicit keyboard focus stylingThe Drover.Content element has tabIndex={-1} which means it can receive programmatic focus, but there's no explicit focus styling in the className.
<Drover.Content ref={contentRef} - className="min-w-60 w-full bg-gray-1 dark:bg-black drop-shadow-2xl p-0 border-gray-6 rounded-lg" + className="min-w-60 w-full bg-gray-1 dark:bg-black drop-shadow-2xl p-0 border-gray-6 rounded-lg focus:outline-none focus:ring-2 focus:ring-accent-7" side="right" align="start" sideOffset={12} onKeyDown={handleItemDroverKeyDown} tabIndex={-1} >Adding focus styling ensures users can see when the content has focus, improving the keyboard navigation experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/components/logs/checkbox/filter-item.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/dashboard/components/logs/checkbox/filter-item.tsx (5)
apps/dashboard/components/logs/checkbox/filters-popover.tsx (1)
FilterItemConfig(17-23)apps/dashboard/components/ui/drover.tsx (1)
Drover(189-198)apps/dashboard/components/keyboard-button.tsx (1)
KeyboardButton(11-38)internal/ui/src/components/button.tsx (1)
Button(443-443)internal/icons/src/icons/caret-right.tsx (1)
CaretRight(15-37)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/components/logs/checkbox/filter-item.tsx (2)
1-8: LGTM! Well-structured importsThe import statements are well-organized, importing components and utilities from appropriate locations.
9-21: LGTM! Well-defined type interfacesThe type definitions for
FilterItemConfigandFilterItemPropsare clear and well-structured, providing good type safety for the component.
2 participants
What does this PR do?
Our current keyboard shortcuts were being hijacked by OS and Browser default shortcuts. So right now they mainly rely on
option+shift+keycombo and just simple letters likeStatus - S, Identity - I. The reason foroption+shiftcombo is they are not used by major browsers.Fixes # (issue)
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?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit