feat: logs v2 selective display #2820
Conversation
ogzhanolguncu
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces significant enhancements to the logs dashboard in the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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! 🙏 |
|
From Oguzhan Olguncu ‣ @chronark this includes feedback changes too, fyi. |
![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
🔭 Outside diff range comments (4)
apps/dashboard/lib/trpc/routers/logs/llm-search.ts (1)
Line range hint
24-26: Verify the model identifier.The model identifier
gpt-4o-2024-08-06appears to be incorrect. OpenAI's model naming typically doesn't include an 'o'.- model: "gpt-4o-2024-08-06", + model: "gpt-4-0125-preview",apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filter-checkbox.tsx (2)
Line range hint
72-84: Consider using native HTML checkbox elements for better accessibility.The current implementation uses a
labelwith a customrole="checkbox". While it works, using native<input type="checkbox">elements would provide better accessibility support out of the box.- <label - className="flex items-center gap-[18px] cursor-pointer" - role="checkbox" - aria-checked={checkboxes.every((checkbox) => checkbox.checked)} - onKeyDown={handleKeyDown} - > + <label className="flex items-center gap-[18px] cursor-pointer"> + <input + type="checkbox" + className="hidden" + checked={checkboxes.every((checkbox) => checkbox.checked)} + onChange={handleSelectAll} + onKeyDown={handleKeyDown} + />🧰 Tools
🪛 Biome (1.9.4)
[error] 92-94: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
Line range hint
92-103: Apply consistent accessibility improvements to individual checkboxes.Similar to the "Select All" checkbox, individual checkboxes should also use native HTML elements.
- <label - className="flex gap-[18px] items-center py-1 cursor-pointer" - role="checkbox" - aria-checked={checkbox.checked} - onKeyDown={(e) => handleKeyDown(e, index)} - > + <label className="flex gap-[18px] items-center py-1 cursor-pointer"> + <input + type="checkbox" + className="hidden" + checked={checkbox.checked} + onChange={() => handleCheckboxChange(index)} + onKeyDown={(e) => handleKeyDown(e, index)} + />apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx (1)
Line range hint
10-66: Remove duplicate path entries in options array.The options array contains duplicate paths which could lead to confusion:
/v1/auth.loginappears at id: 4 and id: 10/v1/auth.logoutappears at id: 5 and id: 11/v1/auth.refreshTokenappears at id: 6 and id: 12/v1/data.deleteappears at id: 7 and id: 13/v1/data.fetchappears at id: 8 and id: 14/v1/data.submitappears at id: 9 and id: 15Remove the duplicate entries or ensure they are intentional. If these paths need to be differentiated, consider adding additional metadata to distinguish them.
🧰 Tools
🪛 Biome (1.9.4)
[error] 139-154: A form label must be associated with an input.
Consider adding a
fororhtmlForattribute to the label element or moving the input element to inside the label element.(lint/a11y/noLabelWithoutControl)
[error] 139-145: The HTML element with the interactive role "checkbox" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "checkbox".
Add a tabIndex attribute to make this element focusable.(lint/a11y/useFocusableInteractive)
[error] 140-142: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
♻️ Duplicate comments (1)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx (1)
140-154: 🛠️ Refactor suggestionApply consistent accessibility improvements from FilterCheckbox component.
The same accessibility concerns apply here. Use native HTML checkbox elements instead of custom roles.
🧰 Tools
🪛 Biome (1.9.4)
[error] 139-154: A form label must be associated with an input.
Consider adding a
fororhtmlForattribute to the label element or moving the input element to inside the label element.(lint/a11y/noLabelWithoutControl)
[error] 140-142: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
🧹 Nitpick comments (15)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-display/index.tsx (1)
10-16: Enhance keyboard interaction hint visibility.The keyboard shortcut hint is only visible in the title attribute. Consider making it more discoverable by adding a visible keyboard shortcut indicator.
<Button variant="ghost" className={cn("group-data-[state=open]:bg-gray-4 px-2")} aria-label="Filter logs" aria-haspopup="true" title="Press 'F' to toggle filters" > <Sliders className="text-accent-9 size-4" /> - <span className="text-accent-12 font-medium text-[13px]">Display</span> + <span className="text-accent-12 font-medium text-[13px]">Display <kbd className="ml-1 text-[10px] text-accent-9">(F)</kbd></span> </Button>apps/dashboard/components/virtual-table/types.ts (2)
1-8: Add JSDoc comments to improve type documentation.The
ColumnWidthtype provides great flexibility, but could benefit from JSDoc comments explaining when to use each option.+/** + * Defines the width configuration for a table column. + * @typedef {ColumnWidth} + */ type ColumnWidth = - | number // Fixed pixel width - | string // CSS values like "165px", "15%", etc. - | "1fr" // Flex grow - | "min" // Minimum width based on content - | "auto" // Automatic width based on content - | { min: number; max: number } // Responsive range - | { flex: number }; // Flex grow ratio + | number /** Fixed pixel width */ + | string /** CSS values like "165px", "15%", etc. */ + | "1fr" /** Flex grow - column takes up remaining space */ + | "min" /** Minimum width based on content */ + | "auto" /** Automatic width based on content */ + | { min: number; max: number } /** Responsive range between min and max pixels */ + | { flex: number }; /** Flex grow ratio relative to other columns */
18-18: Consider adding type for noTruncate metadata.The
noTruncateproperty could benefit from additional metadata to control truncation behavior.- noTruncate?: boolean; // Add this to disable truncation for specific columns + truncation?: { + enabled: boolean; + maxLength?: number; + showTooltip?: boolean; + };apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/status-filter.tsx (2)
Line range hint
12-37: Extract status options configuration.Consider moving the status options to a separate configuration file and expanding the ranges to cover all common HTTP status codes.
Create a new file
status-options.config.ts:export const HTTP_STATUS_RANGES = { SUCCESS: { range: [200, 299], display: "2xx", label: "Success", colorClass: "bg-success-9" }, CLIENT_ERROR: { range: [400, 499], display: "4xx", label: "Warning", colorClass: "bg-warning-8" }, SERVER_ERROR: { range: [500, 599], display: "5xx", label: "Error", colorClass: "bg-error-9" } } as const; export const createStatusOptions = () => Object.entries(HTTP_STATUS_RANGES).map(([key, config], index) => ({ id: index + 1, status: config.range[0], display: config.display, label: config.label, color: config.colorClass, checked: false }));
Line range hint
54-63: Simplify status color logic using the configuration.The color logic in
createFilterValuecan be simplified by using the status ranges configuration.createFilterValue={(option) => ({ value: option.status, metadata: { - colorClass: - option.status >= 500 - ? "bg-error-9" - : option.status >= 400 - ? "bg-warning-8" - : "bg-success-9", + colorClass: Object.values(HTTP_STATUS_RANGES).find( + ({range: [min, max]}) => option.status >= min && option.status <= max + )?.colorClass ?? "bg-accent-9" }, })}apps/dashboard/app/(app)/logs-v2/components/table/log-details/index.tsx (1)
35-37: Consider adding error handling for context-related errors.While the implementation is correct, consider wrapping the component with an error boundary to gracefully handle potential context-related errors.
class LogDetailsErrorBoundary extends React.Component<{children: React.ReactNode}> { componentDidCatch(error: Error) { // Handle context errors gracefully console.error('Failed to load log details:', error); } render() { return this.props.children; } }apps/dashboard/app/(app)/logs-v2/context/logs.tsx (2)
26-32: Consider making default properties immutable.To prevent accidental modifications, consider making the default properties array readonly.
-const DEFAULT_DISPLAY_PROPERTIES: DisplayProperty[] = [ +const DEFAULT_DISPLAY_PROPERTIES: readonly DisplayProperty[] = [
54-65: Consider memoizing the context value.To prevent unnecessary re-renders of context consumers, consider memoizing the context value.
+import { useMemo } from 'react'; export const LogsProvider = ({ children }: PropsWithChildren) => { const [selectedLog, setSelectedLog] = useState<Log | null>(null); const [displayProperties, setDisplayProperties] = useState<Set<DisplayProperty>>( new Set(DEFAULT_DISPLAY_PROPERTIES), ); + const value = useMemo( + () => ({ + selectedLog, + setSelectedLog, + displayProperties, + toggleDisplayProperty, + }), + [selectedLog, displayProperties] + ); return ( - <LogsContext.Provider - value={{ - selectedLog, - setSelectedLog, - displayProperties, - toggleDisplayProperty, - }} - > + <LogsContext.Provider value={value}> {children} </LogsContext.Provider> ); };apps/dashboard/app/(app)/logs-v2/hooks/use-keyboard-shortcut.tsx (1)
36-46: Consider extracting modifier check logic.The modifier check logic could be extracted into a separate function for better readability and reusability.
+const hasUnwantedModifiers = (combo: KeyCombo, event: KeyboardEvent): boolean => { + return ( + (combo.ctrl === undefined && event.ctrlKey) || + (combo.meta === undefined && event.metaKey) || + (combo.shift === undefined && event.shiftKey) || + (combo.alt === undefined && event.altKey) + ); +}; -const hasUnwantedModifiers = - (combo.ctrl === undefined && e.ctrlKey) || - (combo.meta === undefined && e.metaKey) || - (combo.shift === undefined && e.shiftKey) || - (combo.alt === undefined && e.altKey); +if (hasUnwantedModifiers(combo, e)) { + return; +}apps/dashboard/components/virtual-table/components/table-row.tsx (1)
65-65: Consider memoizing the grid template calculation.The
gridTemplateColumnscalculation could be expensive for tables with many columns. Consider memoizing the result usinguseMemoto optimize performance.- const gridTemplateColumns = calculateGridTemplateColumns(columns); + const gridTemplateColumns = useMemo( + () => calculateGridTemplateColumns(columns), + [columns] + );Also applies to: 109-113
apps/dashboard/app/(app)/logs-v2/filters.schema.ts (1)
57-102: Consider adding input validation for the status field.While the function handles status conversion well, it should validate the numeric value before pushing to filters.
if (filterGroup.field === "status") { const numericValue = typeof filter.value === "string" ? Number.parseInt(filter.value) : filter.value; + if (isNaN(numericValue) || numericValue < 100 || numericValue > 599) { + return; // Skip invalid status codes + } uniqueFilters.push({ id: crypto.randomUUID(),apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-display/components/display-popover.tsx (2)
78-130: Consider memoizing keyboard navigation calculations.The
handleKeyNavigationfunction recalculates layout dimensions on every key press. Consider memoizing these calculations.+ const itemsPerRow = useMemo( + () => Math.floor(384 / 120), + [] + ); + const handleKeyNavigation = useCallback( (e: KeyboardEvent) => { - const itemsPerRow = Math.floor(384 / 120); // Approximate width / item width const totalItems = DISPLAY_PROPERTIES.length;
13-25: Consider moving DISPLAY_PROPERTIES to a separate constants file.The display properties array could be reused across components. Consider moving it to a dedicated constants file.
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx (1)
Line range hint
89-98: Consider debouncing the scroll handler.The scroll event handler could fire frequently during scrolling. Consider debouncing the handler for better performance.
import { debounce } from 'lodash'; // Inside component: const debouncedHandleScroll = useCallback( debounce(() => { if (scrollContainerRef.current) { const { scrollTop, scrollHeight, clientHeight } = scrollContainerRef.current; const isBottom = Math.abs(scrollHeight - clientHeight - scrollTop) < 1; setIsAtBottom(isBottom); } }, 100), [] );🧰 Tools
🪛 Biome (1.9.4)
[error] 139-154: A form label must be associated with an input.
Consider adding a
fororhtmlForattribute to the label element or moving the input element to inside the label element.(lint/a11y/noLabelWithoutControl)
[error] 139-145: The HTML element with the interactive role "checkbox" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "checkbox".
Add a tabIndex attribute to make this element focusable.(lint/a11y/useFocusableInteractive)
[error] 140-142: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
apps/dashboard/app/(app)/logs-v2/components/table/logs-table.tsx (1)
100-118: Consider memoizing additional columns.The additionalColumns array is recreated on every render. Consider moving it outside the component or memoizing it with useMemo to optimize performance.
-const additionalColumns: Column<Log>[] = [ +const additionalColumns: Column<Log>[] = useMemo(() => [ // ... column definitions -].map((key) => ({ +], []).map((key) => ({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/dashboard/app/(app)/logs-v2/components/control-cloud/index.tsx(4 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-display/components/display-popover.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-display/index.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filter-checkbox.tsx(3 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filters-popover.tsx(8 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx(2 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/status-filter.tsx(3 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-search/index.tsx(7 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/index.tsx(2 hunks)apps/dashboard/app/(app)/logs-v2/components/logs-client.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/table/log-details/index.tsx(2 hunks)apps/dashboard/app/(app)/logs-v2/components/table/logs-table.tsx(6 hunks)apps/dashboard/app/(app)/logs-v2/context/logs.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/filters.schema.ts(2 hunks)apps/dashboard/app/(app)/logs-v2/hooks/use-keyboard-shortcut.tsx(1 hunks)apps/dashboard/components/virtual-table/components/table-row.tsx(3 hunks)apps/dashboard/components/virtual-table/types.ts(1 hunks)apps/dashboard/lib/trpc/routers/logs/llm-search.ts(1 hunks)
👮 Files not reviewed due to content moderation or server errors (1)
- apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-search/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-display/components/display-popover.tsx
[error] 46-47: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filter-checkbox.tsx
[error] 72-74: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx
[error] 140-142: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 163-165: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- 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/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
apps/dashboard/app/(app)/logs-v2/components/logs-client.tsx (1)
19-25: Great architectural improvement!Moving to context-based state management with
LogsProvidereffectively eliminates prop drilling and centralizes log state management. This change improves code maintainability and component reusability.apps/dashboard/app/(app)/logs-v2/components/controls/index.tsx (1)
1-2: LGTM! Clean replacement of static display control with dynamic component.The changes effectively replace the static display control with the new
LogsDisplaycomponent, aligning with the PR's objective of implementing selective display functionality.Also applies to: 32-33
apps/dashboard/app/(app)/logs-v2/components/table/log-details/index.tsx (1)
5-5: LGTM! Good migration to context-based state management.The shift from prop-based to context-based state management reduces prop drilling and improves maintainability.
Also applies to: 27-28
apps/dashboard/app/(app)/logs-v2/context/logs.tsx (1)
6-17: LGTM! Well-defined type-safe display properties.The DisplayProperty type ensures type safety for all possible display options.
apps/dashboard/app/(app)/logs-v2/hooks/use-keyboard-shortcut.tsx (1)
36-46: LGTM! Good enhancement to prevent unwanted shortcut triggers.The addition of unwanted modifier detection prevents shortcuts from triggering when users accidentally press additional modifier keys.
apps/dashboard/components/virtual-table/components/table-row.tsx (1)
5-40: LGTM! Well-structured column width calculation logic.The implementation handles various width specifications comprehensively and provides good fallbacks.
apps/dashboard/lib/trpc/routers/logs/llm-search.ts (1)
Line range hint
57-76: LGTM! Comprehensive error handling.The error handling is robust, with specific error messages for different scenarios and proper error logging.
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx (1)
Line range hint
163-175: Apply consistent accessibility improvements to individual checkboxes.The same accessibility improvements should be applied to individual checkboxes.
🧰 Tools
🪛 Biome (1.9.4)
[error] 161-168: The HTML element with the interactive role "checkbox" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "checkbox".
Add a tabIndex attribute to make this element focusable.(lint/a11y/useFocusableInteractive)
[error] 163-165: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
apps/dashboard/app/(app)/logs-v2/components/control-cloud/index.tsx (3)
30-34: LGTM! Consistent status code formatting.The status code formatting is now consistently using lowercase 'xx' suffix, which improves readability.
109-111: LGTM! Enhanced keyboard navigation.The addition of the Command+C shortcut to focus filters improves accessibility and user experience.
213-222: LGTM! Improved layout structure.The new layout with separate rows for "Clear filters" and "Focus filters" improves visual organization and makes keyboard shortcuts more discoverable.
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filters-popover.tsx (3)
23-23: LGTM! Improved keyboard shortcut.Changed status filter shortcut from 's' to 'e' to avoid conflict with the search shortcut.
45-51: LGTM! Proper cleanup of filter state.Added cleanup effect to prevent unnecessary renders when opening/closing the popover.
178-194: LGTM! Enhanced focus management.The new focus management logic properly handles:
- Focus transfer to sub-popover
- First focusable element selection
- Fallback focus behavior
apps/dashboard/app/(app)/logs-v2/components/table/logs-table.tsx (2)
89-98: LGTM! Well-structured warning icon component.The WarningIcon component effectively handles different status ranges and maintains consistent styling.
206-227: LGTM! Robust column visibility management.The visibleColumns implementation effectively:
- Combines basic and additional columns
- Filters based on display properties
- Properly handles the warning icon integration
...app/(app)/logs-v2/components/controls/components/logs-display/components/display-popover.tsx
Show resolved
Hide resolved
|
From Oguzhan Olguncu ‣ @chronark give it a try |
|
after the gym |
|
From Oguzhan Olguncu ‣ Can we move comments to main branch and merge this first? |
|
sure |
|
if you figure out how to move them |
|
From Oguzhan Olguncu ‣ leave it then i'll fix it tomorrow and merge it. it's not that urgent |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-search/index.tsx (3)
Line range hint
13-48: Consider extracting toast configuration to constants.The toast configuration (duration, position, style) is duplicated. Consider extracting these to shared constants to improve maintainability.
+const TOAST_CONFIG = { + duration: 8000, + important: true, + position: "top-right" as const, + style: { + whiteSpace: "pre-line", + }, +}; const queryLLMForStructuredOutput = trpc.logs.llmSearch.useMutation({ onSuccess(data) { if (data) { const transformedFilters = transformStructuredOutputToFilters(data, filters); updateFilters(transformedFilters); } else { - toast.error("Try to be more descriptive about your query", { - duration: 8000, - important: true, - position: "top-right", - style: { - whiteSpace: "pre-line", - }, - }); + toast.error("Try to be more descriptive about your query", TOAST_CONFIG); } }, onError(error) { - toast.error(error.message, { - duration: 8000, - important: true, - position: "top-right", - style: { - whiteSpace: "pre-line", - }, - }); + toast.error(error.message, TOAST_CONFIG); }, });
80-115: Consider extracting UI text to constants.The loading message "AI consults the Palantír..." is hardcoded. Consider moving it to a constants file for better maintainability and potential internationalization.
+const UI_TEXT = { + LOADING_MESSAGE: "AI consults the Palantír...", + PLACEHOLDER: "Search and filter with AI…", +}; {isLoading ? ( <div className="text-accent-11 text-sm animate-pulse"> - AI consults the Palantír... + {UI_TEXT.LOADING_MESSAGE} </div> ) : ( <input // ... - placeholder="Search and filter with AI…" + placeholder={UI_TEXT.PLACEHOLDER} // ... /> )}
Line range hint
118-168: Consider increasing tooltip delay for better accessibility.The current tooltip delay of 150ms might be too short for users who need more time to read or process the information. Consider increasing it to at least 300ms for better accessibility.
-<Tooltip delayDuration={150}> +<Tooltip delayDuration={300}>apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-display/components/display-popover.tsx (2)
65-75: Add cleanup for keyboard shortcut and improve focus management.Consider these improvements:
- Add cleanup for keyboard shortcut subscription
- Consider using
useCallbackfor the toggle handler- Add focus trap for better keyboard navigation
useKeyboardShortcut("d", () => { setOpen((prev) => !prev); if (!open) { setFocusedIndex(0); } -}); +}, [open]); +// Add focus trap +useEffect(() => { + if (!open) return; + + const handleTabKey = (e: KeyboardEvent) => { + if (e.key === 'Tab') { + e.preventDefault(); + } + }; + + document.addEventListener('keydown', handleTabKey); + return () => document.removeEventListener('keydown', handleTabKey); +}, [open]);
79-80: Extract magic numbers into named constants.The layout calculation uses magic numbers which could make maintenance difficult. Consider extracting these into named constants or calculating them dynamically based on the container width.
-const itemsPerRow = Math.floor(384 / 120); // Approximate width / item width +const POPOVER_WIDTH = 384; +const ITEM_WIDTH = 120; +const itemsPerRow = Math.floor(POPOVER_WIDTH / ITEM_WIDTH);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/dashboard/app/(app)/logs-v2/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/control-cloud/index.tsx(4 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-display/components/display-popover.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/index.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-search/index.tsx(6 hunks)apps/dashboard/app/(app)/logs-v2/context/logs.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/logs-v2/context/logs.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-display/components/display-popover.tsx
[error] 45-46: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- 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/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-search/index.tsx (2)
1-12: LGTM! Well-organized imports and hooks setup.The imports are logically grouped and the new additions support the enhanced filtering functionality.
71-76: LGTM! Clean preset query handling.The handlePresetQuery function is well-implemented, following the single responsibility principle.
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-display/components/display-popover.tsx (4)
1-24: LGTM! Well-structured imports and constants.The imports are well-organized, and the
DISPLAY_PROPERTIESarray provides a clear, maintainable way to define display properties.
26-56: Replace div with button element for better accessibility.This is a duplicate of a previous review comment. The
DisplayPropertyItemcomponent should use a native button element instead of a div with role="button" for better accessibility.🧰 Tools
🪛 Biome (1.9.4)
[error] 45-46: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
58-63: LGTM! Clean and focused header component.The
PopoverHeadercomponent is well-structured with clear responsibility for displaying the title and keyboard shortcut.
118-125: Add error handling for invalid property access.The property access at
DISPLAY_PROPERTIES[focusedIndex]could potentially be undefined iffocusedIndexis out of bounds. Consider adding error handling.if (focusedIndex !== null) { + if (focusedIndex >= DISPLAY_PROPERTIES.length) { + console.error('Invalid focus index'); + return; + } const prop = DISPLAY_PROPERTIES[focusedIndex]; if (isDisplayProperty(prop.id)) { toggleDisplayProperty(prop.id); } }apps/dashboard/app/(app)/logs-v2/components/charts/index.tsx (1)
36-36: Verify if removing seconds from timestamp affects debugging capabilities.While the simplified timestamp format improves readability, consider if second-level precision is important for debugging scenarios in the logs dashboard.
Let's check for timestamp format consistency across the codebase:
✅ Verification successful
Simplified timestamp format in chart tooltips is acceptable ✅
The removal of seconds from chart tooltips doesn't impact debugging capabilities because:
- Full timestamp precision (including seconds and milliseconds) is preserved in the log details view
- Chart tooltips are for overview purposes, where second-level precision is less critical
- Detailed timestamps are still available when needed for debugging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for date formatting patterns to ensure consistency rg -A 2 "format\([^,]+,\s*['\"].*(?:HH|hh|mm|ss|SS).*['\"]" --type tsLength of output: 3354
apps/dashboard/app/(app)/logs-v2/components/control-cloud/index.tsx (3)
30-34: LGTM! Status code format change improves readability.The change from uppercase (XX) to lowercase (xx) in status codes follows common conventions and improves readability.
199-199: LGTM! UI improvements enhance usability.The changes improve the UI by:
- Better spacing with
items-startalignment- Clear visual hierarchy with divider
- Consistent keyboard shortcut indicators
Also applies to: 212-217
109-111: Consider potential conflict with browser shortcuts.The Cmd+C (⌘+C) shortcut might conflict with the browser's copy functionality. Consider using a different key combination or implementing a check to prevent interference with native browser shortcuts.
Alternative implementation:
- useKeyboardShortcut({ key: "c", meta: true }, () => { + useKeyboardShortcut({ key: "f", meta: true }, () => { setFocusedIndex(0); });
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-search/index.tsx
Show resolved
Hide resolved
|
From Oguzhan Olguncu ‣ @perkinsjr Can you skim over this when you are free? See how it looks give feedback on main branch. I'll rebase from this one and try to open PR for data fetching |
|
Sure I will do it now. |
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps 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
Release Notes: Logs Dashboard Improvements
New Features
Enhancements
UI/UX Updates
Performance