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 and standardizes the rate limit filtering logic across the dashboard application. Multiple components have been updated to use new, ratelimit-specific types and enums in place of generic filter types. The changes include updates to import paths, type declarations, parser functions, and schema definitions for both logs and search functionalities. Additionally, outdated type definition files have been removed and replaced with more structured validation and transformation utilities, including new helper functions for structured output and type guards. These modifications aim to align the filtering logic with updated requirements for rate limit handling. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant LLM as LLM Service
participant T as transformStructuredOutputToFilters
participant S as Filter State Manager
U->>LLM: Submit structured search request
LLM-->>T: Return structured filter output
T->>T: Merge and transform filters
T-->>S: Provide unique ratelimit filters
S->>U: Update filter view
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: 2
🧹 Nitpick comments (11)
apps/dashboard/components/logs/validation/utils/structured-output-schema-generator.ts (2)
5-39: Validate existence of field configuration in refine block.
Currently, if a field somehow isn't covered in the record (though theoretically enforced by TypeScript), accessingconfig.operatorscould raise a runtime error. Consider guarding against an undefined config before checking operators to ensure a safer fallback.Possible snippet:
... .refine( (data) => { const config = filterFieldConfig[data.field as keyof TConfig]; + if (!config) { + return false; + } return data.filters.every((filter) => { ...
41-60: Consider rejecting unknown config types.
If a field is neither string nor number config, the function now returns true at line 59. This can inadvertently pass invalid inputs. You may want to reject such cases explicitly to avoid unintentional acceptance of unsupported field types.Suggested adjustment:
if (isNumberConfig(config) && typeof value === "number") { return config.validate ? config.validate(value) : true; } - return true; + return false; // Explicitly reject unknown configapps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/query-logs.schema.ts (1)
2-2: LGTM! Consider standardizing operator types.The update to use
ratelimitFilterOperatorEnumfor identifiers is good. However, other sections (requestIds, status) still usez.literal("is").Consider standardizing all operator types to use
ratelimitFilterOperatorEnumfor consistency, or document why certain sections use different operator types.Let's verify the operator usage:
#!/bin/bash # Search for all operator definitions in the schema rg -A 2 'operator:.*' apps/dashboard/app/\(app\)/ratelimits/.*schema.tsAlso applies to: 14-14
apps/dashboard/components/logs/validation/filter.types.ts (2)
14-28: Consider adding JSDoc comments.The
NumberConfigandStringConfigtypes are well-structured, but could benefit from documentation explaining the purpose of each configuration option.Add JSDoc comments to improve maintainability:
+/** + * Configuration for number-type fields + * @template TOperator - The type of operators allowed for this field + */ export type NumberConfig<TOperator extends string = string> = BaseFieldConfig<number, TOperator> & { type: "number"; validate?: (value: number) => boolean; }; +/** + * Configuration for string-type fields + * @template TOperator - The type of operators allowed for this field + */ export type StringConfig<TOperator extends string = string> = BaseFieldConfig<string, TOperator> & { type: "string"; validValues?: readonly string[]; validate?: (value: string) => boolean; getColorClass?: (value: string) => string; };
30-48: Consider adding validation for metadata properties.The
FilterValuetype is well-structured with good use of generics. Consider adding validation for metadata properties to ensure consistent styling.Add a type for valid color classes:
+/** Valid color classes for filter styling */ +export type FilterColorClass = 'bg-blue-100' | 'bg-green-100' | 'bg-red-100'; export type FilterValue< TField extends string = string, TOperator extends FilterOperator = FilterOperator, TValue extends string | number = string | number, > = { id: string; field: TField; operator: TOperator; value: TValue; metadata?: { - colorClass?: string; + colorClass?: FilterColorClass; icon?: ReactNode; }; };apps/dashboard/components/logs/validation/utils/nuqs-parsers.ts (1)
27-27: Consider expanding VALID_OPERATORS for future extensibility.The current set of operators might be too restrictive. Consider adding common comparison operators like 'startsWith', 'endsWith', etc., if they might be needed in the future.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/filter-checkbox.tsx (1)
50-55: Consider making the operator configurable.The filter operator is hardcoded to "is", which might not be suitable for all use cases. Consider making it configurable through props.
interface BaseCheckboxFilterProps<TCheckbox extends BaseCheckboxOption> { options: TCheckbox[]; filterField: "identifiers" | "status"; checkPath: string; + operator?: RatelimitFilterValue["operator"]; // ... other props } // In the component: const newFilters: RatelimitFilterValue[] = selectedValues.map((filterValue) => ({ id: crypto.randomUUID(), field: filterField, - operator: "is", + operator: operator ?? "is", ...filterValue, }));apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/identifiers-filter.tsx (1)
78-83: Consider making the operator configurable for consistency.Similar to FilterCheckbox, the operator is hardcoded to "is". For consistency, consider making it configurable or extracting it to a shared constant.
+const DEFAULT_IDENTIFIER_OPERATOR = "is" as const; + // Later in the code: const identifiersFilters: RatelimitFilterValue[] = selectedPaths.map((path) => ({ id: crypto.randomUUID(), field: "identifiers", - operator: "is", + operator: DEFAULT_IDENTIFIER_OPERATOR, value: path, }));apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/hooks/use-filters.test.ts (1)
42-44: Consider improving TypeScript type assertions.Instead of using
@ts-expect-error, consider properly typing the test data or using type assertions to handle the TypeScript errors more elegantly.Example improvement:
- //@ts-expect-error ts yells for no reason - expect(parseAsFilterValArray.parse(null)).toEqual([]); + expect(parseAsFilterValArray.parse(null as unknown as string)).toEqual([]);Also applies to: 73-75, 82-84
apps/dashboard/components/logs/validation/utils/transform-structured-output-filter-format.ts (2)
1-35: LGTM! Consider restricting value types further.The documentation and type definitions are well-structured. The use of generics provides good type safety while maintaining flexibility.
Consider restricting the
TValuetype to specific union types based on your use cases, rather than allowing any string or number:- TValue extends string | number, + TValue extends string | number | boolean | null,
36-58: Consider using for...of loop for better control flow.The uniqueness check logic is solid, but using
forEachwith an early return doesn't skip to the next iteration - it only skips the current filter.Consider this alternative implementation for better control flow:
- for (const filterGroup of data.filters) { - filterGroup.filters.forEach((filter) => { + for (const filterGroup of data.filters) { + for (const filter of filterGroup.filters) { const baseFilter = { field: filterGroup.field, operator: filter.operator, value: filter.value, }; const filterKey = `${baseFilter.field}-${baseFilter.operator}-${baseFilter.value}`; if (seenFilters.has(filterKey)) { - return; + continue; } uniqueFilters.push({ id: crypto.randomUUID(), ...baseFilter, }); seenFilters.add(filterKey); - }); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/query-timeseries.schema.ts(2 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(3 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/hooks/use-checkbox-state.ts(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/identifiers-filter.tsx(2 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/query-logs.schema.ts(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/filters.schema.ts(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/filters.type.ts(0 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/hooks/use-filters.test.ts(4 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/hooks/use-filters.ts(4 hunks)apps/dashboard/components/logs/validation/filter.types.ts(1 hunks)apps/dashboard/components/logs/validation/utils/nuqs-parsers.ts(1 hunks)apps/dashboard/components/logs/validation/utils/structured-output-schema-generator.ts(1 hunks)apps/dashboard/components/logs/validation/utils/transform-structured-output-filter-format.ts(1 hunks)apps/dashboard/components/logs/validation/utils/type-guards.ts(1 hunks)apps/dashboard/lib/trpc/routers/ratelimit/llm-search/utils.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/filters.type.ts
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
🔇 Additional comments (23)
apps/dashboard/components/logs/validation/utils/structured-output-schema-generator.ts (1)
1-4: Imports look good.
These imports from "zod" and local type-guard utilities appear correct and facilitate typed schema validation.apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/filters.schema.ts (4)
5-6: Imports align with the new filter utility.
These references properly integrate the shared types and schema creation utility.
9-37: Explicit field configuration improves clarity.
Defining each field with its operators and optional validations ensures maintainability. Using "as const" is a neat way to preserve literal types.
39-53: Zod enums and factory usage appear correct.
Declaring separateratelimitFilterOperatorEnumandratelimitFilterFieldEnumensures consistency. ThefilterOutputSchemacreation is cohesive with the new utility.
55-82: Structured type definitions enhance maintainability.
These ratelimit-specific types neatly capture all required filter and query parameter data.apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/hooks/use-filters.ts (4)
1-4: Refined import and parser setup.
Bringing in specialized parser helpers (e.g., parseAsFilterValueArray) is a clean approach. The customparseAsFilterValArrayensures consistent handling of "is"/"contains" operators.Also applies to: 16-16
18-29: Consolidated filter payload logic.
Defining requestIds/identifiers/status as arrays with the new parser clarifies which fields accept multiple filters. The subsequent grouping intoactiveFiltersis straightforward and consistent.
56-58: Color-class metadata usage is well-structured.
Applying thegetColorClassfunction fromratelimitFilterFieldConfigis a convenient way to keep view-related styling logic in a single location.
63-74: Dynamic filter generation is clean and extensible.
Using a single loop for "startTime", "endTime", and "since" simplifies the code. The subsequent grouping by field inupdateFiltersis also neatly managed.Also applies to: 79-92
apps/dashboard/components/logs/validation/utils/type-guards.ts (1)
4-9: LGTM! Well-structured type guards.The type guard functions are well-implemented, following TypeScript best practices for type narrowing. They provide clear and focused functionality for distinguishing between number and string configurations.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/query-timeseries.schema.ts (1)
2-2: LGTM! Verify schema compatibility.The update to use
ratelimitFilterOperatorEnumaligns with the refactoring objectives. The schema maintains proper validation and type safety.Let's verify that all operator values in the codebase are compatible with the new enum:
Also applies to: 13-13
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for operator usage in timeseries queries rg -A 2 'operator:.*ratelimitFilterOperatorEnum' apps/dashboard/app/\(app\)/ratelimits/Length of output: 851
Schema Operator Enum Verification Complete
The shell script output confirms that references to the operator in bothquery-timeseries.schema.tsandquery-logs.schema.tsnow useratelimitFilterOperatorEnum, ensuring compatibility across timeseries and table queries. This update aligns with the refactoring objectives and maintains proper type validation.apps/dashboard/components/logs/validation/filter.types.ts (2)
4-7: LGTM! Clear enum definition.The
filterOperatorEnumis well-defined using zod with clear, descriptive values.
9-12: LGTM! Well-designed base configuration.The
BaseFieldConfigtype effectively uses generics to ensure type safety while remaining flexible.apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/hooks/use-checkbox-state.ts (1)
1-7: LGTM! Well-structured hook with good accessibility support.The type update to RatelimitFilterValue is correct, and the implementation includes proper keyboard navigation and accessibility features.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/identifiers-filter.tsx (1)
37-43: LGTM! Well-implemented checkbox state management.Good use of the useCheckboxState hook with proper configuration and synchronization with options.
apps/dashboard/lib/trpc/routers/ratelimit/llm-search/utils.ts (3)
1-4: LGTM! Import changes align with the refactoring.The change from
filterFieldConfigtoratelimitFilterFieldConfigcorrectly aligns with the broader effort to make filter types more specific to rate limits.
9-25: Great addition of comprehensive JSDoc documentation!The added JSDoc comments significantly improve code documentation by:
- Clearly explaining the purpose of the schema creation
- Providing a practical example of usage with OpenAI's parse completion
- Demonstrating the type safety enforcement
96-96: LGTM! Configuration usage is consistent.The usage of
ratelimitFilterFieldConfigingetSystemPromptmaintains consistency with the new rate limit-specific types.apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/control-cloud/index.tsx (2)
9-9: LGTM! Import statement updated correctly.The change from
FilterValuetoRatelimitFilterValuemaintains consistency with the rate limit-specific type system.
47-47: LGTM! Type definition updated appropriately.The
ControlPillPropsinterface correctly uses the newRatelimitFilterValuetype, ensuring type safety for rate limit-specific filters.apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/hooks/use-filters.test.ts (2)
1-4: LGTM! Import statements updated correctly.The imports are properly updated to use the new parser utilities from the correct path.
10-10: LGTM! Parser initialization is well-structured.The initialization of
parseAsFilterValArraywith specific operators provides good reusability across test cases.apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-search/index.tsx (1)
1-1: LGTM! Verify import path resolution.The import path has been updated correctly, and the usage of
transformStructuredOutputToFiltersis consistent with its new implementation.Run this script to verify the import path resolution:
Also applies to: 16-17
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the import path resolution for transformStructuredOutputToFilters # Test: Check if the import path is valid fd -t f "transform-structured-output-filter-format.ts" apps/dashboard/components/logs/validation/utils/ # Test: Check for any other usages that might need updating rg -l "transformStructuredOutputToFilters" --type tsLength of output: 634
Import Path Correctly Resolved
The file exists at
apps/dashboard/components/logs/validation/utils/transform-structured-output-filter-format.ts
and the search confirms that the usage of transformStructuredOutputToFilters is consistent across the codebase. The import paths in all the referenced files, including the one in apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-search/index.tsx and the other usages found, correctly resolve to this file.
apps/dashboard/components/logs/validation/utils/transform-structured-output-filter-format.ts
Show resolved
Hide resolved
|
From Oguzhan Olguncu ‣ @chronark @perkinsjr Guys I hate pinging for reviews but can I get reviews for filter type refactors? They blocker for future PRs 🙏 |
| export type RatelimitFilterValue = FilterValue<RatelimitFilterField, RatelimitFilterOperator>; | ||
|
|
||
| export type RatelimitQuerySearchParams = { | ||
| startTime?: number | null; |
There was a problem hiding this comment.
do we really need undefined and null?
I have no idea, just curious
There was a problem hiding this comment.
This is needed for nuqs's default value. It's weird sometimes its complaining for null sometimes for undefined, so I just dumped both them there 😄
|
From Oguzhan Olguncu ‣ https://github.com/unkeyed/unkey/actions/runs/13204139800/job/36863149284?pr=2877 @chronark :pepesad: CI failling again can you merge it |
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