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 ↗︎
|
📝 WalkthroughWalkthroughThis pull request replaces the previous Changes
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (12)
✨ 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)
internal/ui/tailwind.config.js (1)
78-78: Clarify the "accent" → "brand" association.
Noting that"accent"is also labeled as"brand"in Figma ensures alignment between design and development. Please double-check for references that may still use “brand” terminology in code or documentation.apps/dashboard/components/virtual-table/hooks/useTableHeight.ts (1)
2-3: Use a configurable spacing constant.
It’s helpful to have a top-level constant for “breathing space.” Consider making it a configurable parameter if different pages or layouts might require custom spacing.apps/dashboard/components/virtual-table/index.tsx (1)
279-307: Remove the commented-out code block.
The new<LoadMoreFooter />is fully integrated, so the old load-more UI snippet is no longer needed. Eliminating it cleans up the file and avoids confusion.- {/* <div className="sticky bottom-0 left-0 right-0 w-full items-center ... */} - {/* ... */} - {/* </div> */} + // Removed old code as it's no longer usedapps/dashboard/components/virtual-table/components/loading-indicator.tsx (1)
1-124: Comprehensive “load more” footer implementation.
This component provides a clean UI/UX pattern with proximity detection and loading states. A couple of ideas:
• MakePROXIMITY_THRESHOLDconfigurable for different user interactions.
• Consider makingonLoadMorea required prop if loading more is always intended.
Overall, this is a handy addition that aligns well with common usage patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/dashboard/components/virtual-table/components/loading-indicator.tsx(1 hunks)apps/dashboard/components/virtual-table/constants.ts(0 hunks)apps/dashboard/components/virtual-table/hooks/useTableHeight.ts(1 hunks)apps/dashboard/components/virtual-table/index.tsx(3 hunks)apps/dashboard/components/virtual-table/types.ts(0 hunks)internal/ui/tailwind.config.js(2 hunks)
💤 Files with no reviewable changes (2)
- apps/dashboard/components/virtual-table/constants.ts
- apps/dashboard/components/virtual-table/types.ts
🧰 Additional context used
🧬 Code Definitions (1)
apps/dashboard/components/virtual-table/index.tsx (2)
apps/dashboard/components/virtual-table/hooks/useTableHeight.ts (1)
useTableHeight(4-27)apps/dashboard/components/virtual-table/components/loading-indicator.tsx (1)
LoadMoreFooter(14-123)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
internal/ui/tailwind.config.js (1)
68-68: No issues with alpha color usage.
The addition of"grayA"is consistent with how alpha colors are handled. Good job keeping the naming approach uniform.apps/dashboard/components/virtual-table/hooks/useTableHeight.ts (3)
12-12: Recalculation logic appears sound.
SubtractingBREATHING_SPACEis a neat and direct approach to control the table height within the viewport.
25-25: Dependency array correctly updated.
Removing now-unneeded dependencies keeps the effect properly scoped tocontainerRef.
4-4:Details
❓ Verification inconclusive
Simplified hook signature.
Removing extra parameters reduces complexity. Verify all references toheaderHeightandtableBorderare eliminated to avoid potential runtime issues.
🏁 Script executed:
#!/bin/bash # Searching for references to removed parameters in the entire repository rg -A 3 "useTableHeight\(" rg -A 3 "headerHeight" rg -A 3 "tableBorder"Length of output: 847
Simplified hook signature verified – manual confirm required for remaining headerHeight references.
The refactoring of
useTableHeightto accept only acontainerRefappears correct based on the repository search. No call to this hook is passing extra parameters, andtableBorderhas been fully removed. However, note thatheaderHeightstill exists in the virtual table’s type definitions (in types.ts) and constants (in constants.ts). Please ensure that these static references toheaderHeightare intentional and remain decoupled from the hook’s implementation to avoid any unintended runtime dependencies.apps/dashboard/components/virtual-table/index.tsx (2)
5-5: Updated import aligns with the new loading component.
This import replacement is consistent with removing the previousLoadingIndicator. Confirm that any old references are removed.
58-58: Hook usage reflects parameter removal.
Using onlycontainerRefis consistent with the simplifieduseTableHeightsignature.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/dashboard/lib/trpc/routers/ratelimit/query-logs/index.ts (1)
69-74: Consider more specific error handlingThe current error handling doesn't distinguish between failures in the count query versus the logs query. This might make debugging more difficult.
-if (countResult.err || logsResult.err) { +if (countResult.err || logsResult.err) { + const errorSource = countResult.err ? 'count query' : 'logs query'; throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", - message: "Something went wrong when fetching data from clickhouse.", + message: `Something went wrong when fetching data from clickhouse (${errorSource}).`, }); }apps/dashboard/components/virtual-table/components/loading-indicator.tsx (2)
16-32: Consider improving visibility logic and empty returnThe component correctly initializes default values and has early return logic, but there are two minor issues:
- The
shouldShowlogic only checks ifonLoadMoreexists, but doesn't considerhasMore. Consider hiding the component when there's nothing more to load.- Returning
undefinedwhenhideis true could cause issues in some contexts. Consider returningnullinstead.- const shouldShow = !!onLoadMore; + const shouldShow = !!onLoadMore && hasMore; - if (hide) { - return; - } + if (hide) { + return null; + }
33-64: Consider making the component more responsiveThe component has a fixed width of 740px which might not work well on smaller screens. Consider using responsive width classes to ensure it works across different device sizes.
- <div className="w-[740px] border bg-gray-1 dark:bg-black border-gray-6 h-[60px] flex items-center justify-center p-[18px] rounded-[10px] drop-shadow-lg shadow-sm mb-5"> + <div className="w-full max-w-[740px] mx-auto border bg-gray-1 dark:bg-black border-gray-6 h-[60px] flex items-center justify-center p-[18px] rounded-[10px] drop-shadow-lg shadow-sm mb-5">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/table/hooks/use-logs-query.ts(3 hunks)apps/dashboard/app/(app)/logs/components/table/logs-table.tsx(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/hooks/use-logs-query.ts(3 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/logs-table.tsx(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.ts(3 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/logs-table.tsx(2 hunks)apps/dashboard/components/virtual-table/components/loading-indicator.tsx(1 hunks)apps/dashboard/components/virtual-table/index.tsx(3 hunks)apps/dashboard/components/virtual-table/types.ts(1 hunks)apps/dashboard/lib/trpc/routers/logs/query-logs/index.ts(2 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-logs/index.ts(2 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts(2 hunks)internal/clickhouse/src/logs.ts(2 hunks)internal/clickhouse/src/ratelimits.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/components/virtual-table/index.tsx
🧰 Additional context used
🧬 Code Definitions (3)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/logs-table.tsx (1)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/hooks/use-logs-query.ts (1)
useRatelimitOverviewLogsQuery(14-128)
internal/clickhouse/src/ratelimits.ts (2)
internal/clickhouse/src/client/client.ts (1)
query(24-61)internal/clickhouse/src/client/noop.ts (1)
query(6-22)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (1)
apps/dashboard/app/(app)/logs/components/table/hooks/use-logs-query.ts (1)
useLogsQuery(18-241)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (41)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/hooks/use-logs-query.ts (3)
15-15: LGTM: Good addition of totalCount stateThe addition of
totalCountstate is a good enhancement that will help provide better pagination information in the UI.
113-115: LGTM: Properly handling totalCount updateThis implementation correctly updates the totalCount when data is available and safely handles the case where no pages exist.
124-124: LGTM: Exposing totalCount in the return valueExposing the totalCount in the return object makes it available to components that use this hook, which aligns with the PR objective of improving the load more component functionality.
apps/dashboard/components/virtual-table/types.ts (1)
47-53: Well-structured properties for the new load more footerThe type definition for
loadMoreFooterPropsis well-structured and provides a clear API for the new load more component. The optional nature of all properties gives flexibility in how this feature can be used across different tables.apps/dashboard/lib/trpc/routers/ratelimit/query-logs/index.ts (4)
13-13: LGTM: Adding total property to response schemaAdding the
totalproperty to the response schema is a good enhancement that will support the new load more functionality.
59-66: LGTM: Destructuring queries for better readabilityGood refactoring to destructure the count and logs queries for clarity.
67-68: LGTM: Using Promise.all for concurrent executionUsing
Promise.allto execute both queries concurrently is an excellent performance optimization.
76-88: LGTM: Adding total count to the responseGood implementation of adding the total count to the response object from the count query result.
apps/dashboard/app/(app)/logs/components/table/hooks/use-logs-query.ts (3)
25-25: New state variable for tracking total count.This addition of the
totalCountstate enables proper pagination indicators in the UI.
217-220: LGTM: Effective update of totalCount from initial data.The conditional check ensures that
totalCountis only updated wheninitialData.pageshas content, preventing potential errors.
239-239: Properly exposing totalCount through the hook return value.This change completes the integration of the total count functionality, making it available to consuming components.
apps/dashboard/lib/trpc/routers/logs/query-logs/index.ts (5)
13-13: Schema updated to include total count information.Adding the
totalfield to theLogsResponseschema ensures type safety for the new count information.
50-55: Improved query structure with separate count and logs queries.The destructuring approach clearly separates the distinct query responsibilities.
57-59: Performance optimization with concurrent queries.Using
Promise.allto execute both queries concurrently is an excellent performance improvement compared to sequential execution.
66-66: Variable assignment updated for clarity.Updating the variable assignment to use
logsResult.valimproves code readability.
72-72: Total count properly included in the response.This change ensures the total count information is returned to the client for use in pagination UI.
apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts (5)
13-13: Schema updated consistently with logs router.Adding the
totalfield to theRatelimitOverviewLogsResponseschema maintains consistency with other similar schemas.
59-65: Improved query structure for ratelimit overview logs.The destructuring approach clearly separates the count and logs queries, mirroring the pattern used in the logs router.
67-69: Performance optimization with concurrent queries.Using
Promise.allto execute both queries concurrently is a good performance practice. The error handling properly checks both query results.
76-76: Variable assignment updated for consistency.Using
logsResult.valmaintains consistency with the variable naming changes in the logs router.
80-80: Total count properly included in the response.Including the total count in the response enables pagination UI to show accurate information to users.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/logs-table.tsx (2)
29-32: Updated hook usage to include pagination information.The destructuring now includes
hasMoreandtotalCount, which are necessary for the new load more functionality.
218-230: Enhanced UI with informative load more footer.The new
loadMoreFooterPropsconfiguration provides users with valuable context about:
- The number of items currently displayed
- The total number of items available
- A clear call-to-action for loading more items
The
hide: isLoadingproperty ensures the footer is only displayed when appropriate.apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.ts (3)
24-24: Good addition of totalCount state variable.This state variable is necessary for tracking the total number of logs available, which will be used for displaying pagination information in the new LoadMoreFooter component.
197-199: Well implemented totalCount update.The code correctly sets the total count from the first page of results, ensuring the pagination information is accurate as soon as data is available.
219-219: Good exposure of totalCount in return value.Including totalCount in the return object allows consuming components to access this information for pagination display.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/logs-table.tsx (2)
69-74: Correctly destructured additional properties from useRatelimitLogsQuery hook.The code now properly retrieves totalCount and hasMore from the updated hook to support the load more functionality.
221-233: Well-structured loadMoreFooterProps implementation.The implementation provides a clear UI for pagination with:
- A descriptive button label
- Conditional display based on loading state
- Informative count text showing progress through available results
- Proper usage of hasMore for determining when to show the load more option
This creates a more interactive and informative experience for users browsing through logs.
internal/clickhouse/src/ratelimits.ts (5)
341-342: Good approach with comment documenting the separation of concerns.The comment clearly explains that this is the main query for paginated results, which helps with code readability and maintenance.
405-421: Well-implemented count query for pagination support.The added count query is properly structured to:
- Return only the total count without retrieving all data
- Use the same filtering conditions as the main query
- Return the count in a consistent, well-typed format
This implementation follows the principle of separation of concerns and avoids performance issues that could arise from counting large datasets.
423-426: Good restructuring of the return value.The function now returns both query results in a clear and structured object, allowing consumers to access both the paginated data and the total count.
653-668: Consistent implementation for overview logs count.The implementation follows the same pattern as the main logs query, ensuring consistency across the codebase. The count query specifically counts distinct identifiers, which is appropriate for this context.
670-673: Consistent return structure for overview logs.The return structure mirrors that of the getRatelimitLogs function, maintaining a predictable API pattern throughout the codebase.
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (2)
119-123: Correctly destructured additional properties from useLogsQuery hook.The code appropriately retrieves the hasMore and total properties from the updated hook to support the load more functionality in the logs table.
250-262: Consistent implementation of loadMoreFooterProps across tables.This implementation maintains consistency with the ratelimits table, using the same pattern for:
- Conditional display based on loading state
- Clear button text
- Informative count information
- Proper tracking of pagination state
This ensures a unified user experience across different sections of the dashboard.
internal/clickhouse/src/logs.ts (4)
75-131: Improved query structure with consolidated filter conditionsThe refactoring to consolidate filter conditions into a single string variable with clear section comments makes the query logic more maintainable and easier to follow. The SQL conditions are well-structured and properly parameterized to prevent SQL injection.
133-143: Well-implemented total count query to support paginationAdding a total count query is essential for implementing the "load more" functionality. The implementation correctly reuses the filter conditions and has proper schema validation.
145-179: Simplified query structure using consolidated filter conditionsThe logs query now uses the consolidated filter conditions, which makes the code more DRY and maintainable. The cursor pagination implementation is correctly applied after the other filters.
181-184: Enhanced return value with both logs and total countThe function now returns both the logs query and total count query results, which is necessary for the new load more component to function properly. This is a good architectural decision for supporting pagination.
apps/dashboard/components/virtual-table/components/loading-indicator.tsx (2)
3-14: Well-defined props interface for the new LoadMoreFooter componentThe
LoadMoreFooterPropstype clearly defines all the necessary properties needed for a flexible load more component, including loading states, counts, and customization options.
53-61: Well implemented button with proper loading and disabled statesThe Button component correctly handles loading states and disables interaction when either fetching is in progress or there's nothing more to load. This prevents users from triggering multiple requests and provides good UX feedback.
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/clickhouse/src/ratelimits.ts (1)
404-419: Consider adding a comment explaining the count query's purposeThis count query is important for the feature, but its purpose might not be immediately clear to future developers.
const countQuery = ch.query({ + // Get total count of matching logs for pagination UI query: ` SELECT count(*) as total_count FROM ratelimits.raw_ratelimits_v1 r WHERE workspace_id = {workspaceId: String} AND namespace_id = {namespaceId: String} AND time BETWEEN {startTime: UInt64} AND {endTime: UInt64} ${hasRequestIds ? "AND request_id IN {requestIds: Array(String)}" : ""} AND (${identifierConditions}) AND (${statusCondition})`, params: extendedParamsSchema, schema: z.object({ total_count: z.number().int(), }), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/clickhouse/src/logs.ts(2 hunks)internal/clickhouse/src/ratelimits.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
internal/clickhouse/src/ratelimits.ts (2)
internal/clickhouse/src/client/client.ts (1)
query(24-61)internal/clickhouse/src/client/noop.ts (1)
query(6-22)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/encryption
- 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/billing
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
internal/clickhouse/src/ratelimits.ts (2)
341-402: Excellent enhancement to enable pagination with total countThe refactoring of this function allows for more sophisticated UI controls by returning both the paginated logs data and the total count. This supports the new load more functionality mentioned in the PR objectives.
Also applies to: 404-424
651-670: Well-implemented count query for overview logsThe addition of a count query that returns the total number of distinct identifiers is a good approach for providing accurate pagination information. This maintains consistency with the implementation pattern used in
getRatelimitLogs.internal/clickhouse/src/logs.ts (4)
75-130: Well-structured filter conditions with clear SQL organizationThe new
filterConditionsstring consolidates all filtering logic in one place, using proper parameterization for security and CASE statements for conditional filtering. The commented sections make it clear what each filter does, improving maintainability.
132-142: Added total count query to support load more functionalityAdding a count query is essential for implementing the "load more" component mentioned in the PR objectives. This allows the UI to show progress (e.g., "X of Y records").
144-179: Refactored main query to use consolidated filter conditionsThe main query now properly uses the shared filter conditions, ensuring consistency between the count and data retrieval queries. The pagination logic with cursor is maintained correctly.
180-183:Details
❓ Verification inconclusive
Updated return structure to include both queries
The function now returns both the logs data and total count queries, aligning with the PR objective of supporting a "load more" component for tables.
Make sure all code that calls
getLogs()has been updated to handle the new return structure. The previous implementation returned a single query result, but now it returns an object withlogsQueryandtotalQuery.
🏁 Script executed:
#!/bin/bash # Find all places where getLogs is imported and used rg "getLogs|logs\.(logsQuery|totalQuery)" --type ts -A 5 -B 5Length of output: 5175
Action Required: Verify Updated Return Structure for
getLogsThe
getLogsfunction ininternal/clickhouse/src/logs.tsnow returns an object with bothlogsQueryandtotalQuery, which supports the "load more" component for tables. However, please ensure that all call sites (for example, ininternal/clickhouse/src/index.ts) have been updated to correctly handle this new return structure. Specifically:
- Confirm that code previously expecting a single value now extracts the
logsQueryandtotalQueryproperties.- Verify that any downstream usage is updated to accommodate the split between query results.
3 participants
What does this PR do?
This PR adds a "load more" component to our Table component. To be able to show the total count, ClickHouse queries were updated accordingly.
The new component is currently disabled for the
/apis/[apiId]table.Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor