Conversation
ogzhanolguncu
requested review from
MichaelUnkey,
chronark,
mcstepp and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
📝 WalkthroughWalkthroughThis pull request simplifies pagination handling across log query implementations. The changes remove the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Hook
participant TRPC
participant DB
Client->>Hook: Request logs (optional cursor)
Hook->>TRPC: Execute query with numeric cursor
TRPC->>DB: Run SQL query using cursorTime condition
DB-->>TRPC: Return logs and last log time (nextCursor)
TRPC-->>Hook: Respond with logs and nextCursor
Hook-->>Client: Render logs with updated pagination
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (4)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/query-logs.schema.ts (1)
40-40: Simplified cursor schema from object to numberThe cursor schema has been simplified from an object with
requestIdandtimeproperties to a single nullable number value, which aligns with the PR's goal of removingrequest_idfrom cursors.However, there are redundant nullable modifiers in the schema definition.
- cursor: z.number().nullable().optional().nullable(), + cursor: z.number().nullable().optional(),apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/query-logs.schema.ts (1)
14-14: Simplified cursor schema from object to numberThe cursor schema has been simplified from an object with
requestIdandtimeproperties to a single nullable number value, which aligns with the PR's goal of removingrequest_idfrom cursors.However, there are redundant nullable modifiers in the schema definition.
- cursor: z.number().nullable().optional().nullable(), + cursor: z.number().nullable().optional(),apps/dashboard/app/(app)/logs/components/table/query-logs.schema.ts (1)
59-59: Redundant nullable() call in cursor schema definitionThe cursor field definition has
.nullable()applied twice, which is redundant and may cause confusion. You should only apply it once.- cursor: z.number().nullable().optional().nullable(), + cursor: z.number().nullable().optional(),apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/query-logs.schema.ts (1)
33-33: Redundant nullable() call in cursor schema definitionThe cursor field definition has
.nullable()applied twice, which is redundant and may cause confusion. You should only apply it once.- cursor: z.number().nullable().optional().nullable(), + cursor: z.number().nullable().optional(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/hooks/use-logs-query.ts(0 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/query-logs.schema.ts(1 hunks)apps/dashboard/app/(app)/logs/components/table/hooks/use-logs-query.ts(0 hunks)apps/dashboard/app/(app)/logs/components/table/query-logs.schema.ts(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/hooks/use-logs-query.ts(0 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/query-logs.schema.ts(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.ts(0 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/query-logs.schema.ts(1 hunks)apps/dashboard/app/auth/actions.ts(1 hunks)apps/dashboard/lib/trpc/routers/api/keys/query-overview-logs/index.ts(3 hunks)apps/dashboard/lib/trpc/routers/api/keys/query-overview-logs/utils.ts(1 hunks)apps/dashboard/lib/trpc/routers/logs/query-logs/index.ts(3 hunks)apps/dashboard/lib/trpc/routers/logs/query-logs/utils.test.ts(0 hunks)apps/dashboard/lib/trpc/routers/logs/query-logs/utils.ts(0 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-logs/index.ts(3 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-logs/utils.ts(1 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts(3 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/utils.ts(1 hunks)internal/clickhouse/src/keys/keys.ts(2 hunks)internal/clickhouse/src/logs.ts(1 hunks)internal/clickhouse/src/ratelimits.ts(3 hunks)
💤 Files with no reviewable changes (6)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/hooks/use-logs-query.ts
- apps/dashboard/app/(app)/logs/components/table/hooks/use-logs-query.ts
- apps/dashboard/lib/trpc/routers/logs/query-logs/utils.ts
- apps/dashboard/lib/trpc/routers/logs/query-logs/utils.test.ts
- apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/hooks/use-logs-query.ts
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.ts
🧰 Additional context used
🧠 Learnings (1)
internal/clickhouse/src/ratelimits.ts (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2896
File: internal/clickhouse/src/ratelimits.ts:468-592
Timestamp: 2025-04-02T16:10:31.720Z
Learning: The cursor logic in getRatelimitOverviewLogs query uses (time, request_id) < (cursorTime, cursorRequestId) comparison which works well with descending order but may need adjustment for ascending sorts based on real usage patterns.
🧬 Code Graph Analysis (1)
apps/dashboard/app/auth/actions.ts (1)
apps/dashboard/lib/auth/types.ts (1)
errorMessages(178-194)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (26)
apps/dashboard/app/auth/actions.ts (1)
167-167: Fixed missing comma in object literalAdded the missing comma after the
messageproperty, ensuring proper JavaScript object syntax.apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/utils.ts (1)
39-39: Simplified cursor handling by removing requestIdThe cursor handling has been simplified by replacing the extraction of separate
requestIdandtimeproperties with a singlecursorTimevalue. This aligns with the PR's goal of removingrequest_idfrom cursors.apps/dashboard/lib/trpc/routers/ratelimit/query-logs/utils.ts (1)
37-37: Cursor handling simplification looks goodThe simplified cursor handling correctly implements the refactoring that removes the request_id from cursors.
internal/clickhouse/src/logs.ts (1)
145-169: Good refactoring of the pagination queryThe SQL query has been successfully simplified to use only the time-based cursor, which streamlines the pagination logic. This is a positive change that aligns with removing request_id from cursors throughout the application.
apps/dashboard/lib/trpc/routers/api/keys/query-overview-logs/utils.ts (1)
54-54: Simplified cursor handlingThe cursor handling has been streamlined by directly using the cursor value instead of extracting the time property from a cursor object. This change aligns with the PR goal of removing request_id from cursors.
apps/dashboard/lib/trpc/routers/ratelimit/query-logs/index.ts (3)
14-14: Simplified cursor schemaThe nextCursor schema has been changed from an object to a single optional integer, which matches the PR objective of simplifying cursor handling.
59-59: Simplified cursor parameter handlingThe cursor parameter handling is now more straightforward, directly using the input.cursor value instead of extracting time and requestId from an object.
78-78: Simplified cursor value in responseThe nextCursor value is now directly set to the timestamp of the last record instead of constructing an object with both time and requestId properties.
internal/clickhouse/src/ratelimits.ts (4)
582-582: Updated ORDER BY clause to remove request_idThe ORDER BY clause has been simplified to only reference time-related fields, removing request_id. This is consistent with the PR objectives.
I notice that you're using the retrieved learning about cursor logic in getRatelimitOverviewLogs that previously leveraged both time and request_id for pagination. The current implementation still includes time direction handling which should maintain proper pagination order.
586-602: Streamlined cursor condition logicThe cursor condition logic has been simplified to focus solely on time-based comparisons rather than composite conditions with both time and requestId. This change:
- Makes the code more readable
- Aligns with the PR objective of removing request_id from cursors
- Maintains proper direction handling for ascending/descending pagination
357-359:Details
✅ Verification successful
Simplified cursor condition
The cursor condition has been simplified to only check the time field rather than using a compound condition with both time and requestId. This aligns with the PR goal of removing request_id from cursors.
🏁 Script executed:
#!/bin/bash # Check existing query patterns for time-based cursors in other files echo "Checking for similar cursor pattern usage across the codebase:" rg --type ts "cursorTime.*IS NULL OR time" -A 1 -B 1Length of output: 419
Cursor condition simplified and verified
The updated implementation now leverages a time-only condition:
AND ( {cursorTime: Nullable(UInt64)} IS NULL OR time < {cursorTime: Nullable(UInt64)} )This change meets the PR’s goal of removing the compound condition that previously involved
request_id. A grep search across the codebase confirms that the pattern is correctly applied ininternal/clickhouse/src/ratelimits.tsand no remnants of request_id-based conditions remain.
398-398:Details
❓ Verification inconclusive
Removed request_id from ORDER BY clause
The ORDER BY clause now only sorts by time in descending order, removing the request_id field. This could potentially affect pagination consistency if multiple records share the exact same timestamp.
Consider testing pagination with data that contains duplicate timestamps to ensure consistent results. The uniqueness of time values at millisecond precision should be sufficient in most cases, but this could be a concern with high-frequency events.
🏁 Script executed:
#!/bin/bash # Look for similar ORDER BY patterns in other query files echo "Checking for similar ORDER BY patterns across the codebase:" rg --type ts "ORDER BY.*time DESC" -A 3 -B 3Length of output: 2136
Action: Verify Pagination Consistency When Ordering by Timestamp Only
The change in
internal/clickhouse/src/ratelimits.tsnow orders results solely by time (usingORDER BY fr.time DESC), which aligns with similar patterns in other parts of the codebase (e.g., inlogs.tsandlatest_verifications.ts). Although millisecond precision generally provides sufficient uniqueness under normal conditions, there's a potential risk of pagination inconsistencies when multiple records share the same timestamp, particularly with high-frequency events.Recommendations:
- Test Pagination: Run pagination tests with data containing duplicate timestamps to confirm that the results stay consistent.
- Monitor Edge Cases: Validate that the reliance on the millisecond-precision timestamp does not lead to unexpected ordering issues.
apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts (3)
14-14: Simplified cursor schemaThe nextCursor schema has been changed from an object to a single optional integer value, which matches the PR objective of removing request_id from cursors.
59-59: Simplified cursor parameter handlingThe cursor parameter handling has been streamlined to directly use the input.cursor value instead of extracting time and requestId from an object structure.
80-82: Simplified nextCursor value in responseThe nextCursor value is now directly set to the timestamp of the last record instead of constructing an object with both time and requestId properties. This simplification is consistent with the changes in other files.
internal/clickhouse/src/keys/keys.ts (5)
216-217: Change to time-only cursor condition handling looks correct.The removal of
request_idfrom cursors has been implemented correctly here, with the conditional now only checking forcursorTimeinstead of both time and request ID.
219-219: SQL cursor condition correctly updated.The cursor condition SQL has been properly updated to only reference
cursorTimeinstead of both time and request ID parameters.
225-226: Pagination conditions correctly simplified.Both ascending and descending time conditions have been updated to only reference the time parameter, consistently with the cursor refactoring.
Also applies to: 229-230
253-254: Updated comment accurately reflects the implementation change.The comment now correctly indicates that pagination uses only time as a cursor, which matches the implementation.
209-211: Order by clause correctly simplified.The ORDER BY clause has been updated to only consider time for pagination, which aligns with the cursor simplification approach.
apps/dashboard/lib/trpc/routers/logs/query-logs/index.ts (3)
14-14: Schema update to simplify nextCursor type.The
nextCursorproperty has been correctly updated from an object to a simple integer, aligning with the removal ofrequest_idfrom cursors.
50-50: Simplified cursor parameter passing.The function call now correctly passes
input.cursor ?? nulldirectly ascursorTime, rather than extracting both time and request ID from a complex object.
70-70: nextCursor construction simplified.The construction of
nextCursornow correctly uses just the time from the last log entry instead of creating an object with both time and request ID.apps/dashboard/lib/trpc/routers/api/keys/query-overview-logs/index.ts (3)
13-13: Schema update to simplify nextCursor type.The
nextCursorproperty has been correctly updated from an object to a simple integer, consistent with the cursor simplification across the codebase.
44-44: Simplified cursor parameter passing.The function call now correctly passes
input.cursor ?? nulldirectly ascursorTime, aligning with the updated parameter structure in the clickhouse API.
96-96: nextCursor construction simplified.The construction of
nextCursorhas been correctly updated to only use the time value from the last log entry, consistent with the cursor simplification strategy.
|
btw, AND was required for running the cursor after WHERE string filter
like this. anyways I made it shorter case was redundant |
|
yeah that’s why I deleted my comment, haha |
|
now you gaslighting me :pepehands: |
* refactor: remove request_id from cursors (#3093) * refactor: remove requestid from logs cursor * fix: cursor object * refactor: remove request_id from ratelimit overview cursor * refactor: remove request_id from ratelimit logs * refactor: remove request id from api cursor * fix: type issue and make query shorter --------- Co-authored-by: Andreas Thomas <dev@chronark.com> * feat: errors v2 (#3110) * feat: /v2/apis/createApi endpoint * ci: install api dependencies * feat: errors * [autofix.ci] apply automated fixes * revert: breaking changes * revert: bad copypaste * fix: bad copypaste --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * feat: use axiom * fix: don't hide the cool dots --------- Co-authored-by: Oğuzhan Olguncu <21091016+ogzhanolguncu@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* add: initial layout * feat: add new api keys page * feat: add color to expires badge * [autofix.ci] apply automated fixes * feat: add chart * feat: finalize usage chart * fix: expires component * fix: timestamps and chart colors * fix: build issues * feat: add draft of status popover * fix: status cleanup * feat: finalize status design * fix: Style issues * feat: add configurable skeletons to table * fix: identitiy * fix: total count * chore: remove unused component * feat: add two new operator to key id filter * fix: type issues * fix: identity filter * feat: add ai saerch to keys * fix: add status condition for disabled * fix: some styling issues * refactor: improve schema structure * chore: remove unused types * fix: navbar * feat: add dynamic key to popover menu * fix: coderabbit issues * fix: number formatting and filtering logic for outcome explainer * fix: empty state message * refactor: refinements * chore: remove old keys * fix: import paths * fix: header wording * fix: secret hover (#3111) * refactor: remove request_id from cursors (#3093) * refactor: remove requestid from logs cursor * fix: cursor object * refactor: remove request_id from ratelimit overview cursor * refactor: remove request_id from ratelimit logs * refactor: remove request id from api cursor * fix: type issue and make query shorter --------- Co-authored-by: Andreas Thomas <dev@chronark.com> * feat: errors v2 (#3110) * feat: /v2/apis/createApi endpoint * ci: install api dependencies * feat: errors * [autofix.ci] apply automated fixes * revert: breaking changes * revert: bad copypaste * fix: bad copypaste --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * feat: use axiom * fix: don't hide the cool dots --------- Co-authored-by: Oğuzhan Olguncu <21091016+ogzhanolguncu@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * revert: bad commits * fix: merge errors * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: chronark <dev@chronark.com>
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