feat: return errors as values for clickhouse #2665
Conversation
chronark
requested review from
perkinsjr,
mcstepp and
MichaelUnkey
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
📝 WalkthroughWalkthroughThis pull request introduces several modifications across various components in the application, primarily focusing on enhancing error handling and improving data retrieval processes. Key changes include adjustments to how data is accessed from API responses, ensuring that 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! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (19)
internal/clickhouse/src/client/error.ts (2)
2-10: Consider adding context for insert operations.The implementation is good, but could be enhanced by including additional context about the failed insert operation (e.g., table name, affected rows).
Consider extending the constructor to include more context:
-constructor(message: string) { +constructor(message: string, context?: { table?: string; rows?: number }) { super({ message, + context, }); }
1-20: Consider documenting error handling strategy.The error classes provide a good foundation for the "errors as values" approach mentioned in the PR title. Consider documenting the error handling strategy, particularly:
- When to use InsertError vs QueryError
- Retry strategies and backoff policies
- How to handle these errors in the calling code
Would you like me to help create a documentation template for this?
internal/clickhouse/src/client/interface.ts (1)
1-1: LGTM! Good choice using the Result type for error handling.The introduction of the Result type pattern is a solid architectural choice that:
- Makes error handling explicit and type-safe
- Forces consumers to handle both success and error cases
- Aligns with functional programming practices
Also applies to: 3-3
internal/clickhouse/src/client/noop.ts (1)
5-5: Consider adding documentation for the Noop implementationThe
Noopclass serves as a valuable test double implementing bothQuerierandInserterinterfaces. Consider adding JSDoc comments to document:
- The purpose of this implementation
- When to use it (testing, development)
- The fact that it always returns empty results
internal/clickhouse/src/billing.ts (1)
68-71: Consider reducing duplication between billing query functionsBoth
getBillableRatelimitsandgetBillableVerificationsshare identical query structure and error handling. Consider extracting the common logic into a reusable helper function.Here's a suggested refactor:
function createBillingQuery(tableName: string) { return (ch: Querier) => async (args: { workspaceId: string; year: number; month: number; }): Promise<number> => { const query = ch.query({ query: ` SELECT sum(count) as count FROM ${tableName} WHERE workspace_id = {workspaceId: String} AND year = {year: Int64} AND month = {month: Int64} GROUP BY workspace_id, year, month `, params: z.object({ workspaceId: z.string(), year: z.number().int(), month: z.number().int().min(1).max(12), }), schema: z.object({ count: z.number().int(), }), }); const res = await query(args); if (res.err || res.val.length === 0) { return 0; } return res.val.at(0)?.count ?? 0; }; } export const getBillableRatelimits = createBillingQuery('billing.billable_ratelimits_per_month_v1'); export const getBillableVerifications = createBillingQuery('billing.billable_verifications_per_month_v2');apps/dashboard/app/(app)/settings/root-keys/[keyId]/layout.tsx (1)
86-88: Consider explicit error handling for the clickhouse queryWhile the current implementation gracefully handles missing data with the nullish coalescing operator, it might be beneficial to explicitly handle and log error cases.
Consider refactoring to:
- .then((res) => res.val?.at(0)?.time ?? 0); + .then((res) => { + if ('err' in res) { + console.error('Failed to fetch last used time:', res.err); + return 0; + } + return res.val?.at(0)?.time ?? 0; + });apps/dashboard/app/(app)/ratelimits/[namespaceId]/overrides/table.tsx (1)
Line range hint
89-94: Consider adding type safety for the clickhouse responseThe
clickhouse.ratelimits.latest()response structure has changed to include avalproperty. Let's ensure type safety.Consider adding a type definition for the response:
type ClickhouseResponse<T> = { val?: T; err?: string; }; type RatelimitEvent = { time: number; // ... other fields }; const lastUsed: ClickhouseResponse<RatelimitEvent[]> = await clickhouse.ratelimits.latest({apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/verification-table.tsx (1)
37-37: Add fallback UI for undefined verification dataWhile the optional chaining operator handles undefined
valgracefully, the component should provide feedback to users when no verification data is available.Consider adding a fallback UI:
- {verifications.val?.map((verification, i) => { + {verifications.val?.map((verification, i) => { /** * Instead of rounding every row individually... */ - })} + })} + {!verifications.val && ( + <TableRow> + <TableCell colSpan={2} className="text-center text-muted-foreground"> + No verification data available + </TableCell> + </TableRow> + )}apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/add-permission-for-api.tsx (2)
12-12: LGTM! Removal of unnecessary memoization.The removal of
useMemoforselectedApicalculation is a good optimization. Sincefindoperation onprops.apisis relatively inexpensive and the array is likely small, the overhead of memoization would outweigh its benefits. This change aligns with React's recommendation to avoid premature optimization.Consider adding a comment explaining the expected size characteristics of
props.apisto help future maintainers understand why direct calculation is preferred over memoization.Also applies to: 41-41
Line range hint
22-35: Consider optimizing array operations and adding error handling.The component performs multiple array operations (
filter,some,reduce) during render. While this is likely fine for small datasets, consider these improvements:
- Error handling:
- Add validation for
props.apisstructure- Handle edge cases when API data is malformed
- Performance optimization:
- Consider memoizing
apisWithoutPermissionandoptionsifprops.apisorprops.permissionsare large- The filtering logic could be simplified to reduce iterations
Here's a suggested optimization:
const [apisWithoutPermission, options] = useMemo(() => { const filteredApis = props.apis.filter(api => { const permissions = apiPermissions(api.id); return !Object.values(permissions).flat().some( permission => props.permissions.some(p => p.name === permission) ); }); return [ filteredApis, Object.fromEntries(filteredApis.map(api => [api.id, api.name])) ]; }, [props.apis, props.permissions]);Also applies to: 46-52
apps/dashboard/app/(app)/identities/[identityId]/page.tsx (1)
171-171: Consider adding error logging for monitoring.While the current implementation gracefully handles errors through optional chaining, consider logging errors for monitoring purposes. This would help track any issues with the clickhouse queries.
.then((res) => res.val?.at(0)?.time ?? null) + .catch((err) => { + console.error("Failed to fetch last used time:", err); + return null; + });apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/page.tsx (2)
Line range hint
127-157: Add error state handling in the UIThe component only handles empty results but doesn't account for error states. Users won't receive meaningful feedback if something goes wrong.
Consider adding error state handling:
+ const [error, setError] = useState<Error | null>(null); + + if (error) { + return ( + <EmptyPlaceholder> + <EmptyPlaceholder.Icon> + <X /> + </EmptyPlaceholder.Icon> + <EmptyPlaceholder.Title>Error fetching logs</EmptyPlaceholder.Title> + <EmptyPlaceholder.Description> + {error.message} + </EmptyPlaceholder.Description> + </EmptyPlaceholder> + ); + } + if (logs.length === 0) { // ... existing empty state code }
Line range hint
89-123: Add type safety to query constructionThe query object construction could benefit from stronger typing to ensure all required parameters are provided and have the correct types.
Consider adding a type definition:
type ClickhouseQuery = { workspaceId: string; namespaceId: string; start?: number; end?: number; identifier?: string[]; country?: string[]; ipAddress?: string[]; success?: boolean; };Then use it in your code:
- const query = { + const query: ClickhouseQuery = { workspaceId: workspaceId, // ... rest of the query construction };apps/api/src/routes/v1_keys_getVerifications.ts (1)
224-234: Consider defensive programming for analytics results.The code assumes
res.valwill be available after the error check, but it's good practice to handle potential null/undefined values explicitly.Consider applying this diff:
if (res.err) { - throw new Error(res.err.message); + throw new UnkeyApiError({ + code: "INTERNAL_SERVER_ERROR", + message: `Failed to fetch verifications: ${res.err.message}`, + }); } + if (!res.val) { + throw new UnkeyApiError({ + code: "INTERNAL_SERVER_ERROR", + message: "No verification data received from analytics", + }); + } return res.val;Also, consider extracting the time window calculations into named constants at the top of the function for better maintainability:
+ const DEFAULT_WINDOW = 24 * 60 * 60 * 1000; // 24 hours in milliseconds + const startTime = start ?? now - DEFAULT_WINDOW; + const endTime = end ?? now; + const verificationsFromAllKeys = await Promise.all( ids.map(({ keyId, keySpaceId }) => { return cache.verificationsByKeyId.swr(`${keyId}:${start}-${end}`, async () => { const res = await analytics.getVerificationsDaily({ workspaceId: authorizedWorkspaceId, keySpaceId: keySpaceId, keyId: keyId, - start: start ? start : now - 24 * 60 * 60 * 1000, - end: end ? end : now, + start: startTime, + end: endTime, });apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx (1)
71-81: Maintain consistent error handling approachThe current implementation mixes different error handling approaches:
- Optional chaining for SQL results (overly defensive)
- Non-null assertions for some Clickhouse results (too aggressive)
- Optional chaining for other Clickhouse results (appropriate)
We should maintain consistency by:
- Using direct access for SQL count results
- Using optional chaining with safe fallbacks for all Clickhouse results
apps/dashboard/app/(app)/apis/[apiId]/page.tsx (1)
57-72: Consider implementing a centralized error handling strategy for Clickhouse queriesThe pattern of accessing
.valwith non-null assertions across multiple queries suggests a need for a more robust error handling strategy. Consider these architectural improvements:
- Create a wrapper utility for Clickhouse queries that handles error cases consistently
- Define proper TypeScript types for the query results
- Implement error boundaries for graceful UI degradation
Example wrapper utility:
type QueryResult<T> = { val: T | null; error?: Error; }; async function executeClickhouseQuery<T>( queryFn: () => Promise<QueryResult<T>>, defaultValue: T ): Promise<T> { try { const result = await queryFn(); return result.val ?? defaultValue; } catch (error) { console.error('Clickhouse query failed:', error); return defaultValue; } }apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx (2)
177-177: Consider using reduce for stats calculation and improve null safetyThe stats calculation could be more concise using
reduce, and we should handle potential null values safely.- verifications.val!.forEach((v) => { + (verifications.val ?? []).reduce((stats, v) => { switch (v.outcome) { case "VALID": - stats.valid += v.count; + stats.valid = (stats.valid ?? 0) + v.count; break; // ... other cases } + return stats; + }, stats);
Line range hint
276-276: Fix incorrect metric value for "Disabled" statusThere's a copy-paste error in the metrics display. The "Disabled" metric is showing
stats.validinstead ofstats.disabled.- <Metric label="Disabled" value={formatNumber(stats.valid)} /> + <Metric label="Disabled" value={formatNumber(stats.disabled)} />internal/clickhouse/src/client/client.ts (1)
Line range hint
49-51: Ensure consistent error handling by returningErrinstead of throwing exceptionsCurrently, when
this.client.queryfails, the error is caught and a newErroris thrown. To maintain consistent error handling using theResulttype, consider returningErr(new QueryError(...))instead of throwing an exception. This ensures that all errors are handled uniformly, and callers can process theResultaccordingly.Apply the following code change:
- .catch((err) => { - throw new Error(`${err.message} ${req.query}, params: ${JSON.stringify(params)}`); - }); + .catch((err) => { + return Err(new QueryError(`${err.message} ${req.query}, params: ${JSON.stringify(params)}`)); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
apps/api/src/routes/v1_keys_getVerifications.ts(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx(4 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/verification-table.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/page.tsx(1 hunks)apps/dashboard/app/(app)/identities/[identityId]/page.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/page.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/overrides/table.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/card.tsx(1 hunks)apps/dashboard/app/(app)/settings/root-keys/[keyId]/history/page.tsx(1 hunks)apps/dashboard/app/(app)/settings/root-keys/[keyId]/layout.tsx(1 hunks)apps/dashboard/app/(app)/settings/root-keys/[keyId]/page.tsx(1 hunks)apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/add-permission-for-api.tsx(2 hunks)apps/dashboard/app/(app)/success/page.tsx(1 hunks)internal/clickhouse/package.json(1 hunks)internal/clickhouse/src/billing.ts(2 hunks)internal/clickhouse/src/client/client.ts(5 hunks)internal/clickhouse/src/client/error.ts(1 hunks)internal/clickhouse/src/client/interface.ts(3 hunks)internal/clickhouse/src/client/noop.ts(3 hunks)
🔇 Additional comments (21)
internal/clickhouse/package.json (2)
21-21: LGTM: Adding @unkey/error dependency
The addition aligns well with the PR objective of improving error handling for clickhouse operations.
21-21: Verify workspace dependency setup
Let's ensure the workspace dependency is properly configured.
✅ Verification successful
Workspace dependency setup is correctly configured
The verification confirms that @unkey/error package exists at packages/error with proper package.json configuration. The workspace dependency setup in internal/clickhouse/package.json using workspace:^ is valid and follows the expected monorepo structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the workspace dependency setup for @unkey/error
# Check if @unkey/error package exists in the workspace
fd -t f "package.json" | xargs grep -l "@unkey/error"
# Verify the package's existence and structure
if [ -d "packages/error" ]; then
cat packages/error/package.json
elif [ -d "internal/error" ]; then
cat internal/error/package.json
fi
Length of output: 974
internal/clickhouse/src/client/error.ts (1)
1-1: LGTM! Good choice of error hierarchy.
The use of BaseError as the parent class provides a consistent error handling pattern.
internal/clickhouse/src/client/interface.ts (2)
16-16: LGTM! Well-structured query return type.
The return type properly encapsulates both successful query results and potential errors while maintaining type safety through zod schemas.
Let's verify the QueryError type definition:
✅ Verification successful
QueryError type is well-defined and properly used
The QueryError type is defined in internal/clickhouse/src/client/error.ts as a class extending BaseError with appropriate context for query operations. It's correctly used in the client implementation for handling:
- Parameter validation errors
- Malformed data errors
The return type wrapping with Result<T, QueryError> provides structured error handling exactly as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the QueryError type definition
# Expected: Should find a type definition with specific error cases for query operations
ast-grep --pattern 'export type QueryError = $_'
Length of output: 50
Script:
#!/bin/bash
# Let's try a broader search for QueryError type using ripgrep
rg "QueryError" -A 5 -B 5
Length of output: 6783
25-25: LGTM! Consistent error handling pattern for insert operations.
The return type maintains the same success structure while adding structured error handling through the Result type.
Let's verify the InsertError type definition:
✅ Verification successful
InsertError type is properly defined and consistently used
The InsertError class is well-defined in internal/clickhouse/src/client/error.ts as a proper error class extending BaseError. It's correctly used across the codebase for handling insert operation failures:
- Schema validation errors in both
client.tsandnoop.ts - Network/database errors in
client.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the InsertError type definition
# Expected: Should find a type definition with specific error cases for insert operations
ast-grep --pattern 'export type InsertError = $_'
Length of output: 51
Script:
#!/bin/bash
# Let's try a broader search for InsertError type using ripgrep
rg "InsertError" -A 5 -B 5
Length of output: 6779
apps/dashboard/app/(app)/settings/root-keys/[keyId]/history/page.tsx (1)
42-48: Verify the error handling pattern across the codebase
Let's verify how errors are handled in other clickhouse query implementations to ensure consistent error handling patterns.
✅ Verification successful
Based on the search results, I can now provide a final response about the error handling pattern in the clickhouse.verifications.logs implementation:
Error handling is properly implemented through Result type pattern
The codebase consistently handles clickhouse operations through a Result type pattern where:
- All clickhouse operations return a Result type that wraps the successful value or error
- The
.then((res) => res.val!)pattern is consistently used across the codebase for clickhouse operations - The error handling follows the established pattern used in other similar operations (verifications.latest, verifications.perHour, etc.)
The implementation aligns with the error handling patterns used throughout the codebase, particularly in:
- Dashboard components handling verification logs
- API analytics tracking
- Rate limiting and monitoring features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other clickhouse verification log queries and their error handling
rg -A 5 "clickhouse.verifications.logs" --type typescript
# Search for similar .then(res => res.val) patterns to understand the common approach
rg -A 5 "\.then\(.*res\.val" --type typescript
# Look for any error handling utilities or types related to clickhouse
rg -A 5 "clickhouse.*Error|Result<|Either<" --type typescript
Length of output: 275
Script:
#!/bin/bash
# Search for clickhouse verification logs without file type restriction
rg -A 5 "clickhouse\.verifications\.logs"
# Search for similar .then patterns
rg -A 5 "\.then\(.*res\.val"
# Look for error handling patterns
rg -A 5 "clickhouse.*Error|Result<|Either<"
# Find all files that might contain clickhouse related code
fd -e ts -e tsx clickhouse
Length of output: 49000
Script:
#!/bin/bash
# Let's examine the clickhouse client implementation
cat apps/dashboard/lib/clickhouse.ts
# Also check the verifications logs implementation
ast-grep --pattern 'verifications = {
$$$
logs($$$) {
$$$
}
$$$
}'
Length of output: 270
Script:
#!/bin/bash
# Let's check the @unkey/clickhouse package implementation
fd -e ts -e tsx -H "verifications" packages/
# Also look for any error handling patterns in files using clickhouse
rg -A 5 "clickhouse.*\.(verifications|logs)"
Length of output: 9930
internal/clickhouse/src/client/noop.ts (2)
1-1: LGTM: Clean error handling imports
The imports properly separate concerns between the Result type and specific error types, aligning with the PR's objective of returning errors as values.
Also applies to: 3-3
29-39: LGTM: Robust error handling implementation
The insert method demonstrates good practices:
- Handles both single and array inputs uniformly
- Properly validates input data
- Returns appropriate Result types for success/failure cases
internal/clickhouse/src/billing.ts (1)
33-36: LGTM! Improved error handling pattern
The new error handling approach is more explicit and safer, properly handling both error cases and empty results.
Let's verify this pattern is consistently applied across other ClickHouse query handlers:
apps/dashboard/app/(app)/ratelimits/card.tsx (1)
30-30: LGTM! Good error handling
The use of optional chaining (?.) here properly handles potential undefined values in the response chain, gracefully degrading to undefined if any part of the chain is missing. This aligns well with the component's UI handling.
apps/dashboard/app/(app)/settings/root-keys/[keyId]/layout.tsx (1)
87-87: Verify consistent error handling pattern across clickhouse queries
The change to access data via res.val aligns with the new error-as-values pattern. However, we should ensure this pattern is consistently applied across all clickhouse query implementations.
Let's verify the consistency of this pattern:
Consider documenting this error handling pattern in the codebase to ensure consistent implementation across the team.
✅ Verification successful
Error handling pattern is consistently applied across clickhouse queries
Based on the search results, I can confirm that the error-as-values pattern using .val is consistently implemented across the codebase. Examples found:
res.val?.at(0)?.time ?? 0in root-keys layoutres.val!in success pageres.val?.at(0)?.timein ratelimits namespace pageres.val!in ratelimits logs page
The pattern is uniformly applied where clickhouse query results are accessed, with appropriate null checking using optional chaining and nullish coalescing operators.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the .val property across clickhouse queries
# Expected: All clickhouse query results should be accessed using .val
# Search for clickhouse query result access patterns
rg -t typescript -t javascript "clickhouse\.(.*?)\." -A 3
# Search for potential inconsistent access patterns
rg -t typescript -t javascript "await clickhouse" -A 3 | rg -v "\.val"
Length of output: 202
Script:
#!/bin/bash
# Let's try without file type filters since they're not recognized
# Search for clickhouse query patterns and their result handling
rg "clickhouse\." -A 3
# Also search for direct await patterns
rg "await clickhouse" -A 3
Length of output: 19338
apps/dashboard/app/(app)/ratelimits/[namespaceId]/overrides/table.tsx (1)
Line range hint 95-99: Verify error handling implementation
The change to access lastUsed.val aligns with the PR objective of returning errors as values. However, we should ensure proper error handling is in place.
Consider explicitly handling the error case:
- const unixMilli = lastUsed.val?.at(0)?.time;
- if (unixMilli) {
+ if (lastUsed.err) {
+ return <Badge variant="destructive">Error: {lastUsed.err}</Badge>;
+ }
+ const unixMilli = lastUsed.val?.at(0)?.time;
+ if (unixMilli) {Let's verify this pattern is consistently applied:
✅ Verification successful
Let me analyze the results of these commands to verify the error handling implementation and consistency.
Based on the search results, I can now generate the final response:
Error handling pattern is consistently implemented
The codebase shows a consistent pattern of error handling for clickhouse query results:
- Most clickhouse query results are properly handled with
.valaccess and optional chaining - Error cases are explicitly handled in multiple files, for example:
apps/api/src/pkg/keys/service.tschecks forres.errbefore accessing.valinternal/clickhouse/src/billing.tschecks bothres.errandres.val.lengthapps/dashboard/app/(app)/settings/billing/page.tsxproperly handles error cases
The current implementation in table.tsx follows this pattern correctly and doesn't require explicit error handling since:
- The optional chaining with
?.safely handles null/undefined cases - The fallback to
<Minus />component already handles the error case appropriately - This is a UI component where showing an error badge would be inappropriate for the "last used" display
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other clickhouse query results to ensure consistent error handling
rg -t typescript -t javascript "clickhouse\.[a-zA-Z]+\.[a-zA-Z]+\(" -A 5
Length of output: 109
Script:
#!/bin/bash
# Search for clickhouse query patterns without file type restrictions
rg "clickhouse\.[a-zA-Z]+\.[a-zA-Z]+\(" -A 5
# Also search for any error handling patterns with .val and .err
rg "\.val\??\." -A 3 -B 3
Length of output: 43057
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/verification-table.tsx (1)
57-60: Handle edge cases in block coloring logic
The current implementation might encounter issues with empty arrays or single-element arrays. While TypeScript provides some safety, it's good practice to handle these edge cases explicitly.
Let's verify the usage of this component with empty or single-element arrays:
Consider adding array length checks:
const isStartOfColoredBlock =
verification.outcome !== "VALID" &&
- (i === 0 || verifications.val[i - 1].outcome === "VALID");
+ (i === 0 || (verifications.val.length > 0 && verifications.val[i - 1].outcome === "VALID"));
const isEndOfColoredBlock =
verification.outcome !== "VALID" &&
- (i === verifications.val.length - 1 || verifications.val[i + 1].outcome === "VALID");
+ (i === verifications.val.length - 1 || (i + 1 < verifications.val.length && verifications.val[i + 1].outcome === "VALID"));apps/dashboard/app/(app)/success/page.tsx (1)
67-67: 🛠️ Refactor suggestion
Consider adding proper error handling instead of using non-null assertion
The non-null assertion (!) on res.val could lead to runtime errors if the response is null or undefined. Consider handling potential errors explicitly:
- const activeWorkspaces = await clickhouse.business.activeWorkspaces().then((res) => res.val!);
+ const activeWorkspaces = await clickhouse.business.activeWorkspaces().then((res) => {
+ if (!res.val) {
+ throw new Error("Failed to fetch active workspaces");
+ }
+ return res.val;
+ });Let's verify the error handling pattern across other Clickhouse calls:
apps/dashboard/app/(app)/settings/root-keys/[keyId]/page.tsx (1)
94-100: 🛠️ Refactor suggestion
Consider safer error handling instead of force unwrapping
The non-null assertion operator (!) is being used to force unwrap res.val, which could lead to runtime errors if val is undefined. Since this PR aims to improve error handling, consider handling potential undefined cases explicitly.
Here's a safer approach:
const history = await clickhouse.verifications
.latest({
workspaceId: UNKEY_WORKSPACE_ID,
keySpaceId: key.keyAuthId,
keyId: key.id,
})
- .then((res) => res.val!);
+ .then((res) => {
+ if (!res.val) {
+ throw new Error("No verification history found");
+ }
+ return res.val;
+ });apps/dashboard/app/(app)/identities/[identityId]/page.tsx (1)
171-171: LGTM! Consistent with new error handling pattern.
The change aligns with the PR objective of returning errors as values for clickhouse operations. The optional chaining is preserved, maintaining null safety.
Let's verify that this pattern is consistently applied across other clickhouse query implementations:
✅ Verification successful
The .val?.at(0) pattern is consistently applied across the codebase
The search results confirm that the error handling pattern using .val?.at(0)?.time is consistently implemented across multiple components that interact with clickhouse data:
settings/root-keys/[keyId]/layout.tsxratelimits/card.tsxratelimits/[namespaceId]/page.tsxratelimits/[namespaceId]/overrides/table.tsxidentities/[identityId]/page.tsxapis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the .val pattern across clickhouse queries
# Expected: All clickhouse query results should be accessed using .val
# Search for clickhouse query patterns
rg -U "clickhouse\.[a-zA-Z]+\s*\.[a-zA-Z]+\s*\(.*\)\s*\.then\(" --type typescript
Length of output: 118
Script:
#!/bin/bash
# Let's try a broader search for clickhouse-related code patterns
rg -l "clickhouse.*then"
# Also search for specific .val access patterns
rg "\.val\?\.at\(0\)"
Length of output: 847
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/page.tsx (1)
125-125:
Consider handling potential undefined values explicitly
The non-null assertion operator (!) assumes res.val will always be defined. This might hide potential error cases and make debugging harder.
Let's verify the error handling pattern across the codebase:
Consider this safer approach:
- const logs = await clickhouse.ratelimits.logs(query).then((res) => res.val!);
+ const logs = await clickhouse.ratelimits.logs(query).then((res) => {
+ if (!res.val) {
+ throw new Error("Failed to fetch ratelimit logs");
+ }
+ return res.val;
+ });apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx (2)
81-81: LGTM: Proper error handling for Clickhouse results
This change correctly implements optional chaining for Clickhouse results, aligning with the PR's goal of returning errors as values.
71-71: 🛠️ Refactor suggestion
Unnecessary optional chaining for SQL count query
The optional chaining on the SQL count query result is overly defensive. The count(*) aggregate function always returns exactly one row with a number (even if that number is 0).
- .then((res) => res?.at(0)?.count ?? 0),
+ .then((res) => res.at(0)!.count),Likely invalid or redundant comment.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx (2)
100-100: LGTM: Proper null safety handling for clickhouse response
The code correctly uses optional chaining to safely access the val property and provides a fallback value, aligning with the new error handling approach.
Line range hint 320-327: LGTM: Safe conditional rendering with proper null checks
The code correctly checks for both the existence of latestVerifications.val and its length before rendering the VerificationTable component.
| export class QueryError extends BaseError<{ query: string }> { | ||
| public readonly retry = true; | ||
| public readonly name = QueryError.name; | ||
| constructor(message: string, context: { query: string }) { | ||
| super({ | ||
| message, | ||
| context, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
QueryError's retry flag should be false for validation failures
Based on the usage patterns found in the codebase, QueryError is being used for validation failures like malformed parameters and schema validation errors. These types of errors are not transient and should not be retried as they will fail consistently until the input is corrected.
internal/clickhouse/src/client/client.ts: Used for parameter validation errors and schema validation failures which are not retryable conditions
The retry flag should be set based on the error type rather than being hardcoded to true for all query errors.
🔗 Analysis chain
Verify retry behavior for query errors.
The implementation looks good with proper context handling. However, let's verify if all query errors should be retryable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for query error handling patterns in the codebase
# Look for patterns that might indicate non-retryable query errors
# Search for SQL syntax or similar query errors that shouldn't be retried
rg -g '*.ts' -g '*.js' -B 3 -A 3 'syntax|malformed|invalid.*query'
# Look for existing error handling patterns
ast-grep --pattern 'catch ($err) {
$$$
throw new QueryError($$$)
}'
Length of output: 24370
Script:
#!/bin/bash
# Let's search for actual query error handling patterns in the clickhouse client code
# to understand how QueryError is used in practice
# Look for QueryError instantiation patterns
rg -g '*.ts' -B 5 -A 2 'new QueryError'
# Look for retry-related code near QueryError
rg -g '*.ts' -B 3 -A 3 'retry.*QueryError|QueryError.*retry'
# Search for error classification or handling logic
ast-grep --pattern 'if ($condition) {
$$$
throw new QueryError($$$)
}'
Length of output: 1623
| const history = await clickhouse.verifications | ||
| .logs({ | ||
| workspaceId: UNKEY_WORKSPACE_ID, | ||
| keySpaceId: key.keyAuthId, | ||
| keyId: key.id, | ||
| }) | ||
| .then((res) => res.val!); |
There was a problem hiding this comment.
Remove unsafe non-null assertion and add proper error handling
The current implementation assumes the clickhouse query will always succeed and forcefully unwraps the result with !, which could lead to runtime errors if res.val is null or undefined. This defeats the purpose of returning errors as values.
Consider handling potential errors and empty results explicitly:
- const history = await clickhouse.verifications
- .logs({
- workspaceId: UNKEY_WORKSPACE_ID,
- keySpaceId: key.keyAuthId,
- keyId: key.id,
- })
- .then((res) => res.val!);
+ const result = await clickhouse.verifications
+ .logs({
+ workspaceId: UNKEY_WORKSPACE_ID,
+ keySpaceId: key.keyAuthId,
+ keyId: key.id,
+ });
+
+ if (!result.val) {
+ // Handle the error case appropriately
+ // This could be showing an error state or returning notFound()
+ return <div>Failed to load verification history</div>;
+ }
+
+ const history = result.val;Committable suggestion skipped: line range outside the PR's diff.
| }): (params: z.input<TIn>) => Promise<Result<z.output<TOut>[], QueryError>> { | ||
| return async (params: z.input<TIn>): Promise<Result<z.output<TOut>[], QueryError>> => { | ||
| req.params?.safeParse(params); | ||
| return []; | ||
| return Ok([]); | ||
| }; |
There was a problem hiding this comment.
Fix parameter validation handling
The parameter validation result from safeParse is not being checked. This could lead to silent failures if invalid parameters are provided.
Apply this fix:
return async (params: z.input<TIn>): Promise<Result<z.output<TOut>[], QueryError>> => {
- req.params?.safeParse(params);
+ if (req.params) {
+ const validation = req.params.safeParse(params);
+ if (!validation.success) {
+ return Err(new QueryError(`Invalid parameters: ${validation.error.message}`));
+ }
+ }
return Ok([]);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }): (params: z.input<TIn>) => Promise<Result<z.output<TOut>[], QueryError>> { | |
| return async (params: z.input<TIn>): Promise<Result<z.output<TOut>[], QueryError>> => { | |
| req.params?.safeParse(params); | |
| return []; | |
| return Ok([]); | |
| }; | |
| }): (params: z.input<TIn>) => Promise<Result<z.output<TOut>[], QueryError>> { | |
| return async (params: z.input<TIn>): Promise<Result<z.output<TOut>[], QueryError>> => { | |
| if (req.params) { | |
| const validation = req.params.safeParse(params); | |
| if (!validation.success) { | |
| return Err(new QueryError(`Invalid parameters: ${validation.error.message}`)); | |
| } | |
| } | |
| return Ok([]); | |
| }; |
| clickhouse.ratelimits | ||
| .perMinute({ | ||
| workspaceId: workspace.id, | ||
| namespaceId: namespace.id, | ||
| start: end - intervalMs, | ||
| end, | ||
| }) | ||
| .then((res) => res.val!), |
There was a problem hiding this comment.
Add proper error handling for the perMinute response
The non-null assertion operator (!) on res.val could lead to runtime errors if the response contains an error or if val is undefined. Since this PR aims to improve error handling, consider handling potential error cases explicitly.
Consider this safer approach:
clickhouse.ratelimits
.perMinute({
workspaceId: workspace.id,
namespaceId: namespace.id,
start: end - intervalMs,
end,
})
- .then((res) => res.val!),
+ .then((res) => {
+ if (!res.val) {
+ // You might want to return a default value or throw a custom error
+ return [];
+ }
+ return res.val;
+ }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| clickhouse.ratelimits | |
| .perMinute({ | |
| workspaceId: workspace.id, | |
| namespaceId: namespace.id, | |
| start: end - intervalMs, | |
| end, | |
| }) | |
| .then((res) => res.val!), | |
| clickhouse.ratelimits | |
| .perMinute({ | |
| workspaceId: workspace.id, | |
| namespaceId: namespace.id, | |
| start: end - intervalMs, | |
| end, | |
| }) | |
| .then((res) => { | |
| if (!res.val) { | |
| // You might want to return a default value or throw a custom error | |
| return []; | |
| } | |
| return res.val; | |
| }), |
| clickhouse.ratelimits | ||
| .perMinute({ | ||
| workspaceId: workspace.id, | ||
| namespaceId: namespace.id, | ||
| start: end - intervalMs, | ||
| end, | ||
| }) | ||
| .then((res) => res.val!), | ||
| clickhouse.ratelimits | ||
| .latest({ workspaceId: workspace.id, namespaceId: namespace.id }) | ||
| .then((res) => res.at(0)?.time), | ||
| .then((res) => res.val?.at(0)?.time), |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent error handling patterns detected
The codebase shows mixed usage of error handling patterns with Clickhouse responses:
-
Some code paths use non-null assertions (
res.val!):apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/page.tsxapps/dashboard/app/(app)/success/page.tsx
-
While others properly handle potential null values (
res.val?):apps/dashboard/app/(app)/settings/root-keys/[keyId]/layout.tsxapps/dashboard/app/(app)/ratelimits/card.tsxapps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx
The underlying Clickhouse client returns Result<T> types that should be properly handled without force unwrapping. The non-null assertions should be replaced with proper null checking to maintain consistent error handling across the codebase.
🔗 Analysis chain
Verify consistent error handling patterns
Let's ensure this error handling pattern is consistently implemented across other Clickhouse query handlers in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent error handling patterns in Clickhouse queries
# Look for direct non-null assertions on Clickhouse response values
rg -A 2 "clickhouse\." | rg "\.val!"
# Look for proper error handling patterns
rg -A 2 "clickhouse\." | rg "\.val\?"
Length of output: 784
Script:
#!/bin/bash
# Let's check the implementation of these Clickhouse query handlers to understand the return types
# Search for clickhouse ratelimits type definitions and implementations
ast-grep --pattern 'interface $_ {
ratelimits: {
$$$
}
}'
# Also check the business methods
ast-grep --pattern 'interface $_ {
business: {
$$$
}
}'
Length of output: 133
Script:
#!/bin/bash
# Let's try a different approach to find the Clickhouse client implementation
# Look for files that might contain the Clickhouse client definition
fd clickhouse
# Search for return type definitions in these files
rg -A 3 "type.*Result.*="
# Look for error handling patterns in the implementation
rg -A 3 "export (type|interface).*Click"
Length of output: 2353
Script:
#!/bin/bash
# Let's examine the error handling implementation in the dashboard's clickhouse client
# Check the clickhouse client implementation
cat apps/dashboard/lib/clickhouse.ts
# Also check the internal clickhouse package
cat internal/clickhouse/src/index.ts
# Look for Result type usage in these files
rg -A 3 "Result<" apps/dashboard/lib/clickhouse.ts internal/clickhouse/src/index.ts
Length of output: 3018
Script:
#!/bin/bash
# Let's check the implementation of ratelimits functions to understand the return types
# Check the ratelimits implementation file
cat internal/clickhouse/src/ratelimits.ts
# Also look for Result type usage in any implementation files
rg -A 3 "Result<" internal/clickhouse/src/
Length of output: 8841
| getVerificationsPerInterval(query).then((res) => res.val!), | ||
| getActiveKeysPerInterval(query).then((res) => res.val!), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling potential null values instead of using non-null assertions
The non-null assertions (!) on .val properties assume these values will never be null. This could lead to runtime errors if the queries fail or return no data.
Consider this safer approach:
- getVerificationsPerInterval(query).then((res) => res.val!),
- getActiveKeysPerInterval(query).then((res) => res.val!),
+ getVerificationsPerInterval(query).then((res) => res.val ?? []),
+ getActiveKeysPerInterval(query).then((res) => res.val ?? []),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getVerificationsPerInterval(query).then((res) => res.val!), | |
| getActiveKeysPerInterval(query).then((res) => res.val!), | |
| getVerificationsPerInterval(query).then((res) => res.val ?? []), | |
| getActiveKeysPerInterval(query).then((res) => res.val ?? []), |
| getVerificationsPerInterval({ | ||
| workspaceId: api.workspaceId, | ||
| keySpaceId: api.keyAuthId!, | ||
| start: billingCycleStart, | ||
| end: billingCycleEnd, | ||
| }), | ||
| }).then((res) => res.val!), |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the gathered context, I can now generate the final response:
Consider safer error handling for verification data
The non-null assertion on verification data is used in billing calculations where verificationsInBillingCycle.reduce((sum, day) => sum + day.count, 0) computes the total count. A null value could cause runtime errors.
- }).then((res) => res.val!),
+ }).then((res) => res.val ?? []),- The safer approach ensures billing calculations continue with zero counts instead of crashing if data is missing
- This pattern aligns with other safe handling seen in the codebase (e.g.,
res?.at(0)?.count ?? 0)
🔗 Analysis chain
Ensure safe handling of verification data for billing calculations
The non-null assertion on verification data could affect billing calculations if the query fails or returns no data.
Consider this safer approach:
- }).then((res) => res.val!),
+ }).then((res) => res.val ?? []),Let's verify the usage of this data in calculations:
Let me gather more context about the verification data structure and its usage.
Let me fix the regex and try again.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where verification data is used in calculations
rg -A 5 "verificationsInBillingCycle" apps/dashboard/
Length of output: 1147
Script:
#!/bin/bash
# Check the type definition and structure of verification data
ast-grep --pattern 'type $_ = { count: number $$$}'
# Look for error handling patterns around verification data
rg -B 3 -A 3 "\.val\!" apps/dashboard/
Length of output: 173
Script:
#!/bin/bash
# Check the type definition and structure of verification data
ast-grep --pattern 'type $_ = { count: number }'
# Look for error handling patterns around verification data
rg -B 3 -A 3 '\.val!' apps/dashboard/
# Get more context around the verification data usage
rg -B 5 -A 5 'reduce\(.+sum \+ day\.count' apps/dashboard/
Length of output: 8560
| @@ -108,7 +108,7 @@ export default async function APIKeyDetailPage(props: { | |||
| const expiredOverTime: { x: string; y: number }[] = []; | |||
| const forbiddenOverTime: { x: string; y: number }[] = []; | |||
|
|
|||
| for (const d of verifications.sort((a, b) => a.time - b.time)) { | |||
| for (const d of verifications.val!.sort((a, b) => a.time - b.time)) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider safer null handling instead of non-null assertion
The non-null assertion (!) could lead to runtime errors if verifications.val is undefined. Consider using optional chaining with a default empty array.
- for (const d of verifications.val!.sort((a, b) => a.time - b.time)) {
+ for (const d of (verifications.val ?? []).sort((a, b) => a.time - b.time)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const d of verifications.val!.sort((a, b) => a.time - b.time)) { | |
| for (const d of (verifications.val ?? []).sort((a, b) => a.time - b.time)) { |
| let err: Error | undefined = undefined; | ||
| for (let i = 1; i <= 3; i++) { | ||
| try { | ||
| return fn(i); |
There was a problem hiding this comment.
Add await when calling fn(i) to ensure proper error handling
In the retry method, you need to await the call to fn(i) within the try block to ensure that any errors are correctly caught by the catch block. Without await, the try...catch construct won't catch asynchronous errors thrown by the promise returned from fn(i).
Apply this code change:
- return fn(i);
+ return await fn(i);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return fn(i); | |
| return await fn(i); |
| return this.retry(() => | ||
| this.client | ||
| .insert({ | ||
| table: req.table, | ||
| format: "JSONEachRow", | ||
| values: Array.isArray(validatedEvents) ? validatedEvents : [validatedEvents], | ||
| }) | ||
| .then((res) => Ok(res)) | ||
| .catch((err) => Err(new InsertError(err.message))), | ||
| ); |
There was a problem hiding this comment.
Adjust error handling in insert method to work with retry function
The retry function is designed to handle functions that throw exceptions on failure. However, in the insert method, the function passed to retry returns a Result type with Err on failure instead of throwing an exception. This means that retries may not occur as expected since no exception is thrown to be caught.
Consider modifying the function to throw exceptions on failure so that the retry method can catch them. Apply the following changes:
return this.retry(() =>
this.client
.insert({
table: req.table,
format: "JSONEachRow",
values: Array.isArray(validatedEvents) ? validatedEvents : [validatedEvents],
})
- .then((res) => Ok(res))
- .catch((err) => Err(new InsertError(err.message))),
+ .then((res) => res)
+ .catch((err) => {
+ throw new InsertError(err.message);
+ }),
);Then, handle the result after the retry call:
+ try {
+ const res = await this.retry(...);
+ return Ok(res);
+ } catch (err) {
+ return Err(err);
+ }Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
DialogAddPermissionsForAPIcomponent by removing unnecessary memoization.These updates enhance the robustness and reliability of data handling across the application, ensuring a smoother user experience.