Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request refactors multiple logs dashboard and ratelimits components. It replaces custom chart implementations with a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant Chart as LogsTimeseriesBarChart
participant LC as LogsChart/RatelimitLogsChart
participant UF as updateFilters
U->>Chart: Drag/select time range
Chart->>LC: onSelectionChange({start, end})
LC->>UF: updateFilters(timeRange)
UF-->>LC: Filters updated
sequenceDiagram
participant U as User
participant CC as ControlCloud
participant CP as ControlPill
participant UF as updateFilters
U->>CC: Interact with filter (focus, remove)
CC->>CP: Render filter item with formatting
U->>CC: Trigger filter removal (key press/click)
CC->>UF: updateFilters(filter removal)
UF-->>CC: Filters updated
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
apps/dashboard/app/(app)/logs/components/table/hooks/use-logs-query.ts (1)
58-68: Improve error handling for method filters.The error handling for method filters could be more robust.
case "methods": { - if (typeof filter.value !== "string") { - console.error("Method filter value type has to be 'string'"); - return; - } + if (typeof filter.value !== "string") { + console.error(`Invalid method filter value: expected string, got ${typeof filter.value}`); + return; + } + if (!["GET", "POST", "PUT", "DELETE", "PATCH"].includes(filter.value.toUpperCase())) { + console.warn(`Unexpected HTTP method: ${filter.value}`); + } params.method?.filters.push({ operator: "is", - value: filter.value, + value: filter.value.toUpperCase(), }); break; }
🧹 Nitpick comments (20)
apps/dashboard/components/logs/llm-search/index.tsx (2)
20-29: Improve error handling.The error is only logged to the console, which might not be sufficient for production environments. Consider adding error reporting or analytics.
Apply this diff to improve error handling:
try { onSearch(query); } catch (error) { - console.error("Search failed:", error); + // Log to error reporting service + reportError?.(error); + // Optionally, notify the user + throw error; // Let the parent component handle the error }
67-84: Consider adding aria-live for loading state.The loading message should be announced to screen readers.
Apply this diff to improve accessibility:
- <div className="text-accent-11 text-[13px] animate-pulse"> + <div + className="text-accent-11 text-[13px] animate-pulse" + aria-live="polite" + role="status" + > AI consults the Palantír... </div>apps/dashboard/app/(app)/logs/components/controls/components/logs-refresh.tsx (1)
12-15: Add error handling for query invalidation.Consider adding error handling for the trpc query invalidation to gracefully handle potential failures.
const handleRefresh = () => { - logs.queryLogs.invalidate(); - logs.queryTimeseries.invalidate(); + try { + logs.queryLogs.invalidate(); + logs.queryTimeseries.invalidate(); + } catch (error) { + console.error('Failed to refresh logs:', error); + // Consider showing a user-friendly error notification + } };apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-refresh.tsx (2)
12-15: Add error handling for query invalidation.Consider adding error handling for the trpc query invalidation to gracefully handle potential failures.
const handleRefresh = () => { - ratelimit.logs.query.invalidate(); - ratelimit.logs.queryRatelimitTimeseries.invalidate(); + try { + ratelimit.logs.query.invalidate(); + ratelimit.logs.queryRatelimitTimeseries.invalidate(); + } catch (error) { + console.error('Failed to refresh ratelimit logs:', error); + // Consider showing a user-friendly error notification + } };
1-26: Consider creating a shared higher-order component.Both logs and ratelimits implementations are nearly identical. Consider creating a shared higher-order component to reduce code duplication.
// shared/createLogsRefresh.tsx type LogsRefreshConfig = { useLogsContext: () => { toggleLive: (value: boolean) => void; isLive: boolean }; trpcUtils: { invalidateQueries: () => void; }; }; export const createLogsRefresh = (config: LogsRefreshConfig) => { return function LogsRefresh() { const { toggleLive, isLive } = config.useLogsContext(); const { filters } = useFilters(); const hasRelativeFilter = filters.find((f) => f.field === "since"); const handleRefresh = () => { try { config.trpcUtils.invalidateQueries(); } catch (error) { console.error('Failed to refresh logs:', error); } }; return ( <RefreshButton onRefresh={handleRefresh} isEnabled={Boolean(hasRelativeFilter)} isLive={isLive} toggleLive={toggleLive} /> ); }; };apps/dashboard/components/logs/refresh-button/index.tsx (2)
14-14: Move constant to a shared constants file.Consider moving
REFRESH_TIMEOUT_MSto a shared constants file for better maintainability.-const REFRESH_TIMEOUT_MS = 1000; +import { REFRESH_TIMEOUT_MS } from '@/constants/timeouts';
20-22: Consider debouncing keyboard shortcut handler.Add debouncing to prevent rapid-fire refreshes from keyboard shortcuts.
+import { useDebouncedCallback } from 'use-debounce'; export const RefreshButton = ({ onRefresh, isEnabled, isLive, toggleLive }: RefreshButtonProps) => { + const debouncedHandleRefresh = useDebouncedCallback( + () => isEnabled && handleRefresh(), + 300, + { leading: true, trailing: false } + ); + - useKeyboardShortcut("r", () => { - isEnabled && handleRefresh(); - }); + useKeyboardShortcut("r", debouncedHandleRefresh);apps/dashboard/components/logs/checkbox/filters-popover.tsx (1)
139-141: Shortcut binding with meta key.
Attaching a shortcut to each filter item is a helpful accessibility feature. Make sure to document these shortcuts for users.apps/dashboard/components/logs/control-cloud/utils.ts (2)
3-8: LGTM! Consider adding JSDoc comments.The function correctly handles both string and number types, with special handling for timestamps.
Add JSDoc comments to document the function's purpose and parameters:
+/** + * Formats a value based on its type and field. + * @param value - The value to format + * @param field - The field name to determine formatting + * @returns The formatted value as a string + */ export const defaultFormatValue = (value: string | number, field: string): string => {
10-15: LGTM! Consider adding JSDoc comments.The function correctly handles the special case for the "since" field.
Add JSDoc comments to document the function's purpose and parameters:
+/** + * Formats an operator based on the field. + * @param operator - The operator to format + * @param field - The field name to determine formatting + * @returns The formatted operator as a string + */ export const formatOperator = (operator: string, field: string): string => {apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/index.tsx (1)
9-22: Consider extracting shared filter configurations.There appears to be code duplication between this file and
apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/index.tsx, where both define similarFILTER_ITEMSconfigurations. Consider extracting common filter configurations into a shared constants file.+// In shared/constants/filters.ts +export const COMMON_FILTER_ITEMS: FilterItemConfig[] = [ + { + id: "status", + label: "Status", + shortcut: "e", + component: <StatusFilter />, + }, +]; +// In current file +import { COMMON_FILTER_ITEMS } from '@/shared/constants/filters'; + +const FILTER_ITEMS: FilterItemConfig[] = [ + ...COMMON_FILTER_ITEMS, + { + id: "identifiers", + label: "Identifier", + shortcut: "p", + component: <IdentifiersFilter />, + }, +];apps/dashboard/app/(app)/logs/components/control-cloud/index.tsx (1)
6-25: Enhance type safety for field name formatting.Consider using TypeScript's discriminated unions to ensure type safety for field names.
+type FieldName = + | "startTime" + | "endTime" + | "status" + | "paths" + | "methods" + | "requestId" + | "since"; -const formatFieldName = (field: string): string => { +const formatFieldName = (field: FieldName): string => { switch (field) { case "startTime": return "Start time"; // ... rest of the cases default: - return field.charAt(0).toUpperCase() + field.slice(1); + const _exhaustiveCheck: never = field; + return _exhaustiveCheck; } };apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx (1)
17-43: Consider memoizing the selection handler.The selection handler is recreated on every render. Consider using useCallback to memoize it for better performance.
+import { useCallback } from 'react'; - const handleSelectionChange = ({ + const handleSelectionChange = useCallback(({ start, end, }: { start: number; end: number; }) => { // ... handler implementation - }; + }, [filters, updateFilters]);apps/dashboard/components/logs/control-cloud/control-pill.tsx (1)
38-43: Consider enhancing keyboard event handling.The current implementation only handles Backspace and Delete keys. Consider adding support for Escape key to blur/unfocus the pill.
const handleKeyDown = (e: KeyboardEvent) => { if (e.key === "Backspace" || e.key === "Delete") { e.preventDefault(); onRemove(filter.id); } + if (e.key === "Escape") { + e.preventDefault(); + e.currentTarget.blur(); + } };apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/paths-filter.tsx (1)
31-44: Consider memoizing selectedPaths calculation.The
selectedPathscalculation could be memoized to avoid unnecessary recalculations on re-renders.+const selectedPaths = useMemo( + () => checkboxes.filter((c) => c.checked).map((c) => c.path), + [checkboxes] +); const handleApplyFilter = useCallback(() => { - const selectedPaths = checkboxes.filter((c) => c.checked).map((c) => c.path); - // Keep all non-paths filters and add new path filters const otherFilters = filters.filter((f) => f.field !== "paths"); const pathFilters: LogsFilterValue[] = selectedPaths.map((path) => ({ id: crypto.randomUUID(), field: "paths", operator: "is", value: path, })); updateFilters([...otherFilters, ...pathFilters]); -}, [checkboxes, filters, updateFilters]); +}, [selectedPaths, filters, updateFilters]);apps/dashboard/components/logs/checkbox/filter-checkbox.tsx (1)
54-59: Improve type safety in newFilters creation.The
newFiltersarray is typed asLogsFilterValue[]but should use the genericTFiltertype for better type safety.- const newFilters: LogsFilterValue[] = selectedValues.map((filterValue) => ({ + const newFilters: TFilter[] = selectedValues.map((filterValue) => ({ id: crypto.randomUUID(), field: filterField, operator: "is", ...filterValue, }));apps/dashboard/components/logs/control-cloud/index.tsx (2)
8-15: Props type could be more specific.The
TFilter extends FilterValuegeneric type could be more restrictive. Consider adding constraints for required fields.-type ControlCloudProps<TFilter extends FilterValue> = { +type ControlCloudProps<TFilter extends FilterValue & { id: string; field: string }> = {
27-43: Consider debouncing keyboard shortcuts.The keyboard shortcuts for adding time filters could trigger multiple times if held down.
Consider wrapping the callback in a debounce function:
+import { debounce } from "lodash"; -useKeyboardShortcut({ key: "d", meta: true }, () => { +useKeyboardShortcut({ key: "d", meta: true }, debounce(() => { const timestamp = Date.now(); updateFilters([/*...*/] as TFilter[]); -}); +}, 300));apps/dashboard/app/(app)/logs/components/table/hooks/use-logs-query.ts (1)
146-184: Consider implementing retry logic for polling.The polling implementation could benefit from retry logic for failed requests.
const pollForNewLogs = useCallback(async () => { + let retries = 3; try { const latestTime = realtimeLogs[0]?.time ?? historicalLogs[0]?.time; const result = await queryClient.logs.queryLogs.fetch({/*...*/}); // ... rest of the implementation } catch (error) { console.error("Error polling for new logs:", error); + if (retries > 0) { + retries--; + await new Promise(resolve => setTimeout(resolve, 1000)); + return pollForNewLogs(); + } } }, [/*...*/]);apps/dashboard/components/logs/chart/index.tsx (1)
156-202: Memoize tooltip content for better performance.The tooltip content function could be memoized to prevent unnecessary re-renders.
+const TooltipContent = memo(({ active, payload, label }: TooltipProps<any, any>) => { if (!active || !payload?.length || payload?.[0]?.payload.total === 0) { return null; } return (/* ... */); +}); +TooltipContent.displayName = 'TooltipContent'; // In the ChartTooltip component: -content={({ active, payload, label }) => {/*...*/}} +content={TooltipContent}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
apps/dashboard/app/(app)/logs/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/control-cloud/index.tsx(2 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/filters-popover.tsx(0 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/methods-filter.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/paths-filter.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/status-filter.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/index.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-live-switch.tsx(2 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-refresh.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-search/index.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/table/hooks/use-logs-query.ts(1 hunks)apps/dashboard/app/(app)/logs/constants.ts(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/components/logs-chart-error.tsx(0 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/components/logs-chart-loading.tsx(0 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/utils/calculate-timepoints.ts(0 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/utils/format-timestamp.ts(0 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/control-cloud/index.tsx(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/filter-checkbox.tsx(0 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/hooks/use-checkbox-state.ts(0 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/identifiers-filter.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/status-filter.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-live-switch.tsx(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-refresh.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-search/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.ts(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/constants.ts(1 hunks)apps/dashboard/components/logs/chart/index.tsx(1 hunks)apps/dashboard/components/logs/checkbox/filter-checkbox.tsx(3 hunks)apps/dashboard/components/logs/checkbox/filters-popover.tsx(6 hunks)apps/dashboard/components/logs/checkbox/hooks/index.ts(1 hunks)apps/dashboard/components/logs/control-cloud/control-pill.tsx(1 hunks)apps/dashboard/components/logs/control-cloud/index.tsx(1 hunks)apps/dashboard/components/logs/control-cloud/utils.ts(1 hunks)apps/dashboard/components/logs/live-switch-button/index.tsx(1 hunks)apps/dashboard/components/logs/llm-search/index.tsx(1 hunks)apps/dashboard/components/logs/refresh-button/index.tsx(1 hunks)apps/dashboard/components/virtual-table/index.tsx(1 hunks)apps/dashboard/lib/trpc/routers/logs/llm-search.ts(0 hunks)
💤 Files with no reviewable changes (8)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/components/logs-chart-error.tsx
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/components/logs-chart-loading.tsx
- apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/filters-popover.tsx
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/utils/format-timestamp.ts
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/hooks/use-checkbox-state.ts
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/utils/calculate-timepoints.ts
- apps/dashboard/lib/trpc/routers/logs/llm-search.ts
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/filter-checkbox.tsx
✅ Files skipped from review due to trivial changes (7)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/constants.ts
- apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/status-filter.tsx
- apps/dashboard/app/(app)/logs/constants.ts
- apps/dashboard/components/virtual-table/index.tsx
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/status-filter.tsx
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.ts
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/identifiers-filter.tsx
🧰 Additional context used
🧠 Learnings (3)
apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/index.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2801
File: apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/methods-filter.tsx:39-62
Timestamp: 2025-01-10T10:09:42.433Z
Learning: In the logs-v2 filters implementation, handler improvements and type safety enhancements should be deferred until real data integration, as the handlers will need to be modified to work with the actual data source.
apps/dashboard/app/(app)/logs/components/controls/components/logs-live-switch.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2825
File: apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-datetime/index.tsx:0-0
Timestamp: 2025-01-30T20:38:00.058Z
Learning: In the logs dashboard, keyboard shortcuts that toggle UI elements (like popovers) should be implemented in the component that owns the state being toggled, not in the presentational wrapper components. For example, the 'T' shortcut for toggling the datetime filter is implemented in DatetimePopover, not in LogsDateTime.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/index.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2801
File: apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/methods-filter.tsx:39-62
Timestamp: 2025-01-10T10:09:42.433Z
Learning: In the logs-v2 filters implementation, handler improvements and type safety enhancements should be deferred until real data integration, as the handlers will need to be modified to work with the actual data source.
⏰ 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: Test Packages / Test ./internal/billing
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (29)
apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/methods-filter.tsx (2)
1-1: LGTM! Import path change aligns with the refactoring goals.The change from relative to absolute imports improves maintainability and follows a consistent pattern across the codebase.
3-32: LGTM! Clean and well-structured component implementation.The component is well-typed, follows single responsibility principle, and effectively uses the reusable
FilterCheckboxcomponent.A few positive observations:
- Strong typing with TypeScript
- Constant options defined outside the component
- Clear and descriptive prop names
- Simple and focused rendering logic
apps/dashboard/app/(app)/logs/components/controls/components/logs-search/index.tsx (1)
46-54: LGTM! Clean implementation using the new LogsLLMSearch component.The component has been nicely refactored to use the new
LogsLLMSearchcomponent, making it more maintainable and reusable.apps/dashboard/components/logs/llm-search/index.tsx (1)
31-40: LGTM! Good keyboard interaction implementation.The keyboard event handling is well implemented with both Escape and Enter key support.
apps/dashboard/app/(app)/logs/components/controls/components/logs-refresh.tsx (1)
17-24: LGTM! Clean implementation with good separation of concerns.The component properly delegates refresh functionality to the shared RefreshButton component while maintaining its specific business logic.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/control-cloud/index.tsx (2)
1-2: Imports look consistent.
No issues found. The new import paths forControlCloudandHISTORICAL_DATA_WINDOWmatch the intended refactor and appear to be correct.
25-33: Clean integration with the newControlCloud.
The usage offilters,updateFilters, andremoveFilterhere looks straightforward and conforms to the new filter refactor. The functionRatelimitLogsControlCloudeffectively delegates logic toControlCloud, enhancing maintainability.apps/dashboard/app/(app)/logs/components/charts/index.tsx (2)
15-41: Verify time-based filter updates.
ThehandleSelectionChangeproperly removesstartTime,endTime, andsincefilters before adding new ones with updated values. One small caution: ensure thatcrypto.randomUUID()is consistently available in all targeted browsers/environments or add a fallback if necessary.
44-68: Adoption ofLogsTimeseriesBarChartlooks good.
PassingenableSelectionand leveragingonSelectionChangefor filter updates is a clean approach. Configuration for success, warning, and error states is organized and consistent.apps/dashboard/components/logs/checkbox/filters-popover.tsx (8)
7-9: Well-structured type definitions.
DefiningFilterItemConfigandFiltersPopoverPropsclarifies the component’s contract. The default implementation ofgetFilterCountis a neat fallback for counting active filters.Also applies to: 16-20
22-27: Clear property-driven approach forFiltersPopover.
Injecting filter items and relying on props for configuration is a beneficial move toward modular design. This improves reusability and testability.
32-34: Cleanup logic foractiveFilteron unmount.
SettingactiveFiltertonullin the cleanup effect is a good practice to avoid stale references.
59-59: Arrow key navigation for filter items.
Cycling through items with modulo arithmetic is a clean approach. Make sure to handle edge cases whenitemsis empty.Also applies to: 64-64
92-92: Rendering filter items with dynamic counts.
Mapping overitemsand computing counts viagetFilterCount(item.id)ensures each filter type is tracked and displayed accurately.Also applies to: 95-95
106-111:PopoverHeaderextraction improves readability.
Defining a dedicated header component keeps the markup concise and helps maintain the open/close logic more cleanly.
116-116:FilterItemprops incorporate filter counts effectively.
AcceptingfilterCountas a prop rather than computing it internally aligns with the new modular filtering approach.Also applies to: 119-119, 125-125
190-191: Visibility of nonzero filter counts.
Conditionally displaying the filter count badge provides intuitive UI feedback. Using a small, rounded element is a neat, minimal approach.apps/dashboard/app/(app)/logs/components/controls/components/logs-live-switch.tsx (1)
1-2: LGTM! Good refactoring.The changes improve code organization by:
- Using a dedicated LiveSwitchButton component
- Centralizing constants
- Moving keyboard shortcut to the component that owns the state
Also applies to: 33-33
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-live-switch.tsx (1)
1-2: LGTM! Good consistency.The changes maintain consistency with the logs version while using the appropriate context for ratelimits.
Also applies to: 33-33
apps/dashboard/components/logs/live-switch-button/index.tsx (3)
6-9: LGTM! Props interface is well-defined.The props interface clearly defines the required properties for the component.
11-13: LGTM! Keyboard shortcut implementation follows best practices.The keyboard shortcut is correctly implemented in the component that owns the toggle functionality.
14-34: LGTM! Good visual feedback implementation.The component provides excellent visual feedback:
- Clear title with shortcut information
- Animated background for live state
- Consistent styling using the design system
apps/dashboard/app/(app)/logs/components/control-cloud/index.tsx (1)
47-59: LGTM! Clean implementation of LogsControlCloud.The component effectively delegates filter management to the ControlCloud component while maintaining a clean and focused interface.
apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/index.tsx (1)
31-57: LGTM! Clean implementation of LogsFilters.The component effectively uses FiltersPopover with the configured filter items and maintains good accessibility with ARIA labels and keyboard shortcuts.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx (1)
46-65: LGTM! Clean implementation of chart configuration.The LogsTimeseriesBarChart is well configured with clear success/error states and proper event handling.
apps/dashboard/components/logs/checkbox/hooks/index.ts (1)
4-18: LGTM! Type safety improvements.The addition of the generic type parameter
TFilter extends FilterValueenhances type safety while maintaining backward compatibility.apps/dashboard/components/logs/control-cloud/control-pill.tsx (1)
1-90: LGTM! Well-structured component with good accessibility.The component is well-implemented with proper:
- Focus management
- Keyboard interactions
- TypeScript type safety
- Conditional rendering
apps/dashboard/components/logs/checkbox/filter-checkbox.tsx (1)
1-126: LGTM! Well-structured component with good type safety.The component is well-implemented with proper:
- Generic type constraints
- Callback memoization
- Accessibility features
- Flexible rendering options
🧰 Tools
🪛 Biome (1.9.4)
[error] 75-91: 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] 75-81: 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] 76-78: 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] 94-109: 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] 94-101: 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] 96-98: 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/components/logs/control-cloud/index.tsx (1)
71-75: Add error handling for DOM queries.The DOM query for buttons could fail if the structure changes.
Add error handling and logging:
const buttons = document.querySelectorAll("[data-pill-index] button"); +if (buttons.length === 0) { + console.warn("No filter buttons found in ControlCloud"); + return focusedIndex; +} const currentButton = buttons[focusedIndex] as HTMLElement;
apps/dashboard/app/(app)/logs/components/controls/components/logs-search/index.tsx
Show resolved
Hide resolved
...app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-search/index.tsx
Show resolved
Hide resolved
|
From Oguzhan Olguncu ‣ @chronark this also includes table padding fix + prompt fix |
|
ah my bad, I thought james had approved it |
|
From Oguzhan Olguncu ‣ Take a look and tell me if you don't like anything i'll fix it in the next iteration. I'll start implementing ratelimit overview today |
2 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps 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
New Features
Refactor
Chores