feat: new filter logic #2804
feat: new filter logic #2804
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe pull request introduces a new Changes
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! 🙏 |
ogzhanolguncu
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx (1)
Line range hint
11-86: Remove duplicate path entries.The options array contains duplicate entries for several paths (e.g., "/v1/auth.login" appears multiple times).
-const options: CheckboxOption[] = [ +const options: CheckboxOption[] = Array.from(new Set([ { id: 1, path: "/v1/analytics.export", checked: false, }, // ... other unique entries ... - { - id: 10, - path: "/v1/auth.login", // Duplicate - checked: false, - }, - // ... remove other duplicates ... -] as const; +])) as const;
🧹 Nitpick comments (8)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/methods-filter.tsx (2)
24-36: Consider adding runtime type validation for filter values.The type casting of filter values to
HttpMethodassumes the values are valid HTTP methods. Consider adding runtime validation to prevent potential type errors.const methodFilters = filters .filter((f) => f.field === "methods") - .map((f) => f.value as HttpMethod); + .map((f) => { + if (!['GET', 'POST', 'PUT', 'DELETE', 'PATCH'].includes(f.value)) { + console.warn(`Invalid HTTP method: ${f.value}`); + return null; + } + return f.value as HttpMethod; + }) + .filter((method): method is HttpMethod => method !== null);
59-72: Consider SSR compatibility for UUID generation.The
crypto.randomUUID()might not be available during server-side rendering. Consider using a more SSR-friendly UUID generation method or ensuring this code only runs on the client side.+const generateId = () => { + if (typeof window === 'undefined') { + return Math.random().toString(36).substring(2); + } + return crypto.randomUUID(); +}; const methodFilters: FilterValue[] = selectedMethods.map((method) => ({ - id: crypto.randomUUID(), + id: generateId(), field: "methods", operator: "is", value: method, }));apps/dashboard/app/(app)/logs-v2/query-state.ts (1)
116-124: Simplify thegetColorClassfunctionThe
getColorClassfunction infilterFieldConfig.statuscan be simplified for readability by using a more concise conditional structure.Apply this diff to streamline the function:
getColorClass: (value: number) => { - if (value >= 500) { - return "bg-error-9"; - } - if (value >= 400) { - return "bg-warning-8"; - } - return "bg-success-9"; + return value >= 500 + ? "bg-error-9" + : value >= 400 + ? "bg-warning-8" + : "bg-success-9"; },apps/dashboard/app/(app)/logs-v2/components/control-cloud/index.tsx (3)
9-23: Consider using an enum or constant map for field name mapping.The switch statement in
formatFieldNamecould be replaced with a more maintainable object map or enum.-const formatFieldName = (field: string): string => { - switch (field) { - case "status": - return "Status"; - case "paths": - return "Path"; - case "methods": - return "Method"; - case "requestId": - return "Request ID"; - default: - return field.charAt(0).toUpperCase() + field.slice(1); - } -}; +const FIELD_NAME_MAP: Record<string, string> = { + status: "Status", + paths: "Path", + methods: "Method", + requestId: "Request ID", +}; + +const formatFieldName = (field: string): string => { + return FIELD_NAME_MAP[field] ?? field.charAt(0).toUpperCase() + field.slice(1); +};
25-40: Consider using HTTP status code constants.The status code ranges could be defined as constants for better maintainability and readability.
+const HTTP_STATUS = { + SUCCESS: 2, + WARNING: 4, + ERROR: 5, +} as const; + const formatValue = (value: string | number): string => { if (typeof value === "string" && /^\d+$/.test(value)) { const statusFamily = Math.floor(Number.parseInt(value) / 100); switch (statusFamily) { - case 5: + case HTTP_STATUS.ERROR: return "5XX (Error)"; - case 4: + case HTTP_STATUS.WARNING: return "4XX (Warning)"; - case 2: + case HTTP_STATUS.SUCCESS: return "2XX (Success)"; default: return `${statusFamily}xx`; } } return String(value); };
89-91: Consider adding loading state handling.The component should handle loading states when filters are being fetched or updated.
+ const [isLoading, setIsLoading] = useState(false); + if (filters.length === 0) { return null; }apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/status-filter.tsx (1)
77-93: Optimize filter update logic.The filter update logic could be optimized by memoizing the filter transformation.
+const getStatusColorClass = (status: number) => + status >= 500 ? "bg-error-9" : status >= 400 ? "bg-warning-8" : "bg-success-9"; const handleApplyFilter = useCallback(() => { const selectedStatuses = checkboxes.filter((c) => c.checked).map((c) => c.status); // Keep all non-status filters and add new status filters const otherFilters = filters.filter((f) => f.field !== "status"); const statusFilters: FilterValue[] = selectedStatuses.map((status) => ({ id: crypto.randomUUID(), field: "status", operator: "is", value: status, metadata: { - colorClass: status >= 500 ? "bg-error-9" : status >= 400 ? "bg-warning-8" : "bg-success-9", + colorClass: getStatusColorClass(status), }, })); updateFilters([...otherFilters, ...statusFilters]); }, [checkboxes, filters, updateFilters]);apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx (1)
148-161: Consider debouncing filter updates.The filter update logic could benefit from debouncing to prevent excessive updates when users are rapidly selecting multiple paths.
+import { debounce } from 'lodash'; const handleApplyFilter = useCallback( - () => { + debounce(() => { const selectedPaths = checkboxes.filter((c) => c.checked).map((c) => c.path); const otherFilters = filters.filter((f) => f.field !== "paths"); const pathFilters: FilterValue[] = selectedPaths.map((path) => ({ id: crypto.randomUUID(), field: "paths", operator: "is", value: path, })); updateFilters([...otherFilters, ...pathFilters]); - }, + }, 300), [checkboxes, filters, updateFilters] );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/dashboard/app/(app)/logs-v2/components/control-cloud/index.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filters-popover.tsx(5 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/methods-filter.tsx(3 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx(5 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/status-filter.tsx(4 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/index.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/logs-client.tsx(2 hunks)apps/dashboard/app/(app)/logs-v2/page.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/query-state.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/methods-filter.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.
🪛 Biome (1.9.4)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/methods-filter.tsx
[error] 78-87: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/status-filter.tsx
[error] 99-108: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- 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 API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
apps/dashboard/app/(app)/logs-v2/page.tsx (2)
Line range hint
1-35: LGTM! Clean and well-structured code.The overall structure follows Next.js best practices:
- Clear separation of server/client components
- Proper error handling with
notFound()- Clean component hierarchy with
LogsContainerPage
18-20: Verify the intentional removal of beta feature flag.The condition has been simplified to only check for workspace existence, removing the
betaFeatures.logsPagecheck. This change makes the logs page accessible to all users with a valid workspace.Please confirm if this is intentional as part of making the new filter logic generally available. Run this script to check for any related beta feature configurations:
✅ Verification successful
Beta feature flag removal is part of the v2 implementation.
The logs-v2 directory represents a new implementation of the logs feature, intended to be generally available to all users. The original logs page remains beta-flagged while this new version is being rolled out.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any beta feature configurations related to logs page # Search for betaFeatures configurations rg -A 5 "betaFeatures.*logsPage" # Search for feature flag definitions rg -A 5 "logsPage.*feature"Length of output: 1375
Script:
#!/bin/bash # Search for references to logs-v2 and migration context rg -l "logs-v2" # Look for migration-related comments or documentation rg -i "migration.*logs" # Check for any documentation about the v2 implementation fd "README" --exec cat {} \;Length of output: 23838
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/methods-filter.tsx (3)
1-1: Type safety improvements look good!The changes to use
HttpMethodtype instead of string and the const assertion on the options array improve type safety and prevent accidental mutations.Also applies to: 8-8, 13-17
84-86: Great UX improvement!The dynamic text that changes between "Select All" and "Unselect All" based on the checkbox state provides clear feedback to users.
21-22: Verify timing of filter state management implementation.Based on the learning from PR #2801, handler improvements were to be deferred until real data integration. Please confirm if the data integration is now ready for these changes.
✅ Verification successful
Filter state management implementation is appropriately timed
The codebase shows evidence of data integration readiness with implemented API endpoints and Clickhouse integration. This confirms that proceeding with filter state management is aligned with the development timeline.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for data integration implementation rg -g '!node_modules' -g '!*.test.*' -g '!*.spec.*' -A 5 'useFilters.*methods'Length of output: 80
Script:
#!/bin/bash # Search for useFilters hook implementation ast-grep --pattern 'export function useFilters' # Check logs-v2 structure and data integration fd . apps/dashboard/app/logs-v2 -t f # Look for data fetching or API integration rg -g '!node_modules' -g '!*.test.*' -g '!*.spec.*' -A 5 'fetch.*logs|api.*logs' apps/dashboard/app/logs-v2Length of output: 379
Script:
#!/bin/bash # Find logs-v2 related files fd logs-v2 # Search for useFilters implementation rg -g '!node_modules' -g '!*.test.*' -g '!*.spec.*' 'export.*useFilters' # Look for data integration patterns rg -g '!node_modules' -g '!*.test.*' -g '!*.spec.*' -A 5 'api.*logs|fetchLogs|getLogData'Length of output: 9565
apps/dashboard/app/(app)/logs-v2/query-state.ts (2)
161-161: Ensure browser compatibility ofcrypto.randomUUID()The function
crypto.randomUUID()may not be supported in all browsers, particularly older ones. To maintain compatibility across different environments, consider using theuuidlibrary or a polyfill for UUID generation.
222-286:⚠️ Potential issueInitialize all keys in
newParamsIn the
updateFiltersfunction, ensure that all keys ofQuerySearchParamsare initialized innewParamsto prevent unintended state persistence. Currently,startTimeandendTimeare handled differently compared to other fields.Apply this diff to initialize all parameters:
const newParams: Partial<QuerySearchParams> = { paths: null, host: null, requestId: null, + startTime: null, + endTime: null, methods: null, status: null, };Likely invalid or redundant comment.
apps/dashboard/app/(app)/logs-v2/components/logs-client.tsx (1)
6-6: IntegrateControlCloudcomponent correctlyThe
ControlCloudcomponent is correctly imported and integrated into theLogsClientcomponent. This enhances the UI with the new filter management features.Also applies to: 26-26
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/index.tsx (1)
1-1: Display active filters count effectivelyThe implementation of displaying the active filters count using
useFiltersand conditionally rendering the badge works correctly. Thecnutility is effectively used to apply styles based on the filters' state.Also applies to: 8-8, 14-14, 21-25
apps/dashboard/app/(app)/logs-v2/components/control-cloud/index.tsx (1)
78-80: Consider adding error handling for keyboard shortcut.The keyboard shortcut handler should handle potential errors when clearing filters.
useKeyboardShortcut({ key: "d", meta: true }, () => { - updateFilters([]); + try { + updateFilters([]); + } catch (error) { + console.error('Failed to clear filters:', error); + // Consider showing a user-friendly error message + } });apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filters-popover.tsx (1)
21-35: Verify uniqueness of filter paths.The filter items contain potentially duplicate paths (e.g., "/v1/auth.login"). This could cause issues with filter identification and management.
.../app/(app)/logs-v2/components/controls/components/logs-filters/components/methods-filter.tsx
Show resolved
Hide resolved
...app/(app)/logs-v2/components/controls/components/logs-filters/components/filters-popover.tsx
Show resolved
Hide resolved
...d/app/(app)/logs-v2/components/controls/components/logs-filters/components/status-filter.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/dashboard/app/(app)/logs-v2/query-state.ts (2)
4-4: Consider expanding HTTP status codes coverage.The
STATUSESarray only includes three basic status codes (200, 400, 500). Consider adding other commonly used status codes like 201 (Created), 401 (Unauthorized), 403 (Forbidden), 404 (Not Found), etc., to provide more granular filtering options.
110-149: Add input validation for numeric fields.The
filterFieldConfigfor numeric fields (status,startTime,endTime) lacks value range validation. Consider adding:
- Min/max bounds for timestamps
- Valid status code ranges (100-599)
Example enhancement:
status: { type: "number", operators: ["is"] as const, + validate: (value: number) => value >= 100 && value <= 599, getColorClass: (value: number) => { if (value >= 500) { return "bg-error-9"; } // ... }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/logs-v2/query-state.ts(1 hunks)
⏰ 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: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/dashboard/app/(app)/logs-v2/query-state.ts (4)
39-98: LGTM! Robust parser implementations.The parser implementations correctly handle:
- Values containing colons using the
/:(.+)/split pattern- Input validation for operators
- Error cases with appropriate fallbacks
151-329: LGTM! Well-structured filter management.The
useFiltershook implementation is robust with:
- Proper memoization of filters
- Clear separation of concerns
- Type-safe filter management functions
- Efficient state updates
151-329: Add unit tests for filter logic.The filter management logic is complex and would benefit from comprehensive unit tests covering:
- Parser edge cases (empty values, invalid operators)
- Filter state management (add, remove, update)
- URL parameter synchronization
Would you like me to help generate unit test cases for this functionality?
159-159: Consider cross-browser compatibility for crypto.randomUUID().The
crypto.randomUUID()API might not be supported in all browsers. Consider using a fallback or a UUID library likeuuidfor better compatibility.Run this script to check browser compatibility:
Also applies to: 173-173, 182-182, 191-191, 200-200, 211-211
…logs-v2-filter-cloud
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/dashboard/app/(app)/logs-v2/query-state.ts (6)
4-4: Consider expanding HTTP status codes coverage.The
STATUSESarray only includes basic status codes (200, 400, 500). Consider adding other commonly used status codes like 201 (Created), 401 (Unauthorized), 403 (Forbidden), 404 (Not Found), etc., to provide more granular filtering options.-export const STATUSES = [200, 400, 500] as const; +export const STATUSES = [200, 201, 400, 401, 403, 404, 500, 502, 503] as const;
10-13: Consider using more specific types for FilterUrlValue.The
valueproperty uses a broadstring | numbertype. Consider using union types based on the actual possible values to improve type safety.type FilterUrlValue = { - value: string | number; + value: string | ResponseStatus | HttpMethod; operator: FilterOperator; };
89-90: Enhance error handling in parseAsFilterValueArray.The catch block silently returns an empty array for all errors. Consider logging errors or handling specific error cases differently.
- } catch { + } catch (error) { + console.warn('Failed to parse filter value array:', error); return []; }
110-149: Add JSDoc documentation for filterFieldConfig.The
filterFieldConfigobject contains important configuration that would benefit from documentation explaining the purpose of each field type, valid operators, and color classes.+/** + * Configuration for different filter fields defining their types, valid operators, + * and display properties. + * @property {Object} status - Configuration for HTTP status code filters + * @property {Object} methods - Configuration for HTTP method filters + * @property {Object} paths - Configuration for URL path filters + * ... + */ export const filterFieldConfig = {
159-159: Consider using a more reliable ID generation method.The
crypto.randomUUID()might not be available in all environments. Consider using a more widely supported UUID library or a simpler ID generation method.- id: crypto.randomUUID(), + id: `${field}_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`,Also applies to: 171-171, 180-180, 189-189, 198-198, 209-209
154-218: Optimize filters memo calculation.The
filtersmemo recalculates all filter arrays on every search params change. Consider breaking this into smaller memos for each filter type to prevent unnecessary recalculations.Example optimization:
const statusFilters = useMemo(() => { return searchParams.status?.map(status => ({ id: generateId(), field: "status", operator: status.operator, value: status.value as ResponseStatus, metadata: { colorClass: filterFieldConfig.status.getColorClass(status.value as number), }, })) ?? []; }, [searchParams.status]); // Similar memos for other filter types const filters = useMemo(() => [ ...statusFilters, ...methodFilters, ...pathFilters, ...(searchParams.host ? [hostFilter] : []), ...(searchParams.requestId ? [requestIdFilter] : []), ...timeFilters, ], [statusFilters, methodFilters, pathFilters, hostFilter, requestIdFilter, timeFilters]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/logs-v2/query-state.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Test Packages / Test ./internal/clickhouse
- 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 Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Docs
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/dashboard/app/(app)/logs-v2/query-state.ts (1)
237-274: 🛠️ Refactor suggestionAdd runtime validation for filter values.
The switch statement uses type assertions without runtime validation. Consider adding runtime checks to ensure type safety.
switch (filter.field) { case "status": + const statusValue = Number(filter.value); + if (isNaN(statusValue) || !STATUSES.includes(statusValue)) { + console.warn(`Invalid status value: ${filter.value}`); + return; + } responseStatusFilters.push({ - value: filter.value, + value: statusValue, operator: filter.operator, }); break; case "methods": + if (!METHODS.includes(filter.value as string)) { + console.warn(`Invalid method value: ${filter.value}`); + return; + } methodFilters.push({
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
ControlCloudcomponent to display and manage active filters.Bug Fixes
Refactor