Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
|
Claude finished @colegottdank's task —— View job Code Review for Dashboard Update PRScore: 3/10 - Critical issues found, do not merge until fixed Todo List:
Critical Issues Found1. Provider Endpoint Time Filter Bug (Critical -
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Summary
This PR modernizes the dashboard UI with significant refactoring and new visualization components.
Key Changes:
- Added new provider metrics endpoint (
/v1/providers) with 12-hour caching - Created reusable panel components (
ModelsPanel,ModelsByCostPanel,TopProvidersPanel,ErrorsPanel) - Introduced
BarListWithTooltipscomponent with expandable modal for better data visualization - Replaced
AuthHeaderwith newHeadercomponent andLivePillfor improved UX - Established shared color palette system (
chartColors.ts) for consistent styling - Improved time formatting for minute-level increments and adjusted hourly threshold from 7 to 2 days
Issues Found:
- The new provider endpoint doesn't filter by time range or user filters, causing it to return all-time data regardless of dashboard filters
- Sorting logic for "n/a" items has a subtle bug that doesn't guarantee they sort to the end
Confidence Score: 2/5
- This PR should not be merged until the provider endpoint filtering issue is resolved
- The provider metrics endpoint has a critical bug where it ignores time filters and user filters, which will cause incorrect data to be displayed on the dashboard. Users will see all-time provider data even when they select specific time ranges. The sorting bug is less critical but should also be fixed. The rest of the changes are well-structured refactoring and new components that follow good patterns.
- Pay close attention to
valhalla/jawn/src/controllers/public/providerController.ts(missing filter parameters) andweb/components/templates/dashboard/panels/utils.ts(sorting bug)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| valhalla/jawn/src/controllers/public/providerController.ts | 2/5 | new provider metrics endpoint created but missing time filter and user filter support |
| web/services/hooks/providers.tsx | 4/5 | new hook created to fetch provider metrics, follows proper TanStack Query patterns |
| web/components/templates/dashboard/panels/utils.ts | 3/5 | utility functions for sorting and coloring chart data, has sorting bug with "n/a" items |
| web/components/templates/dashboard/panels/barListPanel.tsx | 5/5 | new reusable bar list component with tooltips and expandable modal |
| web/components/templates/dashboard/dashboardPage.tsx | 4/5 | major refactor replacing AuthHeader with Header, integrating new panel components and improved layout |
Sequence Diagram
sequenceDiagram
participant User
participant DashboardPage
participant useDashboardPage
participant useProviders
participant JawnClient
participant ProviderController
participant ClickHouse
participant TopProvidersPanel
User->>DashboardPage: Select time filter
DashboardPage->>useDashboardPage: Update timeFilter state
useDashboardPage->>useProviders: Call with timeFilter, limit, userFilters
useProviders->>JawnClient: GET /v1/providers
JawnClient->>ProviderController: getProviders(request)
ProviderController->>ProviderController: Check cache (12hr TTL)
alt Cache miss
ProviderController->>ClickHouse: Query request_response_rmt
Note over ProviderController,ClickHouse: No time filter applied
ClickHouse-->>ProviderController: Return all-time provider metrics
ProviderController->>ProviderController: Cache result
end
ProviderController-->>JawnClient: Return ProviderMetric[]
JawnClient-->>useProviders: Return providers data
useProviders-->>useDashboardPage: Return providers
useDashboardPage-->>DashboardPage: Pass providers to panels
DashboardPage->>TopProvidersPanel: Render with providers data
TopProvidersPanel->>TopProvidersPanel: sortAndColorData()
TopProvidersPanel->>TopProvidersPanel: useExpandableBarList()
TopProvidersPanel-->>User: Display provider metrics
25 files reviewed, 2 comments
| @Get("/") | ||
| public async getProviders( | ||
| @Request() request: JawnAuthenticatedRequest | ||
| ): Promise<Result<ProviderMetric[], string>> { | ||
| const result = await cacheResultCustom( | ||
| "v1/public/providers" + JSON.stringify(request.authParams), | ||
| async () => { | ||
| const result = await dbQueryClickhouse<ProviderMetric>( | ||
| ` | ||
| SELECT | ||
| provider, | ||
| count(DISTINCT request_id) as total_requests | ||
| FROM request_response_rmt | ||
| WHERE organization_id = {val_0: UUID} | ||
| GROUP BY provider | ||
| ORDER BY total_requests DESC | ||
| LIMIT 1000 | ||
| `, | ||
| [request.authParams.organizationId] | ||
| ); | ||
| return ok(result); | ||
| }, | ||
| kvCache | ||
| ); |
There was a problem hiding this comment.
logic: the getProviders endpoint does not filter by time range or user filters, but the frontend hook passes these parameters (see web/services/hooks/providers.tsx:90-93). this means the provider data will not respect the dashboard's time filter selection, showing all-time data instead of the selected time range.
similar endpoints in the codebase (e.g., in dashboardController.ts) accept and use timeFilter and userFilters parameters in their queries.
Prompt To Fix With AI
This is a comment left during a code review.
Path: valhalla/jawn/src/controllers/public/providerController.ts
Line: 19:42
Comment:
**logic:** the `getProviders` endpoint does not filter by time range or user filters, but the frontend hook passes these parameters (see `web/services/hooks/providers.tsx:90-93`). this means the provider data will not respect the dashboard's time filter selection, showing all-time data instead of the selected time range.
similar endpoints in the codebase (e.g., in `dashboardController.ts`) accept and use `timeFilter` and `userFilters` parameters in their queries.
How can I resolve this? If you propose a fix, please make it concise.| name: item.name || "n/a", | ||
| value: item.value, | ||
| })) | ||
| .sort((a, b) => b.value - a.value - (b.name === "n/a" ? 1 : 0)) |
There was a problem hiding this comment.
logic: the sorting logic doesn't reliably push "n/a" items to the end. subtracting 1 when b.name === "n/a" only slightly penalizes it but doesn't guarantee it will be sorted last.
| .sort((a, b) => b.value - a.value - (b.name === "n/a" ? 1 : 0)) | |
| .sort((a, b) => { | |
| if (a.name === "n/a" && b.name !== "n/a") return 1; | |
| if (b.name === "n/a" && a.name !== "n/a") return -1; | |
| return b.value - a.value; | |
| }) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/components/templates/dashboard/panels/utils.ts
Line: 34:34
Comment:
**logic:** the sorting logic doesn't reliably push "n/a" items to the end. subtracting 1 when `b.name === "n/a"` only slightly penalizes it but doesn't guarantee it will be sorted last.
```suggestion
.sort((a, b) => {
if (a.name === "n/a" && b.name !== "n/a") return 1;
if (b.name === "n/a" && a.name !== "n/a") return -1;
return b.value - a.value;
})
```
How can I resolve this? If you propose a fix, please make it concise.
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
1 participant
No description provided.